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

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

[Paul Menzel]

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.

One problem is, that the warning is shown for each link and not just per
controller.

Paul Menzel (2):
  ata: Add module parameter `debounce_delay_ms`
  ata: Warn about removal of debounce delay in Linux 5.19

 Documentation/admin-guide/kernel-parameters.txt |  6 ++++++
 drivers/ata/libata-core.c                       |  4 ++++
 drivers/ata/libata-sata.c                       | 12 +++++++++---
 drivers/ata/libata.h                            |  1 +
 4 files changed, 20 insertions(+), 3 deletions(-)

-- 
2.30.2

[PATCH 1/2] ata: Add module parameter `debounce_delay_ms`

[Paul Menzel]

The 200 ms delay in `sata_resume_lin()` is probably only needed for a
few old controllers, so allow users to test this by setting the delay
time (preferrably 0). To be able to track defaults, make it a signed
integer with negative values being the default, which currently stays at
200 ms.

This parameter could also be made boolean, but making the delay
configurable adds more flexibility, but also does not directly match the
existing boolean flag `ATA_LFLAG_NO_DEBOUNCE_DELAY`.

Note, if that flag is set for a board, then the parameter is ignored.

History:

Commit 4effb658a0 from October 2003 [1, historical git archive] with the
commit message

> [libata] Merge Serial ATA core, and drivers for:
>
> Intel ICH5 (production)
> ServerWorks / Apple K2 (beta)
> VIA (beta)
> Silicon Image 3112 (broken!)
> Various Promise (alpha/beta)

adds the code below:

    void sata_phy_reset(struct ata_port *ap)
    {
    […]
        /* wait for phy to become ready, if necessary */
        do {
            msleep(200);
            sstatus = scr_read(ap, SCR_STATUS);
            if ((sstatus & 0xf) != 1)
                break;
        } while (time_before(jiffies, timeout));
    […]
    }

Later on in commit d7bb4cc75759 ([PATCH] libata-hp-prep: implement
sata_phy_debounce()) the commit is refactored [2], and the comment
clarified.

     /*
      * Writes to SControl sometimes get ignored under certain
      * controllers (ata_piix SIDPR).  Make sure DET actually is
      * cleared.
      */
     do {
             scontrol = (scontrol & 0x0f0) | 0x300;
             if ((rc = sata_scr_write(link, SCR_CONTROL, scontrol)))
                     return rc;
             /*
              * Some PHYs react badly if SStatus is pounded
              * immediately after resuming.  Delay 200ms before
              * debouncing.
              */
             if (!(link->flags & ATA_LFLAG_NO_DEBOUNCE_DELAY))
                     ata_msleep(link->ap, 200);

             /* is SControl restored correctly? */
             if ((rc = sata_scr_read(link, SCR_CONTROL, &scontrol)))
                     return rc;
     } while ((scontrol & 0xf0f) != 0x300 && --tries);

A lot of PHYs do not need a delay though, so delaying 200 ms increases
the boot time by 30 percent unnecessarily for a lot of systems, making
“instant booting” quite hard.

[1]: https://git.kernel.org/pub/scm/linux/kernel/git/history/history.git/commit/?id=4effb658a0f800e159c29a2d881cac76c326087a
[2]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=d7bb4cc7575929a60b0a718daa1bce87bea9a9cc

Signed-off-by: Paul Menzel <pmenzel@molgen.mpg.de>
---
 Documentation/admin-guide/kernel-parameters.txt | 6 ++++++
 drivers/ata/libata-core.c                       | 4 ++++
 drivers/ata/libata-sata.c                       | 6 ++++--
 drivers/ata/libata.h                            | 1 +
 4 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 2fba82431efbe..8cc0e790f49d6 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -2548,6 +2548,12 @@
 	lapic_timer_c2_ok	[X86,APIC] trust the local apic timer
 			in C2 power state.
 
+	libata.libata_debounce_delay_ms=	[LIBATA] Set debounce delay in
+			ms
+
+			libata.dma<0	  Use default value from code
+			libata.dma>1	  Debounce delay in milliseconds
+
 	libata.dma=	[LIBATA] DMA control
 			libata.dma=0	  Disable all PATA and SATA DMA
 			libata.dma=1	  PATA and SATA Disk DMA only
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 67f88027680ac..b0d76cb8a7531 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -154,6 +154,10 @@ static int atapi_an;
 module_param(atapi_an, int, 0444);
 MODULE_PARM_DESC(atapi_an, "Enable ATAPI AN media presence notification (0=0ff [default], 1=on)");
 
+int debounce_delay_ms = -1;
+module_param_named(debounce_delay_ms, libata_debounce_delay_ms, int, 0644);
+MODULE_PARM_DESC(debounce_delay_ms, "Delay amount milliseconds in sata_link_resume() to work around controller issues (negative values mean default value in code (200 ms)");
+
 MODULE_AUTHOR("Jeff Garzik");
 MODULE_DESCRIPTION("Library module for ATA devices");
 MODULE_LICENSE("GPL");
diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
index 071158c0c44c1..29a815e2b7248 100644
--- a/drivers/ata/libata-sata.c
+++ b/drivers/ata/libata-sata.c
@@ -315,10 +315,12 @@ int sata_link_resume(struct ata_link *link, const unsigned long *params,
 		/*
 		 * Some PHYs react badly if SStatus is pounded
 		 * immediately after resuming.  Delay 200ms before
-		 * debouncing.
+		 * debouncing. Duration can be configured with module
+		 * parameter debounce_delay_ms.
 		 */
 		if (!(link->flags & ATA_LFLAG_NO_DEBOUNCE_DELAY))
-			ata_msleep(link->ap, 200);
+			ata_msleep(link->ap,
+					(libata_debounce_delay_ms < 0) ? 200 : libata_debounce_delay_ms);
 
 		/* is SControl restored correctly? */
 		if ((rc = sata_scr_read(link, SCR_CONTROL, &scontrol)))
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index 51e01acdd2410..a26e77ee25aa2 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -35,6 +35,7 @@ extern int atapi_passthru16;
 extern int libata_fua;
 extern int libata_noacpi;
 extern int libata_allow_tpm;
+extern int libata_debounce_delay_ms;
 extern const struct device_type ata_port_type;
 extern struct ata_link *ata_dev_phys_link(struct ata_device *dev);
 #ifdef CONFIG_ATA_FORCE
-- 
2.30.2

[PATCH 2/2] ata: Warn about removal of debounce delay in Linux 5.19

[Paul Menzel]

The delay is only needed for a few buggy chipsets (PHYs(?)). As 200 ms
is quite a lot in today times, announce the change of the default in
Linux 5.19, and call for tests and reports.
---
 drivers/ata/libata-sata.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
index 29a815e2b7248..026ffcfaeaf26 100644
--- a/drivers/ata/libata-sata.c
+++ b/drivers/ata/libata-sata.c
@@ -318,9 +318,13 @@ int sata_link_resume(struct ata_link *link, const unsigned long *params,
 		 * debouncing. Duration can be configured with module
 		 * parameter debounce_delay_ms.
 		 */
-		if (!(link->flags & ATA_LFLAG_NO_DEBOUNCE_DELAY))
+		if (!(link->flags & ATA_LFLAG_NO_DEBOUNCE_DELAY)) {
 			ata_msleep(link->ap,
 					(libata_debounce_delay_ms < 0) ? 200 : libata_debounce_delay_ms);
+			if (libata_debounce_delay_ms < 0)
+				/* negative values are default */
+				ata_link_warn(link, "Due to historical reasons a 200 ms delay is applied in sata_link_resume(). Most controllers do not need that, so the default will change to 0 ms in Linux 5.19. Please test with lower values using libata.debounce_delay_ms and report the results <linux-ide@vger.kernel.org>.\n");
+		}
 
 		/* is SControl restored correctly? */
 		if ((rc = sata_scr_read(link, SCR_CONTROL, &scontrol)))
-- 
2.30.2

Re: [PATCH 2/2] ata: Warn about removal of debounce delay in Linux 5.19

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2053 bytes --]

Hi Paul,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on next-20220113]
[cannot apply to linus/master v5.16 v5.16-rc8 v5.16-rc7 v5.16]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Paul-Menzel/Make-delay-before-debouncing-configurable/20220113-235010
base:    27c9d5b3c24af29de643533984f1ba3e650c7c78
config: m68k-randconfig-r012-20220113 (https://download.01.org/0day-ci/archive/20220114/202201140447.hEUZcatB-lkp(a)intel.com/config)
compiler: m68k-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/4cc8a654f25f25bd6a46a91a5d1e3e3cfb5d2232
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Paul-Menzel/Make-delay-before-debouncing-configurable/20220113-235010
        git checkout 4cc8a654f25f25bd6a46a91a5d1e3e3cfb5d2232
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=m68k SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   m68k-linux-ld: drivers/ata/libata-core.o:(__param+0x10): undefined reference to `libata_debounce_delay_ms'
   m68k-linux-ld: drivers/ata/libata-sata.o: in function `sata_link_resume':
   libata-sata.c:(.text+0x944): undefined reference to `libata_debounce_delay_ms'
>> m68k-linux-ld: libata-sata.c:(.text+0x95a): undefined reference to `libata_debounce_delay_ms'

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

Re: [PATCH 1/2] ata: Add module parameter `debounce_delay_ms`

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 1779 bytes --]

