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

[Damien Le Moal <dlemoal@kernel.org>]

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