[PATCH 1/11] ACPI / ACPICA: Fix global lock acquisition

[PATCH 1/11] ACPI / ACPICA: Fix global lock acquisition

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

There are two problems with the ACPICA's current implementation of
the global lock acquisition.  First, acpi_ev_global_lock_handler(),
which in fact is an interface to the outside of the kernel, doesn't
validate its input, so it only works correctly if the other side
(i.e. the ACPI firmware) is fully specification-compliant (as far
as the global lock is concerned).  Unfortunately, that's known not
to be the case on some systems (i.e. we get spurious global lock
signaling interrupts without the pending flag set on some systems).
Second, acpi_ev_global_lock_handler() attempts to acquire the global
lock on behalf of a thread waiting for it without checking if there
actually is such a thread.  Both of these shortcomings need to be
addressed to prevent all possible race conditions from happening.

Rework acpi_ev_global_lock_handler() so that it doesn't try to
acquire the global lock and make it signal the availability of the
global lock to the waiting thread instead.  Make sure that the
availability of the global lock can only be signaled when there
is a thread waiting for it and that it can't be signaled more than
once in a row (to keep acpi_gbl_global_lock_semaphore in balance).

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/acpi/acpica/evmisc.c |   94 +++++++++++++++++++++++++------------------
 1 file changed, 55 insertions(+), 39 deletions(-)

Index: linux-2.6/drivers/acpi/acpica/evmisc.c
===================================================================
--- linux-2.6.orig/drivers/acpi/acpica/evmisc.c
+++ linux-2.6/drivers/acpi/acpica/evmisc.c
@@ -284,41 +284,41 @@ static void ACPI_SYSTEM_XFACE acpi_ev_no
  * RETURN:      ACPI_INTERRUPT_HANDLED
  *
  * DESCRIPTION: Invoked directly from the SCI handler when a global lock
- *              release interrupt occurs. Attempt to acquire the global lock,
- *              if successful, signal the thread waiting for the lock.
+ *              release interrupt occurs.  If there's a thread waiting for
+ *              the global lock, signal it.
  *
  * NOTE: Assumes that the semaphore can be signaled from interrupt level. If
  * this is not possible for some reason, a separate thread will have to be
  * scheduled to do this.
  *
  ******************************************************************************/
+static u8 acpi_ev_global_lock_pending;
+static spinlock_t _acpi_ev_global_lock_pending_lock;
+#define acpi_ev_global_lock_pending_lock &_acpi_ev_global_lock_pending_lock
 
 static u32 acpi_ev_global_lock_handler(void *context)
 {
-	u8 acquired = FALSE;
+	acpi_status status;
+	acpi_cpu_flags flags;
 
-	/*
-	 * Attempt to get the lock.
-	 *
-	 * If we don't get it now, it will be marked pending and we will
-	 * take another interrupt when it becomes free.
-	 */
-	ACPI_ACQUIRE_GLOBAL_LOCK(acpi_gbl_FACS, acquired);
-	if (acquired) {
+	flags = acpi_os_acquire_lock(acpi_ev_global_lock_pending_lock);
 
-		/* Got the lock, now wake all threads waiting for it */
+	if (!acpi_ev_global_lock_pending) {
+		goto out;
+	}
 
-		acpi_gbl_global_lock_acquired = TRUE;
-		/* Send a unit to the semaphore */
+	/* Send a unit to the semaphore */
 
-		if (ACPI_FAILURE
-		    (acpi_os_signal_semaphore
-		     (acpi_gbl_global_lock_semaphore, 1))) {
-			ACPI_ERROR((AE_INFO,
-				    "Could not signal Global Lock semaphore"));
-		}
+	status = acpi_os_signal_semaphore(acpi_gbl_global_lock_semaphore, 1);
+	if (ACPI_FAILURE(status)) {
+		ACPI_ERROR((AE_INFO, "Could not signal Global Lock semaphore"));
 	}
 
+	acpi_ev_global_lock_pending = FALSE;
+
+ out:
+	acpi_os_release_lock(acpi_ev_global_lock_pending_lock, flags);
+
 	return (ACPI_INTERRUPT_HANDLED);
 }
 
@@ -415,6 +415,7 @@ static int acpi_ev_global_lock_acquired;
 
 acpi_status acpi_ev_acquire_global_lock(u16 timeout)
 {
+	acpi_cpu_flags flags;
 	acpi_status status = AE_OK;
 	u8 acquired = FALSE;
 
@@ -467,32 +468,47 @@ acpi_status acpi_ev_acquire_global_lock(
 		return_ACPI_STATUS(AE_OK);
 	}
 
-	/* Attempt to acquire the actual hardware lock */
+	flags = acpi_os_acquire_lock(acpi_ev_global_lock_pending_lock);
+
+	do {
+
+		/* Attempt to acquire the actual hardware lock */
 
-	ACPI_ACQUIRE_GLOBAL_LOCK(acpi_gbl_FACS, acquired);
-	if (acquired) {
+		ACPI_ACQUIRE_GLOBAL_LOCK(acpi_gbl_FACS, acquired);
+		if (acquired) {
+			acpi_gbl_global_lock_acquired = TRUE;
 
-		/* We got the lock */
+			ACPI_DEBUG_PRINT((ACPI_DB_EXEC,
+					  "Acquired hardware Global Lock\n"));
+			break;
+		}
+
+		acpi_ev_global_lock_pending = TRUE;
+
+		acpi_os_release_lock(acpi_ev_global_lock_pending_lock, flags);
 
+		/*
+		 * Did not get the lock. The pending bit was set above, and we
+		 * must wait until we get the global lock released interrupt.
+		 */
 		ACPI_DEBUG_PRINT((ACPI_DB_EXEC,
-				  "Acquired hardware Global Lock\n"));
+				  "Waiting for hardware Global Lock\n"));
 
-		acpi_gbl_global_lock_acquired = TRUE;
-		return_ACPI_STATUS(AE_OK);
-	}
+		/*
+		 * Wait for handshake with the global lock interrupt handler.
+		 * This interface releases the interpreter if we must wait.
+		 */
+		status = acpi_ex_system_wait_semaphore(
+						acpi_gbl_global_lock_semaphore,
+						ACPI_WAIT_FOREVER);
 
-	/*
-	 * Did not get the lock. The pending bit was set above, and we must now
-	 * wait until we get the global lock released interrupt.
-	 */
-	ACPI_DEBUG_PRINT((ACPI_DB_EXEC, "Waiting for hardware Global Lock\n"));
+		flags = acpi_os_acquire_lock(acpi_ev_global_lock_pending_lock);
 
-	/*
-	 * Wait for handshake with the global lock interrupt handler.
-	 * This interface releases the interpreter if we must wait.
-	 */
-	status = acpi_ex_system_wait_semaphore(acpi_gbl_global_lock_semaphore,
-					       ACPI_WAIT_FOREVER);
+	} while (ACPI_SUCCESS(status));
+
+	acpi_ev_global_lock_pending = FALSE;
+
+	acpi_os_release_lock(acpi_ev_global_lock_pending_lock, flags);
 
 	return_ACPI_STATUS(status);
 }

