| Message ID | 1250666661.23178.120.camel@sli10-desk.sh.intel.com (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
On Wed, Aug 19, 2009 at 03:24:21PM +0800, Shaohua Li wrote: > + ret = acpi_gpe_pme_check(dev); > + > + acpi_disable_gpe(dev->wakeup.gpe_device, dev->wakeup.gpe_number); I don't think we want to unconditionally disable the GPE. > + > + acpi_init_gpe_pme(); I'm also not keen on haing the notifier being at the ACPI level. Are we guaranteed that the GPE will only be used for wakeups, and will never trigger any other sort of notification? Keeping this at the bus level may be more practical.
On Wed, Aug 19, 2009 at 07:42:20PM +0800, Matthew Garrett wrote: > On Wed, Aug 19, 2009 at 03:24:21PM +0800, Shaohua Li wrote: > > > + ret = acpi_gpe_pme_check(dev); > > + > > + acpi_disable_gpe(dev->wakeup.gpe_device, dev->wakeup.gpe_number); > > I don't think we want to unconditionally disable the GPE. yes, we need something you proposed to add reference for GPE disable/enable. > > + > > + acpi_init_gpe_pme(); > > I'm also not keen on haing the notifier being at the ACPI level. Are we > guaranteed that the GPE will only be used for wakeups, and will never > trigger any other sort of notification? Keeping this at the bus level > may be more practical. the notification handler checks if this is a wakeup event. Because the ACPI wakeup event can be sent to any kind of buses, so move the code to acpi level can reduce a lot of duplicate code, otherwise you must implement the same mechanism for every bus. Thanks, Shaohua -- 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
On Wednesday 19 August 2009, Shaohua Li wrote: > > In ACPI platform, if native PME isn't enabled, GPE is used to report wakeup event. > --- > drivers/acpi/Kconfig | 9 ++++++ > drivers/acpi/bus.c | 15 +++++++++++ > drivers/acpi/sleep/wakeup.c | 60 ++++++++++++++++++++++++++++++++++++++++++++ > include/acpi/acpi_bus.h | 4 ++ > 4 files changed, 88 insertions(+) > > Index: linux/drivers/acpi/Kconfig > =================================================================== > --- linux.orig/drivers/acpi/Kconfig 2009-08-05 11:00:06.000000000 +0800 > +++ linux/drivers/acpi/Kconfig 2009-08-19 10:07:05.000000000 +0800 > @@ -45,6 +45,15 @@ config ACPI_SLEEP > depends on SUSPEND || HIBERNATION > default y > > +config ACPI_GPE_WAKEUP > + bool "ACPI wakeup event support" > + depends on PM_SLEEP && EXPERIMENTAL It shouldn't depend on PM_SLEEP. > + help > + Enable ACPI to detect wakeup event. For example, PCI device can > + invoke PME, and in ACPI platform, the PME will invoke a GPE. With > + the option, we can detect which device invokes wakeup event. > + > + I don't think we need yet another CONFIG switch for this purpose. It would be better to make it depend on CONFIG_PM_RUNTIME or just on CONFIG_PM. > config ACPI_PROCFS > bool "Deprecated /proc/acpi files" > depends on PROC_FS > Index: linux/drivers/acpi/wakeup.c > =================================================================== > --- linux.orig/drivers/acpi/wakeup.c 2009-07-24 11:31:05.000000000 +0800 > +++ linux/drivers/acpi/wakeup.c 2009-08-19 10:16:27.000000000 +0800 > @@ -124,6 +124,63 @@ void acpi_disable_wakeup_device(u8 sleep > } > } > > +#ifdef CONFIG_ACPI_GPE_WAKEUP > +static int acpi_gpe_pme_check(struct acpi_device *dev) acpi_gpe_wakeup_check() would be a better name IMO. Everywhere below too. > +{ > + struct device *ldev; > + > + ldev = acpi_get_physical_device(dev->handle); > + if (!ldev) > + return -ENODEV; > + /* > + * AML code might already clear the event, so ignore the return value. > + * Actually we can't correctly detect which device invokes GPE if the > + * event is cleared. > + */ I'm not sure what the comment is for. Does it mean we should ignore the result of the callback below? > + if (ldev->bus->pm && ldev->bus->pm->wakeup_event) Is ldev->bus guaranteed not to be NULL? > + ldev->bus->pm->wakeup_event(ldev); You're supposed to call pm_runtime_resume() in such cases (or pm_request_resume() in case it can't sleep). > + put_device(ldev); > + return 0; > +} > + > +static int acpi_gpe_pme_handler(struct notifier_block *nb, > + unsigned long type, void *data) > +{ > + int ret; > + acpi_handle handle = data; > + struct acpi_device *dev; > + > + if (type != ACPI_NOTIFY_DEVICE_WAKE) > + return NOTIFY_DONE; > + > + if (acpi_bus_get_device(handle, &dev)) > + return NOTIFY_DONE; > + > + ret = acpi_gpe_pme_check(dev); > + > + acpi_disable_gpe(dev->wakeup.gpe_device, dev->wakeup.gpe_number); I agree with Matthew on this. > + /* FIXME: spurious interrupt, disables it? */ Why FIXME? > + if (ret) > + printk(KERN_ERR"Spurious GPE %d detected\n", > + dev->wakeup.gpe_number); > + > + return NOTIFY_OK; > +} > + > +static struct notifier_block acpi_gpe_pme_nb = { > + .notifier_call = acpi_gpe_pme_handler, > +}; > + > +static void acpi_init_gpe_pme(void) > +{ > + register_acpi_bus_notifier(&acpi_gpe_pme_nb); > +} What exactly is the mechanism for invoking these notifiers? You don't seem to set up the GPE like the Matthew's code does. When and where is the GPE actually set up? > +#else > +static inline void acpi_init_gpe_pme(void) {} > +#endif > + > int __init acpi_wakeup_device_init(void) > { > struct list_head *node, *next; > @@ -144,5 +201,7 @@ int __init acpi_wakeup_device_init(void) > dev->wakeup.state.enabled = 1; > } > mutex_unlock(&acpi_device_lock); > + > + acpi_init_gpe_pme(); > return 0; > } Thanks, Rafael -- 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
Index: linux/drivers/acpi/Kconfig =================================================================== --- linux.orig/drivers/acpi/Kconfig 2009-08-05 11:00:06.000000000 +0800 +++ linux/drivers/acpi/Kconfig 2009-08-19 10:07:05.000000000 +0800 @@ -45,6 +45,15 @@ config ACPI_SLEEP depends on SUSPEND || HIBERNATION default y +config ACPI_GPE_WAKEUP + bool "ACPI wakeup event support" + depends on PM_SLEEP && EXPERIMENTAL + help + Enable ACPI to detect wakeup event. For example, PCI device can + invoke PME, and in ACPI platform, the PME will invoke a GPE. With + the option, we can detect which device invokes wakeup event. + + config ACPI_PROCFS bool "Deprecated /proc/acpi files" depends on PROC_FS Index: linux/drivers/acpi/wakeup.c =================================================================== --- linux.orig/drivers/acpi/wakeup.c 2009-07-24 11:31:05.000000000 +0800 +++ linux/drivers/acpi/wakeup.c 2009-08-19 10:16:27.000000000 +0800 @@ -124,6 +124,63 @@ void acpi_disable_wakeup_device(u8 sleep } } +#ifdef CONFIG_ACPI_GPE_WAKEUP +static int acpi_gpe_pme_check(struct acpi_device *dev) +{ + struct device *ldev; + + ldev = acpi_get_physical_device(dev->handle); + if (!ldev) + return -ENODEV; + /* + * AML code might already clear the event, so ignore the return value. + * Actually we can't correctly detect which device invokes GPE if the + * event is cleared. + */ + if (ldev->bus->pm && ldev->bus->pm->wakeup_event) + ldev->bus->pm->wakeup_event(ldev); + + put_device(ldev); + return 0; +} + +static int acpi_gpe_pme_handler(struct notifier_block *nb, + unsigned long type, void *data) +{ + int ret; + acpi_handle handle = data; + struct acpi_device *dev; + + if (type != ACPI_NOTIFY_DEVICE_WAKE) + return NOTIFY_DONE; + + if (acpi_bus_get_device(handle, &dev)) + return NOTIFY_DONE; + + ret = acpi_gpe_pme_check(dev); + + acpi_disable_gpe(dev->wakeup.gpe_device, dev->wakeup.gpe_number); + + /* FIXME: spurious interrupt, disables it? */ + if (ret) + printk(KERN_ERR"Spurious GPE %d detected\n", + dev->wakeup.gpe_number); + + return NOTIFY_OK; +} + +static struct notifier_block acpi_gpe_pme_nb = { + .notifier_call = acpi_gpe_pme_handler, +}; + +static void acpi_init_gpe_pme(void) +{ + register_acpi_bus_notifier(&acpi_gpe_pme_nb); +} +#else +static inline void acpi_init_gpe_pme(void) {} +#endif + int __init acpi_wakeup_device_init(void) { struct list_head *node, *next; @@ -144,5 +201,7 @@ int __init acpi_wakeup_device_init(void) dev->wakeup.state.enabled = 1; } mutex_unlock(&acpi_device_lock); + + acpi_init_gpe_pme(); return 0; }