Re: [PATCH v2] ata: libata: skip sata_down_spd_limit() when no device is present

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] ata: libata: sata_down_spd_limit() DET == 0 check breaks link recovery
- [Medium] ata: libata: sata_down_spd_limit() DET check fails on replug and LPM
--

commit 2169b3134adb850a6a9a8662fa4f3cc24faf938a
Author: tongzhiwei <tongzhiwei@cmss.chinamobile.com>

ata: libata: skip sata_down_spd_limit() when no device is present

This commit changes sata_down_spd_limit() to return -EINVAL when reading
SCR_STATUS indicates that the SStatus DET field is 0, intending to prevent
probe attempts during a hot-unplug from lowering sata_spd_limit.

>     ata: libata: skip sata_down_spd_limit() when no device is present
>     
>     A hot-unplug goes through ata_eh_schedule_probe() -> ata_dev_init() ->
>     sata_spd = 0.  After commit 69f2c9346313 ("ata: libata: Fix
>     sata_down_spd_limit() when no link speed is reported"), when the device
>     is unplugged, every probe lowers sata_spd_limit.  This breaks
>     commit 2dc0b46b5ea3 ("libata: sata_down_spd_limit should return
>     if driver has not recorded sstatus speed").
>     
>     As a result, when the device is plugged back in, it is limited to a
>     low speed that is never recovered.
>     
>     Use the SStatus DET field to detect the no-device case and bail out
>     of sata_down_spd_limit() when no device is present (DET == 0).
>     
>     Fixes: 69f2c9346313 ("ata: libata: Fix sata_down_spd_limit() when no link speed is reported")
>     Assisted-by: LLM
>     Signed-off-by: tongzhiwei <tongzhiwei@cmss.chinamobile.com>

[Severity: Medium]
Is this description of how sata_spd_limit persists across hot-unplug probes
accurate, and does checking for (sstatus & 0xf) == 0 inside
sata_down_spd_limit() actually avoid the speed drop when a drive is plugged
back in?

At the beginning of every ata_eh_schedule_probe() call, ata_dev_init()
already resets link->sata_spd_limit back to link->hw_sata_spd_limit:

drivers/ata/libata-eh.c:ata_eh_schedule_probe()
    ata_eh_detach_dev(dev);
    ata_dev_init(dev);
    ...
    ata_ering_record(&dev->ering, 0, AC_ERR_OTHER);
    ata_ering_map(&dev->ering, ata_count_probe_trials_cb, &trials);

    if (trials > ATA_EH_PROBE_TRIALS)
        sata_down_spd_limit(link, 1);

What actually persists across ata_dev_init() is dev->ering, which is only
cleared once ata_dev_read_id() succeeds in ata_eh_revalidate_and_attach().

When a device is hot-unplugged and plugged back in within
ATA_EH_PROBE_TRIAL_INTERVAL (60 seconds), the unplugged probes accumulate
entries in dev->ering. On the subsequent plug-in probe, ata_eh_recover()
calls ata_eh_schedule_probe() before ata_eh_reset(). Because the drive is now
physically attached, SStatus.DET (sstatus & 0xf) is already 0x1 or 0x3 when
ata_eh_schedule_probe() increments trials past ATA_EH_PROBE_TRIALS (2) and
calls sata_down_spd_limit(link, 1). Doesn't (sstatus & 0xf) == 0 evaluate to
false on that plug-in probe and still force the reconnected drive down to
1.5 Gbps?

Also, what happens when Link Power Management (LPM) is enabled on an empty
port (such as ATA_LPM_MED_POWER_WITH_DIPM, ATA_LPM_MIN_POWER_WITH_PARTIAL, or
ATA_LPM_MIN_POWER)? In drivers/ata/libata-sata.c:sata_link_scr_lpm():

		} else {
			/* empty port, power off */
			scontrol &= ~0xf;
			scontrol |= (0x1 << 2);
		}

sata_link_scr_lpm() sets SControl.DET to 0x4, causing SStatus.DET to report
0x4 (PHY offline) instead of 0x0. Similarly, some controllers leave
SStatus.DET stuck at 0x1 after hot-unplugging, as noted in
sata_link_debounce(). In both cases, wouldn't (sstatus & 0xf) == 0 also be
bypassed while no device is present?

> diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
> index ad40f516c6d43..775da21cbbae7 100644
> --- a/drivers/ata/libata-sata.c
> +++ b/drivers/ata/libata-sata.c
> @@ -549,6 +549,9 @@ int sata_down_spd_limit(struct ata_link *link, u32 spd_limit)
>  	 * If not, use cached value in link->sata_spd.
>  	 */
>  	rc = sata_scr_read(link, SCR_STATUS, &sstatus);
> +	/* No device present (DET == 0): nothing to recover by lowering speed. */
> +	if (rc == 0 && (sstatus & 0xf) == 0)
> +		return -EINVAL;

[Severity: High]
Won't bailing out of sata_down_spd_limit() on an instantaneous
(sstatus & 0xf) == 0 read break link speed step-down recovery when a
connected device fails speed negotiation or drops its link?

For example, when SStatus.DET is oscillating (such as cycling between 0 and 1
when high-speed negotiation fails), sata_link_debounce() returns -EPIPE so
that the caller lowers the link speed:

drivers/ata/libata-sata.c:sata_link_debounce()
		/* Check deadline.  If debouncing failed, return
		 * -EPIPE to tell upper layer to lower link speed.
		 */
		if (time_after(jiffies, deadline))
			return -EPIPE;

drivers/ata/libata-eh.c:ata_eh_reset()
	} else if (rc == -EPIPE)
		sata_down_spd_limit(failed_link, 0);

Likewise, ata_eh_reset() calls sata_down_spd_limit(link, 0) when
try == max_tries - 1, ata_eh_schedule_probe() calls
sata_down_spd_limit(link, 1) to recover from failed link speed negotiations
that cause repeated DEVXCHG or PHRDY CHG events, and ata_eh_speed_down()
calls sata_down_spd_limit(link, 0) when a previously established link (with
link->sata_spd recorded) suffers errors and drops.

If sata_scr_read() in sata_down_spd_limit() samples SCR_STATUS while the
dropped or oscillating link has DET == 0, won't this check return -EINVAL and
prevent libata from stepping down link->sata_spd_limit to recover the device?

>  	if (rc == 0 && ata_sstatus_online(sstatus))
>  		spd = (sstatus >> 4) & 0xf;
>  	else

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261020032054.430-1-tongzhiwei@cmss.chinamobile.com?part=1