diff mbox series

[v2,1/3] ata: ahci: Add new board low_power_no_debounce_delay

Message ID 20220321212431.13717-1-pmenzel@molgen.mpg.de
State New
Headers show
Series [v2,1/3] ata: ahci: Add new board low_power_no_debounce_delay | expand

Commit Message

Paul Menzel March 21, 2022, 9:24 p.m. UTC 
Some adapters do not require the default 200 ms debounce delay in
`sata_link_resume()`. So, create the new board
`board_ahci_low_power_no_debounce_delay` with the link flag
`ATA_LFLAG_NO_DEBOUNCE_DELAY`.

Signed-off-by: Paul Menzel <pmenzel@molgen.mpg.de>
Cc: Hans de Goede <hdegoede@redhat.com>
Cc: Mario Limonciello <mario.limonciello@amd.com>
---
v2: Resend

 drivers/ata/ahci.c | 9 +++++++++
 1 file changed, 9 insertions(+)

Comments

Mario Limonciello March 21, 2022, 9:51 p.m. UTC | #1 
[Public]

> -----Original Message-----
> From: Paul Menzel <pmenzel@molgen.mpg.de>
> Sent: Monday, March 21, 2022 16:25
> To: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> Cc: Paul Menzel <pmenzel@molgen.mpg.de>; Hans de Goede
> <hdegoede@redhat.com>; Limonciello, Mario
> <Mario.Limonciello@amd.com>; linux-ide@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: [PATCH v2 3/3] ata: ahci: Skip 200 ms debounce delay for AMD 300
> Series Chipset SATA Controller
> 
> AMD chipsets for AMD Ryzen contain two SATA controllers, for example on
> the
> Dell OptiPlex 5055 Ryzen CPU/0P03DX:
> 
>     01:00.1 SATA controller [0106]: Advanced Micro Devices, Inc. [AMD] 300
> Series Chipset SATA Controller [1022:43b7] (rev 02)
>     07:00.2 SATA controller [0106]: Advanced Micro Devices, Inc. [AMD] FCH
> SATA Controller [AHCI mode] [1022:7901] (rev 51)
> 
> The 300 Series Chipset SATA Controller [1022:43b7] does not need the 200 ms
> delay before debouncing the PHY in `sata_link_resume()`, so skip it by
> mapping it to the board with no debounce delay.
> 
> Tested on the Dell OptiPlex 5055 Ryzen CPU/0P03DX, BIOS 1.1.50 07/28/2021
> Linux 5.17 with an HDD connected to ata1 connected to 01:00.1, and no other
> storage devices. (Only ata9 is connected to 07:00.2.)
> 
> Currently, without this patch (with 200 ms delay), device probe for ata1
> takes 468 ms (= 0.896 s - 0.428 s), ata2 takes 840 ms, ata5 takes 1,125 ms,
> and ata6 takes 1,464 ms:
> 
>     [    0.427251] calling  ahci_pci_driver_init+0x0/0x1a @ 1
>     [    0.427271] ahci 0000:01:00.1: version 3.0
>     [    0.427371] ahci 0000:01:00.1: SSS flag set, parallel bus scan disabled
>     [    0.427405] ahci 0000:01:00.1: AHCI 0001.0301 32 slots 8 ports 6 Gbps 0x33
> impl SATA mode
>     [    0.427409] ahci 0000:01:00.1: flags: 64bit ncq sntf stag pm led clo only pmp
> pio slum part sxs deso sadm sds apst
>     [    0.427814] scsi host0: ahci
>     [    0.427895] scsi host1: ahci
>     [    0.427968] scsi host2: ahci
>     [    0.428038] scsi host3: ahci
>     [    0.428113] scsi host4: ahci
>     [    0.428184] scsi host5: ahci
>     [    0.428255] scsi host6: ahci
>     [    0.428325] scsi host7: ahci
>     [    0.428352] ata1: SATA max UDMA/133 abar m131072@0xf0600000 port
> 0xf0600100 irq 36
>     [    0.428356] ata2: SATA max UDMA/133 abar m131072@0xf0600000 port
> 0xf0600180 irq 36
>     [    0.428359] ata3: DUMMY
>     [    0.428360] ata4: DUMMY
>     [    0.428362] ata5: SATA max UDMA/133 abar m131072@0xf0600000 port
> 0xf0600300 irq 36
>     [    0.428365] ata6: SATA max UDMA/133 abar m131072@0xf0600000 port
> 0xf0600380 irq 36
>     [    0.428368] ata7: DUMMY
>     [    0.428369] ata8: DUMMY
>     [    0.428481] ahci 0000:07:00.2: AHCI 0001.0301 32 slots 1 ports 6 Gbps 0x1
> impl SATA mode
>     [    0.428486] ahci 0000:07:00.2: flags: 64bit ncq sntf ilck pm led clo only pmp
> fbs pio slum part
>     [    0.428611] scsi host8: ahci
>     [    0.428639] ata9: SATA max UDMA/133 abar m4096@0xf0108000 port
> 0xf0108100 irq 38
>     [    0.428653] initcall ahci_pci_driver_init+0x0/0x1a returned 0 after 1367
> usecs
>     […]
>     [    0.531949] ata9: SATA link down (SStatus 0 SControl 300)
>     [    0.895730] ata1: SATA link up 6.0 Gbps (SStatus 133 SControl 300)
>     [    0.924392] ata1.00: ATA-8: ST500LM021-1KJ152, 0005SDM1, max
> UDMA/133
>     [    0.924410] ata1.00: 976773168 sectors, multi 16: LBA48 NCQ (depth 32)
>     [    0.963276] ata1.00: configured for UDMA/133
>     [    0.963355] scsi 0:0:0:0: Direct-Access     ATA      ST500LM021-1KJ15 SDM1
> PQ: 0 ANSI: 5
>     [    0.963478] sd 0:0:0:0: Attached scsi generic sg0 type 0
>     [    0.963568] sd 0:0:0:0: [sda] 976773168 512-byte logical blocks: (500 GB/466
> GiB)
>     [    0.963594] sd 0:0:0:0: [sda] 4096-byte physical blocks
>     [    0.963616] sd 0:0:0:0: [sda] Write Protect is off
>     [    0.963631] sd 0:0:0:0: [sda] Mode Sense: 00 3a 00 00
>     [    0.963644] sd 0:0:0:0: [sda] Write cache: enabled, read cache: enabled,
> doesn't support DPO or FUA
>     [    0.974119]  sda: sda1 sda2 sda3
>     [    0.974299] sd 0:0:0:0: [sda] Attached SCSI disk
>     [    1.268377] ata2: SATA link down (SStatus 0 SControl 300)
>     [    1.580394] ata5: SATA link down (SStatus 0 SControl 300)
>     [    1.892390] ata6: SATA link down (SStatus 0 SControl 300)
> 
> With this patch (no delay) device probe for ata1 takes 268 ms
> (= 0.696 s - 0.428 s), ata2 takes 440 ms, ata5 takes 545 ms, and ata6 takes
> 650 ms:
> 
>     [    0.426850] calling  ahci_pci_driver_init+0x0/0x1a @ 1
>     [    0.426869] ahci 0000:01:00.1: version 3.0
>     [    0.426970] ahci 0000:01:00.1: SSS flag set, parallel bus scan disabled
>     [    0.427004] ahci 0000:01:00.1: AHCI 0001.0301 32 slots 8 ports 6 Gbps 0x33
> impl SATA mode
>     [    0.427008] ahci 0000:01:00.1: flags: 64bit ncq sntf stag pm led clo only pmp
> pio slum part sxs deso sadm sds apst
>     [    0.427412] scsi host0: ahci
>     [    0.427493] scsi host1: ahci
>     [    0.427569] scsi host2: ahci
>     [    0.427653] scsi host3: ahci
>     [    0.427728] scsi host4: ahci
>     [    0.427801] scsi host5: ahci
>     [    0.427876] scsi host6: ahci
>     [    0.427950] scsi host7: ahci
>     [    0.427978] ata1: SATA max UDMA/133 abar m131072@0xf0600000 port
> 0xf0600100 irq 36
>     [    0.427982] ata2: SATA max UDMA/133 abar m131072@0xf0600000 port
> 0xf0600180 irq 36
>     [    0.427985] ata3: DUMMY
>     [    0.427986] ata4: DUMMY
>     [    0.427988] ata5: SATA max UDMA/133 abar m131072@0xf0600000 port
> 0xf0600300 irq 36
>     [    0.427991] ata6: SATA max UDMA/133 abar m131072@0xf0600000 port
> 0xf0600380 irq 36
>     [    0.427994] ata7: DUMMY
>     [    0.427995] ata8: DUMMY
>     [    0.428116] ahci 0000:07:00.2: AHCI 0001.0301 32 slots 1 ports 6 Gbps 0x1
> impl SATA mode
>     [    0.428124] ahci 0000:07:00.2: flags: 64bit ncq sntf ilck pm led clo only pmp
> fbs pio slum part
>     [    0.428250] scsi host8: ahci
>     [    0.428278] ata9: SATA max UDMA/133 abar m4096@0xf0108000 port
> 0xf0108100 irq 38
>     [    0.428295] initcall ahci_pci_driver_init+0x0/0x1a returned 0 after 1409
> usecs
>     […]
>     [    0.532308] ata9: SATA link down (SStatus 0 SControl 300)
>     […]
>     [    0.696316] ata1: SATA link up 6.0 Gbps (SStatus 133 SControl 300)
>     [    0.725963] ata1.00: ATA-8: ST500LM021-1KJ152, 0005SDM1, max
> UDMA/133
>     [    0.725982] ata1.00: 976773168 sectors, multi 16: LBA48 NCQ (depth 32)
>     [    0.764845] ata1.00: configured for UDMA/133
>     [    0.764932] scsi 0:0:0:0: Direct-Access     ATA      ST500LM021-1KJ15 SDM1
> PQ: 0 ANSI: 5
>     [    0.765056] sd 0:0:0:0: Attached scsi generic sg0 type 0
>     [    0.765120] sd 0:0:0:0: [sda] 976773168 512-byte logical blocks: (500 GB/466
> GiB)
>     [    0.765147] sd 0:0:0:0: [sda] 4096-byte physical blocks
>     [    0.765175] sd 0:0:0:0: [sda] Write Protect is off
>     [    0.765189] sd 0:0:0:0: [sda] Mode Sense: 00 3a 00 00
>     [    0.765198] sd 0:0:0:0: [sda] Write cache: enabled, read cache: enabled,
> doesn't support DPO or FUA
>     [    0.866546]  sda: sda1 sda2 sda3
>     [    0.867239] sd 0:0:0:0: [sda] Attached SCSI disk
>     [    0.868330] ata2: SATA link down (SStatus 0 SControl 300)
>     [    0.973337] ata5: SATA link down (SStatus 0 SControl 300)
>     [    1.077832] ata6: SATA link down (SStatus 0 SControl 300)
> 
> Signed-off-by: Paul Menzel <pmenzel@molgen.mpg.de>
> Cc: Hans de Goede <hdegoede@redhat.com>
> Cc: Mario Limonciello <mario.limonciello@amd.com>
> ---
> v2: New patch for second SATA controller in Ryzen systems
> 
>  drivers/ata/ahci.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> index 44b79fe43d13d..ac7f230c12ebc 100644
> --- a/drivers/ata/ahci.c
> +++ b/drivers/ata/ahci.c
> @@ -453,6 +453,7 @@ static const struct pci_device_id ahci_pci_tbl[] = {
>  		.class_mask = 0xffffff,
>  		board_ahci_al },
>  	/* AMD */
> +	{ PCI_VDEVICE(AMD, 0x43b7),
> board_ahci_low_power_no_debounce_delay }, /* AMD 300 Series Chipset
> SATA Controller */
>  	{ PCI_VDEVICE(AMD, 0x7800), board_ahci }, /* AMD Hudson-2 */
>  	{ PCI_VDEVICE(AMD, 0x7801), board_ahci_no_debounce_delay }, /*
> AMD Hudson-2 (AHCI mode) */
>  	{ PCI_VDEVICE(AMD, 0x7900), board_ahci }, /* AMD CZ */
> --
> 2.30.2

