[PATCH 0/4] fix spurious wake from suspend to freeze caused by ACPI battery driver

[PATCH 0/4] fix spurious wake from suspend to freeze caused by ACPI battery driver

[Zhang Rui]

Hi, all,

There is a bug report complaining about spurious wake from suspend to freeze.
And I can reproduce the problem on a Toshiba PORTEGE Z830 ultrabook.
https://bugzilla.kernel.org/show_bug.cgi?id=76221

The root cause is that,
1. ACPI battery device receives an ACPI notification about battery remaining
capacity change every 10 seconds.
2. commit 948dcf96622814d2a850a12851d27824530a9747 registers a wakeup source
for every power supply device, to prevent the system from sleeping when there
is a power supply event.

And this results in that the ACPI notification wakes the system up from
suspend-to-freeze shortly.

As ACPI battery driver has the knowledge of whether an important battery change
happens, e.g. battery remaining capacity critical low, etc, it is reasonable
to have its own wakeup source and wakeup the system only when necessary.
And this patch set is made to fix the problem in this way.

Any comments are welcome.

thanks,
rui

[PATCH 1/4] PM: unregister the wakeup source when disabling a device' wakeup capability

[Zhang Rui]

When enabling a device' wakeup capability, a wakeup source
is created for the device automatically. But the wakeup source
is not unregistered when disabling the device' wakeup capability.

