Messages in this thread | | | Subject | Re: [PATCH 5.6 regression fix 1/2] ACPI: PM: Add acpi_s2idle_register_wake_callback() | From | Hans de Goede <> | Date | Fri, 3 Apr 2020 12:27:42 +0200 |
| |
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
| |