| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 3 Apr 2020 17:00:11 +0200
From: Hans de Goede <hdegoede@...hat.com>
To: Andy Shevchenko <andy.shevchenko@...il.com>
Cc: "Rafael J . Wysocki" <rjw@...ysocki.net>,
Len Brown <lenb@...nel.org>,
Darren Hart <dvhart@...radead.org>,
Andy Shevchenko <andy@...radead.org>,
ACPI Devel Maling List <linux-acpi@...r.kernel.org>,
Platform Driver <platform-driver-x86@...r.kernel.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
"5 . 4+" <stable@...r.kernel.org>
Subject: Re: [PATCH v2 1/2] ACPI: PM: Add acpi_[un]register_wakeup_handler()
Hi,
On 4/3/20 4:17 PM, Andy Shevchenko wrote:
> On Fri, Apr 3, 2020 at 1:52 PM Hans de Goede <hdegoede@...hat.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@...r.kernel.org> # 5.4+
>> Signed-off-by: Hans de Goede <hdegoede@...hat.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
>>
>
>
Powered by blists - more mailing lists