+Nehal from AMD

I seem to recall that we were talking about trying to drop the debounce delay for
everything, weren't we?

So perhaps it would be right to add a 4th patch in the series to do just that.  Then
If this turns out to be problematic for anything other than the controllers in the
series that you identified as not problematic then that 4th patch can potentially
be reverted alone?
Damien Le Moal March 23, 2022, 5:01 a.m. UTC | #2 
On 3/22/22 06:51, Limonciello, Mario wrote:
> [Public]
> 
>> -----Original Message-----
>> From: Paul Menzel <pmenzel@molgen.mpg.de>
>> Sent: Monday, March 21, 2022 16:25
>> To: Damien Le Moal <damien.lemoal@opensource.wdc.com>
>> Cc: Paul Menzel <pmenzel@molgen.mpg.de>; Hans de Goede
>> <hdegoede@redhat.com>; Limonciello, Mario
>> <Mario.Limonciello@amd.com>; linux-ide@vger.kernel.org; linux-
>> kernel@vger.kernel.org
>> Subject: [PATCH v2 3/3] ata: ahci: Skip 200 ms debounce delay for AMD 300
>> Series Chipset SATA Controller
>>
>> AMD chipsets for AMD Ryzen contain two SATA controllers, for example on
>> the
>> Dell OptiPlex 5055 Ryzen CPU/0P03DX:
>>
>>     01:00.1 SATA controller [0106]: Advanced Micro Devices, Inc. [AMD] 300
>> Series Chipset SATA Controller [1022:43b7] (rev 02)
>>     07:00.2 SATA controller [0106]: Advanced Micro Devices, Inc. [AMD] FCH
>> SATA Controller [AHCI mode] [1022:7901] (rev 51)
>>
>> The 300 Series Chipset SATA Controller [1022:43b7] does not need the 200 ms
>> delay before debouncing the PHY in `sata_link_resume()`, so skip it by
>> mapping it to the board with no debounce delay.
>>
>> Tested on the Dell OptiPlex 5055 Ryzen CPU/0P03DX, BIOS 1.1.50 07/28/2021
>> Linux 5.17 with an HDD connected to ata1 connected to 01:00.1, and no other
>> storage devices. (Only ata9 is connected to 07:00.2.)
>>
>> Currently, without this patch (with 200 ms delay), device probe for ata1
>> takes 468 ms (= 0.896 s - 0.428 s), ata2 takes 840 ms, ata5 takes 1,125 ms,
>> and ata6 takes 1,464 ms:
>>
>>     [    0.427251] calling  ahci_pci_driver_init+0x0/0x1a @ 1
>>     [    0.427271] ahci 0000:01:00.1: version 3.0
>>     [    0.427371] ahci 0000:01:00.1: SSS flag set, parallel bus scan disabled
>>     [    0.427405] ahci 0000:01:00.1: AHCI 0001.0301 32 slots 8 ports 6 Gbps 0x33
>> impl SATA mode
>>     [    0.427409] ahci 0000:01:00.1: flags: 64bit ncq sntf stag pm led clo only pmp
>> pio slum part sxs deso sadm sds apst
>>     [    0.427814] scsi host0: ahci
>>     [    0.427895] scsi host1: ahci
>>     [    0.427968] scsi host2: ahci
>>     [    0.428038] scsi host3: ahci
>>     [    0.428113] scsi host4: ahci
>>     [    0.428184] scsi host5: ahci
>>     [    0.428255] scsi host6: ahci
>>     [    0.428325] scsi host7: ahci
>>     [    0.428352] ata1: SATA max UDMA/133 abar m131072@0xf0600000 port
>> 0xf0600100 irq 36
>>     [    0.428356] ata2: SATA max UDMA/133 abar m131072@0xf0600000 port
>> 0xf0600180 irq 36
>>     [    0.428359] ata3: DUMMY
>>     [    0.428360] ata4: DUMMY
>>     [    0.428362] ata5: SATA max UDMA/133 abar m131072@0xf0600000 port
>> 0xf0600300 irq 36
>>     [    0.428365] ata6: SATA max UDMA/133 abar m131072@0xf0600000 port
>> 0xf0600380 irq 36
>>     [    0.428368] ata7: DUMMY
>>     [    0.428369] ata8: DUMMY
>>     [    0.428481] ahci 0000:07:00.2: AHCI 0001.0301 32 slots 1 ports 6 Gbps 0x1
>> impl SATA mode
>>     [    0.428486] ahci 0000:07:00.2: flags: 64bit ncq sntf ilck pm led clo only pmp
>> fbs pio slum part
>>     [    0.428611] scsi host8: ahci
>>     [    0.428639] ata9: SATA max UDMA/133 abar m4096@0xf0108000 port
>> 0xf0108100 irq 38
>>     [    0.428653] initcall ahci_pci_driver_init+0x0/0x1a returned 0 after 1367
>> usecs
>>     […]
>>     [    0.531949] ata9: SATA link down (SStatus 0 SControl 300)
>>     [    0.895730] ata1: SATA link up 6.0 Gbps (SStatus 133 SControl 300)
>>     [    0.924392] ata1.00: ATA-8: ST500LM021-1KJ152, 0005SDM1, max
>> UDMA/133
>>     [    0.924410] ata1.00: 976773168 sectors, multi 16: LBA48 NCQ (depth 32)
>>     [    0.963276] ata1.00: configured for UDMA/133
>>     [    0.963355] scsi 0:0:0:0: Direct-Access     ATA      ST500LM021-1KJ15 SDM1
>> PQ: 0 ANSI: 5
>>     [    0.963478] sd 0:0:0:0: Attached scsi generic sg0 type 0
>>     [    0.963568] sd 0:0:0:0: [sda] 976773168 512-byte logical blocks: (500 GB/466
>> GiB)
>>     [    0.963594] sd 0:0:0:0: [sda] 4096-byte physical blocks
>>     [    0.963616] sd 0:0:0:0: [sda] Write Protect is off
>>     [    0.963631] sd 0:0:0:0: [sda] Mode Sense: 00 3a 00 00
>>     [    0.963644] sd 0:0:0:0: [sda] Write cache: enabled, read cache: enabled,
>> doesn't support DPO or FUA
>>     [    0.974119]  sda: sda1 sda2 sda3
>>     [    0.974299] sd 0:0:0:0: [sda] Attached SCSI disk
>>     [    1.268377] ata2: SATA link down (SStatus 0 SControl 300)
>>     [    1.580394] ata5: SATA link down (SStatus 0 SControl 300)
>>     [    1.892390] ata6: SATA link down (SStatus 0 SControl 300)
>>
>> With this patch (no delay) device probe for ata1 takes 268 ms
>> (= 0.696 s - 0.428 s), ata2 takes 440 ms, ata5 takes 545 ms, and ata6 takes
>> 650 ms:
>>
>>     [    0.426850] calling  ahci_pci_driver_init+0x0/0x1a @ 1
>>     [    0.426869] ahci 0000:01:00.1: version 3.0
>>     [    0.426970] ahci 0000:01:00.1: SSS flag set, parallel bus scan disabled
>>     [    0.427004] ahci 0000:01:00.1: AHCI 0001.0301 32 slots 8 ports 6 Gbps 0x33
>> impl SATA mode
>>     [    0.427008] ahci 0000:01:00.1: flags: 64bit ncq sntf stag pm led clo only pmp
>> pio slum part sxs deso sadm sds apst
>>     [    0.427412] scsi host0: ahci
>>     [    0.427493] scsi host1: ahci
>>     [    0.427569] scsi host2: ahci
>>     [    0.427653] scsi host3: ahci
>>     [    0.427728] scsi host4: ahci
>>     [    0.427801] scsi host5: ahci
>>     [    0.427876] scsi host6: ahci
>>     [    0.427950] scsi host7: ahci
>>     [    0.427978] ata1: SATA max UDMA/133 abar m131072@0xf0600000 port
>> 0xf0600100 irq 36
>>     [    0.427982] ata2: SATA max UDMA/133 abar m131072@0xf0600000 port
>> 0xf0600180 irq 36
>>     [    0.427985] ata3: DUMMY
>>     [    0.427986] ata4: DUMMY
>>     [    0.427988] ata5: SATA max UDMA/133 abar m131072@0xf0600000 port
>> 0xf0600300 irq 36
>>     [    0.427991] ata6: SATA max UDMA/133 abar m131072@0xf0600000 port
>> 0xf0600380 irq 36
>>     [    0.427994] ata7: DUMMY
>>     [    0.427995] ata8: DUMMY
>>     [    0.428116] ahci 0000:07:00.2: AHCI 0001.0301 32 slots 1 ports 6 Gbps 0x1
>> impl SATA mode
>>     [    0.428124] ahci 0000:07:00.2: flags: 64bit ncq sntf ilck pm led clo only pmp
>> fbs pio slum part
>>     [    0.428250] scsi host8: ahci
>>     [    0.428278] ata9: SATA max UDMA/133 abar m4096@0xf0108000 port
>> 0xf0108100 irq 38
>>     [    0.428295] initcall ahci_pci_driver_init+0x0/0x1a returned 0 after 1409
>> usecs
>>     […]
>>     [    0.532308] ata9: SATA link down (SStatus 0 SControl 300)
>>     […]
>>     [    0.696316] ata1: SATA link up 6.0 Gbps (SStatus 133 SControl 300)
>>     [    0.725963] ata1.00: ATA-8: ST500LM021-1KJ152, 0005SDM1, max
>> UDMA/133
>>     [    0.725982] ata1.00: 976773168 sectors, multi 16: LBA48 NCQ (depth 32)
>>     [    0.764845] ata1.00: configured for UDMA/133
>>     [    0.764932] scsi 0:0:0:0: Direct-Access     ATA      ST500LM021-1KJ15 SDM1
>> PQ: 0 ANSI: 5
>>     [    0.765056] sd 0:0:0:0: Attached scsi generic sg0 type 0
>>     [    0.765120] sd 0:0:0:0: [sda] 976773168 512-byte logical blocks: (500 GB/466
>> GiB)
>>     [    0.765147] sd 0:0:0:0: [sda] 4096-byte physical blocks
>>     [    0.765175] sd 0:0:0:0: [sda] Write Protect is off
>>     [    0.765189] sd 0:0:0:0: [sda] Mode Sense: 00 3a 00 00
>>     [    0.765198] sd 0:0:0:0: [sda] Write cache: enabled, read cache: enabled,
>> doesn't support DPO or FUA
>>     [    0.866546]  sda: sda1 sda2 sda3
>>     [    0.867239] sd 0:0:0:0: [sda] Attached SCSI disk
>>     [    0.868330] ata2: SATA link down (SStatus 0 SControl 300)
>>     [    0.973337] ata5: SATA link down (SStatus 0 SControl 300)
>>     [    1.077832] ata6: SATA link down (SStatus 0 SControl 300)
>>
>> Signed-off-by: Paul Menzel <pmenzel@molgen.mpg.de>
>> Cc: Hans de Goede <hdegoede@redhat.com>
>> Cc: Mario Limonciello <mario.limonciello@amd.com>
>> ---
>> v2: New patch for second SATA controller in Ryzen systems
>>
>>  drivers/ata/ahci.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
>> index 44b79fe43d13d..ac7f230c12ebc 100644
>> --- a/drivers/ata/ahci.c
>> +++ b/drivers/ata/ahci.c
>> @@ -453,6 +453,7 @@ static const struct pci_device_id ahci_pci_tbl[] = {
>>  		.class_mask = 0xffffff,
>>  		board_ahci_al },
>>  	/* AMD */
>> +	{ PCI_VDEVICE(AMD, 0x43b7),
>> board_ahci_low_power_no_debounce_delay }, /* AMD 300 Series Chipset
>> SATA Controller */
>>  	{ PCI_VDEVICE(AMD, 0x7800), board_ahci }, /* AMD Hudson-2 */
>>  	{ PCI_VDEVICE(AMD, 0x7801), board_ahci_no_debounce_delay }, /*
>> AMD Hudson-2 (AHCI mode) */
>>  	{ PCI_VDEVICE(AMD, 0x7900), board_ahci }, /* AMD CZ */
>> --
>> 2.30.2
> 
> +Nehal from AMD
> 
> I seem to recall that we were talking about trying to drop the debounce delay for
> everything, weren't we?
> 
> So perhaps it would be right to add a 4th patch in the series to do just that.  Then
> If this turns out to be problematic for anything other than the controllers in the
> series that you identified as not problematic then that 4th patch can potentially
> be reverted alone?

