Re: [PATCH 4/4] ata: libata-sata: Improve sata_link_debounce()

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

On 3/25/22 18:10, Paul Menzel wrote:
> Dear Damien,
> 
> 
> Am 23.03.22 um 09:17 schrieb Damien Le Moal:
>> sata_link_debounce() polls the SStatus register DET field to ensure that
>> a stable value is provided, to reliably detect device presence and PHY
>> readiness. Polling is done for at least timing->duration if there is no
>> device detected. For the device detected case, polling last up to
> 
> last*s*
> 
>> deadline to ensure that the PHY becomes ready.
>>
>> However, the PHY ready state is actually never checked, leading to the
>> poll loop duration always reaching the maximum duration.
>>
>> For adapters that do not require a debounce delay (link flag
>> ATA_LFLAG_DEBOUNCE_DELAY no set), add a check to test if DET indicates
> 
> no*t*?
> 
>> device present *and* PHY ready and bail out of the polling loop if it
>> does.
>>
>> While at it, add comments to clarify the various checks in
>> sata_link_debounce() polling loop.
> 
> Were you able to verify that the check now terminates the loop on your 
> devices earlier?

Yes it does for me. The function goes from taking about 100ms to taking
only one iteration of the loop, which has only one 5ms sleep.

> 
> Are below the right lines to compare? On the Dell OptiPlex 5055 (AMD FCH 
> SATA Controller [AHCI mode] [1022:7901]) I see:
> 
> Before (this very patch)
> 
>      [    0.428015] ata1: hard resetting link
>      [    0.696316] ata1: SATA link up 6.0 Gbps (SStatus 133 SControl 300)
> 
> After:
> 
>      [    0.428907] ata1: hard resetting link
>      [    0.648092] ata1: SATA link up 6.0 Gbps (SStatus 133 SControl 300)
> 
> So a decrease of (268 - 219 = 49) ms. Nice.

For a regular boot, sata_deb_timing_normal duration is used. So the save
is 100ms almost (there is still one iteration of the loop needed, which
takes interval = 5ms).

Together with the long 200ms sleep in sata_link_resume, the gain overall
should be about 300ms.

> 
>> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
>> ---
>>   drivers/ata/libata-sata.c | 24 ++++++++++++++++++++++--
>>   1 file changed, 22 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
>> index 87ad03c2e49f..15423723c9dd 100644
>> --- a/drivers/ata/libata-sata.c
>> +++ b/drivers/ata/libata-sata.c
>> @@ -276,8 +276,27 @@ int sata_link_debounce(struct ata_link *link,
>>   
>>   		/* DET stable? */
>>   		if (cur == last) {
>> +			/*
>> +			 * If the device presence was detected but PHY
>> +			 * communication is not yet established, wait until
>> +			 * deadline.
>> +			 */
>>   			if (cur == 1 && time_before(jiffies, deadline))
>>   				continue;
>> +
>> +			/*
>> +			 * If PHY is ready and the device is present, and the
>> +			 * driver did not request debounce delay, bail out early
>> +			 * assuming that the link is stable.
>> +			 */
>> +			if (cur == 3 &&
>> +			    !(link->flags & ATA_LFLAG_DEBOUNCE_DELAY))
>> +				return 0;
>> +
>> +			/*
>> +			 * If DET value has remained stable for
>> +			 * timing->duration, bail out.
>> +			 */
>>   			if (time_after(jiffies,
>>   				ata_deadline(last_jiffies, timing->duration)))
>>   				return 0;
>> @@ -288,8 +307,9 @@ int sata_link_debounce(struct ata_link *link,
>>   		last = cur;
>>   		last_jiffies = jiffies;
>>   
>> -		/* Check deadline.  If debouncing failed, return
>> -		 * -EPIPE to tell upper layer to lower link speed.
>> +		/*
>> +		 * If debouncing has not succeeded before dealine, return
> 
> dea*d*line
> 
>> +		 * -EPIPE to tell the upper layer to lower the link speed.
>>   		 */
>>   		if (time_after(jiffies, deadline))
>>   			return -EPIPE;
> 
> One separate for the new check would have been nice, but looks good to 
> me overall.
> 
> 
> Kind regards,
> 
> Paul


-- 
Damien Le Moal
Western Digital Research