[PATCH] ata: libahci: Stop the SW activity blink timers on port suspend

[PATCH] ata: libahci: Stop the SW activity blink timers on port suspend

[Niklas Cassel]

The SW activity blink timers are not stopped when a port is suspended.
Every command goes through ahci_qc_issue() -> ahci_sw_activity(), so
with SW activity enabled, a blink timer can still be pending when
ahci_port_suspend() runs, e.g. armed by the commands issued just before
it, and ahci_sw_activity_blink() re-arms it as long as there was
activity.

If the timer fires while the port is suspended, ahci_sw_activity_blink()
-> ahci_transmit_led_message() -> ahci_rpm_get_port() calls
pm_runtime_get_sync() from timer softirq context, and the EM registers
of a suspended controller get written. If the port is resumed while the
timer is still pending, ahci_port_resume() -> ahci_start_port() ->
ahci_init_sw_activity() calls timer_setup() on a timer that is still
queued in the timer wheel, corrupting it.

Stop the timers in ahci_port_suspend(). Use timer_delete_sync() rather
than timer_shutdown_sync(): the timers are only stopped until the port
is resumed, while timer_shutdown_sync() is meant for timers that are
torn down for good.

Fixes: 18f7ba4c2f4b ("libata/ahci: enclosure management support")
Cc: stable@vger.kernel.org
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
Developed with the assistance of an LLM.

Tested in QEMU (q35, KVM) with KASAN, UBSAN, DEBUG_OBJECTS_TIMERS and
lockdep, using TEST-ONLY hooks: one fakes enclosure management (QEMU's
AHCI has none), the other arms the blink timer as ahci_qc_issue() does
and then calls ->port_suspend() and ->port_resume() back to back, while
the timer is still pending.

  unpatched                              patched
  ODEBUG: init active ... timer_list     no warnings,
  hint: ahci_sw_activity_blink,          I/O after resume works
  from ahci_port_resume()

The timer firing while the port is suspended is not covered, as the
fake EM support never reaches ahci_rpm_get_port().

 drivers/ata/libahci.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index ef760d81d1d1..2e71ba7ea374 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -2491,8 +2491,21 @@ static void ahci_handle_s2idle(struct ata_port *ap)
 
 static int ahci_port_suspend(struct ata_port *ap, pm_message_t mesg)
 {
+	struct ahci_port_priv *pp = ap->private_data;
 	const char *emsg = NULL;
-	int rc;
+	int rc, i;
+
+	/*
+	 * Stop the SW activity blink timers, so that they cannot fire while
+	 * the port is suspended. ahci_init_sw_activity() sets them up again
+	 * on resume.
+	 */
+	for (i = 0; i < EM_MAX_SLOTS; i++) {
+		struct ahci_em_priv *emp = &pp->em_priv[i];
+
+		if (emp->timer.function)
+			timer_delete_sync(&emp->timer);
+	}
 
 	rc = ahci_deinit_port(ap, &emsg);
 	if (rc == 0)
-- 
2.56.0

Re: [PATCH] ata: libahci: Stop the SW activity blink timers on port suspend

[Damien Le Moal]

On 11/10/26 22:56, Niklas Cassel wrote:
> The SW activity blink timers are not stopped when a port is suspended.
> Every command goes through ahci_qc_issue() -> ahci_sw_activity(), so
> with SW activity enabled, a blink timer can still be pending when
> ahci_port_suspend() runs, e.g. armed by the commands issued just before
> it, and ahci_sw_activity_blink() re-arms it as long as there was
> activity.
> 
> If the timer fires while the port is suspended, ahci_sw_activity_blink()
> -> ahci_transmit_led_message() -> ahci_rpm_get_port() calls
> pm_runtime_get_sync() from timer softirq context, and the EM registers
> of a suspended controller get written. If the port is resumed while the
> timer is still pending, ahci_port_resume() -> ahci_start_port() ->
> ahci_init_sw_activity() calls timer_setup() on a timer that is still
> queued in the timer wheel, corrupting it.
> 
> Stop the timers in ahci_port_suspend(). Use timer_delete_sync() rather
> than timer_shutdown_sync(): the timers are only stopped until the port
> is resumed, while timer_shutdown_sync() is meant for timers that are
> torn down for good.
> 
> Fixes: 18f7ba4c2f4b ("libata/ahci: enclosure management support")
> Cc: stable@vger.kernel.org
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> ---
> Developed with the assistance of an LLM.
> 
> Tested in QEMU (q35, KVM) with KASAN, UBSAN, DEBUG_OBJECTS_TIMERS and
> lockdep, using TEST-ONLY hooks: one fakes enclosure management (QEMU's
> AHCI has none), the other arms the blink timer as ahci_qc_issue() does
> and then calls ->port_suspend() and ->port_resume() back to back, while
> the timer is still pending.
> 
>   unpatched                              patched
>   ODEBUG: init active ... timer_list     no warnings,
>   hint: ahci_sw_activity_blink,          I/O after resume works
>   from ahci_port_resume()
> 
> The timer firing while the port is suspended is not covered, as the
> fake EM support never reaches ahci_rpm_get_port().
> 
>  drivers/ata/libahci.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
> index ef760d81d1d1..2e71ba7ea374 100644
> --- a/drivers/ata/libahci.c
> +++ b/drivers/ata/libahci.c
> @@ -2491,8 +2491,21 @@ static void ahci_handle_s2idle(struct ata_port *ap)
>  
>  static int ahci_port_suspend(struct ata_port *ap, pm_message_t mesg)
>  {
> +	struct ahci_port_priv *pp = ap->private_data;
>  	const char *emsg = NULL;
> -	int rc;
> +	int rc, i;
> +
> +	/*
> +	 * Stop the SW activity blink timers, so that they cannot fire while
> +	 * the port is suspended. ahci_init_sw_activity() sets them up again
> +	 * on resume.
> +	 */
> +	for (i = 0; i < EM_MAX_SLOTS; i++) {
> +		struct ahci_em_priv *emp = &pp->em_priv[i];
> +
> +		if (emp->timer.function)
> +			timer_delete_sync(&emp->timer);

Looks OK. Did you check that the timer handler does not restart/requeue the
timer? timer_delete_sync() states that it should not.

> +	}
>  
>  	rc = ahci_deinit_port(ap, &emsg);
>  	if (rc == 0)


-- 
Damien Le Moal
Western Digital Research