Re: [PATCH v2] ata: libata: Fix sata_down_spd_limit() when no link speed is reported

[Niklas Cassel <Niklas.Cassel@wdc.com>]

On Mon, Jan 30, 2023 at 09:24:27PM +0900, Damien Le Moal wrote:
> Commit 2dc0b46b5ea3 ("libata: sata_down_spd_limit should return if
> driver has not recorded sstatus speed") changed the behavior of
> sata_down_spd_limit() to return doing nothing if a drive does not report
> a current link speed, to avoid reducing the link speed to the lowest 1.5
> Gbps speed.
> 
> However, the change assumed that a speed was recorded before probing
> (e.g. before a suspend/resume) and set in link->sata_spd. This causes
> problems with adapters/drives combination failing to establish a link
> speed during probe autonegotiation. One example reported of this problem
> is an mvebu adapter with a 3Gbps port-multiplier box: autonegotiation
> fails, leaving no recorded link speed and no reported current link
> speed. Probe retries also fail as no action is taken by sata_set_spd()
> after each retry.
> 
> Fix this by returning early in sata_down_spd_limit() only if we do have
> a recorded link speed, that is, if link->sata_spd is not 0. With this
> fix, a failed probe not leading to a recorded link speed is retried at
> the lower 1.5 Gbps speed, with the link speed potentially increased
> later on the second revalidate of the device if the device reports
> that it supports higher link speeds.
> 
> Reported-by: Marius Dinu <marius@psihoexpert.ro>
> Fixes: 2dc0b46b5ea3 ("libata: sata_down_spd_limit should return if driver has not recorded sstatus speed")
> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> ---
> 
> Changes from v1:
>  * Fixed commit message typos
> 
>  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 884ae73b11ea..2ea572628b1c 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -3109,7 +3109,7 @@ int sata_down_spd_limit(struct ata_link *link, u32 spd_limit)
>  	 */
>  	if (spd > 1)
>  		mask &= (1 << (spd - 1)) - 1;
> -	else
> +	else if (link->sata_spd)
>  		return -EINVAL;
>  
>  	/* were we already at the bottom? */
> -- 
> 2.39.1
> 

Reviewed-by: Niklas Cassel <niklas.cassel@wdc.com>