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

[Rafael J. Wysocki]

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@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx> # 5.4+
> >> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> > 
> > 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.

Thanks!