This results in zombie wakeup sources, after devices/drivers are unregistered.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/base/power/wakeup.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
index 2d56f41..030b069 100644
--- a/drivers/base/power/wakeup.c
+++ b/drivers/base/power/wakeup.c
@@ -318,10 +318,15 @@ int device_init_wakeup(struct device *dev, bool enable)
 {
 	int ret = 0;
 
+	if (!dev)
+		return -EINVAL;
+
 	if (enable) {
 		device_set_wakeup_capable(dev, true);
 		ret = device_wakeup_enable(dev);
 	} else {
+		if (dev->power.can_wakeup)
+			device_wakeup_disable(dev);
 		device_set_wakeup_capable(dev, false);
 	}
 
-- 
1.8.3.2

[PATCH 2/4] ACPI battery: introduce support for POWER_SUPPLY_PROP_CAPACITY_LEVEL

[Zhang Rui]

ACPI battery device receives notifications when
1. the remaining battery capacity becomes critical low
2. the trip point set by the _BTP (Design capacity of Warning by default)
   is reached or crossed.

So it is able to support POWER_SUPPLY_PROP_CAPACITY_LEVEL to report
        POWER_SUPPLY_CAPACITY_LEVEL_CRITICAL,
        POWER_SUPPLY_CAPACITY_LEVEL_LOW,
        POWER_SUPPLY_CAPACITY_LEVEL_NORMAL,
        POWER_SUPPLY_CAPACITY_LEVEL_FULL,
capacity levels to power supply core and user space.

Introduce support for POWER_SUPPLY_PROP_CAPACITY_LEVEL in this patch.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/acpi/battery.c | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
index 9a2c63b..b508bd0 100644
--- a/drivers/acpi/battery.c
+++ b/drivers/acpi/battery.c
@@ -50,6 +50,10 @@
 /* Battery power unit: 0 means mW, 1 means mA */
 #define ACPI_BATTERY_POWER_UNIT_MA	1
 
+#define ACPI_BATTERY_STATE_DISCHARGING	0x1
+#define ACPI_BATTERY_STATE_CHARGING	0x2
+#define ACPI_BATTERY_STATE_CRITICAL	0x4
+
 #define _COMPONENT		ACPI_BATTERY_COMPONENT
 
 ACPI_MODULE_NAME("battery");
@@ -150,7 +154,7 @@ static int acpi_battery_get_state(struct acpi_battery *battery);
 
 static int acpi_battery_is_charged(struct acpi_battery *battery)
 {
-	/* either charging or discharging */
+	/* charging, discharging or critical low */
 	if (battery->state != 0)
 		return 0;
 
@@ -185,9 +189,9 @@ static int acpi_battery_get_property(struct power_supply *psy,
 		return -ENODEV;
 	switch (psp) {
 	case POWER_SUPPLY_PROP_STATUS:
-		if (battery->state & 0x01)
+		if (battery->state & ACPI_BATTERY_STATE_DISCHARGING)
 			val->intval = POWER_SUPPLY_STATUS_DISCHARGING;
-		else if (battery->state & 0x02)
+		else if (battery->state & ACPI_BATTERY_STATE_CHARGING)
 			val->intval = POWER_SUPPLY_STATUS_CHARGING;
 		else if (acpi_battery_is_charged(battery))
 			val->intval = POWER_SUPPLY_STATUS_FULL;
@@ -250,6 +254,17 @@ static int acpi_battery_get_property(struct power_supply *psy,
 		else
 			val->intval = 0;
 		break;
+	case POWER_SUPPLY_PROP_CAPACITY_LEVEL:
+		if (battery->state & ACPI_BATTERY_STATE_CRITICAL)
+			val->intval = POWER_SUPPLY_CAPACITY_LEVEL_CRITICAL;
+		else if (test_bit(ACPI_BATTERY_ALARM_PRESENT, &battery->flags) &&
+			(battery->capacity_now <= battery->alarm))
+			val->intval = POWER_SUPPLY_CAPACITY_LEVEL_LOW;
+		else if (acpi_battery_is_charged(battery))
+			val->intval = POWER_SUPPLY_CAPACITY_LEVEL_FULL;
+		else
+			val->intval = POWER_SUPPLY_CAPACITY_LEVEL_NORMAL;
+		break;
 	case POWER_SUPPLY_PROP_MODEL_NAME:
 		val->strval = battery->model_number;
 		break;
@@ -277,6 +292,7 @@ static enum power_supply_property charge_battery_props[] = {
 	POWER_SUPPLY_PROP_CHARGE_FULL,
 	POWER_SUPPLY_PROP_CHARGE_NOW,
 	POWER_SUPPLY_PROP_CAPACITY,
+	POWER_SUPPLY_PROP_CAPACITY_LEVEL,
 	POWER_SUPPLY_PROP_MODEL_NAME,
 	POWER_SUPPLY_PROP_MANUFACTURER,
 	POWER_SUPPLY_PROP_SERIAL_NUMBER,
@@ -294,6 +310,7 @@ static enum power_supply_property energy_battery_props[] = {
 	POWER_SUPPLY_PROP_ENERGY_FULL,
 	POWER_SUPPLY_PROP_ENERGY_NOW,
 	POWER_SUPPLY_PROP_CAPACITY,
+	POWER_SUPPLY_PROP_CAPACITY_LEVEL,
 	POWER_SUPPLY_PROP_MODEL_NAME,
 	POWER_SUPPLY_PROP_MANUFACTURER,
 	POWER_SUPPLY_PROP_SERIAL_NUMBER,
-- 
1.8.3.2

[PATCH 3/4] Power_supply: allow power supply devices registered w/o wakeup source

[Zhang Rui]

Currently, all the power supply devices are registered with wakeup source,
this results in that every power_supply_changed() invocation brings
the system out of suspend-to-freeze state.

This is overkill as some device drivers, e.g. ACPI battery driver,
have the ability to check the device status and wake up the system
from sleeping only when necessary.

Thus introduce a new API which allows device to be registered
w/o wakeup source.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/power/power_supply_core.c | 15 +++++++++++++--
 include/linux/power_supply.h      |  2 ++
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/power/power_supply_core.c b/drivers/power/power_supply_core.c
index 2660664..5a5a24e 100644
--- a/drivers/power/power_supply_core.c
+++ b/drivers/power/power_supply_core.c
@@ -537,7 +537,7 @@ static void psy_unregister_cooler(struct power_supply *psy)
 }
 #endif
 
-int power_supply_register(struct device *parent, struct power_supply *psy)
+int __power_supply_register(struct device *parent, struct power_supply *psy, bool ws)
 {
 	struct device *dev;
 	int rc;
@@ -568,7 +568,7 @@ int power_supply_register(struct device *parent, struct power_supply *psy)
 	}
 
 	spin_lock_init(&psy->changed_lock);
-	rc = device_init_wakeup(dev, true);
+	rc = device_init_wakeup(dev, ws);
 	if (rc)
 		goto wakeup_init_failed;
 
@@ -606,8 +606,19 @@ int power_supply_register(struct device *parent, struct power_supply *psy)
 success:
 	return rc;
 }
+
+int power_supply_register(struct device *parent, struct power_supply *psy)
+{
+	return __power_supply_register(parent, psy, true);
+}
 EXPORT_SYMBOL_GPL(power_supply_register);
 
+int power_supply_register_no_ws(struct device *parent, struct power_supply *psy)
+{
+	return __power_supply_register(parent, psy, false);
+}
+EXPORT_SYMBOL_GPL(power_supply_register_no_ws);
+
 void power_supply_unregister(struct power_supply *psy)
 {
 	cancel_work_sync(&psy->changed_work);
diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
index c9dc4e0..f2b76ae 100644
--- a/include/linux/power_supply.h
+++ b/include/linux/power_supply.h
@@ -264,6 +264,8 @@ static inline int power_supply_is_system_supplied(void) { return -ENOSYS; }
 
 extern int power_supply_register(struct device *parent,
 				 struct power_supply *psy);
+extern int power_supply_register_no_ws(struct device *parent,
+				 struct power_supply *psy);
 extern void power_supply_unregister(struct power_supply *psy);
 extern int power_supply_powers(struct power_supply *psy, struct device *dev);
 
-- 
1.8.3.2

[PATCH 4/4] ACPI battery: wakeup the system only when necessary

[Zhang Rui]

ACPI Battery device receives notifications from firmware frequently,
and most of these notifications are some general events, like battery
remaining capacity change, etc, which should not wake the system up
if the system is in suspend/hibernate state.

This causes a problem that the system wakes up from suspend to freeze
shortly, because there is an ACPI battery notification every 10 seconds.
https://bugzilla.kernel.org/show_bug.cgi?id=76221

Fix the problem in this patch by registering ACPI battery device'
own wakeup source, and waking up the system only when the battery remaining
capacity is critical low, or lower than the alarm capacity set via _BTP.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/acpi/battery.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
index b508bd0..e6afe28 100644
--- a/drivers/acpi/battery.c
+++ b/drivers/acpi/battery.c
@@ -595,7 +595,8 @@ static int sysfs_add_battery(struct acpi_battery *battery)
 	battery->bat.type = POWER_SUPPLY_TYPE_BATTERY;
 	battery->bat.get_property = acpi_battery_get_property;
 
-	result = power_supply_register(&battery->device->dev, &battery->bat);
+	result = power_supply_register_no_ws(&battery->device->dev, &battery->bat);
+
 	if (result)
 		return result;
 	return device_create_file(battery->bat.dev, &alarm_attr);
@@ -710,7 +711,19 @@ static int acpi_battery_update(struct acpi_battery *battery)
 			return result;
 	}
 	result = acpi_battery_get_state(battery);
+	if (result)
+		return result;
 	acpi_battery_quirks(battery);
+
+	/*
+	 * Wakeup the system if battery is critical low
+	 * or lower than the alarm level
+	 */
+	if ((battery->state & ACPI_BATTERY_STATE_CRITICAL) ||
+	    (test_bit(ACPI_BATTERY_ALARM_PRESENT, &battery->flags) &&
+            (battery->capacity_now <= battery->alarm)))
+		pm_wakeup_event(&battery->device->dev, 0);
+
 	return result;
 }
 
@@ -815,6 +828,8 @@ static int acpi_battery_add(struct acpi_device *device)
 	battery->pm_nb.notifier_call = battery_notify;
 	register_pm_notifier(&battery->pm_nb);
 
+	device_init_wakeup(&device->dev, 1);
+
 	return result;
 
 fail:
@@ -831,6 +846,7 @@ static int acpi_battery_remove(struct acpi_device *device)
 
 	if (!device || !acpi_driver_data(device))
 		return -EINVAL;
+	device_init_wakeup(&device->dev, 0);
 	battery = acpi_driver_data(device);
 	unregister_pm_notifier(&battery->pm_nb);
 	sysfs_remove_battery(battery);
-- 
1.8.3.2