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

[Paul Menzel <pmenzel@molgen.mpg.de>]

Dear Damien, dear Robin,


Am 20.01.22 um 01:14 schrieb Damien Le Moal:
> 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/

Polling and a warning, when polling time exceeds like 10 ms, so users 
can contact the hardware vendor, would indeed be the most flexible solution.

Robin, do you remember, why these patches were not applied? Just lack of 
time and review, or where there issues?

>> 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.

Indeed, I did not think of that.

>>> 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.

Awesome.

>>> 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!

Adding back Damien’s answer text:

>> 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.

Thank you for cooking this up. I tested this on the ASUS F2A85-M PRO 
(AMD, 1022:0x7801), MSI B350M MORTAR (AMD, 1022:0x7901), and IBM S822LC 
(Marvell, 1b4b:9235) with no issues and the expected decrease in boot time.

>>> 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.


Kind regards,

Paul