On Wednesday, 25 of June 2008, Zhao Yakui wrote: > On Wed, 2008-06-25 at 16:11 +0800, Zhang Rui wrote: > > Hi, Rafael, > > > > On Fri, 2008-06-20 at 01:51 +0200, Rafael J. Wysocki wrote: > > > From: Rafael J. Wysocki <rjw@xxxxxxx> > > > > > > PCI ACPI: Introduce acpi_pm_device_sleep_wake function > > > > > > Introduce function acpi_pm_device_sleep_wake() for enabling and > > > disabling the system wake-up capability of devices that are power > > > manageable by ACPI. > > > > > > Introduce callback .sleep_wake() in struct pci_platform_pm_ops and > > > for the ACPI PCI 'driver' make it use acpi_pm_device_sleep_wake(). > > > > acpi_pm_device_sleep_wake only enable/disable the power of wakeup > > device, right? > > If it doesn't set the acpi_device->wakeup.state.enabled, the wake up GPE > > will not be enabled, which means the pci devices which have invoked > > platform_pci_sleep_wake(dev, true) can not wake up the system as > > excepted. > > so this patch won't work. Is this what you want? or do I miss something? > >From the patch it seems that acpi_dev->wakeup.state.enable is not set in > >the function of acpi_pm_device_sleep_wake. If it is not set, it means > >that the corresponding GPE won't be enabled. The device still can't wake > >up the sleeping system. This is a good point, because I forgot about the GPE. However, it's not my intention to set acpi_device->wakeup.state.enabled in acpi_pm_device_sleep_wake(). Instead, I'd like to use the /sys/devices/.../power/wakeup interface and more precisely the dev->power.should_wakeup flag that is supposed to have been set/unset as needed _before_ acpi_pm_device_sleep_wake() is called. Still, I'll need to make the code in drivers/acpi/sleep/wakeup.c enable/disable the wake-up GPEs for devices that have both dev->power.can_wakeup and dev->power.should_wakeup set, as though these devices had acpi_device->wakeup.state.enabled set. > > But what is annoying me is that, > > if acpi_device->wakeup.state.enabled is set in > > acpi_pm_device_sleep_wake, this may bring some other trouble. > > A lot of pci devices will call platform_pci_sleep_wake(dev, true) by > > default, they won't wake up the system before because the wake up GPEs > > are disabled, but they will do if this patch is applied. > > So some of the users may got the "resume immediately after sleep" > > problem, that's a regression, isn't it? That won't happen as long as the devices' dev->power.should_wakeup flags are not set. Note that in the most recent series of patches these flags are unset by default for devices that cannot generate PME#, but there are PCI devices that can generate both PME# and the ACPI-enabled wake-up events and those devices will have dev->power.should_wakeup set by default. Still, if that may cause any problems, we can unset dev->power.should_wakeup for all devices during initialization and let the user space set it as desired. > What Rui said is right. In current kernel when the system enters the > sleeping state, the pci_enable_wake(dev,state, 1) will be called for the > PCI device. If acpi_dev->wakeup.state.enabled is set in > acpi_pm_device_sleep_wake , it means that the wakeup GPE will be > enabled. Maybe some systems will be resumed immediately after sleeping. > this is not what we expected. However, pci_enable_wake() calls device_may_wakeup() and immediately aborts if that returns 'false', so the user space can control its behavior using the /sys/devices/.../power/wakeup interface. > > > > we have discussed this a couple of times on the list, but we have not > > got a solution yet. > > IMO, both Dave's and your patches are good, and we can be a little > > aggressive to merge them, and fix the drivers once we get any > > regressions ASAP. > As the acpi_dev->wakeup.state.enabled is not touched in this patch, it > will be OK to merge them. > > If acpi_dev->wakeup.state.enable is required to be set/unset in > acpi_pm_device_sleep_wake, maybe Rui's suggestion is good. Unless some > device is expected to wake up the sleeping system, we will enable the > corresponding GPE. Otherwise it will be assumed that the device needn't > wake up the sleeping system and unnecessary to enable the device power > and the corresponding GPE. > > i.e. either change pci_enable_wake(dev, 1) to pci_enable_wake(dev, 0) in > > the drivers suspend method, or invoke device_set_wakeup_enable(dev, 0) > > to disable the wakeup ability after the call to device_init_wakeup(dev, > > 1). > > > > any ideas? Please see above. :-) Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
