[patch 2.6.21-rc5-git 2/3] ACPI driver model flags and platform_enable_wake()

[patch 2.6.21-rc5-git 2/3] ACPI driver model flags and platform_enable_wake()

[David Brownell]

This upgrades the ACPI code to start cooperating with the two driver
model wakeup flags and with pci_enable_wake(), based on the previous
patch updating that PCI call:

 - The physical device's "device.power.can_wakeup" flag tracks the ACPI
   flag "acpi_device.wakeup.flags.valid" ... the latter can't entirely
   vanish, since it's initialized before the "real device" node exists.

 - Implements the new platform_enable_wakeup() hook.  This does the same
   thing as manual writes to /proc/acpi/wakeup:  it sets a flag that is
   tested later, to remember whether the GPE should be enabled before
   entering a system sleep state.

 - The GPEs are not set if the physical device's "should_wakeup" flag
   has been cleared.  That's purely a policy input, through sysfs, at the
   level of "that hardware is flakey, ignore its wake events".

ACPI systems see behavioral changes as follows:

   * Wakeup-aware PCI drivers no longer need to have someone override the
     initial system config by writing to /proc/acpi/wakeup; existing calls
     to pci_enable_wake() suffice.  For wakeup-unaware drivers, it's still
     not clearly safe to enable wakeup in /proc/acpi/wakeup.

   * Non-PCI frameworks (as for PS2 serial ports) could call the platform
     hook like PCI does, supporting wakeup-aware drivers.

   * The /sys/devices/.../power/wakeup flags are a different kind of manual
     override.  They _disable_ wakeup for broken hardware or drivers, rather
     than _enabling_ it in the hope that unmodified drivers won't break when
     their hardware triggers wakeup events.

Arguably, it's now time to modify /proc/acpi/wakeup to ignore writes; and
have it display the relevant GPE not an internal flag.

Note a key omission:  only motherboard devices are listed in the ACPI
tables, so add-on devices aren't covered.  The USB framework handles
those devices; but nothing in this patch handles PCI add-in cards.

Another open issue is the ACPI framework's current inability to manage
wakeup events for systems in the S0 state.  That kind of runtime PM is
allowed by PCI and by common hardware (e.g. ICH6).

Includes a semi-related bugfix:  the input devices for the ACPI buttons
weren't set up to link to the "physical" ACPI device node.

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
---
 drivers/acpi/button.c       |    5 +++++
 drivers/acpi/glue.c         |    9 ++++++++-
 drivers/acpi/power.c        |    4 ++++
 drivers/acpi/sleep/wakeup.c |   40 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 57 insertions(+), 1 deletion(-)

