| Message ID | 200909040007.09926.rjw@sisk.pl (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Fri, 2009-09-04 at 06:07 +0800, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rjw@sisk.pl> > > Some PCI devices (not PCI Express), like PCI add-on cards, can > generate PME#, but they don't have any special platform wake-up > support. For this reason, even if they generate PME# to wake up the > system from a sleep state, wake-up events are not generated by the > platform. > > It turns out that, at least on some systems, PCI bridges and the PCI > host bridge have ACPI GPEs associated with them that, if enabled to > generate wake-up events, allow the system to wake up if one of the > add-on devices asserts PME# while the system is in a sleep state. > Following this observation, if a PCI device without direct ACPI > wake-up support is prepared to wake up the system during a transition > into a sleep state (eg. suspend to RAM), configure all the bridges on > the path from the device to the root bridge (including the root > bridge itself) to wake-up the system. >From the description it seems that it will propagate the wake-up function to its parent device. Is it enough to stop the propagation when we find one device that can generate wake-up events in its parent device tree? thanks. > > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl> > --- > drivers/acpi/power.c | 52 ++++++++++++++++++++++++++++++++---------------- > drivers/acpi/scan.c | 1 > drivers/acpi/sleep.c | 2 - > drivers/acpi/wakeup.c | 4 +-- > drivers/pci/pci-acpi.c | 25 +++++++++++++++++++++-- > include/acpi/acpi_bus.h | 2 - > 6 files changed, 63 insertions(+), 23 deletions(-) > > Index: linux-2.6/drivers/pci/pci-acpi.c > =================================================================== > --- linux-2.6.orig/drivers/pci/pci-acpi.c > +++ linux-2.6/drivers/pci/pci-acpi.c > @@ -109,10 +109,31 @@ static bool acpi_pci_can_wakeup(struct p > return handle ? acpi_bus_can_wakeup(handle) : false; > } > > +static void acpi_pci_propagate_wakeup_enable(struct pci_bus *bus, bool enable) > +{ > + while (bus->parent) { > + struct pci_dev *bridge = bus->self; > + > + acpi_pm_device_sleep_wake(&bridge->dev, enable); > + if (bridge->is_pcie) > + return; > + bus = bus->parent; > + } > + > + /* We have reached the root bus. */ > + if (bus->bridge) > + acpi_pm_device_sleep_wake(bus->bridge, enable); > +} > + > static int acpi_pci_sleep_wake(struct pci_dev *dev, bool enable) > { > - return acpi_pci_can_wakeup(dev) ? > - acpi_pm_device_sleep_wake(&dev->dev, enable) : 0; > + if (acpi_pci_can_wakeup(dev)) > + return acpi_pm_device_sleep_wake(&dev->dev, enable); > + > + if (!dev->is_pcie) > + acpi_pci_propagate_wakeup_enable(dev->bus, enable); > + > + return 0; > } > > static struct pci_platform_pm_ops acpi_pci_platform_pm = { > Index: linux-2.6/drivers/acpi/sleep.c > =================================================================== > --- linux-2.6.orig/drivers/acpi/sleep.c > +++ linux-2.6/drivers/acpi/sleep.c > @@ -691,7 +691,7 @@ int acpi_pm_device_sleep_wake(struct dev > struct acpi_device *adev; > int error; > > - if (!device_may_wakeup(dev)) > + if (!device_can_wakeup(dev)) > return -EINVAL; > > handle = DEVICE_ACPI_HANDLE(dev); > Index: linux-2.6/drivers/acpi/scan.c > =================================================================== > --- linux-2.6.orig/drivers/acpi/scan.c > +++ linux-2.6/drivers/acpi/scan.c > @@ -782,6 +782,7 @@ static int acpi_bus_get_wakeup_device_fl > kfree(buffer.pointer); > > device->wakeup.flags.valid = 1; > + device->wakeup.prepare_count = 0; > /* Call _PSW/_DSW object to disable its ability to wake the sleeping > * system for the ACPI device with the _PRW object. > * The _PSW object is depreciated in ACPI 3.0 and is replaced by _DSW. > 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 > @@ -248,7 +248,6 @@ struct acpi_device_perf { > /* Wakeup Management */ > struct acpi_device_wakeup_flags { > u8 valid:1; /* Can successfully enable wakeup? */ > - u8 prepared:1; /* Has the wake-up capability been enabled? */ > u8 run_wake:1; /* Run-Wake GPE devices */ > }; > > @@ -263,6 +262,7 @@ struct acpi_device_wakeup { > struct acpi_handle_list resources; > struct acpi_device_wakeup_state state; > struct acpi_device_wakeup_flags flags; > + int prepare_count; > }; > > /* Device */ > Index: linux-2.6/drivers/acpi/power.c > =================================================================== > --- linux-2.6.orig/drivers/acpi/power.c > +++ linux-2.6/drivers/acpi/power.c > @@ -44,6 +44,8 @@ > #include <acpi/acpi_bus.h> > #include <acpi/acpi_drivers.h> > > +#include "sleep.h" > + > #define _COMPONENT ACPI_POWER_COMPONENT > ACPI_MODULE_NAME("power"); > #define ACPI_POWER_CLASS "power_resource" > @@ -361,17 +363,19 @@ int acpi_device_sleep_wake(struct acpi_d > */ > int acpi_enable_wakeup_device_power(struct acpi_device *dev, int sleep_state) > { > - int i, err; > + int i, err = 0; > > if (!dev || !dev->wakeup.flags.valid) > return -EINVAL; > > + mutex_lock(&acpi_device_lock); > + > /* > * Do not execute the code below twice in a row without calling > * acpi_disable_wakeup_device_power() in between for the same device > */ > - if (dev->wakeup.flags.prepared) > - return 0; > + if (dev->wakeup.prepare_count++) > + goto out; > > /* Open power resource */ > for (i = 0; i < dev->wakeup.resources.count; i++) { > @@ -379,7 +383,8 @@ int acpi_enable_wakeup_device_power(stru > if (ret) { > printk(KERN_ERR PREFIX "Transition power state\n"); > dev->wakeup.flags.valid = 0; > - return -ENODEV; > + err = -ENODEV; > + goto err_out; > } > } > > @@ -388,9 +393,13 @@ int acpi_enable_wakeup_device_power(stru > * in arbitrary power state afterwards. > */ > err = acpi_device_sleep_wake(dev, 1, sleep_state, 3); > - if (!err) > - dev->wakeup.flags.prepared = 1; > > + err_out: > + if (err) > + dev->wakeup.prepare_count = 0; > + > + out: > + mutex_unlock(&acpi_device_lock); > return err; > } > > @@ -402,35 +411,44 @@ int acpi_enable_wakeup_device_power(stru > */ > int acpi_disable_wakeup_device_power(struct acpi_device *dev) > { > - int i, ret; > + int i, err = 0; > > if (!dev || !dev->wakeup.flags.valid) > return -EINVAL; > > + mutex_lock(&acpi_device_lock); > + > /* > * Do not execute the code below twice in a row without calling > * acpi_enable_wakeup_device_power() in between for the same device > */ > - if (!dev->wakeup.flags.prepared) > - return 0; > - > - dev->wakeup.flags.prepared = 0; > + if (--dev->wakeup.prepare_count) { > + if (dev->wakeup.prepare_count < 0) { > + dev_warn(&dev->dev, "unbalanced %s!\n", __func__); > + dev->wakeup.prepare_count = 0; > + } > + goto out; > + } > > - ret = acpi_device_sleep_wake(dev, 0, 0, 0); > - if (ret) > - return ret; > + err = acpi_device_sleep_wake(dev, 0, 0, 0); > + if (err) > + goto out; > > /* Close power resource */ > for (i = 0; i < dev->wakeup.resources.count; i++) { > - ret = acpi_power_off_device(dev->wakeup.resources.handles[i], dev); > + int ret = acpi_power_off_device( > + dev->wakeup.resources.handles[i], dev); > if (ret) { > printk(KERN_ERR PREFIX "Transition power state\n"); > dev->wakeup.flags.valid = 0; > - return -ENODEV; > + err = -ENODEV; > + goto out; > } > } > > - return ret; > + out: > + mutex_unlock(&acpi_device_lock); > + return err; > } > > /* -------------------------------------------------------------------------- > Index: linux-2.6/drivers/acpi/wakeup.c > =================================================================== > --- linux-2.6.orig/drivers/acpi/wakeup.c > +++ linux-2.6/drivers/acpi/wakeup.c > @@ -68,7 +68,7 @@ void acpi_enable_wakeup_device(u8 sleep_ > /* If users want to disable run-wake GPE, > * we only disable it for wake and leave it for runtime > */ > - if ((!dev->wakeup.state.enabled && !dev->wakeup.flags.prepared) > + if ((!dev->wakeup.state.enabled && !dev->wakeup.prepare_count) > || sleep_state > (u32) dev->wakeup.sleep_state) { > if (dev->wakeup.flags.run_wake) { > /* set_gpe_type will disable GPE, leave it like that */ > @@ -100,7 +100,7 @@ void acpi_disable_wakeup_device(u8 sleep > if (!dev->wakeup.flags.valid) > continue; > > - if ((!dev->wakeup.state.enabled && !dev->wakeup.flags.prepared) > + if ((!dev->wakeup.state.enabled && !dev->wakeup.prepare_count) > || sleep_state > (u32) dev->wakeup.sleep_state) { > if (dev->wakeup.flags.run_wake) { > acpi_set_gpe_type(dev->wakeup.gpe_device, > -- > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday 04 September 2009, ykzhao wrote: > On Fri, 2009-09-04 at 06:07 +0800, Rafael J. Wysocki wrote: > > From: Rafael J. Wysocki <rjw@sisk.pl> > > > > Some PCI devices (not PCI Express), like PCI add-on cards, can > > generate PME#, but they don't have any special platform wake-up > > support. For this reason, even if they generate PME# to wake up the > > system from a sleep state, wake-up events are not generated by the > > platform. > > > > It turns out that, at least on some systems, PCI bridges and the PCI > > host bridge have ACPI GPEs associated with them that, if enabled to > > generate wake-up events, allow the system to wake up if one of the > > add-on devices asserts PME# while the system is in a sleep state. > > Following this observation, if a PCI device without direct ACPI > > wake-up support is prepared to wake up the system during a transition > > into a sleep state (eg. suspend to RAM), configure all the bridges on > > the path from the device to the root bridge (including the root > > bridge itself) to wake-up the system. > >From the description it seems that it will propagate the wake-up > function to its parent device. > Is it enough to stop the propagation when we find one device that can > generate wake-up events in its parent device tree? Quite frankly, I'm not sure of that. It really depends on how PME# is routed in given system. The spec permits routing it directly to the chipset as well as routing it through bridges and I don't know if we can assume that the first upstream device capable of generating wake-up events will handle it. That said, I think we can try returning from acpi_pci_propagate_wakeup_enable() as soon as acpi_pm_device_sleep_wake() returns 0 for current device. On the test systems I have it won't make any difference, because the GPE is shared among the root bridge and the first upstream bridge of the device in question. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Sep 04, 2009 at 04:39:46PM +0200, Rafael J. Wysocki wrote: > Quite frankly, I'm not sure of that. It really depends on how PME# is routed > in given system. The spec permits routing it directly to the chipset as well > as routing it through bridges and I don't know if we can assume that the > first upstream device capable of generating wake-up events will handle it. The GPE block is going to be in the chipset, so unless the vendors have explicitly hooked up a link between the PME line from a bridge and the EC, we're presumably always going to get it from the chipset if at all. > That said, I think we can try returning from > acpi_pci_propagate_wakeup_enable() as soon as > acpi_pm_device_sleep_wake() returns 0 for current device. On the test systems > I have it won't make any difference, because the GPE is shared among the > root bridge and the first upstream bridge of the device in question. Yes, it's presumably the case that the PME event in the bridge is just tied to the root bridge in the chipset. Do we know what chipset this hardware is? For Intel, at least, GPE behaviour is defined in the chipset docs.
On Friday 04 September 2009, Matthew Garrett wrote: > On Fri, Sep 04, 2009 at 04:39:46PM +0200, Rafael J. Wysocki wrote: > > > Quite frankly, I'm not sure of that. It really depends on how PME# is routed > > in given system. The spec permits routing it directly to the chipset as well > > as routing it through bridges and I don't know if we can assume that the > > first upstream device capable of generating wake-up events will handle it. > > The GPE block is going to be in the chipset, so unless the vendors have > explicitly hooked up a link between the PME line from a bridge and the > EC, we're presumably always going to get it from the chipset if at all. > > > That said, I think we can try returning from > > acpi_pci_propagate_wakeup_enable() as soon as > > acpi_pm_device_sleep_wake() returns 0 for current device. On the test systems > > I have it won't make any difference, because the GPE is shared among the > > root bridge and the first upstream bridge of the device in question. > > Yes, it's presumably the case that the PME event in the bridge is just > tied to the root bridge in the chipset. Do we know what chipset this > hardware is? For Intel, at least, GPE behaviour is defined in the > chipset docs. One box is Intel, the other one is based on an ATI (pre-AMD) chipset, but the design is similar in that respect. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday 04 September 2009, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rjw@sisk.pl> > > Some PCI devices (not PCI Express), like PCI add-on cards, can > generate PME#, but they don't have any special platform wake-up > support. For this reason, even if they generate PME# to wake up the > system from a sleep state, wake-up events are not generated by the > platform. > > It turns out that, at least on some systems, PCI bridges and the PCI > host bridge have ACPI GPEs associated with them that, if enabled to > generate wake-up events, allow the system to wake up if one of the > add-on devices asserts PME# while the system is in a sleep state. > Following this observation, if a PCI device without direct ACPI > wake-up support is prepared to wake up the system during a transition > into a sleep state (eg. suspend to RAM), configure all the bridges on > the path from the device to the root bridge (including the root > bridge itself) to wake-up the system. I thought it would be better to split this patch into two patches, one replacing the wakeup.prepared flag with a reference counter and the other reworking the PCI ACPI wakeup routine. I the second patch I followed the Yakui's suggestion to propagate wake-up enable until we find the first device with ACPI support for wake-up. The patches will follow in replies to this message. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Sep 05, 2009 at 12:00:32AM +0200, Rafael J. Wysocki wrote: > > Yes, it's presumably the case that the PME event in the bridge is just > > tied to the root bridge in the chipset. Do we know what chipset this > > hardware is? For Intel, at least, GPE behaviour is defined in the > > chipset docs. > > One box is Intel, the other one is based on an ATI (pre-AMD) chipset, but > the design is similar in that respect. GPE 0xb will be the one generated by any Intel chipset whenever the external PCI PME# goes active. 0xd is the equivalent for chipset-level devices that don't have a GPE of their own. I can't see any way that a downstream bridge could reasonably generate a GPE, so I'd bet that it's using 0xb.
On Saturday 05 September 2009, Matthew Garrett wrote: > On Sat, Sep 05, 2009 at 12:00:32AM +0200, Rafael J. Wysocki wrote: > > > Yes, it's presumably the case that the PME event in the bridge is just > > > tied to the root bridge in the chipset. Do we know what chipset this > > > hardware is? For Intel, at least, GPE behaviour is defined in the > > > chipset docs. > > > > One box is Intel, the other one is based on an ATI (pre-AMD) chipset, but > > the design is similar in that respect. > > GPE 0xb will be the one generated by any Intel chipset whenever the > external PCI PME# goes active. 0xd is the equivalent for chipset-level > devices that don't have a GPE of their own. I can't see any way that a > downstream bridge could reasonably generate a GPE, so I'd bet that it's > using 0xb. I agree. Which seems to me that at least for Intel chipsets the "propagation" loop can be stopped as soon as we find the first device with ACPI wakeup support, like in the patch I've just sent. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Index: linux-2.6/drivers/pci/pci-acpi.c =================================================================== --- linux-2.6.orig/drivers/pci/pci-acpi.c +++ linux-2.6/drivers/pci/pci-acpi.c @@ -109,10 +109,31 @@ static bool acpi_pci_can_wakeup(struct p return handle ? acpi_bus_can_wakeup(handle) : false; } +static void acpi_pci_propagate_wakeup_enable(struct pci_bus *bus, bool enable) +{ + while (bus->parent) { + struct pci_dev *bridge = bus->self; + + acpi_pm_device_sleep_wake(&bridge->dev, enable); + if (bridge->is_pcie) + return; + bus = bus->parent; + } + + /* We have reached the root bus. */ + if (bus->bridge) + acpi_pm_device_sleep_wake(bus->bridge, enable); +} + static int acpi_pci_sleep_wake(struct pci_dev *dev, bool enable) { - return acpi_pci_can_wakeup(dev) ? - acpi_pm_device_sleep_wake(&dev->dev, enable) : 0; + if (acpi_pci_can_wakeup(dev)) + return acpi_pm_device_sleep_wake(&dev->dev, enable); + + if (!dev->is_pcie) + acpi_pci_propagate_wakeup_enable(dev->bus, enable); + + return 0; } static struct pci_platform_pm_ops acpi_pci_platform_pm = { Index: linux-2.6/drivers/acpi/sleep.c =================================================================== --- linux-2.6.orig/drivers/acpi/sleep.c +++ linux-2.6/drivers/acpi/sleep.c @@ -691,7 +691,7 @@ int acpi_pm_device_sleep_wake(struct dev struct acpi_device *adev; int error; - if (!device_may_wakeup(dev)) + if (!device_can_wakeup(dev)) return -EINVAL; handle = DEVICE_ACPI_HANDLE(dev); Index: linux-2.6/drivers/acpi/scan.c =================================================================== --- linux-2.6.orig/drivers/acpi/scan.c +++ linux-2.6/drivers/acpi/scan.c @@ -782,6 +782,7 @@ static int acpi_bus_get_wakeup_device_fl kfree(buffer.pointer); device->wakeup.flags.valid = 1; + device->wakeup.prepare_count = 0; /* Call _PSW/_DSW object to disable its ability to wake the sleeping * system for the ACPI device with the _PRW object. * The _PSW object is depreciated in ACPI 3.0 and is replaced by _DSW. 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 @@ -248,7 +248,6 @@ struct acpi_device_perf { /* Wakeup Management */ struct acpi_device_wakeup_flags { u8 valid:1; /* Can successfully enable wakeup? */ - u8 prepared:1; /* Has the wake-up capability been enabled? */ u8 run_wake:1; /* Run-Wake GPE devices */ }; @@ -263,6 +262,7 @@ struct acpi_device_wakeup { struct acpi_handle_list resources; struct acpi_device_wakeup_state state; struct acpi_device_wakeup_flags flags; + int prepare_count; }; /* Device */ Index: linux-2.6/drivers/acpi/power.c =================================================================== --- linux-2.6.orig/drivers/acpi/power.c +++ linux-2.6/drivers/acpi/power.c @@ -44,6 +44,8 @@ #include <acpi/acpi_bus.h> #include <acpi/acpi_drivers.h> +#include "sleep.h" + #define _COMPONENT ACPI_POWER_COMPONENT ACPI_MODULE_NAME("power"); #define ACPI_POWER_CLASS "power_resource" @@ -361,17 +363,19 @@ int acpi_device_sleep_wake(struct acpi_d */ int acpi_enable_wakeup_device_power(struct acpi_device *dev, int sleep_state) { - int i, err; + int i, err = 0; if (!dev || !dev->wakeup.flags.valid) return -EINVAL; + mutex_lock(&acpi_device_lock); + /* * Do not execute the code below twice in a row without calling * acpi_disable_wakeup_device_power() in between for the same device */ - if (dev->wakeup.flags.prepared) - return 0; + if (dev->wakeup.prepare_count++) + goto out; /* Open power resource */ for (i = 0; i < dev->wakeup.resources.count; i++) { @@ -379,7 +383,8 @@ int acpi_enable_wakeup_device_power(stru if (ret) { printk(KERN_ERR PREFIX "Transition power state\n"); dev->wakeup.flags.valid = 0; - return -ENODEV; + err = -ENODEV; + goto err_out; } } @@ -388,9 +393,13 @@ int acpi_enable_wakeup_device_power(stru * in arbitrary power state afterwards. */ err = acpi_device_sleep_wake(dev, 1, sleep_state, 3); - if (!err) - dev->wakeup.flags.prepared = 1; + err_out: + if (err) + dev->wakeup.prepare_count = 0; + + out: + mutex_unlock(&acpi_device_lock); return err; } @@ -402,35 +411,44 @@ int acpi_enable_wakeup_device_power(stru */ int acpi_disable_wakeup_device_power(struct acpi_device *dev) { - int i, ret; + int i, err = 0; if (!dev || !dev->wakeup.flags.valid) return -EINVAL; + mutex_lock(&acpi_device_lock); + /* * Do not execute the code below twice in a row without calling * acpi_enable_wakeup_device_power() in between for the same device */ - if (!dev->wakeup.flags.prepared) - return 0; - - dev->wakeup.flags.prepared = 0; + if (--dev->wakeup.prepare_count) { + if (dev->wakeup.prepare_count < 0) { + dev_warn(&dev->dev, "unbalanced %s!\n", __func__); + dev->wakeup.prepare_count = 0; + } + goto out; + } - ret = acpi_device_sleep_wake(dev, 0, 0, 0); - if (ret) - return ret; + err = acpi_device_sleep_wake(dev, 0, 0, 0); + if (err) + goto out; /* Close power resource */ for (i = 0; i < dev->wakeup.resources.count; i++) { - ret = acpi_power_off_device(dev->wakeup.resources.handles[i], dev); + int ret = acpi_power_off_device( + dev->wakeup.resources.handles[i], dev); if (ret) { printk(KERN_ERR PREFIX "Transition power state\n"); dev->wakeup.flags.valid = 0; - return -ENODEV; + err = -ENODEV; + goto out; } } - return ret; + out: + mutex_unlock(&acpi_device_lock); + return err; } /* -------------------------------------------------------------------------- Index: linux-2.6/drivers/acpi/wakeup.c =================================================================== --- linux-2.6.orig/drivers/acpi/wakeup.c +++ linux-2.6/drivers/acpi/wakeup.c @@ -68,7 +68,7 @@ void acpi_enable_wakeup_device(u8 sleep_ /* If users want to disable run-wake GPE, * we only disable it for wake and leave it for runtime */ - if ((!dev->wakeup.state.enabled && !dev->wakeup.flags.prepared) + if ((!dev->wakeup.state.enabled && !dev->wakeup.prepare_count) || sleep_state > (u32) dev->wakeup.sleep_state) { if (dev->wakeup.flags.run_wake) { /* set_gpe_type will disable GPE, leave it like that */ @@ -100,7 +100,7 @@ void acpi_disable_wakeup_device(u8 sleep if (!dev->wakeup.flags.valid) continue; - if ((!dev->wakeup.state.enabled && !dev->wakeup.flags.prepared) + if ((!dev->wakeup.state.enabled && !dev->wakeup.prepare_count) || sleep_state > (u32) dev->wakeup.sleep_state) { if (dev->wakeup.flags.run_wake) { acpi_set_gpe_type(dev->wakeup.gpe_device,