Re: [PATCH 0/2][RFC] Make delay before debouncing configurable

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

On 1/14/22 00:46, Paul Menzel wrote:
> The 200 ms delay before debouncing the PHY was introduced for some buggy
> old controllers. To decrease the boot time to come closer do instant
> boot, add a parameter so users can override that delay.
> 
> The current implementation has several drawbacks, and is just a proof of
> concept, which some experienced Linux kernel developer can probably
> implement in a better way.

I do not think that a libata module parameter is not the way to go with
this: libata is used by all drivers, so for a system that has multiple
adapters, different delays cannot be specified easily.

I am really thinking that the way to go about this is to remove the
200ms delay by default and add it only for drivers that request it with
a link flag. That is, ATA_LFLAG_NO_DEBOUNCE_DELAY needs to become
ATA_LFLAG_DEBOUNCE_DELAY.

The other large delay is the link stability check in
sata_link_debounce(). 100ms is added (more for hotplug case) to ensure
that the SStatus register DET field provides a stable value. But I
cannot find any text in the AHCI and SATA IO specs that mandate such
large delay.

I tried to address all of the above. Please have a look at the top 4
patches in the sata-timing branch of the libata tree:

git@gitolite.kernel.org:pub/scm/linux/kernel/git/dlemoal/libata

The sata-timing branch is for now based on libata for-5.17 branch.

The 200ms delay in sata_link_resume() is gone by default, replaced with
a 1ms delay (totally arbitrary). The 200ms delay is executed only if a
driver has the ATA_LFLAG_DEBOUNCE_DELAY link flag set.

The next part is sata_link_debounce(): I *think* that we can assume that
a link is stable if we see cur_det == last_det == 3. In this case,
bailing out early seems to be fine, at least on my test box (Intel
dual-socket Xeon server with Intel AHCI chipset). But I only tested
boot/reboot. Hotplug/unplug and suspend/resume need to be tested, but I
need to go to the lab for that (working from home). Will try next week.

Could you give this branch a try and check how that improves device scan
times ?

On my test box, which has *a lot* of drives, I see something like this:

Before:
[   16.696140] ata4: SATA max UDMA/133 abar m524288@0x9d200000 port
0x9d200180 irq 341
[   17.527446] ata4: SATA link up 6.0 Gbps (SStatus 133 SControl 300)
...
-> 831 ms to get the link ready

After:
 [   15.957946] ata4: SATA max UDMA/133 abar m524288@0x9d200000 port
0x9d200180 irq 341
[   16.245066] ata4: SATA link up 6.0 Gbps (SStatus 133 SControl 300)
...
-> 287 ms to get the link ready

There are differences between the many HDDs & SSDs I have connected
though. There is a lot of scheduling side effects at play, so the gains
are variable in my case. A system with a single disk attached should be
used for proper evaluation.

Going forward, if more testing do not show any problem, I am thinking of
pushing these changes to for-next to get things tested more widely and
see who screams that they lost their drives :)
For now, I added the ATA_LFLAG_DEBOUNCE_DELAY to the ata_piix driver
only. Likely, this flag will be needed for most legacy/old adapters
(which I do not have).

Cheers.


-- 
Damien Le Moal
Western Digital Research