[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

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

[Damien Le Moal]

On 2026/10/10 9:54, tongzhiwei wrote:
> 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 */

This contradicts the previous comment, so let's make this consistent. E.g.
something like:

	/*
	 * The SATA speed limit is bound to the attached device, so reset it
	 * but keep the last negotiated speed indicated by link->sata_spd.
	 */

to replace the previous comment.

>  
>  	/* High bits of dev->flags are used to record warm plug
>  	 * requests which occur asynchronously.  Synchronize using


-- 
Damien Le Moal
Western Digital Research

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

[Damien Le Moal]

On 2026/10/15 7:30, Damien Le Moal wrote:
> On 2026/10/10 9:54, tongzhiwei wrote:
>> 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 */
> 
> This contradicts the previous comment, so let's make this consistent. E.g.
> something like:
> 
> 	/*
> 	 * The SATA speed limit is bound to the attached device, so reset it
> 	 * but keep the last negotiated speed indicated by link->sata_spd.
> 	 */
> 
> to replace the previous comment.
> 
>>  
>>  	/* High bits of dev->flags are used to record warm plug
>>  	 * requests which occur asynchronously.  Synchronize using

Looking at the Sashiko comment on this patch, I think it is valid and should be
addressed as well.



-- 
Damien Le Moal
Western Digital Research

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

[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.  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>
---
Changes since v1:
- v1 ("ata: libata: preserve sata_spd link cache across ata_dev_init") took the
  wrong approach (caching sata_spd across ata_dev_init).  This version instead
  bails out of sata_down_spd_limit() when SStatus indicates no device is present
  (DET == 0), without altering ata_dev_init() or the sata_spd cache.

 drivers/ata/libata-sata.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
index ad40f516c6d4..775da21cbbae 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;
 	if (rc == 0 && ata_sstatus_online(sstatus))
 		spd = (sstatus >> 4) & 0xf;
 	else
-- 
2.43.0

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

[sashiko-bot]

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