[PATCH 2/11] ACPI / PM: Do not enable multiple devices to wake up simultaneously

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

If a device is enabled to wake up the system from sleep states via
/proc/acpi/wakeup and there are other devices associated with the
same wakeup GPE, all of these devices are automatically enabled to
wake up the system.  This isn't correct, because the fact the GPE is
shared need not imply that wakeup power has to be enabled for all the
devices at the same time (i.e. it is possible that one device will
have its wakeup power enabled and it will wake up the system from a
sleep state if the shared wakeup GPE is enabled, while another device
having its wakeup power disabled will not wake up the system even
though the GPE is enabled).  Rework acpi_system_write_wakeup_device()
so that it only enables wakeup for one device at a time.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/acpi/proc.c |   26 +-------------------------
 1 file changed, 1 insertion(+), 25 deletions(-)

Index: linux-2.6/drivers/acpi/proc.c
===================================================================
--- linux-2.6.orig/drivers/acpi/proc.c
+++ linux-2.6/drivers/acpi/proc.c
@@ -341,7 +341,6 @@ acpi_system_write_wakeup_device(struct f
 	char strbuf[5];
 	char str[5] = "";
 	unsigned int len = count;
-	struct acpi_device *found_dev = NULL;
 
 	if (len > 4)
 		len = 4;
@@ -363,33 +362,10 @@ acpi_system_write_wakeup_device(struct f
 		if (!strncmp(dev->pnp.bus_id, str, 4)) {
 			dev->wakeup.state.enabled =
 			    dev->wakeup.state.enabled ? 0 : 1;
-			found_dev = dev;
+			physical_device_enable_wakeup(dev);
 			break;
 		}
 	}
-	if (found_dev) {
-		physical_device_enable_wakeup(found_dev);
-		list_for_each_safe(node, next, &acpi_wakeup_device_list) {
-			struct acpi_device *dev = container_of(node,
-							       struct
-							       acpi_device,
-							       wakeup_list);
-
-			if ((dev != found_dev) &&
-			    (dev->wakeup.gpe_number ==
-			     found_dev->wakeup.gpe_number)
-			    && (dev->wakeup.gpe_device ==
-				found_dev->wakeup.gpe_device)) {
-				printk(KERN_WARNING
-				       "ACPI: '%s' and '%s' have the same GPE, "
-				       "can't disable/enable one separately\n",
-				       dev->pnp.bus_id, found_dev->pnp.bus_id);
-				dev->wakeup.state.enabled =
-				    found_dev->wakeup.state.enabled;
-				physical_device_enable_wakeup(dev);
-			}
-		}
-	}
 	mutex_unlock(&acpi_device_lock);
 	return count;
 }

[PATCH 3/11] ACPI / PM: Use device wakeup flags for handling ACPI wakeup devices

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

There are ACPI devices (buttons and the laptop lid) that can wake up
the system from sleep states and have no "physical" companion
devices.  The ACPI subsystem uses two flags, wakeup.state.enabled and
wakeup.flags.always_enabled, for handling those devices, but they
are not accessible through the standard device wakeup infrastructure.
User space can only control them via the /proc/acpi/wakeup interface
that is not really convenient (e.g. the way in which devices are
enabled to wake up the system is not portable between different
systems, because it requires one to know the devices' "names" used in
the system's ACPI tables).

To address this problem, use standard device wakeup flags instead of
the special ACPI flags for handling those devices.  In particular,
use device_set_wakeup_capable() to mark the ACPI wakeup devices
during initialization and use device_set_wakeup_enable() to allow
or disallow them to wake up the system from sleep states.  Rework
the /proc/acpi/wakeup interface to take these changes into account.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/acpi/button.c |    4 ++--
 drivers/acpi/proc.c   |   19 +++++++++++++------
 drivers/acpi/scan.c   |    2 +-
 drivers/acpi/wakeup.c |   18 ++++++++++--------
 4 files changed, 26 insertions(+), 17 deletions(-)

Index: linux-2.6/drivers/acpi/wakeup.c
===================================================================
--- linux-2.6.orig/drivers/acpi/wakeup.c
+++ linux-2.6/drivers/acpi/wakeup.c
@@ -37,11 +37,12 @@ void acpi_enable_wakeup_devices(u8 sleep
 			container_of(node, struct acpi_device, wakeup_list);
 
 		if (!dev->wakeup.flags.valid
-		    || !(dev->wakeup.state.enabled || dev->wakeup.prepare_count)
-		    || sleep_state > (u32) dev->wakeup.sleep_state)
+		    || sleep_state > (u32) dev->wakeup.sleep_state
+		    || !(device_may_wakeup(&dev->dev)
+		        || dev->wakeup.prepare_count))
 			continue;
 
-		if (dev->wakeup.state.enabled)
+		if (device_may_wakeup(&dev->dev))
 			acpi_enable_wakeup_device_power(dev, sleep_state);
 
 		/* The wake-up power should have been enabled already. */
@@ -63,14 +64,15 @@ void acpi_disable_wakeup_devices(u8 slee
 			container_of(node, struct acpi_device, wakeup_list);
 
 		if (!dev->wakeup.flags.valid
-		    || !(dev->wakeup.state.enabled || dev->wakeup.prepare_count)
-		    || (sleep_state > (u32) dev->wakeup.sleep_state))
+		    || sleep_state > (u32) dev->wakeup.sleep_state
+		    || !(device_may_wakeup(&dev->dev)
+		        || dev->wakeup.prepare_count))
 			continue;
 
 		acpi_gpe_wakeup(dev->wakeup.gpe_device, dev->wakeup.gpe_number,
 				ACPI_GPE_DISABLE);
 
-		if (dev->wakeup.state.enabled)
+		if (device_may_wakeup(&dev->dev))
 			acpi_disable_wakeup_device_power(dev);
 	}
 }