Not quite everything :) But you are right, let's try to switch the default
to no delay. I will be posting patches today for that.

Paul,

With these patches, your patches are not necessary anymore as the AMD
chipset falls under the default no-delay. It would be nice if you can test
though.
Paul Menzel March 23, 2022, 6:55 a.m. UTC | #3 
Dear Damien,


Am 23.03.22 um 06:01 schrieb Damien Le Moal:
> On 3/22/22 06:51, Limonciello, Mario wrote:
>> [Public]
>>
>>> -----Original Message-----
>>> From: Paul Menzel <pmenzel@molgen.mpg.de>
>>> Sent: Monday, March 21, 2022 16:25

[…]

>> I seem to recall that we were talking about trying to drop the debounce delay for
>> everything, weren't we?
>>
>> So perhaps it would be right to add a 4th patch in the series to do just that.  Then
>> If this turns out to be problematic for anything other than the controllers in the
>> series that you identified as not problematic then that 4th patch can potentially
>> be reverted alone?
> 
> Not quite everything :) But you are right, let's try to switch the default
> to no delay. I will be posting patches today for that.
> 
> Paul,
> 
> With these patches, your patches are not necessary anymore as the AMD
> chipset falls under the default no-delay.

I am all for improving the situation for all devices, but I am unable to 
judge the regression potential of changing this, as it affects a lot of 
devices. I guess it’d would go through the next tree, and hopefully the 
company QA teams can give it a good spin. I hoped that my patches, as I 
have tested them, and AMD will hopefully too, could go into the current 
merge window.

> It would be nice if you can test though.

Of course, I am going to that either way.


Kind regards,

Paul
Damien Le Moal March 23, 2022, 8:24 a.m. UTC | #4 
On 3/23/22 15:55, Paul Menzel wrote:
> Dear Damien,
> 
> 
> Am 23.03.22 um 06:01 schrieb Damien Le Moal:
>> On 3/22/22 06:51, Limonciello, Mario wrote:
>>> [Public]
>>>
>>>> -----Original Message-----
>>>> From: Paul Menzel <pmenzel@molgen.mpg.de>
>>>> Sent: Monday, March 21, 2022 16:25
> 
> […]
> 
>>> I seem to recall that we were talking about trying to drop the debounce delay for
>>> everything, weren't we?
>>>
>>> So perhaps it would be right to add a 4th patch in the series to do just that.  Then
>>> If this turns out to be problematic for anything other than the controllers in the
>>> series that you identified as not problematic then that 4th patch can potentially
>>> be reverted alone?
>>
>> Not quite everything :) But you are right, let's try to switch the default
>> to no delay. I will be posting patches today for that.
>>
>> Paul,
>>
>> With these patches, your patches are not necessary anymore as the AMD
>> chipset falls under the default no-delay.
> 
> I am all for improving the situation for all devices, but I am unable to 
> judge the regression potential of changing this, as it affects a lot of 
> devices. I guess it’d would go through the next tree, and hopefully the 
> company QA teams can give it a good spin. I hoped that my patches, as I 
> have tested them, and AMD will hopefully too, could go into the current 
> merge window.

Yes, correct, the plan is to get the generic series queued as soon as rc1
so that it can spend plenty of time in linux-next for people to test. That
will hopefully reduce the risk of breaking things in the field. Same for
the default LPM change.

With the default removal of the debounce delay, your patches addressing
only the AMD adapter are not needed anymore: this adapter will not have a
debounce delay unless the ATA_LFLAG_DEBOUNCE_DELAY flag is set.

> 
>> It would be nice if you can test though.
> 
> Of course, I am going to that either way.

Series posted with you on CC. Please test !

> 
> 
> Kind regards,
> 
> Paul
Paul Menzel March 23, 2022, 8:36 a.m. UTC | #5 
Dear Damien,


Am 23.03.22 um 09:24 schrieb Damien Le Moal:
> On 3/23/22 15:55, Paul Menzel wrote:

>> Am 23.03.22 um 06:01 schrieb Damien Le Moal:
>>> On 3/22/22 06:51, Limonciello, Mario wrote:

>>>>> -----Original Message-----
>>>>> From: Paul Menzel <pmenzel@molgen.mpg.de>
>>>>> Sent: Monday, March 21, 2022 16:25
>>
>> […]
>>
>>>> I seem to recall that we were talking about trying to drop the debounce delay for
>>>> everything, weren't we?
>>>>
>>>> So perhaps it would be right to add a 4th patch in the series to do just that.  Then
>>>> If this turns out to be problematic for anything other than the controllers in the
>>>> series that you identified as not problematic then that 4th patch can potentially
>>>> be reverted alone?
>>>
>>> Not quite everything :) But you are right, let's try to switch the default
>>> to no delay. I will be posting patches today for that.
>>>
>>> Paul,
>>>
>>> With these patches, your patches are not necessary anymore as the AMD
>>> chipset falls under the default no-delay.
>>
>> I am all for improving the situation for all devices, but I am unable to
>> judge the regression potential of changing this, as it affects a lot of
>> devices. I guess it’d would go through the next tree, and hopefully the
>> company QA teams can give it a good spin. I hoped that my patches, as I
>> have tested them, and AMD will hopefully too, could go into the current
>> merge window.
> 
> Yes, correct, the plan is to get the generic series queued as soon as rc1
> so that it can spend plenty of time in linux-next for people to test. That
> will hopefully reduce the risk of breaking things in the field. Same for
> the default LPM change.

But 5.18 or 5.19? If 5.18, sounds good to me, if 5.19, I’d be great if 
my patches go into 5.18 cycle, as they have been tested, and it would 
mean the whole change gets tested more widely already.

> With the default removal of the debounce delay, your patches addressing
> only the AMD adapter are not needed anymore: this adapter will not have a
> debounce delay unless the ATA_LFLAG_DEBOUNCE_DELAY flag is set.

Yes, I understand.

>>> It would be nice if you can test though.
>>
>> Of course, I am going to that either way.
> 
> Series posted with you on CC. Please test !

Thank you. I am going to test it in the coming days, and report back.

Maybe more people should be put in Cc (Dell, Lenovo, IBM, x86 subsystem) 
with a request to test this?


Kind regards,

Paul
Paul Menzel March 31, 2022, 2:42 p.m. UTC | #6 
Dear Damien,


Am 23.03.22 um 09:36 schrieb Paul Menzel:

> Am 23.03.22 um 09:24 schrieb Damien Le Moal:
>> On 3/23/22 15:55, Paul Menzel wrote:
> 
>>> Am 23.03.22 um 06:01 schrieb Damien Le Moal:
>>>> On 3/22/22 06:51, Limonciello, Mario wrote:
> 
>>>>>> -----Original Message-----
>>>>>> From: Paul Menzel <pmenzel@molgen.mpg.de>
>>>>>> Sent: Monday, March 21, 2022 16:25
>>>
>>> […]
>>>
>>>>> I seem to recall that we were talking about trying to drop the 
>>>>> debounce delay for everything, weren't we?
>>>>>
>>>>> So perhaps it would be right to add a 4th patch in the series to do
>>>>> just that.  Then If this turns out to be problematic for
>>>>> anything other than the controllers in the series that you
>>>>> identified as not problematic then that 4th patch can
>>>>> potentially be reverted alone?
>>>>
>>>> Not quite everything :) But you are right, let's try to switch the 
>>>> default to no delay. I will be posting patches today for that.
>>>> With these patches, your patches are not necessary anymore as the AMD
>>>> chipset falls under the default no-delay.
>>>
>>> I am all for improving the situation for all devices, but I am unable to
>>> judge the regression potential of changing this, as it affects a lot of
>>> devices. I guess it’d would go through the next tree, and hopefully the
>>> company QA teams can give it a good spin. I hoped that my patches, as I
>>> have tested them, and AMD will hopefully too, could go into the current
>>> merge window.
>>
>> Yes, correct, the plan is to get the generic series queued as soon
>> as rc1 so that it can spend plenty of time in linux-next for people
>> to test. That will hopefully reduce the risk of breaking things in
>> the field. Same for  the default LPM change.
> 
> But 5.18 or 5.19? If 5.18, sounds good to me, if 5.19, I’d be great if 
> my patches go into 5.18 cycle, as they have been tested, and it would 
> mean the whole change gets tested more widely already.
> 
>> With the default removal of the debounce delay, your patches addressing
>> only the AMD adapter are not needed anymore: this adapter will not have a
>> debounce delay unless the ATA_LFLAG_DEBOUNCE_DELAY flag is set.
> 
> Yes, I understand.

The merge window for Linux 5.18 is going to close in three days this 
Sunday. It’d be really great if my patches, tested on hardware, could go 
into that.

>>>> It would be nice if you can test though.
>>>
>>> Of course, I am going to that either way.
>>
>> Series posted with you on CC. Please test !
> 
> Thank you. I am going to test it in the coming days, and report back.
> 
> Maybe more people should be put in Cc (Dell, Lenovo, IBM, x86 subsystem) 
> with a request to test this?
Thank you for the patches, which are a big improvement. Let’s hope, you 
can re-roll them, so they get into Linux very soon for everyone’s benefit.


Kind regards,

