Dear Damien,
Am 28.03.22 um 09:19 schrieb Damien Le Moal:
On 3/25/22 18:10, Paul Menzel wrote:
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.
To my knowledge this was a regular boot (`systemctl reboot`). So where
did the 50 ms go? ;-)
Kind regards,
Paul
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
