[1/1,SRU,J/I/OEM-5.14] PM: s2idle: ACPI: Fix wakeup interrupts handling

From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>

BugLink: https://bugs.launchpad.net/bugs/1961739

After commit e3728b50cd9b ("ACPI: PM: s2idle: Avoid possible race
related to the EC GPE") wakeup interrupts occurring immediately after
the one discarded by acpi_s2idle_wake() may be missed.  Moreover, if
the SCI triggers again immediately after the rearming in
acpi_s2idle_wake(), that wakeup may be missed too.

The problem is that pm_system_irq_wakeup() only calls pm_system_wakeup()
when pm_wakeup_irq is 0, but that's not the case any more after the
interrupt causing acpi_s2idle_wake() to run until pm_wakeup_irq is
cleared by the pm_wakeup_clear() call in s2idle_loop().  However,
there may be wakeup interrupts occurring in that time frame and if
that happens, they will be missed.

To address that issue first move the clearing of pm_wakeup_irq to
the point at which it is known that the interrupt causing
acpi_s2idle_wake() to tun will be discarded, before rearming the SCI
for wakeup.  Moreover, because that only reduces the size of the
time window in which the issue may manifest itself, allow
pm_system_irq_wakeup() to register two second wakeup interrupts in
a row and, when discarding the first one, replace it with the second
one.  [Of course, this assumes that only one wakeup interrupt can be
discarded in one go, but currently that is the case and I am not
aware of any plans to change that.]

Fixes: e3728b50cd9b ("ACPI: PM: s2idle: Avoid possible race related to the EC GPE")
Cc: 5.4+ <stable@vger.kernel.org> # 5.4+
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
(cherry picked from commit cb1f65c1e1424a4b5e4a86da8aa3b8fd8459c8ec)
Signed-off-by: You-Sheng Yang <vicamo.yang@canonical.com>
---
 drivers/acpi/sleep.c        |  1 +
 drivers/base/power/wakeup.c | 41 ++++++++++++++++++++++++++++++-------
 include/linux/suspend.h     |  4 ++--
 kernel/power/main.c         |  5 ++++-
 kernel/power/process.c      |  2 +-
 kernel/power/suspend.c      |  2 --
 6 files changed, 42 insertions(+), 13 deletions(-)

On 24.02.22 08:02, You-Sheng Yang wrote:
> From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
> 
> BugLink: https://bugs.launchpad.net/bugs/1961739
> 
> After commit e3728b50cd9b ("ACPI: PM: s2idle: Avoid possible race
> related to the EC GPE") wakeup interrupts occurring immediately after
> the one discarded by acpi_s2idle_wake() may be missed.  Moreover, if
> the SCI triggers again immediately after the rearming in
> acpi_s2idle_wake(), that wakeup may be missed too.
> 
> The problem is that pm_system_irq_wakeup() only calls pm_system_wakeup()
> when pm_wakeup_irq is 0, but that's not the case any more after the
> interrupt causing acpi_s2idle_wake() to run until pm_wakeup_irq is
> cleared by the pm_wakeup_clear() call in s2idle_loop().  However,
> there may be wakeup interrupts occurring in that time frame and if
> that happens, they will be missed.
> 
> To address that issue first move the clearing of pm_wakeup_irq to
> the point at which it is known that the interrupt causing
> acpi_s2idle_wake() to tun will be discarded, before rearming the SCI
> for wakeup.  Moreover, because that only reduces the size of the
> time window in which the issue may manifest itself, allow
> pm_system_irq_wakeup() to register two second wakeup interrupts in
> a row and, when discarding the first one, replace it with the second
> one.  [Of course, this assumes that only one wakeup interrupt can be
> discarded in one go, but currently that is the case and I am not
> aware of any plans to change that.]
> 
> Fixes: e3728b50cd9b ("ACPI: PM: s2idle: Avoid possible race related to the EC GPE")
> Cc: 5.4+ <stable@vger.kernel.org> # 5.4+
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> (cherry picked from commit cb1f65c1e1424a4b5e4a86da8aa3b8fd8459c8ec)
> Signed-off-by: You-Sheng Yang <vicamo.yang@canonical.com>
Acked-by: Stefan Bader <stefan.bader@canonical.com>
> ---
>   drivers/acpi/sleep.c        |  1 +
>   drivers/base/power/wakeup.c | 41 ++++++++++++++++++++++++++++++-------
>   include/linux/suspend.h     |  4 ++--
>   kernel/power/main.c         |  5 ++++-
>   kernel/power/process.c      |  2 +-
>   kernel/power/suspend.c      |  2 --
>   6 files changed, 42 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
> index 3023224515ab..7ae09e4b4592 100644
> --- a/drivers/acpi/sleep.c
> +++ b/drivers/acpi/sleep.c
> @@ -767,6 +767,7 @@ bool acpi_s2idle_wake(void)
>   			return true;
>   		}
>   
> +		pm_wakeup_clear(acpi_sci_irq);
>   		rearm_wake_irq(acpi_sci_irq);
>   	}
>   
> diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
> index 99bda0da23a8..8666590201c9 100644
> --- a/drivers/base/power/wakeup.c
> +++ b/drivers/base/power/wakeup.c
> @@ -34,7 +34,8 @@ suspend_state_t pm_suspend_target_state;
>   bool events_check_enabled __read_mostly;
>   
>   /* First wakeup IRQ seen by the kernel in the last cycle. */
> -unsigned int pm_wakeup_irq __read_mostly;
> +static unsigned int wakeup_irq[2] __read_mostly;
> +static DEFINE_RAW_SPINLOCK(wakeup_irq_lock);
>   
>   /* If greater than 0 and the system is suspending, terminate the suspend. */
>   static atomic_t pm_abort_suspend __read_mostly;
> @@ -942,19 +943,45 @@ void pm_system_cancel_wakeup(void)
>   	atomic_dec_if_positive(&pm_abort_suspend);
>   }
>   
> -void pm_wakeup_clear(bool reset)
> +void pm_wakeup_clear(unsigned int irq_number)
>   {
> -	pm_wakeup_irq = 0;
> -	if (reset)
> +	raw_spin_lock_irq(&wakeup_irq_lock);
> +
> +	if (irq_number && wakeup_irq[0] == irq_number)
> +		wakeup_irq[0] = wakeup_irq[1];
> +	else
> +		wakeup_irq[0] = 0;
> +
> +	wakeup_irq[1] = 0;
> +
> +	raw_spin_unlock_irq(&wakeup_irq_lock);
> +
> +	if (!irq_number)
>   		atomic_set(&pm_abort_suspend, 0);
>   }
>   
>   void pm_system_irq_wakeup(unsigned int irq_number)
>   {
> -	if (pm_wakeup_irq == 0) {
> -		pm_wakeup_irq = irq_number;
> +	unsigned long flags;
> +
> +	raw_spin_lock_irqsave(&wakeup_irq_lock, flags);
> +
> +	if (wakeup_irq[0] == 0)
> +		wakeup_irq[0] = irq_number;
> +	else if (wakeup_irq[1] == 0)
> +		wakeup_irq[1] = irq_number;
> +	else
> +		irq_number = 0;
> +
> +	raw_spin_unlock_irqrestore(&wakeup_irq_lock, flags);
> +
> +	if (irq_number)
>   		pm_system_wakeup();
> -	}
> +}
> +
> +unsigned int pm_wakeup_irq(void)
> +{
> +	return wakeup_irq[0];
>   }
>   
>   /**
> diff --git a/include/linux/suspend.h b/include/linux/suspend.h
> index 8af13ba60c7e..7709998e3244 100644
> --- a/include/linux/suspend.h
> +++ b/include/linux/suspend.h
> @@ -505,14 +505,14 @@ extern void ksys_sync_helper(void);
>   
>   /* drivers/base/power/wakeup.c */
>   extern bool events_check_enabled;
> -extern unsigned int pm_wakeup_irq;
>   extern suspend_state_t pm_suspend_target_state;
>   
>   extern bool pm_wakeup_pending(void);
>   extern void pm_system_wakeup(void);
>   extern void pm_system_cancel_wakeup(void);
> -extern void pm_wakeup_clear(bool reset);
> +extern void pm_wakeup_clear(unsigned int irq_number);
>   extern void pm_system_irq_wakeup(unsigned int irq_number);
> +extern unsigned int pm_wakeup_irq(void);
>   extern bool pm_get_wakeup_count(unsigned int *count, bool block);
>   extern bool pm_save_wakeup_count(unsigned int count);
>   extern void pm_wakep_autosleep_enabled(bool set);
> diff --git a/kernel/power/main.c b/kernel/power/main.c
> index 44169f3081fd..7e646079fbeb 100644
> --- a/kernel/power/main.c
> +++ b/kernel/power/main.c
> @@ -504,7 +504,10 @@ static ssize_t pm_wakeup_irq_show(struct kobject *kobj,
>   					struct kobj_attribute *attr,
>   					char *buf)
>   {
> -	return pm_wakeup_irq ? sprintf(buf, "%u\n", pm_wakeup_irq) : -ENODATA;
> +	if (!pm_wakeup_irq())
> +		return -ENODATA;
> +
> +	return sprintf(buf, "%u\n", pm_wakeup_irq());
>   }
>   
>   power_attr_ro(pm_wakeup_irq);
> diff --git a/kernel/power/process.c b/kernel/power/process.c
> index 37401c99b7d7..ee78a39463e6 100644
> --- a/kernel/power/process.c
> +++ b/kernel/power/process.c
> @@ -134,7 +134,7 @@ int freeze_processes(void)
>   	if (!pm_freezing)
>   		atomic_inc(&system_freezing_cnt);
>   
> -	pm_wakeup_clear(true);
> +	pm_wakeup_clear(0);
>   	pr_info("Freezing user space processes ... ");
>   	pm_freezing = true;
>   	error = try_to_freeze_tasks(true);
> diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> index eb75f394a059..13d905dd3267 100644
> --- a/kernel/power/suspend.c
> +++ b/kernel/power/suspend.c
> @@ -138,8 +138,6 @@ static void s2idle_loop(void)
>   			break;
>   		}
>   
> -		pm_wakeup_clear(false);
> -
>   		s2idle_enter();
>   	}
>

