Messages in this thread | | | Subject | Re: [PATCH v2 1/2] ACPI: PM: Add acpi_[un]register_wakeup_handler() | From | Hans de Goede <> | Date | Fri, 3 Apr 2020 17:00:11 +0200 |
| |
Hi,
On 4/3/20 4:17 PM, Andy Shevchenko wrote: > On Fri, Apr 3, 2020 at 1:52 PM Hans de Goede <hdegoede@redhat.com> wrote: >> >> Since commit fdde0ff8590b ("ACPI: PM: s2idle: Prevent spurious SCIs from >> waking up the system") the SCI triggering without there being a wakeup >> cause recognized by the ACPI sleep code will no longer wakeup the system. >> >> This works as intended, but this is a problem for devices where the SCI >> is shared with another device which is also a wakeup source. >> >> In the past these, from the pov of the ACPI sleep code, spurious SCIs >> would still cause a wakeup so the wakeup from the device sharing the >> interrupt would actually wakeup the system. This now no longer works. >> >> This is a problem on e.g. Bay Trail-T and Cherry Trail devices where >> some peripherals (typically the XHCI controller) can signal a >> Power Management Event (PME) to the Power Management Controller (PMC) >> to wakeup the system, this uses the same interrupt as the SCI. >> These wakeups are handled through a special INT0002 ACPI device which >> checks for events in the GPE0a_STS for this and takes care of acking >> the PME so that the shared interrupt stops triggering. >> >> The change to the ACPI sleep code to ignore the spurious SCI, causes >> the system to no longer wakeup on these PME events. To make things >> worse this means that the INT0002 device driver interrupt handler will >> no longer run, causing the PME to not get cleared and resulting in the >> system hanging. Trying to wakeup the system after such a PME through e.g. >> the power button no longer works. >> >> Add an acpi_register_wakeup_handler() function which registers >> a handler to be called from acpi_s2idle_wake() and when the handler >> returns true, return true from acpi_s2idle_wake(). >> >> The INT0002 driver will use this mechanism to check the GPE0a_STS >> register from acpi_s2idle_wake() and to tell the system to wakeup >> if a PME is signaled in the register. >> > > Something happened to your editor settings? Some lines looks like too short... > >> Fixes: fdde0ff8590b ("ACPI: PM: s2idle: Prevent spurious SCIs from waking up the system") >> Cc: 5.4+ <stable@vger.kernel.org> # 5.4+ >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> Changes in v2: >> - Move the new helpers to drivers/acpi/wakeup.c >> - Rename the helpers to acpi_[un]register_wakeup_handler(), also give some >> types/variables better names >> --- >> drivers/acpi/sleep.c | 4 +++ >> drivers/acpi/sleep.h | 1 + >> drivers/acpi/wakeup.c | 82 +++++++++++++++++++++++++++++++++++++++++++ >> include/linux/acpi.h | 5 +++ >> 4 files changed, 92 insertions(+) >> >> diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c >> index e5f95922bc21..dc8c71c47285 100644 >> --- a/drivers/acpi/sleep.c >> +++ b/drivers/acpi/sleep.c >> @@ -1025,6 +1025,10 @@ static bool acpi_s2idle_wake(void) >> if (acpi_any_gpe_status_set() && !acpi_ec_dispatch_gpe()) >> return true; >> >> + /* Check wakeups from drivers sharing the SCI. */ >> + if (acpi_check_wakeup_handlers()) >> + return true; >> + >> /* >> * Cancel the wakeup and process all pending events in case >> * there are any wakeup ones in there. >> diff --git a/drivers/acpi/sleep.h b/drivers/acpi/sleep.h >> index 41675d24a9bc..3d90480ce1b1 100644 >> --- a/drivers/acpi/sleep.h >> +++ b/drivers/acpi/sleep.h >> @@ -2,6 +2,7 @@ >> >> extern void acpi_enable_wakeup_devices(u8 sleep_state); >> extern void acpi_disable_wakeup_devices(u8 sleep_state); >> +extern bool acpi_check_wakeup_handlers(void); >> >> extern struct list_head acpi_wakeup_device_list; >> extern struct mutex acpi_device_lock; >> diff --git a/drivers/acpi/wakeup.c b/drivers/acpi/wakeup.c >> index 9614126bf56e..de0f8e626c1c 100644 >> --- a/drivers/acpi/wakeup.c >> +++ b/drivers/acpi/wakeup.c >> @@ -12,6 +12,15 @@ >> #include "internal.h" >> #include "sleep.h" >> >> +struct acpi_wakeup_handler { >> + struct list_head list_node; >> + bool (*wakeup)(void *context); >> + void *context; >> +}; >> + >> +static LIST_HEAD(acpi_wakeup_handler_head); >> +static DEFINE_MUTEX(acpi_wakeup_handler_mutex); >> + >> /* >> * We didn't lock acpi_device_lock in the file, because it invokes oops in >> * suspend/resume and isn't really required as this is called in S-state. At >> @@ -96,3 +105,76 @@ int __init acpi_wakeup_device_init(void) >> mutex_unlock(&acpi_device_lock); >> return 0; >> } >> + >> +/** >> + * acpi_register_wakeup_handler - Register wakeup handler >> + * @wake_irq: The IRQ through which the device may receive wakeups >> + * @wakeup: Wakeup-handler to call when the SCI has triggered a wakeup >> + * @context: Context to pass to the handler when calling it >> + * >> + * Drivers which may share an IRQ with the SCI can use this to register >> + * a handler which returns true when the device they are managing wants >> + * to trigger a wakeup. >> + */ > >> +int acpi_register_wakeup_handler( > > ...this one... > >> + int wake_irq, bool (*wakeup)(void *context), void *context)
As the changelog states, so variables were renamed, which makes this a bit shorter then before. Note it still does not fit in a single line, but I can put the second parameter after the ( now, which does look better. Will fix for v3.
>> +{ >> + struct acpi_wakeup_handler *handler; >> + >> + /* >> + * If the device is not sharing its IRQ with the SCI, there is no >> + * need to register the handler. >> + */ >> + if (!acpi_sci_irq_valid() || wake_irq != acpi_sci_irq) >> + return 0; >> + >> + handler = kmalloc(sizeof(*handler), GFP_KERNEL); >> + if (!handler) >> + return -ENOMEM; >> + >> + handler->wakeup = wakeup; >> + handler->context = context; >> + >> + mutex_lock(&acpi_wakeup_handler_mutex); >> + list_add(&handler->list_node, &acpi_wakeup_handler_head); >> + mutex_unlock(&acpi_wakeup_handler_mutex); >> + >> + return 0; >> +} >> +EXPORT_SYMBOL_GPL(acpi_register_wakeup_handler); >> + >> +/** >> + * acpi_unregister_wakeup_handler - Unregister wakeup handler >> + * @wakeup: Wakeup-handler passed to acpi_register_wakeup_handler() >> + * @context: Context passed to acpi_register_wakeup_handler() >> + */ > >> +void acpi_unregister_wakeup_handler( >> + bool (*wakeup)(void *context), void *context) > > Not sure, but looks like short.
Same as above.
> >> +{ >> + struct acpi_wakeup_handler *handler; >> + >> + mutex_lock(&acpi_wakeup_handler_mutex); >> + list_for_each_entry(handler, &acpi_wakeup_handler_head, list_node) { > >> + if (handler->wakeup == wakeup && >> + handler->context == context) { > > Ditto.
This one now actually fits on a single line (it is 79 chars then) will also fix for v3.
Regards,
Hans
> >> + list_del(&handler->list_node); >> + kfree(handler); >> + break; >> + } >> + } >> + mutex_unlock(&acpi_wakeup_handler_mutex); >> +} >> +EXPORT_SYMBOL_GPL(acpi_unregister_wakeup_handler); >> + >> +bool acpi_check_wakeup_handlers(void) >> +{ >> + struct acpi_wakeup_handler *handler; >> + >> + /* No need to lock, nothing else is running when we're called. */ >> + list_for_each_entry(handler, &acpi_wakeup_handler_head, list_node) { >> + if (handler->wakeup(handler->context)) >> + return true; >> + } >> + >> + return false; >> +} >> diff --git a/include/linux/acpi.h b/include/linux/acpi.h >> index 0f24d701fbdc..efac0f9c01a2 100644 >> --- a/include/linux/acpi.h >> +++ b/include/linux/acpi.h >> @@ -488,6 +488,11 @@ void __init acpi_nvs_nosave_s3(void); >> void __init acpi_sleep_no_blacklist(void); >> #endif /* CONFIG_PM_SLEEP */ >> >> +int acpi_register_wakeup_handler( >> + int wake_irq, bool (*wakeup)(void *context), void *context); >> +void acpi_unregister_wakeup_handler( >> + bool (*wakeup)(void *context), void *context); >> + >> struct acpi_osc_context { >> char *uuid_str; /* UUID string */ >> int rev; >> -- >> 2.26.0 >> > >
| |