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@xxxxxxxxxxxxxxxxxx>
>> ---
>> 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