On 24.02.22 08:02, You-Sheng Yang wrote:
> From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
> 
> BugLink: https://bugs.launchpad.net/bugs/1961739
> 
> After commit e3728b50cd9b ("ACPI: PM: s2idle: Avoid possible race
> related to the EC GPE") wakeup interrupts occurring immediately after
> the one discarded by acpi_s2idle_wake() may be missed.  Moreover, if
> the SCI triggers again immediately after the rearming in
> acpi_s2idle_wake(), that wakeup may be missed too.
> 
> The problem is that pm_system_irq_wakeup() only calls pm_system_wakeup()
> when pm_wakeup_irq is 0, but that's not the case any more after the
> interrupt causing acpi_s2idle_wake() to run until pm_wakeup_irq is
> cleared by the pm_wakeup_clear() call in s2idle_loop().  However,
> there may be wakeup interrupts occurring in that time frame and if
> that happens, they will be missed.
> 
> To address that issue first move the clearing of pm_wakeup_irq to
> the point at which it is known that the interrupt causing
> acpi_s2idle_wake() to tun will be discarded, before rearming the SCI
> for wakeup.  Moreover, because that only reduces the size of the
> time window in which the issue may manifest itself, allow
> pm_system_irq_wakeup() to register two second wakeup interrupts in
> a row and, when discarding the first one, replace it with the second
> one.  [Of course, this assumes that only one wakeup interrupt can be
> discarded in one go, but currently that is the case and I am not
> aware of any plans to change that.]
> 
> Fixes: e3728b50cd9b ("ACPI: PM: s2idle: Avoid possible race related to the EC GPE")
> Cc: 5.4+ <stable@vger.kernel.org> # 5.4+
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> (cherry picked from commit cb1f65c1e1424a4b5e4a86da8aa3b8fd8459c8ec)
> Signed-off-by: You-Sheng Yang <vicamo.yang@canonical.com>

Acked-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com>

Thanks

> ---
>   drivers/acpi/sleep.c        |  1 +
>   drivers/base/power/wakeup.c | 41 ++++++++++++++++++++++++++++++-------
>   include/linux/suspend.h     |  4 ++--
>   kernel/power/main.c         |  5 ++++-
>   kernel/power/process.c      |  2 +-
>   kernel/power/suspend.c      |  2 --
>   6 files changed, 42 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
> index 3023224515ab..7ae09e4b4592 100644
> --- a/drivers/acpi/sleep.c
> +++ b/drivers/acpi/sleep.c
> @@ -767,6 +767,7 @@ bool acpi_s2idle_wake(void)
>   			return true;
>   		}
>   
> +		pm_wakeup_clear(acpi_sci_irq);
>   		rearm_wake_irq(acpi_sci_irq);
>   	}
>   
> diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
> index 99bda0da23a8..8666590201c9 100644
> --- a/drivers/base/power/wakeup.c
> +++ b/drivers/base/power/wakeup.c
> @@ -34,7 +34,8 @@ suspend_state_t pm_suspend_target_state;
>   bool events_check_enabled __read_mostly;
>   
>   /* First wakeup IRQ seen by the kernel in the last cycle. */
> -unsigned int pm_wakeup_irq __read_mostly;
> +static unsigned int wakeup_irq[2] __read_mostly;
> +static DEFINE_RAW_SPINLOCK(wakeup_irq_lock);
>   
>   /* If greater than 0 and the system is suspending, terminate the suspend. */
>   static atomic_t pm_abort_suspend __read_mostly;
> @@ -942,19 +943,45 @@ void pm_system_cancel_wakeup(void)
>   	atomic_dec_if_positive(&pm_abort_suspend);
>   }
>   
> -void pm_wakeup_clear(bool reset)
> +void pm_wakeup_clear(unsigned int irq_number)
>   {
> -	pm_wakeup_irq = 0;
> -	if (reset)
> +	raw_spin_lock_irq(&wakeup_irq_lock);
> +
> +	if (irq_number && wakeup_irq[0] == irq_number)
> +		wakeup_irq[0] = wakeup_irq[1];
> +	else
> +		wakeup_irq[0] = 0;
> +
> +	wakeup_irq[1] = 0;
> +
> +	raw_spin_unlock_irq(&wakeup_irq_lock);
> +
> +	if (!irq_number)
>   		atomic_set(&pm_abort_suspend, 0);
>   }
>   
>   void pm_system_irq_wakeup(unsigned int irq_number)
>   {
> -	if (pm_wakeup_irq == 0) {
> -		pm_wakeup_irq = irq_number;
> +	unsigned long flags;
> +
> +	raw_spin_lock_irqsave(&wakeup_irq_lock, flags);
> +
> +	if (wakeup_irq[0] == 0)
> +		wakeup_irq[0] = irq_number;
> +	else if (wakeup_irq[1] == 0)
> +		wakeup_irq[1] = irq_number;
> +	else
> +		irq_number = 0;
> +
> +	raw_spin_unlock_irqrestore(&wakeup_irq_lock, flags);
> +
> +	if (irq_number)
>   		pm_system_wakeup();
> -	}
> +}
> +
> +unsigned int pm_wakeup_irq(void)
> +{
> +	return wakeup_irq[0];
>   }
>   
>   /**
> diff --git a/include/linux/suspend.h b/include/linux/suspend.h
> index 8af13ba60c7e..7709998e3244 100644
> --- a/include/linux/suspend.h
> +++ b/include/linux/suspend.h
> @@ -505,14 +505,14 @@ extern void ksys_sync_helper(void);
>   
>   /* drivers/base/power/wakeup.c */
>   extern bool events_check_enabled;
> -extern unsigned int pm_wakeup_irq;
>   extern suspend_state_t pm_suspend_target_state;
>   
>   extern bool pm_wakeup_pending(void);
>   extern void pm_system_wakeup(void);
>   extern void pm_system_cancel_wakeup(void);
> -extern void pm_wakeup_clear(bool reset);
> +extern void pm_wakeup_clear(unsigned int irq_number);
>   extern void pm_system_irq_wakeup(unsigned int irq_number);
> +extern unsigned int pm_wakeup_irq(void);
>   extern bool pm_get_wakeup_count(unsigned int *count, bool block);
>   extern bool pm_save_wakeup_count(unsigned int count);
>   extern void pm_wakep_autosleep_enabled(bool set);
> diff --git a/kernel/power/main.c b/kernel/power/main.c
> index 44169f3081fd..7e646079fbeb 100644
> --- a/kernel/power/main.c
> +++ b/kernel/power/main.c
> @@ -504,7 +504,10 @@ static ssize_t pm_wakeup_irq_show(struct kobject *kobj,
>   					struct kobj_attribute *attr,
>   					char *buf)
>   {
> -	return pm_wakeup_irq ? sprintf(buf, "%u\n", pm_wakeup_irq) : -ENODATA;
> +	if (!pm_wakeup_irq())
> +		return -ENODATA;
> +
> +	return sprintf(buf, "%u\n", pm_wakeup_irq());
>   }
>   
>   power_attr_ro(pm_wakeup_irq);
> diff --git a/kernel/power/process.c b/kernel/power/process.c
> index 37401c99b7d7..ee78a39463e6 100644
> --- a/kernel/power/process.c
> +++ b/kernel/power/process.c
> @@ -134,7 +134,7 @@ int freeze_processes(void)
>   	if (!pm_freezing)
>   		atomic_inc(&system_freezing_cnt);
>   
> -	pm_wakeup_clear(true);
> +	pm_wakeup_clear(0);
>   	pr_info("Freezing user space processes ... ");
>   	pm_freezing = true;
>   	error = try_to_freeze_tasks(true);
> diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> index eb75f394a059..13d905dd3267 100644
> --- a/kernel/power/suspend.c
> +++ b/kernel/power/suspend.c
> @@ -138,8 +138,6 @@ static void s2idle_loop(void)
>   			break;
>   		}
>   
> -		pm_wakeup_clear(false);
> -
>   		s2idle_enter();
>   	}
>