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

[sashiko-bot@kernel.org]

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