@@ -84,8 +86,8 @@ int __init acpi_wakeup_device_init(void)
 		struct acpi_device *dev = container_of(node,
 						       struct acpi_device,
 						       wakeup_list);
-		if (dev->wakeup.flags.always_enabled)
-			dev->wakeup.state.enabled = 1;
+		if (device_can_wakeup(&dev->dev))
+			device_set_wakeup_enable(&dev->dev, true);
 	}
 	mutex_unlock(&acpi_device_lock);
 	return 0;
Index: linux-2.6/drivers/acpi/button.c
===================================================================
--- linux-2.6.orig/drivers/acpi/button.c
+++ linux-2.6/drivers/acpi/button.c
@@ -426,7 +426,7 @@ static int acpi_button_add(struct acpi_d
 		acpi_enable_gpe(device->wakeup.gpe_device,
 				device->wakeup.gpe_number);
 		device->wakeup.run_wake_count++;
-		device->wakeup.state.enabled = 1;
+		device_set_wakeup_enable(&device->dev, true);
 	}
 
 	printk(KERN_INFO PREFIX "%s [%s]\n", name, acpi_device_bid(device));
@@ -449,7 +449,7 @@ static int acpi_button_remove(struct acp
 		acpi_disable_gpe(device->wakeup.gpe_device,
 				device->wakeup.gpe_number);
 		device->wakeup.run_wake_count--;
-		device->wakeup.state.enabled = 0;
+		device_set_wakeup_enable(&device->dev, false);
 	}
 
 	acpi_button_remove_fs(device);
Index: linux-2.6/drivers/acpi/scan.c
===================================================================
--- linux-2.6.orig/drivers/acpi/scan.c
+++ linux-2.6/drivers/acpi/scan.c
@@ -803,7 +803,7 @@ static void acpi_bus_set_run_wake_flags(
 	/* Power button, Lid switch always enable wakeup */
 	if (!acpi_match_device_ids(device, button_device_ids)) {
 		device->wakeup.flags.run_wake = 1;
-		device->wakeup.flags.always_enabled = 1;
+		device_set_wakeup_capable(&device->dev, true);
 		return;
 	}
 
Index: linux-2.6/drivers/acpi/proc.c
===================================================================
--- linux-2.6.orig/drivers/acpi/proc.c
+++ linux-2.6/drivers/acpi/proc.c
@@ -311,7 +311,9 @@ acpi_system_wakeup_device_seq_show(struc
 			   dev->pnp.bus_id,
 			   (u32) dev->wakeup.sleep_state,
 			   dev->wakeup.flags.run_wake ? '*' : ' ',
-			   dev->wakeup.state.enabled ? "enabled" : "disabled");
+			   (device_may_wakeup(&dev->dev)
+			     || (ldev && device_may_wakeup(ldev))) ?
+			       "enabled" : "disabled");
 		if (ldev)
 			seq_printf(seq, "%s:%s",
 				   ldev->bus ? ldev->bus->name : "no-bus",
@@ -328,8 +330,10 @@ static void physical_device_enable_wakeu
 {
 	struct device *dev = acpi_get_physical_device(adev->handle);
 
-	if (dev && device_can_wakeup(dev))
-		device_set_wakeup_enable(dev, adev->wakeup.state.enabled);
+	if (dev && device_can_wakeup(dev)) {
+		bool enable = !device_may_wakeup(dev);
+		device_set_wakeup_enable(dev, enable);
+	}
 }
 
 static ssize_t
@@ -360,9 +364,12 @@ acpi_system_write_wakeup_device(struct f
 			continue;
 
 		if (!strncmp(dev->pnp.bus_id, str, 4)) {
-			dev->wakeup.state.enabled =
-			    dev->wakeup.state.enabled ? 0 : 1;
-			physical_device_enable_wakeup(dev);
+			if (device_can_wakeup(&dev->dev)) {
+				bool enable = !device_may_wakeup(&dev->dev);
+				device_set_wakeup_enable(&dev->dev, enable);
+			} else {
+				physical_device_enable_wakeup(dev);
+			}
 			break;
 		}
 	}

[PATCH 4/11] ACPI / PM: Drop special ACPI wakeup flags

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

Drop special ACPI wakeup flags, wakeup.state.enabled and
wakeup.flags.always_enabled, that aren't necessary any more after
we've started to use standard device wakeup flags for handling ACPI
wakeup devices.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/acpi/glue.c     |    5 +----
 include/acpi/acpi_bus.h |    6 ------
 2 files changed, 1 insertion(+), 10 deletions(-)

Index: linux-2.6/drivers/acpi/glue.c
===================================================================
--- linux-2.6.orig/drivers/acpi/glue.c
+++ linux-2.6/drivers/acpi/glue.c
@@ -167,11 +167,8 @@ static int acpi_bind_one(struct device *
 				"firmware_node");
 		ret = sysfs_create_link(&acpi_dev->dev.kobj, &dev->kobj,
 				"physical_node");
-		if (acpi_dev->wakeup.flags.valid) {
+		if (acpi_dev->wakeup.flags.valid)
 			device_set_wakeup_capable(dev, true);
-			device_set_wakeup_enable(dev,
-						acpi_dev->wakeup.state.enabled);
-		}
 	}
 
 	return 0;
