AC adapter state does not change after resume

2.6.24-rc1 regression Andrey Borzenkov 3 participants 8 messages Oct 30, 2007 – Nov 1, 2007 ACPI

Share & tools

Andrey Borzenkov

author

2007-10-30 20:24 UTC #0 lore raw

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

Alexey Starikovskiy

2007-10-30 20:37 UTC #1 lore raw

Quoted 6 lines · Andrey Borzenkov

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.

Rafael J. Wysocki

2007-10-30 21:08 UTC #2 lore raw

In reply to Andrey Borzenkov

Quoted 4 lines · Andrey Borzenkov

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

Andrey Borzenkov

author

2007-10-31 04:20 UTC #3 lore raw

Quoted 9 lines · Rafael J. Wysocki

Quoted 5 lines · Andrey Borzenkov
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

Quoted 2 lines

Does 2.6.23 work correctly?

yes

Alexey Starikovskiy

2007-10-30 21:09 UTC #4 lore raw

In reply to Andrey Borzenkov

Quoted 6 lines · Andrey Borzenkov

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.

ACPI: AC: Update AC state on sysfs read

From: Alexey Starikovskiy <[email protected]>

Signed-off-by Alexey Starikovskiy <[email protected]>

---

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

drivers/acpi/ac.c +3 -0

index e03de37..bb618c8 100644
@@ -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;

Andrey Borzenkov

author

2007-10-31 04:09 UTC #5 lore raw

Quoted 11 lines · Alexey Starikovskiy

Quoted 7 lines · Andrey Borzenkov
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.

Alexey Starikovskiy

2007-10-31 06:27 UTC #6 lore raw

Quoted 18 lines · Andrey Borzenkov

Quoted 10 lines · Alexey Starikovskiy
Quoted 7 lines · Andrey Borzenkov
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.

Quoted 7 lines

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.

ACPI: AC: Update AC state on resume

From: Alexey Starikovskiy <[email protected]>

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

Signed-off-by Alexey Starikovskiy <[email protected]>

---

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

drivers/acpi/ac.c +17 -0

index e03de37..06308ff 100644
@@ -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;

Andrey Borzenkov

author Tested-by

2007-11-01 18:44 UTC #7 lore raw

Quoted 25 lines · Alexey Starikovskiy

Quoted 24 lines · Andrey Borzenkov
Quoted 10 lines · Alexey Starikovskiy
Quoted 7 lines · Andrey Borzenkov
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.

Quoted 4 lines

Quoted 1 line
This will also make superfluous polling redundant.
Ok, here. This one compiles :)

For HAL regression:

Tested-by Andrey Borzenkov <[email protected]>

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.