Paul
Damien Le Moal March 31, 2022, 11:04 p.m. UTC | #7 
On 3/31/22 23:42, Paul Menzel wrote:
> Dear Damien,
> 
> 
> Am 23.03.22 um 09:36 schrieb Paul Menzel:
> 
>> Am 23.03.22 um 09:24 schrieb Damien Le Moal:
>>> On 3/23/22 15:55, Paul Menzel wrote:
>>
>>>> Am 23.03.22 um 06:01 schrieb Damien Le Moal:
>>>>> On 3/22/22 06:51, Limonciello, Mario wrote:
>>
>>>>>>> -----Original Message-----
>>>>>>> From: Paul Menzel <pmenzel@molgen.mpg.de>
>>>>>>> Sent: Monday, March 21, 2022 16:25
>>>>
>>>> […]
>>>>
>>>>>> I seem to recall that we were talking about trying to drop the 
>>>>>> debounce delay for everything, weren't we?
>>>>>>
>>>>>> So perhaps it would be right to add a 4th patch in the series to do
>>>>>> just that.  Then If this turns out to be problematic for
>>>>>> anything other than the controllers in the series that you
>>>>>> identified as not problematic then that 4th patch can
>>>>>> potentially be reverted alone?
>>>>>
>>>>> Not quite everything :) But you are right, let's try to switch the 
>>>>> default to no delay. I will be posting patches today for that.
>>>>> With these patches, your patches are not necessary anymore as the AMD
>>>>> chipset falls under the default no-delay.
>>>>
>>>> I am all for improving the situation for all devices, but I am unable to
>>>> judge the regression potential of changing this, as it affects a lot of
>>>> devices. I guess it’d would go through the next tree, and hopefully the
>>>> company QA teams can give it a good spin. I hoped that my patches, as I
>>>> have tested them, and AMD will hopefully too, could go into the current
>>>> merge window.
>>>
>>> Yes, correct, the plan is to get the generic series queued as soon
>>> as rc1 so that it can spend plenty of time in linux-next for people
>>> to test. That will hopefully reduce the risk of breaking things in
>>> the field. Same for  the default LPM change.
>>
>> But 5.18 or 5.19? If 5.18, sounds good to me, if 5.19, I’d be great if 
>> my patches go into 5.18 cycle, as they have been tested, and it would 
>> mean the whole change gets tested more widely already.
>>
>>> With the default removal of the debounce delay, your patches addressing
>>> only the AMD adapter are not needed anymore: this adapter will not have a
>>> debounce delay unless the ATA_LFLAG_DEBOUNCE_DELAY flag is set.
>>
>> Yes, I understand.
> 
> The merge window for Linux 5.18 is going to close in three days this 
> Sunday. It’d be really great if my patches, tested on hardware, could go 
> into that.
> 
>>>>> It would be nice if you can test though.
>>>>
>>>> Of course, I am going to that either way.
>>>
>>> Series posted with you on CC. Please test !
>>
>> Thank you. I am going to test it in the coming days, and report back.
>>
>> Maybe more people should be put in Cc (Dell, Lenovo, IBM, x86 subsystem) 
>> with a request to test this?
> Thank you for the patches, which are a big improvement. Let’s hope, you 
> can re-roll them, so they get into Linux very soon for everyone’s benefit.

I am waiting for 5.18-rc1 to rebase the patches and re-post them. Given
reviewed-by and tested-by tags, I will queue them for 5.19. With that in
mind, I am not planning to apply your previous patches for 5.18, as they
would conflict and would only end up being churn since the delay removal
by default will undo your changes.
Paul Menzel April 1, 2022, 5:18 a.m. UTC | #8 
Dear Damien,


Thank you for your reply.


Am 01.04.22 um 01:04 schrieb Damien Le Moal:
> On 3/31/22 23:42, Paul Menzel wrote:

>> Am 23.03.22 um 09:36 schrieb Paul Menzel:
>>
>>> Am 23.03.22 um 09:24 schrieb Damien Le Moal:
>>>> On 3/23/22 15:55, Paul Menzel wrote:
>>>
>>>>> Am 23.03.22 um 06:01 schrieb Damien Le Moal:
>>>>>> On 3/22/22 06:51, Limonciello, Mario wrote:
>>>
>>>>>>>> -----Original Message-----
>>>>>>>> From: Paul Menzel <pmenzel@molgen.mpg.de>
>>>>>>>> Sent: Monday, March 21, 2022 16:25
>>>>>
>>>>> […]
>>>>>
>>>>>>> I seem to recall that we were talking about trying to drop the
>>>>>>> debounce delay for everything, weren't we?
>>>>>>>
>>>>>>> So perhaps it would be right to add a 4th patch in the series to do
>>>>>>> just that.  Then If this turns out to be problematic for
>>>>>>> anything other than the controllers in the series that you
>>>>>>> identified as not problematic then that 4th patch can
>>>>>>> potentially be reverted alone?
>>>>>>
>>>>>> Not quite everything :) But you are right, let's try to switch the
>>>>>> default to no delay. I will be posting patches today for that.
>>>>>> With these patches, your patches are not necessary anymore as the AMD
>>>>>> chipset falls under the default no-delay.
>>>>>
>>>>> I am all for improving the situation for all devices, but I am unable to
>>>>> judge the regression potential of changing this, as it affects a lot of
>>>>> devices. I guess it’d would go through the next tree, and hopefully the
>>>>> company QA teams can give it a good spin. I hoped that my patches, as I
>>>>> have tested them, and AMD will hopefully too, could go into the current
>>>>> merge window.
>>>>
>>>> Yes, correct, the plan is to get the generic series queued as soon
>>>> as rc1 so that it can spend plenty of time in linux-next for people
>>>> to test. That will hopefully reduce the risk of breaking things in
>>>> the field. Same for  the default LPM change.
>>>
>>> But 5.18 or 5.19? If 5.18, sounds good to me, if 5.19, I’d be great if
>>> my patches go into 5.18 cycle, as they have been tested, and it would
>>> mean the whole change gets tested more widely already.
>>>
>>>> With the default removal of the debounce delay, your patches addressing
>>>> only the AMD adapter are not needed anymore: this adapter will not have a
>>>> debounce delay unless the ATA_LFLAG_DEBOUNCE_DELAY flag is set.
>>>
>>> Yes, I understand.
>>
>> The merge window for Linux 5.18 is going to close in three days this
>> Sunday. It’d be really great if my patches, tested on hardware, could go
>> into that.
>>
>>>>>> It would be nice if you can test though.
>>>>>
>>>>> Of course, I am going to that either way.
>>>>
>>>> Series posted with you on CC. Please test !
>>>
>>> Thank you. I am going to test it in the coming days, and report back.
>>>
>>> Maybe more people should be put in Cc (Dell, Lenovo, IBM, x86 subsystem)
>>> with a request to test this?
>> Thank you for the patches, which are a big improvement. Let’s hope, you
>> can re-roll them, so they get into Linux very soon for everyone’s benefit.
> 
> I am waiting for 5.18-rc1 to rebase the patches and re-post them. Given
> reviewed-by and tested-by tags, I will queue them for 5.19.

As discussed in the other thread, it’s impossible to be 100 % certain, 
it won’t break anything.

> With that in mind, I am not planning to apply your previous patches
> for 5.18, as they would conflict and would only end up being churn
> since the delay removal by default will undo your changes.
Obviously, I do not agree, as this would give the a little bit more 
testing already, if changing the default is a good idea. Also, if the 
conflict will be hard to resolve, I happily do it (the patches could 
even be reverted on top – git commits are cheap and easy to handle).

Anyway, I wrote my piece, but you are the maintainer, so it’s your call 
and I stop bothering you.


Kind regards,

Paul
Damien Le Moal April 1, 2022, 7:23 a.m. UTC | #9 
On 4/1/22 14:18, Paul Menzel wrote:
> Dear Damien,
> 
> 
> Thank you for your reply.
> 
> 
> Am 01.04.22 um 01:04 schrieb Damien Le Moal:
>> On 3/31/22 23:42, Paul Menzel wrote:
> 
>>> Am 23.03.22 um 09:36 schrieb Paul Menzel:
>>>
>>>> Am 23.03.22 um 09:24 schrieb Damien Le Moal:
>>>>> On 3/23/22 15:55, Paul Menzel wrote:
>>>>
>>>>>> Am 23.03.22 um 06:01 schrieb Damien Le Moal:
>>>>>>> On 3/22/22 06:51, Limonciello, Mario wrote:
>>>>
>>>>>>>>> -----Original Message-----
>>>>>>>>> From: Paul Menzel <pmenzel@molgen.mpg.de>
>>>>>>>>> Sent: Monday, March 21, 2022 16:25
>>>>>>
>>>>>> […]
>>>>>>
>>>>>>>> I seem to recall that we were talking about trying to drop the
>>>>>>>> debounce delay for everything, weren't we?
>>>>>>>>
>>>>>>>> So perhaps it would be right to add a 4th patch in the series to do
>>>>>>>> just that.  Then If this turns out to be problematic for
>>>>>>>> anything other than the controllers in the series that you
>>>>>>>> identified as not problematic then that 4th patch can
>>>>>>>> potentially be reverted alone?
>>>>>>>
>>>>>>> Not quite everything :) But you are right, let's try to switch the
>>>>>>> default to no delay. I will be posting patches today for that.
>>>>>>> With these patches, your patches are not necessary anymore as the AMD
>>>>>>> chipset falls under the default no-delay.
>>>>>>
>>>>>> I am all for improving the situation for all devices, but I am unable to
>>>>>> judge the regression potential of changing this, as it affects a lot of
>>>>>> devices. I guess it’d would go through the next tree, and hopefully the
>>>>>> company QA teams can give it a good spin. I hoped that my patches, as I
>>>>>> have tested them, and AMD will hopefully too, could go into the current
>>>>>> merge window.
>>>>>
>>>>> Yes, correct, the plan is to get the generic series queued as soon
>>>>> as rc1 so that it can spend plenty of time in linux-next for people
>>>>> to test. That will hopefully reduce the risk of breaking things in
>>>>> the field. Same for  the default LPM change.
>>>>
>>>> But 5.18 or 5.19? If 5.18, sounds good to me, if 5.19, I’d be great if
>>>> my patches go into 5.18 cycle, as they have been tested, and it would
>>>> mean the whole change gets tested more widely already.
>>>>
>>>>> With the default removal of the debounce delay, your patches addressing
>>>>> only the AMD adapter are not needed anymore: this adapter will not have a
>>>>> debounce delay unless the ATA_LFLAG_DEBOUNCE_DELAY flag is set.
>>>>
>>>> Yes, I understand.
>>>
>>> The merge window for Linux 5.18 is going to close in three days this
>>> Sunday. It’d be really great if my patches, tested on hardware, could go
>>> into that.
>>>
>>>>>>> It would be nice if you can test though.
>>>>>>
>>>>>> Of course, I am going to that either way.
>>>>>
>>>>> Series posted with you on CC. Please test !
>>>>
>>>> Thank you. I am going to test it in the coming days, and report back.
>>>>
>>>> Maybe more people should be put in Cc (Dell, Lenovo, IBM, x86 subsystem)
>>>> with a request to test this?
>>> Thank you for the patches, which are a big improvement. Let’s hope, you
>>> can re-roll them, so they get into Linux very soon for everyone’s benefit.
>>
>> I am waiting for 5.18-rc1 to rebase the patches and re-post them. Given
>> reviewed-by and tested-by tags, I will queue them for 5.19.
> 
> As discussed in the other thread, it’s impossible to be 100 % certain, 
> it won’t break anything.

Yes, that is why I want to push the patches early in the cycle to be able
to revert if too many problems are reported.

> 
>> With that in mind, I am not planning to apply your previous patches
>> for 5.18, as they would conflict and would only end up being churn
>> since the delay removal by default will undo your changes.
> Obviously, I do not agree, as this would give the a little bit more 
> testing already, if changing the default is a good idea. Also, if the 
> conflict will be hard to resolve, I happily do it (the patches could 
> even be reverted on top – git commits are cheap and easy to handle).

The conflict is not hard to resolve. The point is that my patches changing
the default to no debounce delay completely remove the changes of your
patch to do the same for one or some adapters. So adding your patches now
and then my patches on top does not make much sense at all.

If too many problems show up and I end up reverting/removing the patches,
then I will be happy to take your patches for the adapter you tested. Note
that *all* the machines I have tested so far are OK without a debounce
delay too. So we could add them too... And endup with a long list of
adapters that use the default ahci driver without debounce delay. The goal
of changing the default to no delay is to avoid that. So far, the adapters
I have identified that need the delay have their own declaration, so we
only need to add a flag there. Simpler change that listing up adapters
that are OK without the delay.

> 
> Anyway, I wrote my piece, but you are the maintainer, so it’s your call 
> and I stop bothering you.
> 
> 
> Kind regards,
> 
> Paul
Paul Menzel May 31, 2022, 4:18 p.m. UTC | #10 
Dear Damien,


Am 01.04.22 um 09:23 schrieb Damien Le Moal:
> On 4/1/22 14:18, Paul Menzel wrote:

[…]