--- g26.orig/drivers/acpi/glue.c	2007-04-04 13:57:02.000000000 -0700
+++ g26/drivers/acpi/glue.c	2007-04-04 13:57:03.000000000 -0700
@@ -146,6 +146,7 @@ EXPORT_SYMBOL(acpi_get_physical_device);
 
 static int acpi_bind_one(struct device *dev, acpi_handle handle)
 {
+	struct acpi_device *device;
 	acpi_status status;
 
 	if (dev->archdata.acpi_handle) {
@@ -161,6 +162,11 @@ static int acpi_bind_one(struct device *
 	}
 	dev->archdata.acpi_handle = handle;
 
+	/* init driver model state */
+	status = acpi_bus_get_device(handle, &device);
+	if (!ACPI_FAILURE(status))
+		device_init_wakeup(dev, device->wakeup.flags.valid);
+
 	return 0;
 }
 
@@ -196,7 +202,8 @@ static int acpi_platform_notify(struct d
 	}
 	type = acpi_get_bus_type(dev->bus);
 	if (!type) {
-		DBG("No ACPI bus support for %s\n", dev->bus_id);
+		DBG("No ACPI bus support for %s:%s\n",
+				dev->bus->name, dev->bus_id);
 		ret = -EINVAL;
 		goto end;
 	}
--- g26.orig/drivers/acpi/button.c	2007-04-04 13:53:56.000000000 -0700
+++ g26/drivers/acpi/button.c	2007-04-05 11:54:59.000000000 -0700
@@ -366,6 +366,7 @@ static int acpi_button_add(struct acpi_d
 		error = -ENOMEM;
 		goto err_free_button;
 	}
+	input->cdev.dev = &device->dev;
 
 	/*
 	 * Determine the button type (via hid), as fixed-feature buttons
@@ -444,6 +445,10 @@ static int acpi_button_add(struct acpi_d
 	if (error)
 		goto err_remove_handlers;
 
+	/* for these devices the ACPI node *IS* the "physical" device */
+	device_init_wakeup(&device->dev, device->wakeup.flags.valid);
+	/* REVISIT prefer acpi_bind_one(&device->dev, device->handle); */
+
 	if (device->wakeup.flags.valid) {
 		/* Button's GPE is run-wake GPE */
 		acpi_set_gpe_type(device->wakeup.gpe_device,
--- g26.orig/drivers/acpi/power.c	2007-04-04 13:53:56.000000000 -0700
+++ g26/drivers/acpi/power.c	2007-04-04 13:57:03.000000000 -0700
@@ -322,6 +322,7 @@ int acpi_enable_wakeup_device_power(stru
 		ret = acpi_power_on(dev->wakeup.resources.handles[i], dev);
 		if (ret) {
 			printk(KERN_ERR PREFIX "Transition power state\n");
+			device_init_wakeup(&dev->dev, 0);
 			dev->wakeup.flags.valid = 0;
 			return -1;
 		}
@@ -331,6 +332,7 @@ int acpi_enable_wakeup_device_power(stru
 	status = acpi_evaluate_object(dev->handle, "_PSW", &arg_list, NULL);
 	if (ACPI_FAILURE(status) && (status != AE_NOT_FOUND)) {
 		printk(KERN_ERR PREFIX "Evaluate _PSW\n");
+		device_init_wakeup(&dev->dev, 0);
 		dev->wakeup.flags.valid = 0;
 		ret = -1;
 	}
@@ -360,6 +362,7 @@ int acpi_disable_wakeup_device_power(str
 	status = acpi_evaluate_object(dev->handle, "_PSW", &arg_list, NULL);
 	if (ACPI_FAILURE(status) && (status != AE_NOT_FOUND)) {
 		printk(KERN_ERR PREFIX "Evaluate _PSW\n");
+		device_init_wakeup(&dev->dev, 0);
 		dev->wakeup.flags.valid = 0;
 		return -1;
 	}
@@ -369,6 +372,7 @@ int acpi_disable_wakeup_device_power(str
 		ret = acpi_power_off_device(dev->wakeup.resources.handles[i], dev);
 		if (ret) {
 			printk(KERN_ERR PREFIX "Transition power state\n");
+			device_init_wakeup(&dev->dev, 0);
 			dev->wakeup.flags.valid = 0;
 			return -1;
 		}
--- g26.orig/drivers/acpi/sleep/wakeup.c	2007-04-04 13:53:56.000000000 -0700
+++ g26/drivers/acpi/sleep/wakeup.c	2007-04-04 13:57:03.000000000 -0700
@@ -36,12 +36,22 @@ void acpi_enable_wakeup_device_prep(u8 s
 		struct acpi_device *dev = container_of(node,
 						       struct acpi_device,
 						       wakeup_list);
+		struct device *ldev;
 
 		if (!dev->wakeup.flags.valid ||
 		    !dev->wakeup.state.enabled ||
 		    (sleep_state > (u32) dev->wakeup.sleep_state))
 			continue;
 
+		ldev = acpi_get_physical_device(dev->handle);
+		if (ldev) {
+			int flag = device_may_wakeup(ldev);
+
+			put_device(ldev);
+			if (!flag)
+				continue;
+		}
+
 		spin_unlock(&acpi_device_lock);
 		acpi_enable_wakeup_device_power(dev);
 		spin_lock(&acpi_device_lock);
@@ -68,6 +78,7 @@ void acpi_enable_wakeup_device(u8 sleep_
 		struct acpi_device *dev = container_of(node,
 						       struct acpi_device,
 						       wakeup_list);
+		struct device *ldev;
 
 		/* If users want to disable run-wake GPE,
 		 * we only disable it for wake and leave it for runtime
@@ -89,6 +100,15 @@ void acpi_enable_wakeup_device(u8 sleep_
 		    (sleep_state > (u32) dev->wakeup.sleep_state))
 			continue;
 
+		ldev = acpi_get_physical_device(dev->handle);
+		if (ldev) {
+			int flag = device_may_wakeup(ldev);
+
+			put_device(ldev);
+			if (!flag)
+				continue;
+		}
+
 		spin_unlock(&acpi_device_lock);
 		/* run-wake GPE has been enabled */
 		if (!dev->wakeup.flags.run_wake)
@@ -149,6 +169,24 @@ void acpi_disable_wakeup_device(u8 sleep
 	spin_unlock(&acpi_device_lock);
 }
 
+static int acpi_platform_enable_wakeup(struct device *dev, int is_on)
+{
+	struct acpi_device	*adev;
+	int			status;
+
+	if (!device_can_wakeup(dev))
+		return -EINVAL;
+	if (is_on && !device_may_wakeup(dev))
+		return -EINVAL;
+
+	status = acpi_bus_get_device(DEVICE_ACPI_HANDLE(dev), &adev);
+	if (status < 0)
+		return status;
+
+	adev->wakeup.state.enabled = !!is_on;
+	return 0;
+}
+
 static int __init acpi_wakeup_device_init(void)
 {
 	struct list_head *node, *next;
@@ -156,6 +194,8 @@ static int __init acpi_wakeup_device_ini
 	if (acpi_disabled)
 		return 0;
 
+	platform_enable_wakeup = acpi_platform_enable_wakeup;
+
 	spin_lock(&acpi_device_lock);
 	list_for_each_safe(node, next, &acpi_wakeup_device_list) {
 		struct acpi_device *dev = container_of(node,

Re: [patch 2.6.21-rc5-git 2/3] ACPI driver model flags and platform_enable_wake()

[David Brownell]

I'm curious about the status of the appended patch ... without it,
the new acpi_pm_device_sleep_state() routine isn't really able to
do anything intelligent.  The patch still applies to 2.6.23-rc1-git;
my patches-that-still-haven't-merged queue has had this at the very
top for quite a few months now...

As I recall, Mattia Dongili <malattia@linux.it> reported an error
with a Vaio laptop; it would wake immediately after entering STR.
It didn't reproduce for anyone else.  But that was eventually tracked
down to a bug in iwl3945, which has since been fixed (yes?).

I'm thinking the right thing to do is to put this patch into the ACPI
merge queue ... and this time, be more aggressive at tracking down
any driver bugs it shakes loose.

- Dave


On Thursday 05 April 2007, David Brownell wrote:
> This upgrades the ACPI code to start cooperating with the two driver
> model wakeup flags and with pci_enable_wake(), based on the previous
> patch updating that PCI call:
> 
>  - The physical device's "device.power.can_wakeup" flag tracks the ACPI
>    flag "acpi_device.wakeup.flags.valid" ... the latter can't entirely
>    vanish, since it's initialized before the "real device" node exists.
> 
>  - Implements the new platform_enable_wakeup() hook.  This does the same
>    thing as manual writes to /proc/acpi/wakeup:  it sets a flag that is
>    tested later, to remember whether the GPE should be enabled before
>    entering a system sleep state.
> 
>  - The GPEs are not set if the physical device's "should_wakeup" flag
>    has been cleared.  That's purely a policy input, through sysfs, at the
>    level of "that hardware is flakey, ignore its wake events".
> 
> ACPI systems see behavioral changes as follows:
> 
>    * Wakeup-aware PCI drivers no longer need to have someone override the
>      initial system config by writing to /proc/acpi/wakeup; existing calls
>      to pci_enable_wake() suffice.  For wakeup-unaware drivers, it's still
>      not clearly safe to enable wakeup in /proc/acpi/wakeup.
> 
>    * Non-PCI frameworks (as for PS2 serial ports) could call the platform
>      hook like PCI does, supporting wakeup-aware drivers.
> 
>    * The /sys/devices/.../power/wakeup flags are a different kind of manual
>      override.  They _disable_ wakeup for broken hardware or drivers, rather
>      than _enabling_ it in the hope that unmodified drivers won't break when
>      their hardware triggers wakeup events.
> 
> Arguably, it's now time to modify /proc/acpi/wakeup to ignore writes; and
> have it display the relevant GPE not an internal flag.
> 
> Note a key omission:  only motherboard devices are listed in the ACPI
> tables, so add-on devices aren't covered.  The USB framework handles
> those devices; but nothing in this patch handles PCI add-in cards.
> 
> Another open issue is the ACPI framework's current inability to manage
> wakeup events for systems in the S0 state.  That kind of runtime PM is
> allowed by PCI and by common hardware (e.g. ICH6).
> 
> Includes a semi-related bugfix:  the input devices for the ACPI buttons
> weren't set up to link to the "physical" ACPI device node.
> 
> Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
> ---
>  drivers/acpi/button.c       |    5 +++++
>  drivers/acpi/glue.c         |    9 ++++++++-
>  drivers/acpi/power.c        |    4 ++++
>  drivers/acpi/sleep/wakeup.c |   40 ++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 57 insertions(+), 1 deletion(-)
> 
> --- g26.orig/drivers/acpi/glue.c	2007-04-04 13:57:02.000000000 -0700
> +++ g26/drivers/acpi/glue.c	2007-04-04 13:57:03.000000000 -0700
> @@ -146,6 +146,7 @@ EXPORT_SYMBOL(acpi_get_physical_device);
>  
>  static int acpi_bind_one(struct device *dev, acpi_handle handle)
>  {
> +	struct acpi_device *device;
>  	acpi_status status;
>  
>  	if (dev->archdata.acpi_handle) {
> @@ -161,6 +162,11 @@ static int acpi_bind_one(struct device *
>  	}
>  	dev->archdata.acpi_handle = handle;
>  
> +	/* init driver model state */
> +	status = acpi_bus_get_device(handle, &device);
> +	if (!ACPI_FAILURE(status))
> +		device_init_wakeup(dev, device->wakeup.flags.valid);
> +
>  	return 0;
>  }
>  
> @@ -196,7 +202,8 @@ static int acpi_platform_notify(struct d
>  	}
>  	type = acpi_get_bus_type(dev->bus);
>  	if (!type) {
> -		DBG("No ACPI bus support for %s\n", dev->bus_id);
> +		DBG("No ACPI bus support for %s:%s\n",
> +				dev->bus->name, dev->bus_id);
>  		ret = -EINVAL;
>  		goto end;
>  	}
> --- g26.orig/drivers/acpi/button.c	2007-04-04 13:53:56.000000000 -0700
> +++ g26/drivers/acpi/button.c	2007-04-05 11:54:59.000000000 -0700
> @@ -366,6 +366,7 @@ static int acpi_button_add(struct acpi_d
>  		error = -ENOMEM;
>  		goto err_free_button;
>  	}
> +	input->cdev.dev = &device->dev;
>  
>  	/*
>  	 * Determine the button type (via hid), as fixed-feature buttons
> @@ -444,6 +445,10 @@ static int acpi_button_add(struct acpi_d
>  	if (error)
>  		goto err_remove_handlers;
>  
> +	/* for these devices the ACPI node *IS* the "physical" device */
> +	device_init_wakeup(&device->dev, device->wakeup.flags.valid);
> +	/* REVISIT prefer acpi_bind_one(&device->dev, device->handle); */
> +
>  	if (device->wakeup.flags.valid) {
>  		/* Button's GPE is run-wake GPE */
>  		acpi_set_gpe_type(device->wakeup.gpe_device,
> --- g26.orig/drivers/acpi/power.c	2007-04-04 13:53:56.000000000 -0700
> +++ g26/drivers/acpi/power.c	2007-04-04 13:57:03.000000000 -0700
> @@ -322,6 +322,7 @@ int acpi_enable_wakeup_device_power(stru
>  		ret = acpi_power_on(dev->wakeup.resources.handles[i], dev);
>  		if (ret) {
>  			printk(KERN_ERR PREFIX "Transition power state\n");
> +			device_init_wakeup(&dev->dev, 0);
>  			dev->wakeup.flags.valid = 0;
>  			return -1;
>  		}
> @@ -331,6 +332,7 @@ int acpi_enable_wakeup_device_power(stru
>  	status = acpi_evaluate_object(dev->handle, "_PSW", &arg_list, NULL);
>  	if (ACPI_FAILURE(status) && (status != AE_NOT_FOUND)) {
>  		printk(KERN_ERR PREFIX "Evaluate _PSW\n");
> +		device_init_wakeup(&dev->dev, 0);
>  		dev->wakeup.flags.valid = 0;
>  		ret = -1;
>  	}
> @@ -360,6 +362,7 @@ int acpi_disable_wakeup_device_power(str
>  	status = acpi_evaluate_object(dev->handle, "_PSW", &arg_list, NULL);
>  	if (ACPI_FAILURE(status) && (status != AE_NOT_FOUND)) {
>  		printk(KERN_ERR PREFIX "Evaluate _PSW\n");
> +		device_init_wakeup(&dev->dev, 0);
>  		dev->wakeup.flags.valid = 0;
>  		return -1;
>  	}
> @@ -369,6 +372,7 @@ int acpi_disable_wakeup_device_power(str
>  		ret = acpi_power_off_device(dev->wakeup.resources.handles[i], dev);
>  		if (ret) {
>  			printk(KERN_ERR PREFIX "Transition power state\n");
> +			device_init_wakeup(&dev->dev, 0);
>  			dev->wakeup.flags.valid = 0;
>  			return -1;
>  		}
> --- g26.orig/drivers/acpi/sleep/wakeup.c	2007-04-04 13:53:56.000000000 -0700
> +++ g26/drivers/acpi/sleep/wakeup.c	2007-04-04 13:57:03.000000000 -0700
> @@ -36,12 +36,22 @@ void acpi_enable_wakeup_device_prep(u8 s
>  		struct acpi_device *dev = container_of(node,
>  						       struct acpi_device,
>  						       wakeup_list);
> +		struct device *ldev;
>  
>  		if (!dev->wakeup.flags.valid ||
>  		    !dev->wakeup.state.enabled ||
>  		    (sleep_state > (u32) dev->wakeup.sleep_state))
>  			continue;
>  
> +		ldev = acpi_get_physical_device(dev->handle);
> +		if (ldev) {
> +			int flag = device_may_wakeup(ldev);
> +
> +			put_device(ldev);
> +			if (!flag)
> +				continue;
> +		}
> +
>  		spin_unlock(&acpi_device_lock);
>  		acpi_enable_wakeup_device_power(dev);
>  		spin_lock(&acpi_device_lock);
> @@ -68,6 +78,7 @@ void acpi_enable_wakeup_device(u8 sleep_
>  		struct acpi_device *dev = container_of(node,
>  						       struct acpi_device,
>  						       wakeup_list);
> +		struct device *ldev;
>  
>  		/* If users want to disable run-wake GPE,
>  		 * we only disable it for wake and leave it for runtime
> @@ -89,6 +100,15 @@ void acpi_enable_wakeup_device(u8 sleep_
>  		    (sleep_state > (u32) dev->wakeup.sleep_state))
>  			continue;
>  
> +		ldev = acpi_get_physical_device(dev->handle);
> +		if (ldev) {
> +			int flag = device_may_wakeup(ldev);
> +
> +			put_device(ldev);
> +			if (!flag)
> +				continue;
> +		}
> +
>  		spin_unlock(&acpi_device_lock);
>  		/* run-wake GPE has been enabled */
>  		if (!dev->wakeup.flags.run_wake)
> @@ -149,6 +169,24 @@ void acpi_disable_wakeup_device(u8 sleep
>  	spin_unlock(&acpi_device_lock);
>  }
>  
> +static int acpi_platform_enable_wakeup(struct device *dev, int is_on)
> +{
> +	struct acpi_device	*adev;
> +	int			status;
> +
> +	if (!device_can_wakeup(dev))
> +		return -EINVAL;
> +	if (is_on && !device_may_wakeup(dev))
> +		return -EINVAL;
> +
> +	status = acpi_bus_get_device(DEVICE_ACPI_HANDLE(dev), &adev);
> +	if (status < 0)
> +		return status;
> +
> +	adev->wakeup.state.enabled = !!is_on;
> +	return 0;
> +}
> +
>  static int __init acpi_wakeup_device_init(void)
>  {
>  	struct list_head *node, *next;
> @@ -156,6 +194,8 @@ static int __init acpi_wakeup_device_ini
>  	if (acpi_disabled)
>  		return 0;
>  
> +	platform_enable_wakeup = acpi_platform_enable_wakeup;
> +
>  	spin_lock(&acpi_device_lock);
>  	list_for_each_safe(node, next, &acpi_wakeup_device_list) {
>  		struct acpi_device *dev = container_of(node,
> 

Re: [patch 2.6.21-rc5-git 2/3] ACPI driver model flags and platform_enable_wake()

[TJ]

On Wed, 2007-07-25 at 13:04 -0700, David Brownell wrote:

> As I recall, Mattia Dongili <malattia@linux.it> reported an error
> with a Vaio laptop; it would wake immediately after entering STR.
> It didn't reproduce for anyone else.  But that was eventually tracked
> down to a bug in iwl3945, which has since been fixed (yes?).

I'm not sure if this might inform your thinking here but today I've just
tracked down a similar issue on another Sony Vaio (PCG-SRX51) where S3
STR immediately wakes.

I enabled ACPI_FUTURE_USAGE and called acpi_get_event_status()
immediately upon resume (before the wake flag is cleared) and iterated
each wake event.

I discovered that the PM timer event is the reason the system resumes
but so far haven't discovered why. I have a tenuous theory it could be
related to the kernel timer using the ACPI PM Timer. I tried disabling
that with the boot parameter 'nopmtimer' but that didn't help.

To test this I'm currently building a kernel that disables the PM Timer
wake event during suspend and re-enables it on resume, something the
ACPI specification hints at in section 4.7.2.1.

I'm not sure if this problem occurs on a wider range of systems than the
Sony since at present, without a debug kernel with ACPI_FUTURE_USAGE and
specific additional checks, there is no way to know the reason a system
resumed.

I'm planning on submitting a patch to log the wake event since it would
greatly enhance our abilities to diagnose reasons behind these
situations if the log showed it without a custom-kernel build.

TJ.

Re: [patch 2.6.21-rc5-git 2/3] ACPI driver model flags and platform_enable_wake()

[Mattia Dongili]

On Wed, Jul 25, 2007 at 01:04:02PM -0700, David Brownell wrote:
> I'm curious about the status of the appended patch ... without it,
> the new acpi_pm_device_sleep_state() routine isn't really able to
> do anything intelligent.  The patch still applies to 2.6.23-rc1-git;
> my patches-that-still-haven't-merged queue has had this at the very
> top for quite a few months now...
> 
> As I recall, Mattia Dongili <malattia@linux.it> reported an error
> with a Vaio laptop; it would wake immediately after entering STR.
> It didn't reproduce for anyone else.  But that was eventually tracked
> down to a bug in iwl3945, which has since been fixed (yes?).

not exactly. The problem was a pcmcia express card reader disconnecting
itself after suspend and thus triggering the wakeup.
A good summary is this and following posts:
http://marc.info/?l=linux-acpi&m=118250426218485&w=2

AFAICT it is reproducible by simply suspending and disconnecting an usb
mouse, that should trigger the wakeup event :)

...
> On Thursday 05 April 2007, David Brownell wrote:
> > This upgrades the ACPI code to start cooperating with the two driver
> > model wakeup flags and with pci_enable_wake(), based on the previous
> > patch updating that PCI call:

ciao
-- 
mattia
:wq!