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

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

On 2022/01/20 2:57, Robin H. Johnson wrote:
> Hi, not originally in the thread, but I've run into hardware where the
> delay was bumpy before, when I did early porting around SATA PMP code
> (https://dev.gentoo.org/~robbat2/patches/libata-development/ if you want
> to see really old patches from 2006)
> 
> This series esp of a code approach that didn't get merged might be
> interesting, that implements hotplug by polling:
> https://dev.gentoo.org/~robbat2/patches/libata-development/2007/00-hp-poll/
> 
> On Fri, Jan 14, 2022 at 06:23:26PM +0900, Damien Le Moal wrote:
>> 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 think this is a key thing here; and I like that your patch moves to a
> flag.
> 
>> 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.
> I agree that removing it by default is right, but I'd like to make one
> additional request here:
> Please add a libata.force= flag that lets users enable/disable the delay
> per adapter/link.
> 
> I think this would be valuable to rule out issues where the debounce
> delay is needed on the drive side more than the controller side, esp. in
> cases of poorly implemented port multipliers as Tejun & I found back in
> 2006.
> 
> Maybe libata.force=X.Y:no_debounce_delay & libata.force=X.Y:force_debounce_delay
> 
> The ata_parse_force_one function as it stands can't handle a parameter
> to the value, so you cannot get libata.force=X.Y:debounce_delay=N
> without also improving ata_parse_force_one.

Good point. I will look into adding this.

> 
>> 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.
> Nice find!
> 
>> 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.
> That gets likely single-disk worst/best case, but I'm still worried
> about port multipliers (sadly I don't have the worst-implemented ones
> anymore, I sold them to some Windows users)

:)

I have a e-sata port-multiplier box in the lab. But I need to hook it up to my
test box, which means that I have to get out of home for once and go to the
office :) Will do that. Port-multiplier tests are also needed to complete Hannes
series renaming sysfs fields to match the debug messages.


-- 
Damien Le Moal
Western Digital Research