On 07.09.2022 22:26, Rafael J. Wysocki wrote:
> On Wed, Sep 7, 2022 at 7:31 AM Marek Maślanka <mm@xxxxxxxxxxxx> wrote:
>>
>> On the wake-up, the ACPI GPE that are marked as a wakeup source are
>> turned off. Before turning off, the kernel waits for the currently
>> processing IRQ to finish and assumes that this is an ACPI interrupt that
>> triggered wake-up. In the case the first interrupt after wake-up is not
>> an ACPI interrupt, this might cause the ACPI GPE not to be processed
>> because it will be disabled.
>>
>> The patch makes sure that an ACPI interrupt is processed before
>> disabling GPE that are wakeup sources.
>>
>> This patch fix the issue that is seen on low-end Chromebooks with two
>> cores CPU when HPET IRQ is triggered while resuming the device and is
>> processed before the ACPI GPE interrupt on the same CPU core.
>>
>> Signed-off-by: Marek Maslanka <mm@xxxxxxxxxxxx>
>> ---
>> drivers/acpi/internal.h | 2 ++
>> drivers/acpi/osl.c | 18 ++++++++++++++++++
>> drivers/acpi/sleep.c | 6 ++++++
>> include/linux/acpi.h | 2 ++
>> 4 files changed, 28 insertions(+)
>>
>> diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
>> index 219c02df9a08..e4df1bf2963b 100644
>> --- a/drivers/acpi/internal.h
>> +++ b/drivers/acpi/internal.h
>> @@ -251,6 +251,8 @@ static inline bool force_storage_d3(void)
>> }
>> #endif
>>
>> +extern bool acpi_resume_gpe_irq_handled;
>> +
>> /*--------------------------------------------------------------------------
>> Device properties
>> -------------------------------------------------------------------------- */
>> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
>> index 3269a888fb7a..ea587ac4c68a 100644
>> --- a/drivers/acpi/osl.c
>> +++ b/drivers/acpi/osl.c
>> @@ -42,6 +42,8 @@
>> #define _COMPONENT ACPI_OS_SERVICES
>> ACPI_MODULE_NAME("osl");
>>
>> +#define ACPI_WAIT_FOR_RESUME_GPE_IRQ_MS 500
>
> This is a relatively large number. Doesn't anything smaller work?
>
Smaller values like 5ms also work. I just add extra time just in case. This
long time affects the wakeup time only if something really bad happened, ie.
ACPI interrupt is significantly delayed.
>> +
>> struct acpi_os_dpc {
>> acpi_osd_exec_callback function;
>> void *context;
>> @@ -69,6 +71,8 @@ static struct workqueue_struct *kacpi_hotplug_wq;
>> static bool acpi_os_initialized;
>> unsigned int acpi_sci_irq = INVALID_ACPI_IRQ;
>> bool acpi_permanent_mmap = false;
>> +bool acpi_resume_gpe_irq_handled;
>> +static DECLARE_WAIT_QUEUE_HEAD(acpi_irq_after_suspend_wait);
>>
>> /*
>> * This list of permanent mappings is for memory that may be accessed from
>> @@ -549,6 +553,10 @@ static irqreturn_t acpi_irq(int irq, void *dev_id)
>> handled = (*acpi_irq_handler) (acpi_irq_context);
>>
>> if (handled) {
>> + if (acpi_s2idle_wakeup()) {
>> + acpi_resume_gpe_irq_handled = true;
>> + wake_up(&acpi_irq_after_suspend_wait);
>> + }
>> acpi_irq_handled++;
>> return IRQ_HANDLED;
>> } else {
>> @@ -1768,3 +1776,13 @@ acpi_status acpi_os_enter_sleep(u8 sleep_state,
>> reg_a_value, reg_b_value);
>> return status;
>> }
>> +
>> +int acpi_wait_for_resume_gpe_irq(void)
>> +{
>> + int wait_ms = msecs_to_jiffies(ACPI_WAIT_FOR_RESUME_GPE_IRQ_MS);
>> + int timeout = wait_event_timeout(acpi_irq_after_suspend_wait,
>> + acpi_resume_gpe_irq_handled,
>> + wait_ms);
>> + return timeout;
>
> So this will wait for the full timeout if there are no ACPI events to
> process, right?
>
> Isn't it a bit excessive?
>
Yes. It waits full time if there is no ACPI interrupt
> Moreover, what if there is an ACPI event right after the wait has completed?
Further processing just disables the GPE, and GPE won't be processed. The same
case as we have now.
>> +}
>> +EXPORT_SYMBOL(acpi_wait_for_resume_gpe_irq);
>> diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
>> index ad4b2987b3d6..6ddf28067687 100644
>> --- a/drivers/acpi/sleep.c
>> +++ b/drivers/acpi/sleep.c
>> @@ -721,6 +721,7 @@ int acpi_s2idle_prepare(void)
>> acpi_os_wait_events_complete();
>>
>> s2idle_wakeup = true;
>> + acpi_resume_gpe_irq_handled = false;
>> return 0;
>> }
>>
>> @@ -789,6 +790,7 @@ bool acpi_s2idle_wake(void)
>>
>> void acpi_s2idle_restore(void)
>> {
>> + int timeout;
>> /*
>> * Drain pending events before restoring the working-state configuration
>> * of GPEs.
>> @@ -797,6 +799,10 @@ void acpi_s2idle_restore(void)
>> acpi_ec_flush_work(); /* flush the EC driver's workqueues */
>> acpi_os_wait_events_complete(); /* synchronize Notify handling */
>>
>> + timeout = acpi_wait_for_resume_gpe_irq();
>> + if (timeout == 0)
>> + pr_warn("Failed to wait for ACPI interrupt after resume");
>
> This is perfectly fine, for example if the system is woken up by a
> non-ACPI IRQ (eg. the keyboard one).
>
Right, I wrongly assumed that the wakeup source can be only an ACPI GPE
interrupt.
>> +
>> s2idle_wakeup = false;
>>
>> acpi_enable_all_runtime_gpes();
>> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
>> index ed4aa395cc49..ef2ab7990f1c 100644
>> --- a/include/linux/acpi.h
>> +++ b/include/linux/acpi.h
>> @@ -1475,4 +1475,6 @@ static inline void acpi_device_notify(struct device *dev) { }
>> static inline void acpi_device_notify_remove(struct device *dev) { }
>> #endif
>>
>> +int acpi_wait_for_resume_gpe_irq(void);
>> +
>> #endif /*_LINUX_ACPI_H*/
>> --
>
> Overall, I'm not convinced about the approach.
>
> If the goal is to process all of the ACPI wakeup interrupts, this
> doesn't meet it.
The goal is to process the APCI interrupt that woke-up the system, before
disabling GPE. This will assure the GPE handlers will be invoked and GPE status
bit will be cleared.
I just realised that it should wait for an interrupt only if the wakeup event
was an ACPI non-EC GPE interrupt.
> AFAICS the only way to do that which is not racy is to dispatch every
> wakeup GPE in-line (from the ACPICA code) before disabling it in case
> it is active at that point.
TBH I thought about a similar solution but from my (acpica non-expert) point of
view the patch will be more complex and it needs more effort than kernel patch.
But I might be wrong.
