[PATCH] ata: libata: preserve sata_spd link cache across ata_dev_init

[PATCH] ata: libata: preserve sata_spd link cache across ata_dev_init

[tongzhiwei]

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, which breaks
commit 2dc0b46b5ea3 ("libata: sata_down_spd_limit should return if
driver has not recorded sstatus speed"). The harm is, when the device
is plugged back, it is limited to low speed which will never be recovered.

This patch stops zeroing sata_spd in ata_dev_init (sata_spd was
introduced in commit 008a78961ec7 ("libata: improve SATA PHY speed down
logic")).  sata_spd is then written only on a successful reset.  At
struct ata_port's init time the sata_spd field is zeroed by kzalloc.

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>
---
 drivers/ata/libata-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index f482c0a6d7e9..2ddad2e6ebd2 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -5659,7 +5659,7 @@ void ata_dev_init(struct ata_device *dev)
 
 	/* SATA spd limit is bound to the attached device, reset together */
 	link->sata_spd_limit = link->hw_sata_spd_limit;
-	link->sata_spd = 0;
+	/* link->sata_spd caches last negotiated link speed; not zeroed here */
 
 	/* High bits of dev->flags are used to record warm plug
 	 * requests which occur asynchronously.  Synchronize using
-- 
2.43.0

Re: [PATCH] ata: libata: preserve sata_spd link cache across ata_dev_init

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] ata: state leakage of sata_spd across hotplugs breaks link speed downgrade
- [Medium] ata: libata: sata_spd patch fails to prevent 1.5 Gbps speed drop on replug
--

commit 2309da39e62c734fad9901e897e29150a0def7eb
Author: tongzhiwei <tongzhiwei@cmss.chinamobile.com>

ata: libata: preserve sata_spd link cache across ata_dev_init

This patch modifies ata_dev_init() to preserve the link->sata_spd cache
across device resets. The intent is to prevent the link speed limit from
being permanently lowered by failed probes when a device is unplugged and
later replugged.

> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 682c588897159..27217673fbb13 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -5659,7 +5659,7 @@ void ata_dev_init(struct ata_device *dev)
>  
>  	/* SATA spd limit is bound to the attached device, reset together */
>  	link->sata_spd_limit = link->hw_sata_spd_limit;
> -	link->sata_spd = 0;
> +	/* link->sata_spd caches last negotiated link speed; not zeroed here */

[Severity: High]
Does preserving link->sata_spd here leak the old device's speed state to a
newly hotplugged device?

If a Gen1 SATA device is unplugged, link->sata_spd remains 1. If a Gen3
device is then plugged in, its link->sata_spd_limit is reset to Gen3 here.
If the new device fails to come online at Gen3, error handling calls
sata_down_spd_limit() which consumes this leaked state:

drivers/ata/libata-sata.c:sata_down_spd_limit() {
	...
	if (rc == 0 && ata_sstatus_online(sstatus))
		spd = (sstatus >> 4) & 0xf;
	else
		spd = link->sata_spd;
	...
	if (spd > 1)
		mask &= (1 << (spd - 1)) - 1;
	else if (link->sata_spd)
		return -EINVAL;
	...
}

Could this return -EINVAL when spd is 1, and permanently prevent the limit
from being lowered to Gen2 for the new device, resulting in an infinite
loop of failed probes?

[Severity: Medium]
This problem wasn't introduced by this patch, but does this change actually
prevent the 1.5 Gbps speed drop on replug as intended?

The speed drop appears to be caused by dev->ering persisting across
ata_dev_init() because it is located after ATA_DEVICE_CLEAR_END in
struct ata_device. Probe failures during the unplug phase accumulate in
this uncleared ering.

When replugged, the accumulated errors cause ata_eh_schedule_probe() to
force the limit to 1.5 Gbps:

drivers/ata/libata-eh.c:ata_eh_schedule_probe() {
	...
	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);
	...
}

Does sata_down_spd_limit(link, 1) unconditionally force the limit to 1.5
Gbps regardless of whether link->sata_spd was preserved?

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