Re: [PATCH 5.6 regression fix 1/2] ACPI: PM: Add acpi_s2idle_register_wake_callback()

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 4/1/20 9:09 PM, Rafael J. Wysocki wrote:
> On Wednesday, April 1, 2020 8:26:16 PM CEST Hans de Goede wrote:
>> Hi,
>>
>> On 4/1/20 6:32 PM, Rafael J. Wysocki wrote:
>>> On Mon, Mar 30, 2020 at 12:34 AM 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_s2idle_register_wake_callback() function which registers
>>>> a callback to be called from acpi_s2idle_wake() and when the callback
>>>> 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.
>>>>
>>>> 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>
>>>
>>> I generally agree with the approach, but I would make some, mostly
>>> cosmetic, changes.
>>>
>>> First off, I'd put the new code into drivers/acpi/wakeup.c.
>>>
>>> I'd export one function from there to be called from
>>> acpi_s2idle_wake() and the install/uninstall routines for the users.
>>
>> Ok.
>>
>>>> ---
>>>>    drivers/acpi/sleep.c | 70 ++++++++++++++++++++++++++++++++++++++++++++
>>>>    include/linux/acpi.h |  7 +++++
>>>>    2 files changed, 77 insertions(+)
>>>>
>>>> diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
>>>> index e5f95922bc21..e360e51afa8e 100644
>>>> --- a/drivers/acpi/sleep.c
>>>> +++ b/drivers/acpi/sleep.c
>>>> @@ -943,6 +943,65 @@ static struct acpi_scan_handler lps0_handler = {
>>>>           .attach = lps0_device_attach,
>>>>    };
>>>>
>>>> +struct s2idle_wake_callback {
>>>
>>> I'd call this acpi_wakeup_handler.
>>>
>>>> +       struct list_head list;
>>>
>>> list_node?
>>>
>>>> +       bool (*function)(void *data);
>>>
>>> bool (*wakeup)(void *context)?
>>>
>>>> +       void *user_data;
>>>
>>> context?
>>
>> Sure (for all of the above).
>>
>>>
>>>> +};
>>>> +
>>>> +static LIST_HEAD(s2idle_wake_callback_head);
>>>> +static DEFINE_MUTEX(s2idle_wake_callback_mutex);
>>>> +
>>>> +/*
>>>> + * Drivers which may share an IRQ with the SCI can use this to register
>>>> + * a callback which returns true when the device they are managing wants
>>>> + * to trigger a wakeup.
>>>> + */
>>>> +int acpi_s2idle_register_wake_callback(
>>>> +       int wake_irq, bool (*function)(void *data), void *user_data)
>>>> +{
>>>> +       struct s2idle_wake_callback *callback;
>>>> +
>>>> +       /*
>>>> +        * If the device is not sharing its IRQ with the SCI, there is no
>>>> +        * need to register the callback.
>>>> +        */
>>>> +       if (!acpi_sci_irq_valid() || wake_irq != acpi_sci_irq)
>>>> +               return 0;
>>>> +
>>>> +       callback = kmalloc(sizeof(*callback), GFP_KERNEL);
>>>> +       if (!callback)
>>>> +               return -ENOMEM;
>>>> +
>>>> +       callback->function = function;
>>>> +       callback->user_data = user_data;
>>>> +
>>>> +       mutex_lock(&s2idle_wake_callback_mutex);
>>>> +       list_add(&callback->list, &s2idle_wake_callback_head);
>>>> +       mutex_unlock(&s2idle_wake_callback_mutex);
>>>> +
>>>> +       return 0;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(acpi_s2idle_register_wake_callback);
>>>> +
>>>> +void acpi_s2idle_unregister_wake_callback(
>>>> +       bool (*function)(void *data), void *user_data)
>>>> +{
>>>> +       struct s2idle_wake_callback *cb;
>>>> +
>>>> +       mutex_lock(&s2idle_wake_callback_mutex);
>>>> +       list_for_each_entry(cb, &s2idle_wake_callback_head, list) {
>>>> +               if (cb->function == function &&
>>>> +                   cb->user_data == user_data) {
>>>> +                       list_del(&cb->list);
>>>> +                       kfree(cb);
>>>> +                       break;
>>>> +               }
>>>> +       }
>>>> +       mutex_unlock(&s2idle_wake_callback_mutex);
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(acpi_s2idle_unregister_wake_callback);
>>>> +
>>>>    static int acpi_s2idle_begin(void)
>>>>    {
>>>>           acpi_scan_lock_acquire();
>>>> @@ -992,6 +1051,8 @@ static void acpi_s2idle_sync(void)
>>>>
>>>>    static bool acpi_s2idle_wake(void)
>>>>    {
>>>> +       struct s2idle_wake_callback *cb;
>>>> +
>>>>           if (!acpi_sci_irq_valid())
>>>>                   return pm_wakeup_pending();
>>>>
>>>> @@ -1025,6 +1086,15 @@ static bool acpi_s2idle_wake(void)
>>>>                   if (acpi_any_gpe_status_set() && !acpi_ec_dispatch_gpe())
>>>>                           return true;
>>>>
>>>> +               /*
>>>> +                * Check callbacks registered by drivers sharing the SCI.
>>>> +                * Note no need to lock, nothing else is running.
>>>> +                */
>>>> +               list_for_each_entry(cb, &s2idle_wake_callback_head, list) {
>>>> +                       if (cb->function(cb->user_data))
>>>> +                               return true;
>>>> +               }
>>>
>>> AFAICS this needs to be done in acpi_s2idle_restore() too to clear the
>>> status bits in case one of these wakeup sources triggers along with a
>>> GPE or a fixed event and the other one wins the race.
>>
>> The "wakeup" callback does not actually clear the interrupt source, just like
>> for normal interrupts it relies on the actual interrupt handling (which at this
>> point is still suspended) to do this.
> 
> Of course, you are right, sorry for the confusion.
> 
> What I meant was that the interrupt handler needed to run in acpi_s2idle_restore(),
> but that should be taken care of the acpi_os_wait_events_complete() in there
> which synchronizes the SCI among other things.

Ok, I've prepared a v2 with the other discussed changes. I'll give it a
quick test and then send it out.

Regards,

Hans