>> Am 01.04.22 um 01:04 schrieb Damien Le Moal:
>>> On 3/31/22 23:42, Paul Menzel wrote:
>>
>>>> Am 23.03.22 um 09:36 schrieb Paul Menzel:
>>>>
>>>>> Am 23.03.22 um 09:24 schrieb Damien Le Moal:
>>>>>> On 3/23/22 15:55, Paul Menzel wrote:
>>>>>
>>>>>>> Am 23.03.22 um 06:01 schrieb Damien Le Moal:
>>>>>>>> On 3/22/22 06:51, Limonciello, Mario wrote:
>>>>>
>>>>>>>>>> -----Original Message-----
>>>>>>>>>> From: Paul Menzel <pmenzel@molgen.mpg.de>
>>>>>>>>>> Sent: Monday, March 21, 2022 16:25
>>>>>>>
>>>>>>> […]
>>>>>>>
>>>>>>>>> I seem to recall that we were talking about trying to drop the
>>>>>>>>> debounce delay for everything, weren't we?
>>>>>>>>>
>>>>>>>>> So perhaps it would be right to add a 4th patch in the series to do
>>>>>>>>> just that.  Then If this turns out to be problematic for
>>>>>>>>> anything other than the controllers in the series that you
>>>>>>>>> identified as not problematic then that 4th patch can
>>>>>>>>> potentially be reverted alone?
>>>>>>>>
>>>>>>>> Not quite everything :) But you are right, let's try to switch the
>>>>>>>> default to no delay. I will be posting patches today for that.
>>>>>>>> With these patches, your patches are not necessary anymore as the AMD
>>>>>>>> chipset falls under the default no-delay.
>>>>>>>
>>>>>>> I am all for improving the situation for all devices, but I am unable to
>>>>>>> judge the regression potential of changing this, as it affects a lot of
>>>>>>> devices. I guess it’d would go through the next tree, and hopefully the
>>>>>>> company QA teams can give it a good spin. I hoped that my patches, as I
>>>>>>> have tested them, and AMD will hopefully too, could go into the current
>>>>>>> merge window.
>>>>>>
>>>>>> Yes, correct, the plan is to get the generic series queued as soon
>>>>>> as rc1 so that it can spend plenty of time in linux-next for people
>>>>>> to test. That will hopefully reduce the risk of breaking things in
>>>>>> the field. Same for  the default LPM change.
>>>>>
>>>>> But 5.18 or 5.19? If 5.18, sounds good to me, if 5.19, I’d be great if
>>>>> my patches go into 5.18 cycle, as they have been tested, and it would
>>>>> mean the whole change gets tested more widely already.
>>>>>
>>>>>> With the default removal of the debounce delay, your patches addressing
>>>>>> only the AMD adapter are not needed anymore: this adapter will not have a
>>>>>> debounce delay unless the ATA_LFLAG_DEBOUNCE_DELAY flag is set.
>>>>>
>>>>> Yes, I understand.
>>>>
>>>> The merge window for Linux 5.18 is going to close in three days this
>>>> Sunday. It’d be really great if my patches, tested on hardware, could go
>>>> into that.
>>>>
>>>>>>>> It would be nice if you can test though.
>>>>>>>
>>>>>>> Of course, I am going to that either way.
>>>>>>
>>>>>> Series posted with you on CC. Please test !
>>>>>
>>>>> Thank you. I am going to test it in the coming days, and report back.
>>>>>
>>>>> Maybe more people should be put in Cc (Dell, Lenovo, IBM, x86 subsystem)
>>>>> with a request to test this?
>>>> Thank you for the patches, which are a big improvement. Let’s hope, you
>>>> can re-roll them, so they get into Linux very soon for everyone’s benefit.
>>>
>>> I am waiting for 5.18-rc1 to rebase the patches and re-post them. Given
>>> reviewed-by and tested-by tags, I will queue them for 5.19.
>>
>> As discussed in the other thread, it’s impossible to be 100 % certain,
>> it won’t break anything.
> 
> Yes, that is why I want to push the patches early in the cycle to be able
> to revert if too many problems are reported.
> 
>>
>>> With that in mind, I am not planning to apply your previous patches
>>> for 5.18, as they would conflict and would only end up being churn
>>> since the delay removal by default will undo your changes.
>> Obviously, I do not agree, as this would give the a little bit more
>> testing already, if changing the default is a good idea. Also, if the
>> conflict will be hard to resolve, I happily do it (the patches could
>> even be reverted on top – git commits are cheap and easy to handle).
> 
> The conflict is not hard to resolve. The point is that my patches changing
> the default to no debounce delay completely remove the changes of your
> patch to do the same for one or some adapters. So adding your patches now
> and then my patches on top does not make much sense at all.
> 
> If too many problems show up and I end up reverting/removing the patches,
> then I will be happy to take your patches for the adapter you tested. Note
> that *all* the machines I have tested so far are OK without a debounce
> delay too. So we could add them too... And endup with a long list of
> adapters that use the default ahci driver without debounce delay. The goal
> of changing the default to no delay is to avoid that. So far, the adapters
> I have identified that need the delay have their own declaration, so we
> only need to add a flag there. Simpler change that listing up adapters
> that are OK without the delay.
> 
>> Anyway, I wrote my piece, but you are the maintainer, so it’s your call
>> and I stop bothering you.

I just wanted to inquire about the status of your changes? I do not find 
them in your `for-5.19` branch. As they should be tested in linux-next 
before the merge window opens, if these are not ready yet, could you 
please apply my (tested) patches?


Kind regards,

Paul
Paul Menzel May 31, 2022, 4:21 p.m. UTC | #11 
[Cc: -Nehal-bakulchandra (undeliverable)

Am 31.05.22 um 18:18 schrieb Paul Menzel:
> Dear Damien,
> 
> 
> Am 01.04.22 um 09:23 schrieb Damien Le Moal:
>> On 4/1/22 14:18, Paul Menzel wrote:
> 
> […]
> 
>>> Am 01.04.22 um 01:04 schrieb Damien Le Moal:
>>>> On 3/31/22 23:42, Paul Menzel wrote:
>>>
>>>>> Am 23.03.22 um 09:36 schrieb Paul Menzel:
>>>>>
>>>>>> Am 23.03.22 um 09:24 schrieb Damien Le Moal:
>>>>>>> On 3/23/22 15:55, Paul Menzel wrote:
>>>>>>
>>>>>>>> Am 23.03.22 um 06:01 schrieb Damien Le Moal:
>>>>>>>>> On 3/22/22 06:51, Limonciello, Mario wrote:
>>>>>>
>>>>>>>>>>> -----Original Message-----
>>>>>>>>>>> From: Paul Menzel <pmenzel@molgen.mpg.de>
>>>>>>>>>>> Sent: Monday, March 21, 2022 16:25
>>>>>>>>
>>>>>>>> […]
>>>>>>>>
>>>>>>>>>> I seem to recall that we were talking about trying to drop the
>>>>>>>>>> debounce delay for everything, weren't we?
>>>>>>>>>>
>>>>>>>>>> So perhaps it would be right to add a 4th patch in the series 
>>>>>>>>>> to do
>>>>>>>>>> just that.  Then If this turns out to be problematic for
>>>>>>>>>> anything other than the controllers in the series that you
>>>>>>>>>> identified as not problematic then that 4th patch can
>>>>>>>>>> potentially be reverted alone?
>>>>>>>>>
>>>>>>>>> Not quite everything :) But you are right, let's try to switch the
>>>>>>>>> default to no delay. I will be posting patches today for that.
>>>>>>>>> With these patches, your patches are not necessary anymore as the AMD
>>>>>>>>> chipset falls under the default no-delay.
>>>>>>>>
>>>>>>>> I am all for improving the situation for all devices, but I am unable to
>>>>>>>> judge the regression potential of changing this, as it affects a lot of
>>>>>>>> devices. I guess it’d would go through the next tree, and hopefully the
>>>>>>>> company QA teams can give it a good spin. I hoped that my patches, as I
>>>>>>>> have tested them, and AMD will hopefully too, could go into the current
>>>>>>>> merge window.
>>>>>>>
>>>>>>> Yes, correct, the plan is to get the generic series queued as soon
>>>>>>> as rc1 so that it can spend plenty of time in linux-next for people
>>>>>>> to test. That will hopefully reduce the risk of breaking things in
>>>>>>> the field. Same for  the default LPM change.
>>>>>>
>>>>>> But 5.18 or 5.19? If 5.18, sounds good to me, if 5.19, I’d be great if
>>>>>> my patches go into 5.18 cycle, as they have been tested, and it would
>>>>>> mean the whole change gets tested more widely already.
>>>>>>
>>>>>>> With the default removal of the debounce delay, your patches addressing
>>>>>>> only the AMD adapter are not needed anymore: this adapter will not have a
>>>>>>> debounce delay unless the ATA_LFLAG_DEBOUNCE_DELAY flag is set.
>>>>>>
>>>>>> Yes, I understand.
>>>>>
>>>>> The merge window for Linux 5.18 is going to close in three days this
>>>>> Sunday. It’d be really great if my patches, tested on hardware, 
>>>>> could go into that.
>>>>>
>>>>>>>>> It would be nice if you can test though.
>>>>>>>>
>>>>>>>> Of course, I am going to that either way.
>>>>>>>
>>>>>>> Series posted with you on CC. Please test !
>>>>>>
>>>>>> Thank you. I am going to test it in the coming days, and report back.
>>>>>>
>>>>>> Maybe more people should be put in Cc (Dell, Lenovo, IBM, x86 
>>>>>> subsystem) with a request to test this?
>>>>> Thank you for the patches, which are a big improvement. Let’s hope, you
>>>>> can re-roll them, so they get into Linux very soon for everyone’s 
>>>>> benefit.
>>>>
>>>> I am waiting for 5.18-rc1 to rebase the patches and re-post them. Given
>>>> reviewed-by and tested-by tags, I will queue them for 5.19.
>>>
>>> As discussed in the other thread, it’s impossible to be 100 % certain,
>>> it won’t break anything.
>>
>> Yes, that is why I want to push the patches early in the cycle to be able
>> to revert if too many problems are reported.
>>
>>>> With that in mind, I am not planning to apply your previous patches
>>>> for 5.18, as they would conflict and would only end up being churn
>>>> since the delay removal by default will undo your changes.
>>> Obviously, I do not agree, as this would give the a little bit more
>>> testing already, if changing the default is a good idea. Also, if the
>>> conflict will be hard to resolve, I happily do it (the patches could
>>> even be reverted on top – git commits are cheap and easy to handle).
>>
>> The conflict is not hard to resolve. The point is that my patches changing
>> the default to no debounce delay completely remove the changes of your
>> patch to do the same for one or some adapters. So adding your patches now
>> and then my patches on top does not make much sense at all.
>>
>> If too many problems show up and I end up reverting/removing the patches,
>> then I will be happy to take your patches for the adapter you tested. Note
>> that *all* the machines I have tested so far are OK without a debounce
>> delay too. So we could add them too... And endup with a long list of
>> adapters that use the default ahci driver without debounce delay. The goal
>> of changing the default to no delay is to avoid that. So far, the adapters
>> I have identified that need the delay have their own declaration, so we
>> only need to add a flag there. Simpler change that listing up adapters
>> that are OK without the delay.
>>
>>> Anyway, I wrote my piece, but you are the maintainer, so it’s your call
>>> and I stop bothering you.
> 
> I just wanted to inquire about the status of your changes? I do not find 
> them in your `for-5.19` branch. As they should be tested in linux-next 
> before the merge window opens, if these are not ready yet, could you 
> please apply my (tested) patches?
> 
> 
> Kind regards,
> 
> Paul
Paul Menzel May 31, 2022, 4:24 p.m. UTC | #12 
[Now really. Sorry for the spam.]

