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

[Damien Le Moal <damien.lemoal@opensource.wdc.com>]

On 1/30/23 20:47, Niklas Cassel wrote:
> On Mon, Jan 30, 2023 at 08:07:21PM +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 exampe reported of this problem
> 
> s/exampe/example/
> 
>> is an mvebu adapter with a 3Gbps port-multiplier box: autonegotiation
>> fails, leaving no recorded link speed and no rep@orted current link
> 
> s/rep@orted/reported/
> 
>> 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")
> 
> IIRC, checkpatch.pl with the default options should put the author on the
> commit being fixed on CC. CC:ing the original author is a good practice IMHO.

Funny that I did run checkpatch and it did not complain about the above typos :)
Thanks for the review, will fix this.


> 
>> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.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 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
>>
> 
> With typos fixed:
> Reviewed-by: Niklas Cassel <niklas.cassel@wdc.com>

-- 
Damien Le Moal
Western Digital Research