[2.6.24-rc1 regression] AC adapter state does not change after resume

[2.6.24-rc1 regression] AC adapter state does not change after resume

[Andrey Borzenkov]

[-- Attachment #1: Type: text/plain, Size: 223 bytes --]

I suspect new ACPI AC adapter code but have to add some printk's to be sure.

To reproduce - plug in AC cord, suspend, unplug, resume - kpowersave and sysfs 
still show AC adapter online. Or other way round.

-andrey

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [2.6.24-rc1 regression] AC adapter state does not change after resume

[Alexey Starikovskiy]

Andrey Borzenkov wrote:
> I suspect new ACPI AC adapter code but have to add some printk's to be sure.
> 
> To reproduce - plug in AC cord, suspend, unplug, resume - kpowersave and sysfs 
> still show AC adapter online. Or other way round.
> 
> -andrey
The only change to drivers/acpi/ac.c after .23 was d5b4a3d0efa36de31b86d5677dad6c36cb8735d7.
Don't see how it could lead to less messages from AC.

Regards,
Alex.

Re: [2.6.24-rc1 regression] AC adapter state does not change after resume

[Alexey Starikovskiy]

[-- Attachment #1: Type: text/plain, Size: 303 bytes --]

Andrey Borzenkov wrote:
> I suspect new ACPI AC adapter code but have to add some printk's to be sure.
> 
> To reproduce - plug in AC cord, suspend, unplug, resume - kpowersave and sysfs 
> still show AC adapter online. Or other way round.
> 
> -andrey
Please check if this patch helps.

Regards,
Alex.

[-- Attachment #2: update_ac_state_on_sysfs_read.patch --]
[-- Type: text/x-diff, Size: 642 bytes --]

ACPI: AC: Update AC state on sysfs read

From: Alexey Starikovskiy <astarikovskiy@suse.de>

Signed-off-by: Alexey Starikovskiy <astarikovskiy@suse.de>
---

 drivers/acpi/ac.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/acpi/ac.c b/drivers/acpi/ac.c
index e03de37..bb618c8 100644
--- a/drivers/acpi/ac.c
+++ b/drivers/acpi/ac.c
@@ -91,6 +91,9 @@ static int get_ac_property(struct power_supply *psy,
 			   union power_supply_propval *val)
 {
 	struct acpi_ac *ac = to_acpi_ac(psy);
+
+	if (acpi_ac_get_state(ac))
+		return 0;
 	switch (psp) {
 	case POWER_SUPPLY_PROP_ONLINE:
 		val->intval = ac->state;

Re: [2.6.24-rc1 regression] AC adapter state does not change after resume

[Andrey Borzenkov]

[-- Attachment #1: Type: text/plain, Size: 1006 bytes --]

On Wednesday 31 October 2007, Alexey Starikovskiy wrote:
> Andrey Borzenkov wrote:
> > I suspect new ACPI AC adapter code but have to add some printk's to be
> > sure.
> >
> > To reproduce - plug in AC cord, suspend, unplug, resume - kpowersave and
> > sysfs still show AC adapter online. Or other way round.
> >
> > -andrey
>
> Please check if this patch helps.
>

It does not even compile :)

On serious note (after adding forward declaration) - patch half works. It does 
make "online" return true value (which confirms that underlying ACPI works 
correctly) but HAL still does not notice it.

The problem is, with power_supply interface HAL does not use polling, it 
relies on CHANGE event. This event is apparently never generated if AC 
adapter state changes on resume. So the obvious fix is to compare AC state 
before and after suspend in ->resume function and generate synthetic CHANGE 
event if something has changed.

This will also make superfluous polling redundant.

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [2.6.24-rc1 regression] AC adapter state does not change after resume

[Alexey Starikovskiy]

[-- Attachment #1: Type: text/plain, Size: 1082 bytes --]

Andrey Borzenkov wrote:
> On Wednesday 31 October 2007, Alexey Starikovskiy wrote:
>> Andrey Borzenkov wrote:
>>> I suspect new ACPI AC adapter code but have to add some printk's to be
>>> sure.
>>>
>>> To reproduce - plug in AC cord, suspend, unplug, resume - kpowersave and
>>> sysfs still show AC adapter online. Or other way round.
>>>
>>> -andrey
>> Please check if this patch helps.
>>
> 
> It does not even compile :)
> 
> On serious note (after adding forward declaration) - patch half works. It does 
> make "online" return true value (which confirms that underlying ACPI works 
> correctly) but HAL still does not notice it.
> 
> The problem is, with power_supply interface HAL does not use polling, it 
> relies on CHANGE event. This event is apparently never generated if AC 
> adapter state changes on resume. So the obvious fix is to compare AC state 
> before and after suspend in ->resume function and generate synthetic CHANGE 
> event if something has changed.
> 
> This will also make superfluous polling redundant.
Ok, here. This one compiles :)

Regards,
Alex.

[-- Attachment #2: update_ac_state_on_sysfs_read.patch --]
[-- Type: text/x-diff, Size: 1568 bytes --]

ACPI: AC: Update AC state on resume

From: Alexey Starikovskiy <astarikovskiy@suse.de>

Check if AC state has changed across resume and notify userspace if so.

Signed-off-by: Alexey Starikovskiy <astarikovskiy@suse.de>
---

 drivers/acpi/ac.c |   17 +++++++++++++++++
 1 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/drivers/acpi/ac.c b/drivers/acpi/ac.c
index e03de37..06308ff 100644
--- a/drivers/acpi/ac.c
+++ b/drivers/acpi/ac.c
@@ -54,6 +54,7 @@ extern void *acpi_unlock_ac_dir(struct proc_dir_entry *acpi_ac_dir);
 
 static int acpi_ac_add(struct acpi_device *device);
 static int acpi_ac_remove(struct acpi_device *device, int type);
+static int acpi_ac_resume(struct acpi_device *device);
 static int acpi_ac_open_fs(struct inode *inode, struct file *file);
 
 const static struct acpi_device_id ac_device_ids[] = {
@@ -69,6 +70,7 @@ static struct acpi_driver acpi_ac_driver = {
 	.ops = {
 		.add = acpi_ac_add,
 		.remove = acpi_ac_remove,
+		.resume = acpi_ac_resume,
 		},
 };
 
@@ -294,6 +296,21 @@ static int acpi_ac_add(struct acpi_device *device)
 	return result;
 }
 
+static int acpi_ac_resume(struct acpi_device *device)
+{
+	struct acpi_ac *ac;
+	unsigned old_state;
+	if (!device || !acpi_driver_data(device))
+		return -EINVAL;
+	ac = acpi_driver_data(device);
+	old_state = ac->state;
+	if (acpi_ac_get_state(ac))
+		return 0;
+	if (old_state != ac->state)
+		kobject_uevent(&ac->charger.dev->kobj, KOBJ_CHANGE);
+	return 0;
+}
+
 static int acpi_ac_remove(struct acpi_device *device, int type)
 {
 	acpi_status status = AE_OK;

Re: [2.6.24-rc1 regression] AC adapter state does not change after resume

[Andrey Borzenkov]

[-- Attachment #1: Type: text/plain, Size: 1507 bytes --]

On Wednesday 31 October 2007, Alexey Starikovskiy wrote:
> Andrey Borzenkov wrote:
> > On Wednesday 31 October 2007, Alexey Starikovskiy wrote:
> >> Andrey Borzenkov wrote:
> >>> I suspect new ACPI AC adapter code but have to add some printk's to be
> >>> sure.
> >>>
> >>> To reproduce - plug in AC cord, suspend, unplug, resume - kpowersave
> >>> and sysfs still show AC adapter online. Or other way round.
> >>>
> >>> -andrey
> >>
> >> Please check if this patch helps.
> >
> > It does not even compile :)
> >
> > On serious note (after adding forward declaration) - patch half works. It
> > does make "online" return true value (which confirms that underlying ACPI
> > works correctly) but HAL still does not notice it.
> >
> > The problem is, with power_supply interface HAL does not use polling, it
> > relies on CHANGE event. This event is apparently never generated if AC
> > adapter state changes on resume. So the obvious fix is to compare AC
> > state before and after suspend in ->resume function and generate
> > synthetic CHANGE event if something has changed.
> >
> > This will also make superfluous polling redundant.
>
> Ok, here. This one compiles :)
>

For HAL regression:

Tested-by: Andrey Borzenkov <arvidjaar@mail.ru>

But the patch is incomplete. This synthetic event should be the same as if OS 
were running; i.e. you should send both other events as well. As is you just 
create latent bug to be hit by some netlink consumer sooner or later.

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [2.6.24-rc1 regression] AC adapter state does not change after resume

[Rafael J. Wysocki]

On Tuesday, 30 October 2007 21:24, Andrey Borzenkov wrote:
> I suspect new ACPI AC adapter code but have to add some printk's to be sure.
> 
> To reproduce - plug in AC cord, suspend, unplug, resume - kpowersave and sysfs 
> still show AC adapter online. Or other way round.

Is this a resume from RAM or from disk?

Does 2.6.23 work correctly?

Rafael

Re: [2.6.24-rc1 regression] AC adapter state does not change after resume

[Andrey Borzenkov]

[-- Attachment #1: Type: text/plain, Size: 431 bytes --]

On Wednesday 31 October 2007, Rafael J. Wysocki wrote:
> On Tuesday, 30 October 2007 21:24, Andrey Borzenkov wrote:
> > I suspect new ACPI AC adapter code but have to add some printk's to be
> > sure.
> >
> > To reproduce - plug in AC cord, suspend, unplug, resume - kpowersave and
> > sysfs still show AC adapter online. Or other way round.
>
> Is this a resume from RAM or from disk?
>

RAM

> Does 2.6.23 work correctly?
>

yes

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 189 bytes --]