Am 31.05.22 um 18:21 schrieb Paul Menzel:
> [Cc: -Nehal-bakulchandra (undeliverable)]
> 
> Am 31.05.22 um 18:18 schrieb Paul Menzel:
>> Dear Damien,
>>
>>
>> Am 01.04.22 um 09:23 schrieb Damien Le Moal:
>>> On 4/1/22 14:18, Paul Menzel wrote:
>>
>> […]
>>
>>>> Am 01.04.22 um 01:04 schrieb Damien Le Moal:
>>>>> On 3/31/22 23:42, Paul Menzel wrote:
>>>>
>>>>>> Am 23.03.22 um 09:36 schrieb Paul Menzel:
>>>>>>
>>>>>>> Am 23.03.22 um 09:24 schrieb Damien Le Moal:
>>>>>>>> On 3/23/22 15:55, Paul Menzel wrote:
>>>>>>>
>>>>>>>>> Am 23.03.22 um 06:01 schrieb Damien Le Moal:
>>>>>>>>>> On 3/22/22 06:51, Limonciello, Mario wrote:
>>>>>>>
>>>>>>>>>>>> -----Original Message-----
>>>>>>>>>>>> From: Paul Menzel <pmenzel@molgen.mpg.de>
>>>>>>>>>>>> Sent: Monday, March 21, 2022 16:25
>>>>>>>>>
>>>>>>>>> […]
>>>>>>>>>
>>>>>>>>>>> I seem to recall that we were talking about trying to drop the
>>>>>>>>>>> debounce delay for everything, weren't we?
>>>>>>>>>>>
>>>>>>>>>>> So perhaps it would be right to add a 4th patch in the series 
>>>>>>>>>>> to do
>>>>>>>>>>> just that.  Then If this turns out to be problematic for
>>>>>>>>>>> anything other than the controllers in the series that you
>>>>>>>>>>> identified as not problematic then that 4th patch can
>>>>>>>>>>> potentially be reverted alone?
>>>>>>>>>>
>>>>>>>>>> Not quite everything :) But you are right, let's try to switch the
>>>>>>>>>> default to no delay. I will be posting patches today for that.
>>>>>>>>>> With these patches, your patches are not necessary anymore as the AMD
>>>>>>>>>> chipset falls under the default no-delay.
>>>>>>>>>
>>>>>>>>> I am all for improving the situation for all devices, but I am unable to
>>>>>>>>> judge the regression potential of changing this, as it affects a lot of
>>>>>>>>> devices. I guess it’d would go through the next tree, and hopefully the
>>>>>>>>> company QA teams can give it a good spin. I hoped that my patches, as I
>>>>>>>>> have tested them, and AMD will hopefully too, could go into the current
>>>>>>>>> merge window.
>>>>>>>>
>>>>>>>> Yes, correct, the plan is to get the generic series queued as soon
>>>>>>>> as rc1 so that it can spend plenty of time in linux-next for people
>>>>>>>> to test. That will hopefully reduce the risk of breaking things in
>>>>>>>> the field. Same for  the default LPM change.
>>>>>>>
>>>>>>> But 5.18 or 5.19? If 5.18, sounds good to me, if 5.19, I’d be great if
>>>>>>> my patches go into 5.18 cycle, as they have been tested, and it would
>>>>>>> mean the whole change gets tested more widely already.
>>>>>>>
>>>>>>>> With the default removal of the debounce delay, your patches addressing
>>>>>>>> only the AMD adapter are not needed anymore: this adapter will not have a
>>>>>>>> debounce delay unless the ATA_LFLAG_DEBOUNCE_DELAY flag is set.
>>>>>>>
>>>>>>> Yes, I understand.
>>>>>>
>>>>>> The merge window for Linux 5.18 is going to close in three days this
>>>>>> Sunday. It’d be really great if my patches, tested on hardware, 
>>>>>> could go into that.
>>>>>>
>>>>>>>>>> It would be nice if you can test though.
>>>>>>>>>
>>>>>>>>> Of course, I am going to that either way.
>>>>>>>>
>>>>>>>> Series posted with you on CC. Please test !
>>>>>>>
>>>>>>> Thank you. I am going to test it in the coming days, and report 
>>>>>>> back.
>>>>>>>
>>>>>>> Maybe more people should be put in Cc (Dell, Lenovo, IBM, x86 
>>>>>>> subsystem) with a request to test this?
>>>>>> Thank you for the patches, which are a big improvement. Let’s hope, you
>>>>>> can re-roll them, so they get into Linux very soon for everyone’s 
>>>>>> benefit.
>>>>>
>>>>> I am waiting for 5.18-rc1 to rebase the patches and re-post them. Given
>>>>> reviewed-by and tested-by tags, I will queue them for 5.19.
>>>>
>>>> As discussed in the other thread, it’s impossible to be 100 % certain,
>>>> it won’t break anything.
>>>
>>> Yes, that is why I want to push the patches early in the cycle to be able
>>> to revert if too many problems are reported.
>>>
>>>>> With that in mind, I am not planning to apply your previous patches
>>>>> for 5.18, as they would conflict and would only end up being churn
>>>>> since the delay removal by default will undo your changes.
>>>> Obviously, I do not agree, as this would give the a little bit more
>>>> testing already, if changing the default is a good idea. Also, if the
>>>> conflict will be hard to resolve, I happily do it (the patches could
>>>> even be reverted on top – git commits are cheap and easy to handle).
>>>
>>> The conflict is not hard to resolve. The point is that my patches changing
>>> the default to no debounce delay completely remove the changes of your
>>> patch to do the same for one or some adapters. So adding your patches now
>>> and then my patches on top does not make much sense at all.
>>>
>>> If too many problems show up and I end up reverting/removing the patches,
>>> then I will be happy to take your patches for the adapter you tested. Note
>>> that *all* the machines I have tested so far are OK without a debounce
>>> delay too. So we could add them too... And endup with a long list of
>>> adapters that use the default ahci driver without debounce delay. The goal
>>> of changing the default to no delay is to avoid that. So far, the adapters
>>> I have identified that need the delay have their own declaration, so we
>>> only need to add a flag there. Simpler change that listing up adapters
>>> that are OK without the delay.
>>>
>>>> Anyway, I wrote my piece, but you are the maintainer, so it’s your call
>>>> and I stop bothering you.
>>
>> I just wanted to inquire about the status of your changes? I do not 
>> find them in your `for-5.19` branch. As they should be tested in 
>> linux-next before the merge window opens, if these are not ready yet, 
>> could you please apply my (tested) patches?
>>
>>
>> Kind regards,
>>
>> Paul
Damien Le Moal June 1, 2022, 8:58 a.m. UTC | #13 
On 6/1/22 01:18, Paul Menzel wrote:
>>>> With that in mind, I am not planning to apply your previous patches
>>>> for 5.18, as they would conflict and would only end up being churn
>>>> since the delay removal by default will undo your changes.
>>> Obviously, I do not agree, as this would give the a little bit more
>>> testing already, if changing the default is a good idea. Also, if the
>>> conflict will be hard to resolve, I happily do it (the patches could
>>> even be reverted on top – git commits are cheap and easy to handle).
>>
>> The conflict is not hard to resolve. The point is that my patches changing
>> the default to no debounce delay completely remove the changes of your
>> patch to do the same for one or some adapters. So adding your patches now
>> and then my patches on top does not make much sense at all.
>>
>> If too many problems show up and I end up reverting/removing the patches,
>> then I will be happy to take your patches for the adapter you tested. Note
>> that *all* the machines I have tested so far are OK without a debounce
>> delay too. So we could add them too... And endup with a long list of
>> adapters that use the default ahci driver without debounce delay. The goal
>> of changing the default to no delay is to avoid that. So far, the adapters
>> I have identified that need the delay have their own declaration, so we
>> only need to add a flag there. Simpler change that listing up adapters
>> that are OK without the delay.
>>
>>> Anyway, I wrote my piece, but you are the maintainer, so it’s your call
>>> and I stop bothering you.
> 
> I just wanted to inquire about the status of your changes? I do not find 
> them in your `for-5.19` branch. As they should be tested in linux-next 
> before the merge window opens, if these are not ready yet, could you 
> please apply my (tested) patches?

I could, but 5.19 now has an updated libata.force kernel parameter that
allows one to disable the debounce delay for a particular port or for all
ports of an adapter. See libata.force=x.y:nodbdelay for a port y of
adapter x or libata.force=x:nodbdelay for all ports of adapter x.

I still plan to revisit the arbitrary link debounce timers but I prefer to
have the power management cleanup applied first. The reason is that link
debounce depends on PHY readiness, which itself depends heavily on power
mode transitions. My plan is to get this done during this cycle for
release with 5.20 and then fix on top the arbitrary delays for 5.21.

Is the libata.force solution OK for you until we have a final more solid
fix that can benefit most modern adapters (and not just the ones you
identified) ? If you do have a use case that needs a "nodbdelay" horkage
due to some constraint in the field, then I will apply your patches, but
they likely will be voided by coming changes. Let me know.

Cheers.
Paul Menzel Aug. 30, 2022, 9:05 a.m. UTC | #14 
Dear Damien,


Sorry for the late reply, and thank you for your great work.

Am 01.06.22 um 10:58 schrieb Damien Le Moal:
> On 6/1/22 01:18, Paul Menzel wrote:
>>>>> With that in mind, I am not planning to apply your previous patches
>>>>> for 5.18, as they would conflict and would only end up being churn
>>>>> since the delay removal by default will undo your changes.
>>>> Obviously, I do not agree, as this would give the a little bit more
>>>> testing already, if changing the default is a good idea. Also, if the
>>>> conflict will be hard to resolve, I happily do it (the patches could
>>>> even be reverted on top – git commits are cheap and easy to handle).
>>>
>>> The conflict is not hard to resolve. The point is that my patches changing
>>> the default to no debounce delay completely remove the changes of your
>>> patch to do the same for one or some adapters. So adding your patches now
>>> and then my patches on top does not make much sense at all.
>>>
>>> If too many problems show up and I end up reverting/removing the patches,
>>> then I will be happy to take your patches for the adapter you tested. Note
>>> that *all* the machines I have tested so far are OK without a debounce
>>> delay too. So we could add them too... And endup with a long list of
>>> adapters that use the default ahci driver without debounce delay. The goal
>>> of changing the default to no delay is to avoid that. So far, the adapters
>>> I have identified that need the delay have their own declaration, so we
>>> only need to add a flag there. Simpler change that listing up adapters
>>> that are OK without the delay.
>>>
>>>> Anyway, I wrote my piece, but you are the maintainer, so it’s your call
>>>> and I stop bothering you.
>>
>> I just wanted to inquire about the status of your changes? I do not find
>> them in your `for-5.19` branch. As they should be tested in linux-next
>> before the merge window opens, if these are not ready yet, could you
>> please apply my (tested) patches?
> 
> I could, but 5.19 now has an updated libata.force kernel parameter that
> allows one to disable the debounce delay for a particular port or for all
> ports of an adapter. See libata.force=x.y:nodbdelay for a port y of
> adapter x or libata.force=x:nodbdelay for all ports of adapter x.

This is commit 3af9ca4d341d (ata: libata-core: Improve link flags forced 
settings) [1]. Thank you, this is really useful, but easily overlooked. ;-)

> I still plan to revisit the arbitrary link debounce timers but I prefer to
> have the power management cleanup applied first. The reason is that link
> debounce depends on PHY readiness, which itself depends heavily on power
> mode transitions. My plan is to get this done during this cycle for
> release with 5.20 and then fix on top the arbitrary delays for 5.21.

Nice. Can you share the current status?