Hi Paul,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on next-20220113]
[cannot apply to linus/master v5.16 v5.16-rc8 v5.16-rc7 v5.16]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Paul-Menzel/Make-delay-before-debouncing-configurable/20220113-235010
base:    27c9d5b3c24af29de643533984f1ba3e650c7c78
config: m68k-defconfig (https://download.01.org/0day-ci/archive/20220114/202201140416.gKnMhEkh-lkp(a)intel.com/config)
compiler: m68k-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/81046cd50c93c5842ed8cc6ef28d3415e8341e03
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Paul-Menzel/Make-delay-before-debouncing-configurable/20220113-235010
        git checkout 81046cd50c93c5842ed8cc6ef28d3415e8341e03
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=m68k SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> m68k-linux-ld: drivers/ata/libata-core.o:(__param+0x10): undefined reference to `libata_debounce_delay_ms'

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

Re: [PATCH 1/2] ata: Add module parameter `debounce_delay_ms`

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2063 bytes --]

Hi Paul,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on next-20220113]
[cannot apply to linus/master v5.16 v5.16-rc8 v5.16-rc7 v5.16]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Paul-Menzel/Make-delay-before-debouncing-configurable/20220113-235010
base:    27c9d5b3c24af29de643533984f1ba3e650c7c78
config: openrisc-randconfig-r022-20220113 (https://download.01.org/0day-ci/archive/20220114/202201140438.GWafd2LV-lkp(a)intel.com/config)
compiler: or1k-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/81046cd50c93c5842ed8cc6ef28d3415e8341e03
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Paul-Menzel/Make-delay-before-debouncing-configurable/20220113-235010
        git checkout 81046cd50c93c5842ed8cc6ef28d3415e8341e03
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=openrisc SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> or1k-linux-ld: drivers/ata/libata-core.o:(__param+0x10): undefined reference to `libata_debounce_delay_ms'
   or1k-linux-ld: drivers/ata/libata-sata.o: in function `sata_link_resume':
   libata-sata.c:(.text+0x21b8): undefined reference to `libata_debounce_delay_ms'
>> or1k-linux-ld: libata-sata.c:(.text+0x2230): undefined reference to `libata_debounce_delay_ms'

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

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

[Damien Le Moal]

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

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

[Robin H. Johnson]

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.

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

-- 
Robin Hugh Johnson
Gentoo Linux: Dev, Infra Lead, Foundation Treasurer
E-Mail   : robbat2@gentoo.org
GnuPG FP : 11ACBA4F 4778E3F6 E4EDF38E B27B944E 34884E85
GnuPG FP : 7D0B3CEB E9B85B1F 825BCECF EE05E6F6 A48F6136

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

[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/
> 
> 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

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

[Paul Menzel]

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

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

[Robin H. Johnson]

[-- Attachment #1: Type: text/plain, Size: 1002 bytes --]

On Mon, Feb 14, 2022 at 08:09:53AM +0100, Paul Menzel wrote:
> >> 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?
Disagreement on the per-link configurability via sysfs, and then lack of
time to come to an agreed solution there.

The 2007 linux-ide list has some of that discussion, but there was also
some on IRC that is lost to time.

-- 
Robin Hugh Johnson
Gentoo Linux: Dev, Infra Lead, Foundation Treasurer
E-Mail   : robbat2@gentoo.org
GnuPG FP : 11ACBA4F 4778E3F6 E4EDF38E B27B944E 34884E85
GnuPG FP : 7D0B3CEB E9B85B1F 825BCECF EE05E6F6 A48F6136

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 1113 bytes --]

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

[Damien Le Moal]

On 2/24/22 23:34, Paul Menzel wrote:
[...]
>> 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 is still one issue I noticed. The MSI B350M MORTAR has nine(?) 
> SATA ports, and only one is connected to an SSD. For some of the other 
> unpopulated ports a delay of 100 ms happens (although in Linux kernel 
> threads, but still in serial and not parallel). Unfortunately, that 
> system uses LUKS encryption, and as things are happening in initrd, I do 
> not know if the delays would hold up the overall boot. I need to do more 
> tests.
> 
> ```
> $ grep sata_link_hardreset 20220220-sata-hardreset.txt
>      0.706289 |    0)   scsi_eh-70   |               | 
> sata_link_hardreset() {
>      0.718497 |    0)   scsi_eh-92   |               | 
> sata_link_hardreset() {
>      0.728425 |    0)   scsi_eh-92   | # 9927.978 us |  } /* 
> sata_link_hardreset */
>      0.811159 |    2)   scsi_eh-70   | @ 104870.3 us |  } /* 
> sata_link_hardreset */
>      0.811329 |    2)   scsi_eh-72   |               | 
> sata_link_hardreset() {
>      0.920672 |    3)   scsi_eh-72   | @ 109343.5 us |  } /* 
> sata_link_hardreset */
>      0.920915 |    2)   scsi_eh-78   |               | 
> sata_link_hardreset() {
>      1.024618 |    2)   scsi_eh-78   | @ 103703.7 us |  } /* 
> sata_link_hardreset */
>      1.025027 |    0)   scsi_eh-80   |               | 
> sata_link_hardreset() {
>      1.128589 |    0)   scsi_eh-80   | @ 103561.6 us |  } /* 
> sata_link_hardreset */
> ```

This looks like the delay for 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.

As mentioned in another email on this subject, I cannot find any text in
the AHCI and SATA IO specs that mandate such large delay. Still need to
dig the history of this delay and why it was added.



-- 
Damien Le Moal
Western Digital Research