Index: linux-2.6/include/acpi/acpi_bus.h
===================================================================
--- linux-2.6.orig/include/acpi/acpi_bus.h
+++ linux-2.6/include/acpi/acpi_bus.h
@@ -241,20 +241,14 @@ struct acpi_device_perf {
 struct acpi_device_wakeup_flags {
 	u8 valid:1;		/* Can successfully enable wakeup? */
 	u8 run_wake:1;		/* Run-Wake GPE devices */
-	u8 always_enabled:1;	/* Run-wake devices that are always enabled */
 	u8 notifier_present:1;  /* Wake-up notify handler has been installed */
 };
 
-struct acpi_device_wakeup_state {
-	u8 enabled:1;
-};
-
 struct acpi_device_wakeup {
 	acpi_handle gpe_device;
 	u64 gpe_number;
 	u64 sleep_state;
 	struct acpi_handle_list resources;
-	struct acpi_device_wakeup_state state;
 	struct acpi_device_wakeup_flags flags;
 	int prepare_count;
 	int run_wake_count;

[PATCH 5/11] ACPI / PM: Report wakeup events from buttons

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

Since ACPI buttons and lids can be configured to wake up the system
from sleep states, report wakeup events from these devices.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/acpi/button.c |    5 +++++
 1 file changed, 5 insertions(+)

Index: linux-2.6/drivers/acpi/button.c
===================================================================
--- linux-2.6.orig/drivers/acpi/button.c
+++ linux-2.6/drivers/acpi/button.c
@@ -279,6 +279,9 @@ static int acpi_lid_send_state(struct ac
 	input_report_switch(button->input, SW_LID, !state);
 	input_sync(button->input);
 
+	if (state)
+		pm_wakeup_event(&device->dev, 0);
+
 	ret = blocking_notifier_call_chain(&acpi_lid_notifier, state, device);
 	if (ret == NOTIFY_DONE)
 		ret = blocking_notifier_call_chain(&acpi_lid_notifier, state,
@@ -314,6 +317,8 @@ static void acpi_button_notify(struct ac
 			input_sync(input);
 			input_report_key(input, keycode, 0);
 			input_sync(input);
+
+			pm_wakeup_event(&device->dev, 0);
 		}
 
 		acpi_bus_generate_proc_event(device, event, ++button->pushed);

[PATCH 6/11] ACPI / PM: Blacklist Averatec machine known to require acpi_sleep=nonvs

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

Apparently, Averatec AV1020-ED2 does not resume correctly without
acpi_sleep=nonvs, so add it to the ACPI sleep blacklist.

References: https://bugzilla.kernel.org/show_bug.cgi?id=16396#c86
Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/acpi/sleep.c |    8 ++++++++
 1 file changed, 8 insertions(+)

Index: linux-2.6/drivers/acpi/sleep.c
===================================================================
--- linux-2.6.orig/drivers/acpi/sleep.c
+++ linux-2.6/drivers/acpi/sleep.c
@@ -435,6 +435,14 @@ static struct dmi_system_id __initdata a
 		DMI_MATCH(DMI_PRODUCT_NAME, "VGN-NW130D"),
 		},
 	},
+	{
+	.callback = init_nvs_nosave,
+	.ident = "Averatec AV1020-ED2",
+	.matches = {
+		DMI_MATCH(DMI_SYS_VENDOR, "AVERATEC"),
+		DMI_MATCH(DMI_PRODUCT_NAME, "1000 Series"),
+		},
+	},
 	{},
 };
 #endif /* CONFIG_SUSPEND */

[PATCH 7/11] ACPI / PM: Rename acpi_power_off_device()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

Rename acpi_power_off_device() to acpi_power_off() in analogy with
acpi_power_on().

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/acpi/power.c |    7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

Index: linux-2.6/drivers/acpi/power.c
===================================================================
--- linux-2.6.orig/drivers/acpi/power.c
+++ linux-2.6/drivers/acpi/power.c
@@ -219,7 +219,7 @@ static int acpi_power_on(acpi_handle han
 	return result;
 }
 
-static int acpi_power_off_device(acpi_handle handle)
+static int acpi_power_off(acpi_handle handle)
 {
 	int result = 0;
 	acpi_status status = AE_OK;
@@ -268,7 +268,7 @@ static void __acpi_power_off_list(struct
 	int i;
 
 	for (i = num_res - 1; i >= 0 ; i--)
-		acpi_power_off_device(list->handles[i]);
+		acpi_power_off(list->handles[i]);
 }
 
 static void acpi_power_off_list(struct acpi_handle_list *list)
@@ -430,8 +430,7 @@ int acpi_disable_wakeup_device_power(str
 
 	/* Close power resource */
 	for (i = 0; i < dev->wakeup.resources.count; i++) {
-		int ret = acpi_power_off_device(
-				dev->wakeup.resources.handles[i]);
+		int ret = acpi_power_off(dev->wakeup.resources.handles[i]);
 		if (ret) {
 			printk(KERN_ERR PREFIX "Transition power state\n");
 			dev->wakeup.flags.valid = 0;

[PATCH 8/11] ACPI / PM: Check status of power resources under mutexes

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

It certainly is not a good idea to execute _ON or _OFF and _STA
for the same power resource at the same time which may happen in
some circumstances in theory.  To prevent that from happening,
read the power state of each power resource under its mutex, as
that will prevent the sate from being changed at the same time.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/acpi/power.c |   33 ++++++++++++++++++++-------------
 1 file changed, 20 insertions(+), 13 deletions(-)

Index: linux-2.6/drivers/acpi/power.c
===================================================================
--- linux-2.6.orig/drivers/acpi/power.c
+++ linux-2.6/drivers/acpi/power.c
@@ -145,9 +145,8 @@ static int acpi_power_get_state(acpi_han
 
 static int acpi_power_get_list_state(struct acpi_handle_list *list, int *state)
 {
-	int result = 0, state1;
-	u32 i = 0;
-
+	int cur_state;
+	int i = 0;
 
 	if (!list || !state)
 		return -EINVAL;
@@ -155,25 +154,33 @@ static int acpi_power_get_list_state(str
 	/* The state of the list is 'on' IFF all resources are 'on'. */
 
 	for (i = 0; i < list->count; i++) {
-		/*
-		 * The state of the power resource can be obtained by
-		 * using the ACPI handle. In such case it is unnecessary to
-		 * get the Power resource first and then get its state again.
-		 */
-		result = acpi_power_get_state(list->handles[i], &state1);
+		struct acpi_power_resource *resource;
+		acpi_handle handle = list->handles[i];
+		int result;
+
+		result = acpi_power_get_context(handle, &resource);
 		if (result)
 			return result;
 
-		*state = state1;
+		mutex_lock(&resource->resource_lock);
 
-		if (*state != ACPI_POWER_RESOURCE_STATE_ON)
+		result = acpi_power_get_state(handle, &cur_state);
+
+		mutex_unlock(&resource->resource_lock);
+
+		if (result)
+			return result;
+
+		if (cur_state != ACPI_POWER_RESOURCE_STATE_ON)
 			break;
 	}
 
 	ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Resource list is %s\n",
-			  *state ? "on" : "off"));
+			  cur_state ? "on" : "off"));
 
-	return result;
+	*state = cur_state;
+
+	return 0;
 }
 
 static int __acpi_power_on(struct acpi_power_resource *resource)

[PATCH 9/11] ACPI: Always check if _PRW is present before trying to evaluate it

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

Before evaluating _PRW for devices that are reported as inactive or
not present by their _STA control methods we should check if those
methods are actually present (otherwise the evaulation of _PRW will
obviously fail and a scary message will be printed unnecessarily).

Reported-by: Andreas Mohr <andi@lisas.de>
Reported-by: Maciej Rutecki <maciej.rutecki@gmail.com>
Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/acpi/scan.c |    9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

Index: linux-2.6/drivers/acpi/scan.c
===================================================================
--- linux-2.6.orig/drivers/acpi/scan.c
+++ linux-2.6/drivers/acpi/scan.c
@@ -1388,7 +1388,6 @@ static acpi_status acpi_bus_check_add(ac
 	struct acpi_bus_ops *ops = context;
 	int type;
 	unsigned long long sta;
-	struct acpi_device_wakeup wakeup;
 	struct acpi_device *device;
 	acpi_status status;
 	int result;
@@ -1399,7 +1398,13 @@ static acpi_status acpi_bus_check_add(ac
 
 	if (!(sta & ACPI_STA_DEVICE_PRESENT) &&
 	    !(sta & ACPI_STA_DEVICE_FUNCTIONING)) {
-		acpi_bus_extract_wakeup_device_power_package(handle, &wakeup);
+		struct acpi_device_wakeup wakeup;
+		acpi_handle temp;
+
+		status = acpi_get_handle(handle, "_PRW", &temp);
+		if (ACPI_SUCCESS(status))
+			acpi_bus_extract_wakeup_device_power_package(handle,
+								     &wakeup);
 		return AE_CTRL_DEPTH;
 	}
 

[PATCH 10/11] ACPI: Drop device flag wake_capable

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

The wake_capable ACPI device flag is not necessary, because it is
only used in scan.c for recording the information that _PRW is
present for the given device.  That information is only used by
acpi_add_single_object() to decide whether or not to call
acpi_bus_get_wakeup_device_flags(), so the flag may be dropped
if the _PRW check is moved to acpi_bus_get_wakeup_device_flags().
Moreover, acpi_bus_get_wakeup_device_flags() always returns 0,
so it really should be void.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/acpi/scan.c     |   26 +++++++++-----------------
 include/acpi/acpi_bus.h |    3 +--
 2 files changed, 10 insertions(+), 19 deletions(-)

Index: linux-2.6/drivers/acpi/scan.c
===================================================================
--- linux-2.6.orig/drivers/acpi/scan.c
+++ linux-2.6/drivers/acpi/scan.c
@@ -815,16 +815,22 @@ static void acpi_bus_set_run_wake_flags(
 				!!(event_status & ACPI_EVENT_FLAG_HANDLE);
 }
 
-static int acpi_bus_get_wakeup_device_flags(struct acpi_device *device)
+static void acpi_bus_get_wakeup_device_flags(struct acpi_device *device)
 {
+	acpi_handle temp;
 	acpi_status status = 0;
 	int psw_error;
 
+	/* Presence of _PRW indicates wake capable */
+	status = acpi_get_handle(device->handle, "_PRW", &temp);
+	if (ACPI_FAILURE(status))
+		return;
+
 	status = acpi_bus_extract_wakeup_device_power_package(device->handle,
 							      &device->wakeup);
 	if (ACPI_FAILURE(status)) {
 		ACPI_EXCEPTION((AE_INFO, status, "Extracting _PRW package"));
-		goto end;
+		return;
 	}
 
 	device->wakeup.flags.valid = 1;
@@ -840,11 +846,6 @@ static int acpi_bus_get_wakeup_device_fl
 	if (psw_error)
 		ACPI_DEBUG_PRINT((ACPI_DB_INFO,
 				"error in _DSW or _PSW evaluation\n"));
-
-end:
-	if (ACPI_FAILURE(status))
-		device->flags.wake_capable = 0;
-	return 0;
 }
 
 static void acpi_bus_add_power_resource(acpi_handle handle);
@@ -950,11 +951,6 @@ static int acpi_bus_get_flags(struct acp
 	if (ACPI_SUCCESS(status))
 		device->flags.power_manageable = 1;
 
-	/* Presence of _PRW indicates wake capable */
-	status = acpi_get_handle(device->handle, "_PRW", &temp);
-	if (ACPI_SUCCESS(status))
-		device->flags.wake_capable = 1;
-
 	/* TBD: Performance management */
 
 	return 0;
@@ -1281,11 +1277,7 @@ static int acpi_add_single_object(struct
 	 * Wakeup device management
 	 *-----------------------
 	 */
-	if (device->flags.wake_capable) {
-		result = acpi_bus_get_wakeup_device_flags(device);
-		if (result)
-			goto end;
-	}
+	acpi_bus_get_wakeup_device_flags(device);
 
 	/*
 	 * Performance Management
Index: linux-2.6/include/acpi/acpi_bus.h
===================================================================
--- linux-2.6.orig/include/acpi/acpi_bus.h
+++ linux-2.6/include/acpi/acpi_bus.h
@@ -148,8 +148,7 @@ struct acpi_device_flags {
 	u32 suprise_removal_ok:1;
 	u32 power_manageable:1;
 	u32 performance_manageable:1;
-	u32 wake_capable:1;	/* Wakeup(_PRW) supported? */
-	u32 reserved:23;
+	u32 reserved:24;
 };
 
 /* File System */

[PATCH 11/11] ACPI / Battery: Update information on info notification and resume

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

A notification event 0x81 from an ACPI battery device requires us to
re-read the battery information structure.  Follow this requirement
and remove and re-create the battery's attibutes in sysfs so that
they reflect the reporting units used by the battery at the moment
(those units may actually change sometimes at run time, which happens
on some Thinkpads).

The approach used in this patch was suggested by Matthew Garrett.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
Reported-by: Matthew Garrett <mjg@redhat.com>
---
 drivers/acpi/battery.c |   14 ++++++++++++++
 1 file changed, 14 insertions(+)

Index: linux-2.6/drivers/acpi/battery.c
===================================================================
--- linux-2.6.orig/drivers/acpi/battery.c
+++ linux-2.6/drivers/acpi/battery.c
@@ -631,6 +631,17 @@ static int acpi_battery_update(struct ac
 	return result;
 }
 
+static void acpi_battery_refresh(struct acpi_battery *battery)
+{
+	if (!battery->bat.dev)
+		return;
+
+	acpi_battery_get_info(battery);
+	/* The battery may have changed its reporting units. */
+	sysfs_remove_battery(battery);
+	sysfs_add_battery(battery);
+}
+
 /* --------------------------------------------------------------------------
                               FS Interface (/proc)
    -------------------------------------------------------------------------- */
@@ -914,6 +925,8 @@ static void acpi_battery_notify(struct a
 	if (!battery)
 		return;
 	old = battery->bat.dev;
+	if (event == ACPI_BATTERY_NOTIFY_INFO)
+		acpi_battery_refresh(battery);
 	acpi_battery_update(battery);
 	acpi_bus_generate_proc_event(device, event,
 				     acpi_battery_present(battery));
@@ -983,6 +996,7 @@ static int acpi_battery_resume(struct ac
 	if (!device)
 		return -EINVAL;
 	battery = acpi_driver_data(device);
+	acpi_battery_refresh(battery);
 	battery->update_time = 0;
 	acpi_battery_update(battery);
 	return 0;

[bug?] Battery notifications produce flashing battery icon, syslog spam (Re: [PATCH 11/11] ACPI / Battery: Update information on info notification and resume)

[Jonathan Nieder]

Hi Rafael et al,

Rafael J. Wysocki wrote:

> A notification event 0x81 from an ACPI battery device requires us to
> re-read the battery information structure.  Follow this requirement
> and remove and re-create the battery's attibutes in sysfs so that
> they reflect the reporting units used by the battery at the moment
> (those units may actually change sometimes at run time, which happens
> on some Thinkpads).

Ralf Jung was noticing his system tray power management icon in the
bottom-right corner *flickering* every 30 seconds with recent kernels.
Bisects to v2.6.38-rc1~68^2~4 ("ACPI / Battery: Update information on
info notification and resume"), which has the above description.

Some affected systems:

 HP ProBook 4515s/3077, BIOS 68GPI Ver. F.03 (Adrian Fita)
 HP ProBook 4510s (Paolo Scarabelli)
 HP Compaq 615 (Ralf Jung)

Some symptoms:

 KDE power management icon shows "no battery" for a fraction of a
 second (upower is its backend)

 upowerd makes CPU run at close to 100% CPU all the time (?)

 /var/log/syslog gets lots of messages from laptop-mode, like this:

| Apr 20 10:52:14 zero laptop-mode: Laptop mode
| Apr 20 10:52:14 zero laptop-mode: enabled,
| Apr 20 10:52:14 zero laptop-mode: not active [unchanged]
| Apr 20 10:52:14 zero laptop-mode: Laptop mode
| Apr 20 10:52:14 zero laptop-mode: enabled,
| Apr 20 10:52:14 zero laptop-mode: not active [unchanged]
| Apr 20 10:52:33 zero laptop-mode: Laptop mode
| Apr 20 10:52:33 zero laptop-mode: enabled,

Known problems?  Is there some way to handle notification events
without these side effects?  Anything the submitters can do to help
track details down?

An acpidump from Adrian's system can be found at [1].  More background
at [2].

Thanks,
Jonathan

[1] http://bugs.debian.org/cgi-bin/bugreport.cgi?msg=54;filename=acpidump_HP_ProBook_4515s.dat.gz;att=1;bug=670958
[2] http://bugs.debian.org/670958

Re: [bug?] Battery notifications produce flashing battery icon, syslog spam (Re: [PATCH 11/11] ACPI / Battery: Update information on info notification and resume)

[Adrian Fita]

On 01/05/12 21:47, Jonathan Nieder wrote:
 >
> [...]
 >
> [...]  More background
> at [2].
>
> [2] http://bugs.debian.org/670958

Also, searching on Google after "upowerd device 
removed:/org/freedesktop/UPower/devices/battery_BAT0", reveals much more 
bug reports with the exact issue.

Thanks,
-- 
Fita Adrian

Re: [bug?] Battery notifications produce flashing battery icon, syslog spam (Re: [PATCH 11/11] ACPI / Battery: Update information on info notification and resume)

[Jonathan Nieder]

(cc-ing Andy)
Adrian Fita wrote:

> Also, searching on Google after "upowerd device
> removed:/org/freedesktop/UPower/devices/battery_BAT0", reveals much
> more bug reports with the exact issue.

Thanks.  That confirms the high CPU consumption in upowerd ---
see [1], for example.

[1] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/987807

Re: [bug?] Battery notifications produce flashing battery icon, syslog spam (Re: [PATCH 11/11] ACPI / Battery: Update information on info notification and resume)

[Ralf Jung]

Hi,

in addition to the constant flickering when running on AC, there is a more 
"high frequency" flickering immediately after plugging in the AC: For some 5 
to 10 seconds, the battery appears and disappears (according to upower) around 
once per second. There's also a short moment without battery after plugging 
out the AC.
All this is gone after going to a kernel version without this patch applied.

I did not notice unusual high CPU usage of upower on my system, however I 
noticed disc activity - according to iotop, upower is writing several MiB of 
data per minute to /var/lib/upower/ where it keeps some battery statistics. I 
do not know whether this is out of the ordinary.

Kind regards,
Ralf


On Tuesday 01 May 2012 21:14:08 Jonathan Nieder wrote:
> (cc-ing Andy)
> 
> Adrian Fita wrote:
> > Also, searching on Google after "upowerd device
> > removed:/org/freedesktop/UPower/devices/battery_BAT0", reveals much
> > more bug reports with the exact issue.
> 
> Thanks.  That confirms the high CPU consumption in upowerd ---
> see [1], for example.
> 
> [1] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/987807

Re: [bug?] Battery notifications produce flashing battery icon, syslog spam (Re: [PATCH 11/11] ACPI / Battery: Update information on info notification and resume)

[Paolo Scarabelli]

Hi,

I have the same problem on kernel 3.2.0 (actually I have the same
problem with all kernels I tried since reporting the issue the first time).

I'm almost sure this is related to several Debian bugs open for upower:
#606414, #596721 and #619343.

Is it possible that the high cpu problem with upowerd is caused just to
the log size increasing too much when the laptop is left on for a long time?

In my laptop upowerd starts using a lot of cpu only when I leave it on
overnight.


Regards,


Paolo


On 05/02/2012 03:42 AM, Ralf Jung wrote:
> Hi,
> 
> in addition to the constant flickering when running on AC, there is a more 
> "high frequency" flickering immediately after plugging in the AC: For some 5 
> to 10 seconds, the battery appears and disappears (according to upower) around 
> once per second. There's also a short moment without battery after plugging 
> out the AC.
> All this is gone after going to a kernel version without this patch applied.
> 
> I did not notice unusual high CPU usage of upower on my system, however I 
> noticed disc activity - according to iotop, upower is writing several MiB of 
> data per minute to /var/lib/upower/ where it keeps some battery statistics. I 
> do not know whether this is out of the ordinary.
> 
> Kind regards,
> Ralf
> 
> 
> On Tuesday 01 May 2012 21:14:08 Jonathan Nieder wrote:
>> (cc-ing Andy)
>>
>> Adrian Fita wrote:
>>> Also, searching on Google after "upowerd device
>>> removed:/org/freedesktop/UPower/devices/battery_BAT0", reveals much
>>> more bug reports with the exact issue.
>>
>> Thanks.  That confirms the high CPU consumption in upowerd ---
>> see [1], for example.
>>
>> [1] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/987807
> 

Re: [bug?] Battery notifications produce flashing battery icon, syslog spam (Re: [PATCH 11/11] ACPI / Battery: Update information on info notification and resume)

[Andy Whitcroft]

On Tue, May 01, 2012 at 02:14:08PM -0500, Jonathan Nieder wrote:
> (cc-ing Andy)
> Adrian Fita wrote:
> 
> > Also, searching on Google after "upowerd device
> > removed:/org/freedesktop/UPower/devices/battery_BAT0", reveals much
> > more bug reports with the exact issue.
> 
> Thanks.  That confirms the high CPU consumption in upowerd ---
> see [1], for example.
> 
> [1] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/987807

It does seem somewhat heavyweight to be removing and reinstalling all of
the sysfs files every time we get one of these events.  I am assuming
here that some BIOSs are using this interface to tell us the batery
capacity has changed and triggering the constant add/remove of the
devices and associated flickering.

>From the description of the change this is necessary because the
capacity units may change over time?  Can we not use those to avoid this
update?  I presume it is these two we are referring to?

        int capacity_granularity_1;
        int capacity_granularity_2;

If those are unchanged perhaps we can just skip the update?  Something
like the below (completly untested, for discussion).

Thoughts?

-apw

commit d558d0c38e26e2c7eae68d19f4d2fa3ecd8e31f2
Author: Andy Whitcroft <apw@canonical.com>
Date:   Thu May 3 09:52:28 2012 +0100

    battery: only refresh the sysfs files when pertinant information changes
    
    We only need to regenerate the sysfs files when the capacity units
    change, avoid the update otherwise.
    
    Signed-off-by: Andy Whitcroft <apw@canonical.com>

diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
index eb18c44..f8d37b4 100644
--- a/drivers/acpi/battery.c
+++ b/drivers/acpi/battery.c
@@ -643,10 +643,20 @@ static int acpi_battery_update(struct acpi_battery *battery)
 
 static void acpi_battery_refresh(struct acpi_battery *battery)
 {
+	int cg1, cg2;
+
 	if (!battery->bat.dev)
 		return;
 
+	cg1 = battery->capacity_granularity_1;
+	cg2 = battery->capacity_granularity_2;
+
 	acpi_battery_get_info(battery);
+
+	if (cg1 == battery->capacity_granularity_1 &&
+					cg2 == capacity_granularity_2)
+		return;
+
 	/* The battery may have changed its reporting units. */
 	sysfs_remove_battery(battery);
 	sysfs_add_battery(battery);

Re: [bug?] Battery notifications produce flashing battery icon, syslog spam (Re: [PATCH 11/11] ACPI / Battery: Update information on info notification and resume)

[Matthew Garrett]

On Thu, May 03, 2012 at 09:54:58AM +0100, Andy Whitcroft wrote:
> 
> From the description of the change this is necessary because the
> capacity units may change over time?  Can we not use those to avoid this
> update?  I presume it is these two we are referring to?
> 
>         int capacity_granularity_1;
>         int capacity_granularity_2;

power_unit rather than capacity_granularity, but the idea seems solid.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

[PATCH 1/1] battery: only refresh the sysfs files when pertinant information changes

[Andy Whitcroft]

We only need to regenerate the sysfs files when the capacity units
change, avoid the update otherwise.

Signed-off-by: Andy Whitcroft <apw@canonical.com>
---
 drivers/acpi/battery.c |   10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

	Based on Matthew's feedback here is a version which optimises
	based on the power_unit field as returned from the battery info.
	Could someone who suffers from this issue please test this out
	and report back.  Thanks.

diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
index 86933ca..7dd3f9f 100644
--- a/drivers/acpi/battery.c
+++ b/drivers/acpi/battery.c
@@ -643,11 +643,19 @@ static int acpi_battery_update(struct acpi_battery *battery)
 
 static void acpi_battery_refresh(struct acpi_battery *battery)
 {
+	int power_unit;
+
 	if (!battery->bat.dev)
 		return;
 
+	power_unit = battery->power_unit;
+
 	acpi_battery_get_info(battery);
-	/* The battery may have changed its reporting units. */
+
+	if (power_unit == battery->power_unit)
+		return;
+
+	/* The battery has changed its reporting units. */
 	sysfs_remove_battery(battery);
 	sysfs_add_battery(battery);
 }
-- 
1.7.9.5

Re: [PATCH 1/1] battery: only refresh the sysfs files when pertinant information changes

[Ralf Jung]

Hi,

I applied this to 3.4-rc5, and it fixes the issue. Thanks a lot :)

> We only need to regenerate the sysfs files when the capacity units
> change, avoid the update otherwise.
> 
> Signed-off-by: Andy Whitcroft <apw@canonical.com>
Tested-by: Ralf Jung <post@ralfj.de>
> ---
>  drivers/acpi/battery.c |   10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> 	Based on Matthew's feedback here is a version which optimises
> 	based on the power_unit field as returned from the battery info.
> 	Could someone who suffers from this issue please test this out
> 	and report back.  Thanks.
> 
> diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
> index 86933ca..7dd3f9f 100644
> --- a/drivers/acpi/battery.c
> +++ b/drivers/acpi/battery.c
> @@ -643,11 +643,19 @@ static int acpi_battery_update(struct acpi_battery
> *battery)
> 
>  static void acpi_battery_refresh(struct acpi_battery *battery)
>  {
> +	int power_unit;
> +
>  	if (!battery->bat.dev)
>  		return;
> 
> +	power_unit = battery->power_unit;
> +
>  	acpi_battery_get_info(battery);
> -	/* The battery may have changed its reporting units. */
> +
> +	if (power_unit == battery->power_unit)
> +		return;
> +
> +	/* The battery has changed its reporting units. */
>  	sysfs_remove_battery(battery);
>  	sysfs_add_battery(battery);
>  }

Kind regards,
Ralf Jung

Re: [PATCH 1/1] battery: only refresh the sysfs files when pertinant information changes

[Adrian Fita]

On 04/05/12 16:29, Ralf Jung wrote:
> Hi,
> 
> I applied this to 3.4-rc5, and it fixes the issue. Thanks a lot :)

[...]

Hi. I got to test the patch today against the 3.2.16 stable kernel and I
can confirm that it solved the issue. I no longer see "remove"/"add"
events when running "udevadm monitor --property"; I only see "change"
events.

Thanks alot!

Do you know if this patch will be backported to the 3.2 kernel, which
will be the stable kernel for many linux distributions for many years (I
know that the next Debian Stable/wheezy will be using the 3.2 kernel)?

PS: you misspelled "pertinent" in the patch's title. You wrote "pertinant".

Regards,
-- 
Fita Adrian

Re: [PATCH 1/1] battery: only refresh the sysfs files when pertinant information changes

[Len Brown]

applied to ACPI next branch

thanks,
Len Brown, Intel Open Source Technology Center