> Is the libata.force solution OK for you until we have a final more solid
> fix that can benefit most modern adapters (and not just the ones you
> identified)? If you do have a use case that needs a "nodbdelay" horkage
> due to some constraint in the field, then I will apply your patches, but
> they likely will be voided by coming changes. Let me know.

I think, applying the patch would be an improvement, as people wouldn’t 
need to update their Linux kernel command line, and I do not mind, if it 
gets reverted/dropped later.


Kind regards,

Paul


[1]: 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=3af9ca4d341d2b8756fa9056ca0715915480e251
Damien Le Moal Aug. 31, 2022, 10:13 p.m. UTC | #15 
On 8/30/22 18:05, Paul Menzel wrote:
> Dear Damien,
> 
> 
> Sorry for the late reply, and thank you for your great work.
> 
> Am 01.06.22 um 10:58 schrieb Damien Le Moal:
>> On 6/1/22 01:18, Paul Menzel wrote:
>>>>>> With that in mind, I am not planning to apply your previous patches
>>>>>> for 5.18, as they would conflict and would only end up being churn
>>>>>> since the delay removal by default will undo your changes.
>>>>> Obviously, I do not agree, as this would give the a little bit more
>>>>> testing already, if changing the default is a good idea. Also, if the
>>>>> conflict will be hard to resolve, I happily do it (the patches could
>>>>> even be reverted on top – git commits are cheap and easy to handle).
>>>>
>>>> The conflict is not hard to resolve. The point is that my patches changing
>>>> the default to no debounce delay completely remove the changes of your
>>>> patch to do the same for one or some adapters. So adding your patches now
>>>> and then my patches on top does not make much sense at all.
>>>>
>>>> If too many problems show up and I end up reverting/removing the patches,
>>>> then I will be happy to take your patches for the adapter you tested. Note
>>>> that *all* the machines I have tested so far are OK without a debounce
>>>> delay too. So we could add them too... And endup with a long list of
>>>> adapters that use the default ahci driver without debounce delay. The goal
>>>> of changing the default to no delay is to avoid that. So far, the adapters
>>>> I have identified that need the delay have their own declaration, so we
>>>> only need to add a flag there. Simpler change that listing up adapters
>>>> that are OK without the delay.
>>>>
>>>>> Anyway, I wrote my piece, but you are the maintainer, so it’s your call
>>>>> and I stop bothering you.
>>>
>>> I just wanted to inquire about the status of your changes? I do not find
>>> them in your `for-5.19` branch. As they should be tested in linux-next
>>> before the merge window opens, if these are not ready yet, could you
>>> please apply my (tested) patches?
>>
>> I could, but 5.19 now has an updated libata.force kernel parameter that
>> allows one to disable the debounce delay for a particular port or for all
>> ports of an adapter. See libata.force=x.y:nodbdelay for a port y of
>> adapter x or libata.force=x:nodbdelay for all ports of adapter x.
> 
> This is commit 3af9ca4d341d (ata: libata-core: Improve link flags forced 
> settings) [1]. Thank you, this is really useful, but easily overlooked. ;-)
> 
>> I still plan to revisit the arbitrary link debounce timers but I prefer to
>> have the power management cleanup applied first. The reason is that link
>> debounce depends on PHY readiness, which itself depends heavily on power
>> mode transitions. My plan is to get this done during this cycle for
>> release with 5.20 and then fix on top the arbitrary delays for 5.21.
> 
> Nice. Can you share the current status?

No progress. I need to put together a series with all the patches that
were sent already. Unless Mario can resend something ?

>> Is the libata.force solution OK for you until we have a final more solid
>> fix that can benefit most modern adapters (and not just the ones you
>> identified)? If you do have a use case that needs a "nodbdelay" horkage
>> due to some constraint in the field, then I will apply your patches, but
>> they likely will be voided by coming changes. Let me know.
> 
> I think, applying the patch would be an improvement, as people wouldn’t 
> need to update their Linux kernel command line, and I do not mind, if it 
> gets reverted/dropped later.

Let's see were the lpm stuff goes first.
Paul Menzel Sept. 13, 2022, 3:23 p.m. UTC | #16 
Dear Damien,


Am 01.09.22 um 00:13 schrieb Damien Le Moal:
> On 8/30/22 18:05, Paul Menzel wrote:

[…]

>> Am 01.06.22 um 10:58 schrieb Damien Le Moal:
>>> On 6/1/22 01:18, Paul Menzel wrote:
>>>>>>> With that in mind, I am not planning to apply your previous patches
>>>>>>> for 5.18, as they would conflict and would only end up being churn
>>>>>>> since the delay removal by default will undo your changes.
>>>>>> Obviously, I do not agree, as this would give the a little bit more
>>>>>> testing already, if changing the default is a good idea. Also, if the
>>>>>> conflict will be hard to resolve, I happily do it (the patches could
>>>>>> even be reverted on top – git commits are cheap and easy to handle).
>>>>>
>>>>> The conflict is not hard to resolve. The point is that my patches changing
>>>>> the default to no debounce delay completely remove the changes of your
>>>>> patch to do the same for one or some adapters. So adding your patches now
>>>>> and then my patches on top does not make much sense at all.
>>>>>
>>>>> If too many problems show up and I end up reverting/removing the patches,
>>>>> then I will be happy to take your patches for the adapter you tested. Note
>>>>> that *all* the machines I have tested so far are OK without a debounce
>>>>> delay too. So we could add them too... And endup with a long list of
>>>>> adapters that use the default ahci driver without debounce delay. The goal
>>>>> of changing the default to no delay is to avoid that. So far, the adapters
>>>>> I have identified that need the delay have their own declaration, so we
>>>>> only need to add a flag there. Simpler change that listing up adapters
>>>>> that are OK without the delay.
>>>>>
>>>>>> Anyway, I wrote my piece, but you are the maintainer, so it’s your call
>>>>>> and I stop bothering you.
>>>>
>>>> I just wanted to inquire about the status of your changes? I do not find
>>>> them in your `for-5.19` branch. As they should be tested in linux-next
>>>> before the merge window opens, if these are not ready yet, could you
>>>> please apply my (tested) patches?
>>>
>>> I could, but 5.19 now has an updated libata.force kernel parameter that
>>> allows one to disable the debounce delay for a particular port or for all
>>> ports of an adapter. See libata.force=x.y:nodbdelay for a port y of
>>> adapter x or libata.force=x:nodbdelay for all ports of adapter x.
>>
>> This is commit 3af9ca4d341d (ata: libata-core: Improve link flags forced
>> settings) [1]. Thank you, this is really useful, but easily overlooked. ;-)
>>
>>> I still plan to revisit the arbitrary link debounce timers but I prefer to
>>> have the power management cleanup applied first. The reason is that link
>>> debounce depends on PHY readiness, which itself depends heavily on power
>>> mode transitions. My plan is to get this done during this cycle for
>>> release with 5.20 and then fix on top the arbitrary delays for 5.21.
>>
>> Nice. Can you share the current status?
> 
> No progress. I need to put together a series with all the patches that
> were sent already. Unless Mario can resend something ?

No reply from Mario.

>>> Is the libata.force solution OK for you until we have a final more solid
>>> fix that can benefit most modern adapters (and not just the ones you
>>> identified)? If you do have a use case that needs a "nodbdelay" horkage
>>> due to some constraint in the field, then I will apply your patches, but
>>> they likely will be voided by coming changes. Let me know.
>>
>> I think, applying the patch would be an improvement, as people wouldn’t
>> need to update their Linux kernel command line, and I do not mind, if it
>> gets reverted/dropped later.
> 
> Let's see were the lpm stuff goes first.

It shouldn’t be too much hassle to adapt the lpm series after the patch 
is applied.


Kind regards,

Paul
Mario Limonciello Sept. 13, 2022, 3:28 p.m. UTC | #17 
[Public]



> -----Original Message-----
> From: Paul Menzel <pmenzel@molgen.mpg.de>
> Sent: Tuesday, September 13, 2022 10:23
> To: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> Cc: Limonciello, Mario <Mario.Limonciello@amd.com>; Hans de Goede
> <hdegoede@redhat.com>; linux-ide@vger.kernel.org; LKML <linux-
> kernel@vger.kernel.org>
> Subject: Re: [PATCH v2 3/3] ata: ahci: Skip 200 ms debounce delay for AMD 300
> Series Chipset SATA Controller
> 
> Dear Damien,
> 
> 
> Am 01.09.22 um 00:13 schrieb Damien Le Moal:
> > On 8/30/22 18:05, Paul Menzel wrote:
> 
> […]
> 
> >> Am 01.06.22 um 10:58 schrieb Damien Le Moal:
> >>> On 6/1/22 01:18, Paul Menzel wrote:
> >>>>>>> With that in mind, I am not planning to apply your previous patches
> >>>>>>> for 5.18, as they would conflict and would only end up being churn
> >>>>>>> since the delay removal by default will undo your changes.
> >>>>>> Obviously, I do not agree, as this would give the a little bit more
> >>>>>> testing already, if changing the default is a good idea. Also, if the
> >>>>>> conflict will be hard to resolve, I happily do it (the patches could
> >>>>>> even be reverted on top – git commits are cheap and easy to handle).
> >>>>>
> >>>>> The conflict is not hard to resolve. The point is that my patches changing
> >>>>> the default to no debounce delay completely remove the changes of your
> >>>>> patch to do the same for one or some adapters. So adding your patches
> now
> >>>>> and then my patches on top does not make much sense at all.
> >>>>>
> >>>>> If too many problems show up and I end up reverting/removing the
> patches,
> >>>>> then I will be happy to take your patches for the adapter you tested. Note
> >>>>> that *all* the machines I have tested so far are OK without a debounce
> >>>>> delay too. So we could add them too... And endup with a long list of
> >>>>> adapters that use the default ahci driver without debounce delay. The
> goal
> >>>>> of changing the default to no delay is to avoid that. So far, the adapters
> >>>>> I have identified that need the delay have their own declaration, so we
> >>>>> only need to add a flag there. Simpler change that listing up adapters
> >>>>> that are OK without the delay.
> >>>>>
> >>>>>> Anyway, I wrote my piece, but you are the maintainer, so it’s your call
> >>>>>> and I stop bothering you.
> >>>>
> >>>> I just wanted to inquire about the status of your changes? I do not find
> >>>> them in your `for-5.19` branch. As they should be tested in linux-next
> >>>> before the merge window opens, if these are not ready yet, could you
> >>>> please apply my (tested) patches?
> >>>
> >>> I could, but 5.19 now has an updated libata.force kernel parameter that
> >>> allows one to disable the debounce delay for a particular port or for all
> >>> ports of an adapter. See libata.force=x.y:nodbdelay for a port y of
> >>> adapter x or libata.force=x:nodbdelay for all ports of adapter x.
> >>
> >> This is commit 3af9ca4d341d (ata: libata-core: Improve link flags forced
> >> settings) [1]. Thank you, this is really useful, but easily overlooked. ;-)
> >>
> >>> I still plan to revisit the arbitrary link debounce timers but I prefer to
> >>> have the power management cleanup applied first. The reason is that link
> >>> debounce depends on PHY readiness, which itself depends heavily on power
> >>> mode transitions. My plan is to get this done during this cycle for
> >>> release with 5.20 and then fix on top the arbitrary delays for 5.21.
> >>
> >> Nice. Can you share the current status?
> >
> > No progress. I need to put together a series with all the patches that
> > were sent already. Unless Mario can resend something ?
> 
> No reply from Mario.

I think what happened here is there was related patches from another party
that got tangled up with this.

> 
> >>> Is the libata.force solution OK for you until we have a final more solid
> >>> fix that can benefit most modern adapters (and not just the ones you
> >>> identified)? If you do have a use case that needs a "nodbdelay" horkage
> >>> due to some constraint in the field, then I will apply your patches, but
> >>> they likely will be voided by coming changes. Let me know.
> >>
> >> I think, applying the patch would be an improvement, as people wouldn’t
> >> need to update their Linux kernel command line, and I do not mind, if it
> >> gets reverted/dropped later.
> >
> > Let's see were the lpm stuff goes first.
> 
> It shouldn’t be too much hassle to adapt the lpm series after the patch
> is applied.
> 
> 
> Kind regards,
> 
> Paul
Damien Le Moal Sept. 14, 2022, 8:26 a.m. UTC | #18 
On 2022/09/13 16:28, Limonciello, Mario wrote:
> [Public]
> 
> 
> 
>> -----Original Message-----
>> From: Paul Menzel <pmenzel@molgen.mpg.de>
>> Sent: Tuesday, September 13, 2022 10:23
>> To: Damien Le Moal <damien.lemoal@opensource.wdc.com>
>> Cc: Limonciello, Mario <Mario.Limonciello@amd.com>; Hans de Goede
>> <hdegoede@redhat.com>; linux-ide@vger.kernel.org; LKML <linux-
>> kernel@vger.kernel.org>
>> Subject: Re: [PATCH v2 3/3] ata: ahci: Skip 200 ms debounce delay for AMD 300
>> Series Chipset SATA Controller
>>
>> Dear Damien,
>>
>>
>> Am 01.09.22 um 00:13 schrieb Damien Le Moal:
>>> On 8/30/22 18:05, Paul Menzel wrote:
>>
>> […]
>>
>>>> Am 01.06.22 um 10:58 schrieb Damien Le Moal:
>>>>> On 6/1/22 01:18, Paul Menzel wrote:
>>>>>>>>> With that in mind, I am not planning to apply your previous patches
>>>>>>>>> for 5.18, as they would conflict and would only end up being churn
>>>>>>>>> since the delay removal by default will undo your changes.
>>>>>>>> Obviously, I do not agree, as this would give the a little bit more
>>>>>>>> testing already, if changing the default is a good idea. Also, if the
>>>>>>>> conflict will be hard to resolve, I happily do it (the patches could
>>>>>>>> even be reverted on top – git commits are cheap and easy to handle).
>>>>>>>
>>>>>>> The conflict is not hard to resolve. The point is that my patches changing
>>>>>>> the default to no debounce delay completely remove the changes of your
>>>>>>> patch to do the same for one or some adapters. So adding your patches
>> now
>>>>>>> and then my patches on top does not make much sense at all.
>>>>>>>
>>>>>>> If too many problems show up and I end up reverting/removing the
>> patches,
>>>>>>> then I will be happy to take your patches for the adapter you tested. Note
>>>>>>> that *all* the machines I have tested so far are OK without a debounce
>>>>>>> delay too. So we could add them too... And endup with a long list of
>>>>>>> adapters that use the default ahci driver without debounce delay. The
>> goal
>>>>>>> of changing the default to no delay is to avoid that. So far, the adapters
>>>>>>> I have identified that need the delay have their own declaration, so we
>>>>>>> only need to add a flag there. Simpler change that listing up adapters
>>>>>>> that are OK without the delay.
>>>>>>>
>>>>>>>> Anyway, I wrote my piece, but you are the maintainer, so it’s your call
>>>>>>>> and I stop bothering you.
>>>>>>
>>>>>> I just wanted to inquire about the status of your changes? I do not find
>>>>>> them in your `for-5.19` branch. As they should be tested in linux-next
>>>>>> before the merge window opens, if these are not ready yet, could you
>>>>>> please apply my (tested) patches?
>>>>>
>>>>> I could, but 5.19 now has an updated libata.force kernel parameter that
>>>>> allows one to disable the debounce delay for a particular port or for all
>>>>> ports of an adapter. See libata.force=x.y:nodbdelay for a port y of
>>>>> adapter x or libata.force=x:nodbdelay for all ports of adapter x.
>>>>
>>>> This is commit 3af9ca4d341d (ata: libata-core: Improve link flags forced
>>>> settings) [1]. Thank you, this is really useful, but easily overlooked. ;-)
>>>>
>>>>> I still plan to revisit the arbitrary link debounce timers but I prefer to
>>>>> have the power management cleanup applied first. The reason is that link
>>>>> debounce depends on PHY readiness, which itself depends heavily on power
>>>>> mode transitions. My plan is to get this done during this cycle for
>>>>> release with 5.20 and then fix on top the arbitrary delays for 5.21.
>>>>
>>>> Nice. Can you share the current status?
>>>
>>> No progress. I need to put together a series with all the patches that
>>> were sent already. Unless Mario can resend something ?
>>
>> No reply from Mario.
> 
> I think what happened here is there was related patches from another party
> that got tangled up with this.

Yes, patches from Runa touch the same area. We need to put everything together i
a nice series. Will try to do so, but busy this week and next (Plumbers and
vacation). If you can Mario, that would be welcome too :)
Damien Le Moal Sept. 21, 2022, 5:01 a.m. UTC | #19 
On 9/14/22 00:28, Limonciello, Mario wrote:
> [Public]
> 
> 
> 
>> -----Original Message-----
>> From: Paul Menzel <pmenzel@molgen.mpg.de>
>> Sent: Tuesday, September 13, 2022 10:23
>> To: Damien Le Moal <damien.lemoal@opensource.wdc.com>
>> Cc: Limonciello, Mario <Mario.Limonciello@amd.com>; Hans de Goede
>> <hdegoede@redhat.com>; linux-ide@vger.kernel.org; LKML <linux-
>> kernel@vger.kernel.org>
>> Subject: Re: [PATCH v2 3/3] ata: ahci: Skip 200 ms debounce delay for AMD 300
>> Series Chipset SATA Controller
>>
>> Dear Damien,
>>
>>
>> Am 01.09.22 um 00:13 schrieb Damien Le Moal:
>>> On 8/30/22 18:05, Paul Menzel wrote:
>>
>> […]
>>
>>>> Am 01.06.22 um 10:58 schrieb Damien Le Moal:
>>>>> On 6/1/22 01:18, Paul Menzel wrote:
>>>>>>>>> With that in mind, I am not planning to apply your previous patches
>>>>>>>>> for 5.18, as they would conflict and would only end up being churn
>>>>>>>>> since the delay removal by default will undo your changes.
>>>>>>>> Obviously, I do not agree, as this would give the a little bit more
>>>>>>>> testing already, if changing the default is a good idea. Also, if the
>>>>>>>> conflict will be hard to resolve, I happily do it (the patches could
>>>>>>>> even be reverted on top – git commits are cheap and easy to handle).
>>>>>>>
>>>>>>> The conflict is not hard to resolve. The point is that my patches changing
>>>>>>> the default to no debounce delay completely remove the changes of your
>>>>>>> patch to do the same for one or some adapters. So adding your patches
>> now
>>>>>>> and then my patches on top does not make much sense at all.
>>>>>>>
>>>>>>> If too many problems show up and I end up reverting/removing the
>> patches,
>>>>>>> then I will be happy to take your patches for the adapter you tested. Note
>>>>>>> that *all* the machines I have tested so far are OK without a debounce
>>>>>>> delay too. So we could add them too... And endup with a long list of
>>>>>>> adapters that use the default ahci driver without debounce delay. The
>> goal
>>>>>>> of changing the default to no delay is to avoid that. So far, the adapters
>>>>>>> I have identified that need the delay have their own declaration, so we
>>>>>>> only need to add a flag there. Simpler change that listing up adapters
>>>>>>> that are OK without the delay.
>>>>>>>
>>>>>>>> Anyway, I wrote my piece, but you are the maintainer, so it’s your call
>>>>>>>> and I stop bothering you.
>>>>>>
>>>>>> I just wanted to inquire about the status of your changes? I do not find
>>>>>> them in your `for-5.19` branch. As they should be tested in linux-next
>>>>>> before the merge window opens, if these are not ready yet, could you
>>>>>> please apply my (tested) patches?
>>>>>
>>>>> I could, but 5.19 now has an updated libata.force kernel parameter that
>>>>> allows one to disable the debounce delay for a particular port or for all
>>>>> ports of an adapter. See libata.force=x.y:nodbdelay for a port y of
>>>>> adapter x or libata.force=x:nodbdelay for all ports of adapter x.
>>>>
>>>> This is commit 3af9ca4d341d (ata: libata-core: Improve link flags forced
>>>> settings) [1]. Thank you, this is really useful, but easily overlooked. ;-)
>>>>
>>>>> I still plan to revisit the arbitrary link debounce timers but I prefer to
>>>>> have the power management cleanup applied first. The reason is that link
>>>>> debounce depends on PHY readiness, which itself depends heavily on power
>>>>> mode transitions. My plan is to get this done during this cycle for
>>>>> release with 5.20 and then fix on top the arbitrary delays for 5.21.
>>>>
>>>> Nice. Can you share the current status?
>>>
>>> No progress. I need to put together a series with all the patches that
>>> were sent already. Unless Mario can resend something ?
>>
>> No reply from Mario.
> 
> I think what happened here is there was related patches from another party
> that got tangled up with this.

Niklas and I are investigating this again now because Niklas discovered 
that one AMD AHCI adapter leads to drive resets when the drive goes to 
low power mode. The adapter is:

  SATA controller [0106]: Advanced Micro Devices, Inc. [AMD] FCH SATA 
Controller [AHCI mode] [1022:7901] (rev 51)

If we switch to performance mode (no LPM), the reset disapears. But if 
LPM is enabled, any command sent after the disk goes to low poer mode 
(device initiated), there is a link reset...
Niklas Cassel Sept. 21, 2022, 7:18 p.m. UTC | #20 
On Wed, Sep 21, 2022 at 02:01:24PM +0900, Damien Le Moal wrote:
> On 9/14/22 00:28, Limonciello, Mario wrote:
> > 
> > I think what happened here is there was related patches from another party
> > that got tangled up with this.
> 
> Niklas and I are investigating this again now because Niklas discovered that
> one AMD AHCI adapter leads to drive resets when the drive goes to low power
> mode. The adapter is:
> 
>  SATA controller [0106]: Advanced Micro Devices, Inc. [AMD] FCH SATA
> Controller [AHCI mode] [1022:7901] (rev 51)
> 
> If we switch to performance mode (no LPM), the reset disapears. But if LPM
> is enabled, any command sent after the disk goes to low power mode (device
> initiated), there is a link reset...

FTR, I sent out a patch for this issue:
https://lore.kernel.org/linux-ide/20220921155753.231770-1-niklas.cassel@wdc.com/T/#u


Kind regards,
Niklas
diff mbox series

Patch

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 84456c05e8452..0fc09b86a5590 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -51,6 +51,7 @@  enum board_ids {
 	board_ahci,
 	board_ahci_ign_iferr,
 	board_ahci_low_power,
+	board_ahci_low_power_no_debounce_delay,
 	board_ahci_no_debounce_delay,
 	board_ahci_nomsi,
 	board_ahci_noncq,
@@ -142,6 +143,14 @@  static const struct ata_port_info ahci_port_info[] = {
 		.udma_mask	= ATA_UDMA6,
 		.port_ops	= &ahci_ops,
 	},
+	[board_ahci_low_power_no_debounce_delay] = {
+		AHCI_HFLAGS	(AHCI_HFLAG_USE_LPM_POLICY),
+		.flags		= AHCI_FLAG_COMMON,
+		.link_flags	= ATA_LFLAG_NO_DEBOUNCE_DELAY,
+		.pio_mask	= ATA_PIO4,
+		.udma_mask	= ATA_UDMA6,
+		.port_ops	= &ahci_ops,
+	},
 	[board_ahci_no_debounce_delay] = {
 		.flags		= AHCI_FLAG_COMMON,
 		.link_flags	= ATA_LFLAG_NO_DEBOUNCE_DELAY,