diff mbox series

[v2] ata: ahci: Add mask_port_map module parameter

Message ID 20240405125143.1134539-1-dlemoal@kernel.org
State New
Headers show
Series [v2] ata: ahci: Add mask_port_map module parameter | expand

Commit Message

Damien Le Moal April 5, 2024, 12:51 p.m. UTC
Commits 0077a504e1a4 ("ahci: asm1166: correct count of reported ports")
and 9815e3961754 ("ahci: asm1064: correct count of reported ports")
attempted to limit the ports of the ASM1166 and ASM1064 AHCI controllers
to avoid long boot times caused by the fact that these adapters report
a port map larger than the number of physical ports. The excess ports
are "virtual" to hide port multiplier devices and probing these ports
takes time. However, these commits caused a regression for users that do
use PMP devices, as the ATA devices connected to the PMP cannot be
scanned. These commits have thus been reverted by commit 6cd8adc3e18
("ahci: asm1064: asm1166: don't limit reported ports") to allow the
discovery of devices connected through a port multiplier. But this
revert re-introduced the long boot times for users that do not use a
port multiplier setup.

This patch adds the mask_port_map ahci module parameter to allow users
to manually specify port map masks for controllers. In the case of the
ASMedia 1166 and 1064 controllers, users that do not have port
multiplier devices can mask the excess virtual ports exposed by the
controller to speedup port scanning, thus reducing boot time.

The mask_port_map parameter accepts 2 different formats:
 - mask_port_map=<mask>
   This applies the same mask to all AHCI controllers
   present in the system. This format is convenient for small systems
   that have only a single AHCI controller.
 - mask_port_map=<pci_dev>=<mask>,<pci_dev>=mask,...
   This applies the specified masks only to the PCI device listed. The
   <pci_dev> field is a regular PCI device ID (domain:bus:dev.func).
   This ID can be seen following "ahci" in the kernel messages. E.g.
   for "ahci 0000:01:00.0: 2/2 ports implemented (port mask 0x3)", the
   <pci_dev> field is "0000:01:00.0".

When used, the kerenl messages indicate that a port map mask was forced.
E.g.: without a mask:
modrpobe ahci
dmesg | grep ahci
...
ahci 0000:00:17.0: AHCI vers 0001.0301, 32 command slots, 6 Gbps, SATA mode
ahci 0000:00:17.0: (0000:00:17.0) 8/8 ports implemented (port mask 0xff)

With a mask:

modrpobe ahci mask_port_map=0000:00:17.0=0x1
dmesg | grep ahci
...
ahci 0000:00:17.0: masking port_map 0xff -> 0x1
ahci 0000:00:17.0: AHCI vers 0001.0301, 32 command slots, 6 Gbps, SATA mode
ahci 0000:00:17.0: (0000:00:17.0) 1/8 ports implemented (port mask 0x1)

Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
 drivers/ata/ahci.c | 85 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 85 insertions(+)

Comments

Niklas Cassel April 5, 2024, 1:10 p.m. UTC | #1
On Fri, Apr 05, 2024 at 09:51:43PM +0900, Damien Le Moal wrote:
> Commits 0077a504e1a4 ("ahci: asm1166: correct count of reported ports")
> and 9815e3961754 ("ahci: asm1064: correct count of reported ports")
> attempted to limit the ports of the ASM1166 and ASM1064 AHCI controllers
> to avoid long boot times caused by the fact that these adapters report
> a port map larger than the number of physical ports. The excess ports
> are "virtual" to hide port multiplier devices and probing these ports
> takes time. However, these commits caused a regression for users that do
> use PMP devices, as the ATA devices connected to the PMP cannot be
> scanned. These commits have thus been reverted by commit 6cd8adc3e18
> ("ahci: asm1064: asm1166: don't limit reported ports") to allow the
> discovery of devices connected through a port multiplier. But this
> revert re-introduced the long boot times for users that do not use a
> port multiplier setup.
> 
> This patch adds the mask_port_map ahci module parameter to allow users
> to manually specify port map masks for controllers. In the case of the
> ASMedia 1166 and 1064 controllers, users that do not have port
> multiplier devices can mask the excess virtual ports exposed by the
> controller to speedup port scanning, thus reducing boot time.
> 
> The mask_port_map parameter accepts 2 different formats:
>  - mask_port_map=<mask>
>    This applies the same mask to all AHCI controllers
>    present in the system. This format is convenient for small systems
>    that have only a single AHCI controller.
>  - mask_port_map=<pci_dev>=<mask>,<pci_dev>=mask,...
>    This applies the specified masks only to the PCI device listed. The
>    <pci_dev> field is a regular PCI device ID (domain:bus:dev.func).
>    This ID can be seen following "ahci" in the kernel messages. E.g.
>    for "ahci 0000:01:00.0: 2/2 ports implemented (port mask 0x3)", the
>    <pci_dev> field is "0000:01:00.0".
> 
> When used, the kerenl messages indicate that a port map mask was forced.

Nit: s/kerenl/kernel/

> E.g.: without a mask:
> modrpobe ahci

Nit: s/modrpobe/modprobe/

> dmesg | grep ahci
> ...
> ahci 0000:00:17.0: AHCI vers 0001.0301, 32 command slots, 6 Gbps, SATA mode
> ahci 0000:00:17.0: (0000:00:17.0) 8/8 ports implemented (port mask 0xff)
> 
> With a mask:
> 
> modrpobe ahci mask_port_map=0000:00:17.0=0x1
> dmesg | grep ahci
> ...
> ahci 0000:00:17.0: masking port_map 0xff -> 0x1
> ahci 0000:00:17.0: AHCI vers 0001.0301, 32 command slots, 6 Gbps, SATA mode
> ahci 0000:00:17.0: (0000:00:17.0) 1/8 ports implemented (port mask 0x1)
> 
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---

With nits fixed:
Reviewed-by: Niklas Cassel <cassel@kernel.org>
Niklas Cassel April 5, 2024, 1:14 p.m. UTC | #2
On Fri, Apr 05, 2024 at 09:51:43PM +0900, Damien Le Moal wrote:
> Commits 0077a504e1a4 ("ahci: asm1166: correct count of reported ports")
> and 9815e3961754 ("ahci: asm1064: correct count of reported ports")
> attempted to limit the ports of the ASM1166 and ASM1064 AHCI controllers
> to avoid long boot times caused by the fact that these adapters report
> a port map larger than the number of physical ports. The excess ports
> are "virtual" to hide port multiplier devices and probing these ports
> takes time. However, these commits caused a regression for users that do
> use PMP devices, as the ATA devices connected to the PMP cannot be
> scanned. These commits have thus been reverted by commit 6cd8adc3e18
> ("ahci: asm1064: asm1166: don't limit reported ports") to allow the
> discovery of devices connected through a port multiplier. But this
> revert re-introduced the long boot times for users that do not use a
> port multiplier setup.
> 
> This patch adds the mask_port_map ahci module parameter to allow users
> to manually specify port map masks for controllers. In the case of the
> ASMedia 1166 and 1064 controllers, users that do not have port
> multiplier devices can mask the excess virtual ports exposed by the
> controller to speedup port scanning, thus reducing boot time.
> 
> The mask_port_map parameter accepts 2 different formats:
>  - mask_port_map=<mask>
>    This applies the same mask to all AHCI controllers
>    present in the system. This format is convenient for small systems
>    that have only a single AHCI controller.
>  - mask_port_map=<pci_dev>=<mask>,<pci_dev>=mask,...
>    This applies the specified masks only to the PCI device listed. The
>    <pci_dev> field is a regular PCI device ID (domain:bus:dev.func).
>    This ID can be seen following "ahci" in the kernel messages. E.g.
>    for "ahci 0000:01:00.0: 2/2 ports implemented (port mask 0x3)", the
>    <pci_dev> field is "0000:01:00.0".
> 
> When used, the kerenl messages indicate that a port map mask was forced.
> E.g.: without a mask:
> modrpobe ahci
> dmesg | grep ahci
> ...
> ahci 0000:00:17.0: AHCI vers 0001.0301, 32 command slots, 6 Gbps, SATA mode
> ahci 0000:00:17.0: (0000:00:17.0) 8/8 ports implemented (port mask 0xff)
> 
> With a mask:
> 

Perhaps drop this empty line.

> modrpobe ahci mask_port_map=0000:00:17.0=0x1

s/modrpobe/modprobe/

here as well.

(My R-b from previous email still applies of course.)

> dmesg | grep ahci
> ...
> ahci 0000:00:17.0: masking port_map 0xff -> 0x1
> ahci 0000:00:17.0: AHCI vers 0001.0301, 32 command slots, 6 Gbps, SATA mode
> ahci 0000:00:17.0: (0000:00:17.0) 1/8 ports implemented (port mask 0x1)
> 
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
Conrad Kostecki April 5, 2024, 10:53 p.m. UTC | #3
Hi Damien,

Am 05.04.2024 14:51:43, "Damien Le Moal" <dlemoal@kernel.org> schrieb:

><PATCH v2>
i did run a test on my hardware.
It seems to work and adjusting the port_map. But I noticed a difference, 
that those virtual hostXY ports are not marked as DUMMY.
With the previous patch, only six ports reported "ahci" and rest 
"DUMMY".
I am not sure, if that should also not happen with your patch?
Conrad
[   13.365573] ahci 0000:09:00.0: masking port_map 0xffffff3f -> 0x3f
[   13.376511] ahci 0000:09:00.0: SSS flag set, parallel bus scan 
disabled
[   13.395670] ahci 0000:09:00.0: AHCI 0001.0301 32 slots 32 ports 6 
Gbps 0x3f impl SATA mode
[   13.397111] ahci 0000:09:00.0: flags: 64bit ncq sntf stag pm led only 
pio sxs deso sadm sds apst
[   13.593757] scsi host9: ahci
[   13.597362] scsi host10: ahci
[   13.600949] scsi host11: ahci
[   13.604548] scsi host12: ahci
[   13.612459] scsi host13: ahci
[   13.616027] scsi host14: ahci
[   13.616437] scsi host15: ahci
[   13.616745] scsi host16: ahci
[   13.617039] scsi host17: ahci
[   13.617415] scsi host18: ahci
[   13.617723] scsi host19: ahci
[   13.637158] scsi host20: ahci
[   13.640666] scsi host21: ahci
[   13.651760] scsi host22: ahci
[   13.652311] scsi host23: ahci
[   13.652850] scsi host24: ahci
[   13.656374] scsi host25: ahci
[   13.664120] scsi host26: ahci
[   13.664570] scsi host27: ahci
[   13.686567] scsi host28: ahci
[   13.690179] scsi host29: ahci
[   13.697603] scsi host30: ahci
[   13.698083] scsi host31: ahci
[   13.698518] scsi host32: ahci
[   13.701855] scsi host33: ahci
[   13.702323] scsi host34: ahci
[   13.702745] scsi host35: ahci
[   13.721520] scsi host36: ahci
[   13.725157] scsi host37: ahci
[   13.736948] scsi host38: ahci
[   13.737383] scsi host39: ahci
[   13.748518] scsi host40: ahci
Damien Le Moal April 8, 2024, 12:47 a.m. UTC | #4
On 4/6/24 07:53, Conrad Kostecki wrote:
> Hi Damien,
> 
> Am 05.04.2024 14:51:43, "Damien Le Moal" <dlemoal@kernel.org> schrieb:
> 
>> <PATCH v2>
> i did run a test on my hardware.
> It seems to work and adjusting the port_map. But I noticed a difference, that
> those virtual hostXY ports are not marked as DUMMY.
> With the previous patch, only six ports reported "ahci" and rest "DUMMY".
> I am not sure, if that should also not happen with your patch?

Let me check again.

> Conrad
> [   13.365573] ahci 0000:09:00.0: masking port_map 0xffffff3f -> 0x3f
> [   13.376511] ahci 0000:09:00.0: SSS flag set, parallel bus scan disabled
> [   13.395670] ahci 0000:09:00.0: AHCI 0001.0301 32 slots 32 ports 6 Gbps 0x3f
> impl SATA mode
> [   13.397111] ahci 0000:09:00.0: flags: 64bit ncq sntf stag pm led only pio
> sxs deso sadm sds apst
> [   13.593757] scsi host9: ahci
> [   13.597362] scsi host10: ahci
> [   13.600949] scsi host11: ahci
> [   13.604548] scsi host12: ahci
> [   13.612459] scsi host13: ahci
> [   13.616027] scsi host14: ahci
> [   13.616437] scsi host15: ahci
> [   13.616745] scsi host16: ahci
> [   13.617039] scsi host17: ahci
> [   13.617415] scsi host18: ahci
> [   13.617723] scsi host19: ahci
> [   13.637158] scsi host20: ahci
> [   13.640666] scsi host21: ahci
> [   13.651760] scsi host22: ahci
> [   13.652311] scsi host23: ahci
> [   13.652850] scsi host24: ahci
> [   13.656374] scsi host25: ahci
> [   13.664120] scsi host26: ahci
> [   13.664570] scsi host27: ahci
> [   13.686567] scsi host28: ahci
> [   13.690179] scsi host29: ahci
> [   13.697603] scsi host30: ahci
> [   13.698083] scsi host31: ahci
> [   13.698518] scsi host32: ahci
> [   13.701855] scsi host33: ahci
> [   13.702323] scsi host34: ahci
> [   13.702745] scsi host35: ahci
> [   13.721520] scsi host36: ahci
> [   13.725157] scsi host37: ahci
> [   13.736948] scsi host38: ahci
> [   13.737383] scsi host39: ahci
> [   13.748518] scsi host40: ahci
Damien Le Moal April 8, 2024, 2:26 a.m. UTC | #5
On 4/6/24 07:53, Conrad Kostecki wrote:
> Hi Damien,
> 
> Am 05.04.2024 14:51:43, "Damien Le Moal" <dlemoal@kernel.org> schrieb:
> 
>> <PATCH v2>
> i did run a test on my hardware.
> It seems to work and adjusting the port_map. But I noticed a difference, that
> those virtual hostXY ports are not marked as DUMMY.
> With the previous patch, only six ports reported "ahci" and rest "DUMMY".
> I am not sure, if that should also not happen with your patch?
> Conrad
> [   13.365573] ahci 0000:09:00.0: masking port_map 0xffffff3f -> 0x3f
> [   13.376511] ahci 0000:09:00.0: SSS flag set, parallel bus scan disabled
> [   13.395670] ahci 0000:09:00.0: AHCI 0001.0301 32 slots 32 ports 6 Gbps 0x3f
> impl SATA mode
> [   13.397111] ahci 0000:09:00.0: flags: 64bit ncq sntf stag pm led only pio
> sxs deso sadm sds apst
> [   13.593757] scsi host9: ahci
> [   13.597362] scsi host10: ahci
> [   13.600949] scsi host11: ahci
> [   13.604548] scsi host12: ahci
> [   13.612459] scsi host13: ahci
> [   13.616027] scsi host14: ahci
> [   13.616437] scsi host15: ahci
> [   13.616745] scsi host16: ahci
> [   13.617039] scsi host17: ahci
> [   13.617415] scsi host18: ahci
> [   13.617723] scsi host19: ahci
> [   13.637158] scsi host20: ahci
> [   13.640666] scsi host21: ahci
> [   13.651760] scsi host22: ahci
> [   13.652311] scsi host23: ahci
> [   13.652850] scsi host24: ahci
> [   13.656374] scsi host25: ahci
> [   13.664120] scsi host26: ahci
> [   13.664570] scsi host27: ahci
> [   13.686567] scsi host28: ahci
> [   13.690179] scsi host29: ahci
> [   13.697603] scsi host30: ahci
> [   13.698083] scsi host31: ahci
> [   13.698518] scsi host32: ahci
> [   13.701855] scsi host33: ahci
> [   13.702323] scsi host34: ahci
> [   13.702745] scsi host35: ahci
> [   13.721520] scsi host36: ahci
> [   13.725157] scsi host37: ahci
> [   13.736948] scsi host38: ahci
> [   13.737383] scsi host39: ahci
> [   13.748518] scsi host40: ahci

These messages are normal. ata port stucture which leads to a scsi host are
still created for dummy ports. So seeing all ports here as scsi hosts is
normal. However, after these messages, you should see a "ataX: DUMMY" message.

Example with a qemu VM with a 6-ports AHCI controller:

Using the ahci driver without any port map mask, I get:

[  318.739513] ahci 0000:00:1f.2: AHCI vers 0001.0000, 32 command slots, 1.5
Gbps, SATA mode
[  318.741283] ahci 0000:00:1f.2: 6/6 ports implemented (port mask 0x3f)
[  318.742619] ahci 0000:00:1f.2: flags: 64bit ncq only
[  318.759156] scsi host0: ahci
[  318.764098] scsi host1: ahci
[  318.788722] scsi host2: ahci
[  318.793502] scsi host3: ahci
[  318.797792] scsi host4: ahci
[  318.801843] scsi host5: ahci
[  318.804550] ata13: SATA max UDMA/133 abar m4096@0xfea16000 port 0xfea16100
irq 40 lpm-pol 3
[  318.805753] ata14: SATA max UDMA/133 abar m4096@0xfea16000 port 0xfea16180
irq 40 lpm-pol 3
[  318.807045] ata15: SATA max UDMA/133 abar m4096@0xfea16000 port 0xfea16200
irq 40 lpm-pol 3
[  318.808322] ata16: SATA max UDMA/133 abar m4096@0xfea16000 port 0xfea16280
irq 40 lpm-pol 3
[  318.809595] ata17: SATA max UDMA/133 abar m4096@0xfea16000 port 0xfea16300
irq 40 lpm-pol 3
[  318.810829] ata18: SATA max UDMA/133 abar m4096@0xfea16000 port 0xfea16380
irq 40 lpm-pol 3
...

On the same VM, if I rmmod ahci and modprobe it with mask_port_map=0x1
(enabling only the first port), I get:

[  362.257874] ahci 0000:00:1f.2: masking port_map 0x3f -> 0x1
[  362.260518] ahci 0000:00:1f.2: AHCI vers 0001.0000, 32 command slots, 1.5
Gbps, SATA mode
[  362.262477] ahci 0000:00:1f.2: 1/6 ports implemented (port mask 0x1)
[  362.263917] ahci 0000:00:1f.2: flags: 64bit ncq only
[  362.281765] scsi host0: ahci
[  362.286353] scsi host1: ahci
[  362.290785] scsi host2: ahci
[  362.301082] scsi host3: ahci
[  362.324192] scsi host4: ahci
[  362.329560] scsi host5: ahci
[  362.332692] ata19: SATA max UDMA/133 abar m4096@0xfea16000 port 0xfea16100
irq 40 lpm-pol 3
[  362.335041] ata20: DUMMY
[  362.335872] ata21: DUMMY
[  362.336828] ata22: DUMMY
[  362.337856] ata23: DUMMY
[  362.338660] ata24: DUMMY

So the last 5 ports are now dummies...

Can you confirm that you see this please ? Also, please confirm that boot time
is OK and faster with the port map mask.
Conrad Kostecki April 14, 2024, 1:14 p.m. UTC | #6
Hello Damien,
please apologize, I wasn't able to answer earlier, but I had some health 
issues.


Am 08.04.2024 04:26:06, "Damien Le Moal" <dlemoal@kernel.org> schrieb:

>On 4/6/24 07:53, Conrad Kostecki wrote:
>>  Hi Damien,
>>
>>  Am 05.04.2024 14:51:43, "Damien Le Moal" <dlemoal@kernel.org> schrieb:
>>
>>>  <PATCH v2>
>>  i did run a test on my hardware.
>>  It seems to work and adjusting the port_map. But I noticed a difference, that
>>  those virtual hostXY ports are not marked as DUMMY.
>>  With the previous patch, only six ports reported "ahci" and rest "DUMMY".
>>  I am not sure, if that should also not happen with your patch?
>>  Conrad
>>  [   13.365573] ahci 0000:09:00.0: masking port_map 0xffffff3f -> 0x3f
>>  [   13.376511] ahci 0000:09:00.0: SSS flag set, parallel bus scan disabled
>>  [   13.395670] ahci 0000:09:00.0: AHCI 0001.0301 32 slots 32 ports 6 Gbps 0x3f
>>  impl SATA mode
>>  [   13.397111] ahci 0000:09:00.0: flags: 64bit ncq sntf stag pm led only pio
>>  sxs deso sadm sds apst
>>  [   13.593757] scsi host9: ahci
>>  [   13.597362] scsi host10: ahci
>>  [   13.600949] scsi host11: ahci
>>  [   13.604548] scsi host12: ahci
>>  [   13.612459] scsi host13: ahci
>>  [   13.616027] scsi host14: ahci
>>  [   13.616437] scsi host15: ahci
>>  [   13.616745] scsi host16: ahci
>>  [   13.617039] scsi host17: ahci
>>  [   13.617415] scsi host18: ahci
>>  [   13.617723] scsi host19: ahci
>>  [   13.637158] scsi host20: ahci
>>  [   13.640666] scsi host21: ahci
>>  [   13.651760] scsi host22: ahci
>>  [   13.652311] scsi host23: ahci
>>  [   13.652850] scsi host24: ahci
>>  [   13.656374] scsi host25: ahci
>>  [   13.664120] scsi host26: ahci
>>  [   13.664570] scsi host27: ahci
>>  [   13.686567] scsi host28: ahci
>>  [   13.690179] scsi host29: ahci
>>  [   13.697603] scsi host30: ahci
>>  [   13.698083] scsi host31: ahci
>>  [   13.698518] scsi host32: ahci
>>  [   13.701855] scsi host33: ahci
>>  [   13.702323] scsi host34: ahci
>>  [   13.702745] scsi host35: ahci
>>  [   13.721520] scsi host36: ahci
>>  [   13.725157] scsi host37: ahci
>>  [   13.736948] scsi host38: ahci
>>  [   13.737383] scsi host39: ahci
>>  [   13.748518] scsi host40: ahci
>
>These messages are normal. ata port stucture which leads to a scsi host are
>still created for dummy ports. So seeing all ports here as scsi hosts is
>normal. However, after these messages, you should see a "ataX: DUMMY" message.

That's what I mean. My asm1166 controller has only six ports. So for the 
list, most of them should been listed as DUMMY, but they are not.
With my previous patch for asm1066, this was the case.

>Can you confirm that you see this please ? Also, please confirm that boot time
>is OK and faster with the port map mask.
No, I am currently not able to confirm. It looks like, that's is still 
slow for me. Maybe a little bit faster, but I may be wrong.
The main difference here is, that none of the asm1066 ports is being 
listed as DUMMY with your patch.

As my "ahci" module is build into the kernel, I added to my kernel 
arguments:
ahci.mask_port_map=0000:09:00.0=0x3f

Based on the dmesg output, this should be correct?

Cheers
Conrad
Damien Le Moal April 16, 2024, 11:13 p.m. UTC | #7
On 2024/04/14 23:14, Conrad Kostecki wrote:
> Hello Damien,
> please apologize, I wasn't able to answer earlier, but I had some health 
> issues.
> 
> 
> Am 08.04.2024 04:26:06, "Damien Le Moal" <dlemoal@kernel.org> schrieb:
> 
>> On 4/6/24 07:53, Conrad Kostecki wrote:
>>>  Hi Damien,
>>>
>>>  Am 05.04.2024 14:51:43, "Damien Le Moal" <dlemoal@kernel.org> schrieb:
>>>
>>>>  <PATCH v2>
>>>  i did run a test on my hardware.
>>>  It seems to work and adjusting the port_map. But I noticed a difference, that
>>>  those virtual hostXY ports are not marked as DUMMY.
>>>  With the previous patch, only six ports reported "ahci" and rest "DUMMY".
>>>  I am not sure, if that should also not happen with your patch?
>>>  Conrad
>>>  [   13.365573] ahci 0000:09:00.0: masking port_map 0xffffff3f -> 0x3f
>>>  [   13.376511] ahci 0000:09:00.0: SSS flag set, parallel bus scan disabled
>>>  [   13.395670] ahci 0000:09:00.0: AHCI 0001.0301 32 slots 32 ports 6 Gbps 0x3f
>>>  impl SATA mode
>>>  [   13.397111] ahci 0000:09:00.0: flags: 64bit ncq sntf stag pm led only pio
>>>  sxs deso sadm sds apst
>>>  [   13.593757] scsi host9: ahci
>>>  [   13.597362] scsi host10: ahci
>>>  [   13.600949] scsi host11: ahci
>>>  [   13.604548] scsi host12: ahci
>>>  [   13.612459] scsi host13: ahci
>>>  [   13.616027] scsi host14: ahci
>>>  [   13.616437] scsi host15: ahci
>>>  [   13.616745] scsi host16: ahci
>>>  [   13.617039] scsi host17: ahci
>>>  [   13.617415] scsi host18: ahci
>>>  [   13.617723] scsi host19: ahci
>>>  [   13.637158] scsi host20: ahci
>>>  [   13.640666] scsi host21: ahci
>>>  [   13.651760] scsi host22: ahci
>>>  [   13.652311] scsi host23: ahci
>>>  [   13.652850] scsi host24: ahci
>>>  [   13.656374] scsi host25: ahci
>>>  [   13.664120] scsi host26: ahci
>>>  [   13.664570] scsi host27: ahci
>>>  [   13.686567] scsi host28: ahci
>>>  [   13.690179] scsi host29: ahci
>>>  [   13.697603] scsi host30: ahci
>>>  [   13.698083] scsi host31: ahci
>>>  [   13.698518] scsi host32: ahci
>>>  [   13.701855] scsi host33: ahci
>>>  [   13.702323] scsi host34: ahci
>>>  [   13.702745] scsi host35: ahci
>>>  [   13.721520] scsi host36: ahci
>>>  [   13.725157] scsi host37: ahci
>>>  [   13.736948] scsi host38: ahci
>>>  [   13.737383] scsi host39: ahci
>>>  [   13.748518] scsi host40: ahci
>>
>> These messages are normal. ata port stucture which leads to a scsi host are
>> still created for dummy ports. So seeing all ports here as scsi hosts is
>> normal. However, after these messages, you should see a "ataX: DUMMY" message.
> 
> That's what I mean. My asm1166 controller has only six ports. So for the 
> list, most of them should been listed as DUMMY, but they are not.
> With my previous patch for asm1066, this was the case.

OK. So despite what I thought, it seems that the mask should be applied to
saved_port_map to modified that value permanently instead of using the mask to
set ->port_map. I do not really understand why that would be needed. It seems
that saved_port_map is being reused without the mask applied somewhere, which
should not happen. I need to dig further into this.

In the meantime, can you try to add this patch:

diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index 83431aae74d8..830a59f68620 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -546,6 +546,7 @@ void ahci_save_initial_config(struct device *dev, struct
ahci_host_priv *hpriv)
                        port_map,
                        port_map & hpriv->mask_port_map);
                port_map &= hpriv->mask_port_map;
+               hpriv->saved_port_map = port_map;
        }

        /* cross check port_map and cap.n_ports */

And test again ?

> 
>> Can you confirm that you see this please ? Also, please confirm that boot time
>> is OK and faster with the port map mask.
> No, I am currently not able to confirm. It looks like, that's is still 
> slow for me. Maybe a little bit faster, but I may be wrong.
> The main difference here is, that none of the asm1066 ports is being 
> listed as DUMMY with your patch.
> 
> As my "ahci" module is build into the kernel, I added to my kernel 
> arguments:
> ahci.mask_port_map=0000:09:00.0=0x3f
> 
> Based on the dmesg output, this should be correct?

Yes, this is correct and for your adapter with 6 ports, the mask 0x3f is correct
as well, assuming that the 6 physical ports are the first ones (ports 0 to 5).

> 
> Cheers
> Conrad
>
Conrad Kostecki April 16, 2024, 11:29 p.m. UTC | #8
Hello Damien,

Am 17.04.2024 01:13:08, "Damien Le Moal" <dlemoal@kernel.org> schrieb:

>OK. So despite what I thought, it seems that the mask should be applied to
>saved_port_map to modified that value permanently instead of using the mask to
>set ->port_map. I do not really understand why that would be needed. It seems
>that saved_port_map is being reused without the mask applied somewhere, which
>should not happen. I need to dig further into this.
>
>In the meantime, can you try to add this patch:
>
>diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
>index 83431aae74d8..830a59f68620 100644
>--- a/drivers/ata/libahci.c
>+++ b/drivers/ata/libahci.c
>@@ -546,6 +546,7 @@ void ahci_save_initial_config(struct device *dev, struct
>ahci_host_priv *hpriv)
>                         port_map,
>                         port_map & hpriv->mask_port_map);
>                 port_map &= hpriv->mask_port_map;
>+               hpriv->saved_port_map = port_map;
>         }
>
>         /* cross check port_map and cap.n_ports */
>
>And test again ?

I can confirm, this works for me. The non physical ports are now marked 
again as DUMMY and booting is fast.

Cheers
Conrad
Niklas Cassel April 17, 2024, 5:21 p.m. UTC | #9
Hello Conrad,

On Fri, Apr 05, 2024 at 10:53:55PM +0000, Conrad Kostecki wrote:
> Hi Damien,
>
> Am 05.04.2024 14:51:43, "Damien Le Moal" <dlemoal@kernel.org> schrieb:
>
> > <PATCH v2>
> i did run a test on my hardware.
> It seems to work and adjusting the port_map. But I noticed a difference,
> that those virtual hostXY ports are not marked as DUMMY.
> With the previous patch, only six ports reported "ahci" and rest "DUMMY".
> I am not sure, if that should also not happen with your patch?
> Conrad
> [   13.365573] ahci 0000:09:00.0: masking port_map 0xffffff3f -> 0x3f
> [   13.376511] ahci 0000:09:00.0: SSS flag set, parallel bus scan disabled
> [   13.395670] ahci 0000:09:00.0: AHCI 0001.0301 32 slots 32 ports 6 Gbps
> 0x3f impl SATA mode

This print above suggests that you are testing on a v6.8 based kernel.
(The print has been improved in v6.9)

I do not understand why things are not working for you.


Could you please test with v6.9-rc4 + the attached debug patch.

Please make sure that you don't have any other changes on top of v6.9-rc4
other than the debug patch. (mask_port_map is already included in v6.9-rc4.)



Here is a how v6.9-rc4 + the attached debug patch looks for me with
ahci.mask_port_map=0000:00:03.0=0xf
added to the kernel command line.

(If you use a /etc/modprobe.d/ahci.conf file instead, I assume that should
look something like:
options ahci mask_port_map=0000:00:03.0=0xf
)


[    0.538102] ahci 0000:00:03.0: masking port_map 0x3f -> 0xf
[    0.539063] ahci 0000:00:03.0: port 1/6 is implemented (port_map 0xf)
[    0.539933] ahci 0000:00:03.0: port 2/6 is implemented (port_map 0xf)
[    0.540750] ahci 0000:00:03.0: port 3/6 is implemented (port_map 0xf)
[    0.541663] ahci 0000:00:03.0: port 4/6 is implemented (port_map 0xf)
[    0.542990] ahci 0000:00:03.0: port 5/6 not implemented, mark as dummy (port_map 0xf)
[    0.544121] ahci 0000:00:03.0: port 6/6 not implemented, mark as dummy (port_map 0xf)
[    0.545766] ahci 0000:00:03.0: port 1/6 is implemented, calling init (port_map 0xf)
[    0.546718] ahci 0000:00:03.0: port 2/6 is implemented, calling init (port_map 0xf)
[    0.547642] ahci 0000:00:03.0: port 3/6 is implemented, calling init (port_map 0xf)
[    0.548399] ahci 0000:00:03.0: port 4/6 is implemented, calling init (port_map 0xf)
[    0.549418] ahci 0000:00:03.0: port 5/6 is not implemented, skipping init (port_map 0xf)
[    0.550650] ahci 0000:00:03.0: port 6/6 is not implemented, skipping init (port_map 0xf)
[    0.551306] ahci 0000:00:03.0: AHCI vers 0001.0000, 32 command slots, 1.5 Gbps, SATA mode
[    0.551947] ahci 0000:00:03.0: 4/6 ports implemented (port mask 0xf)
[    0.552444] ahci 0000:00:03.0: flags: 64bit ncq only
[    0.553652] scsi host0: ahci
[    0.554138] scsi host1: ahci
[    0.554535] scsi host2: ahci
[    0.555332] scsi host3: ahci
[    0.555806] scsi host4: ahci
[    0.556212] scsi host5: ahci
[    0.556502] ata1: SATA max UDMA/133 abar m4096@0xfebd1000 port 0xfebd1100 irq 43 lpm-pol 3
[    0.557146] ata2: SATA max UDMA/133 abar m4096@0xfebd1000 port 0xfebd1180 irq 43 lpm-pol 3
[    0.557791] ata3: SATA max UDMA/133 abar m4096@0xfebd1000 port 0xfebd1200 irq 43 lpm-pol 3
[    0.558429] ata4: SATA max UDMA/133 abar m4096@0xfebd1000 port 0xfebd1280 irq 43 lpm-pol 3
[    0.559064] ata5: DUMMY
[    0.559260] ata6: DUMMY

Please post your whole log, including both lines prefixed with "scsi" and
"ata".

As you can see, you should see, with your configuration,
32 "scsi: hostX: ahci" prints,
6  "ataX: SATA max ..." prints,
26 "ataX: DUMMY" prints.


If your operating system is using systemd (considering your gentoo address,
this is not a given), you could run:

$ systemd-analyze

both with and without the kernel module option.

You should be able to see a difference.

Or if you don't have systemd, please just upload the full dmesg with and
without the kernel module option, so that we can look at the timestamps.


Kind regards,
Niklas
From 6b5f6be5ade9bfed6765724877ea72524d965fbb Mon Sep 17 00:00:00 2001
From: Niklas Cassel <cassel@kernel.org>
Date: Wed, 17 Apr 2024 15:41:24 +0200
Subject: [PATCH] mask_port_map debug prints

Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/ata/ahci.c    | 8 +++++++-
 drivers/ata/libahci.c | 8 +++++++-
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 6548f10e61d9..6b54b128ac93 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -2011,8 +2011,14 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 		ahci_update_initial_lpm_policy(ap);
 
 		/* disabled/not-implemented port */
-		if (!(hpriv->port_map & (1 << i)))
+		if (!(hpriv->port_map & (1 << i))) {
 			ap->ops = &ata_dummy_port_ops;
+			dev_info(host->dev, "port %d/%d not implemented, mark as dummy (port_map %#x)\n",
+				 i+1, host->n_ports, hpriv->port_map);
+		} else {
+			dev_info(host->dev, "port %d/%d is implemented (port_map %#x)\n",
+				 i+1, host->n_ports, hpriv->port_map);
+		}
 	}
 
 	/* apply workaround for ASUS P5W DH Deluxe mainboard */
diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index 83431aae74d8..11d97d4f44ad 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -1303,8 +1303,14 @@ void ahci_init_controller(struct ata_host *host)
 		struct ata_port *ap = host->ports[i];
 
 		port_mmio = ahci_port_base(ap);
-		if (ata_port_is_dummy(ap))
+		if (ata_port_is_dummy(ap)) {
+			dev_info(host->dev, "port %d/%d is not implemented, skipping init (port_map %#x)\n",
+				 i+1, host->n_ports, hpriv->port_map);
 			continue;
+		} else {
+			dev_info(host->dev, "port %d/%d is implemented, calling init (port_map %#x)\n",
+				 i+1, host->n_ports, hpriv->port_map);
+		}
 
 		ahci_port_init(host->dev, ap, i, mmio, port_mmio);
 	}
Conrad Kostecki May 5, 2024, 7:42 p.m. UTC | #10
Hello Niklas,
please forgive me, I wasn't able to answer earlier. I have to roll back 
here :-)
I don't know, what I exactly did wrong, but I did now tested again PATCH 
v2 and can confirm, it just works as it should.
Tested on 6.8.9 and also 6.9-rc6 (with has PATCH v2 applied).

My ports are being correct masked and DUMMY is being correctly shown.
So all fine and I am happy with the patch as it is now.

Sorry for the noise, I am really not sure, what I did wrong during first 
testing.

Conrad

Am 17.04.2024 19:21:21, "Niklas Cassel" <cassel@kernel.org> schrieb:

>Hello Conrad,
>
>On Fri, Apr 05, 2024 at 10:53:55PM +0000, Conrad Kostecki wrote:
>>  Hi Damien,
>>
>>  Am 05.04.2024 14:51:43, "Damien Le Moal" <dlemoal@kernel.org> schrieb:
>>
>>  > <PATCH v2>
>>  i did run a test on my hardware.
>>  It seems to work and adjusting the port_map. But I noticed a difference,
>>  that those virtual hostXY ports are not marked as DUMMY.
>>  With the previous patch, only six ports reported "ahci" and rest "DUMMY".
>>  I am not sure, if that should also not happen with your patch?
>>  Conrad
>>  [   13.365573] ahci 0000:09:00.0: masking port_map 0xffffff3f -> 0x3f
>>  [   13.376511] ahci 0000:09:00.0: SSS flag set, parallel bus scan disabled
>>  [   13.395670] ahci 0000:09:00.0: AHCI 0001.0301 32 slots 32 ports 6 Gbps
>>  0x3f impl SATA mode
>
>This print above suggests that you are testing on a v6.8 based kernel.
>(The print has been improved in v6.9)
>
>I do not understand why things are not working for you.
>
>
>Could you please test with v6.9-rc4 + the attached debug patch.
>
>Please make sure that you don't have any other changes on top of v6.9-rc4
>other than the debug patch. (mask_port_map is already included in v6.9-rc4.)
>
>
>
>Here is a how v6.9-rc4 + the attached debug patch looks for me with
>ahci.mask_port_map=0000:00:03.0=0xf
>added to the kernel command line.
>
>(If you use a /etc/modprobe.d/ahci.conf file instead, I assume that should
>look something like:
>options ahci mask_port_map=0000:00:03.0=0xf
>)
>
>
>[    0.538102] ahci 0000:00:03.0: masking port_map 0x3f -> 0xf
>[    0.539063] ahci 0000:00:03.0: port 1/6 is implemented (port_map 0xf)
>[    0.539933] ahci 0000:00:03.0: port 2/6 is implemented (port_map 0xf)
>[    0.540750] ahci 0000:00:03.0: port 3/6 is implemented (port_map 0xf)
>[    0.541663] ahci 0000:00:03.0: port 4/6 is implemented (port_map 0xf)
>[    0.542990] ahci 0000:00:03.0: port 5/6 not implemented, mark as dummy (port_map 0xf)
>[    0.544121] ahci 0000:00:03.0: port 6/6 not implemented, mark as dummy (port_map 0xf)
>[    0.545766] ahci 0000:00:03.0: port 1/6 is implemented, calling init (port_map 0xf)
>[    0.546718] ahci 0000:00:03.0: port 2/6 is implemented, calling init (port_map 0xf)
>[    0.547642] ahci 0000:00:03.0: port 3/6 is implemented, calling init (port_map 0xf)
>[    0.548399] ahci 0000:00:03.0: port 4/6 is implemented, calling init (port_map 0xf)
>[    0.549418] ahci 0000:00:03.0: port 5/6 is not implemented, skipping init (port_map 0xf)
>[    0.550650] ahci 0000:00:03.0: port 6/6 is not implemented, skipping init (port_map 0xf)
>[    0.551306] ahci 0000:00:03.0: AHCI vers 0001.0000, 32 command slots, 1.5 Gbps, SATA mode
>[    0.551947] ahci 0000:00:03.0: 4/6 ports implemented (port mask 0xf)
>[    0.552444] ahci 0000:00:03.0: flags: 64bit ncq only
>[    0.553652] scsi host0: ahci
>[    0.554138] scsi host1: ahci
>[    0.554535] scsi host2: ahci
>[    0.555332] scsi host3: ahci
>[    0.555806] scsi host4: ahci
>[    0.556212] scsi host5: ahci
>[    0.556502] ata1: SATA max UDMA/133 abar m4096@0xfebd1000 port 0xfebd1100 irq 43 lpm-pol 3
>[    0.557146] ata2: SATA max UDMA/133 abar m4096@0xfebd1000 port 0xfebd1180 irq 43 lpm-pol 3
>[    0.557791] ata3: SATA max UDMA/133 abar m4096@0xfebd1000 port 0xfebd1200 irq 43 lpm-pol 3
>[    0.558429] ata4: SATA max UDMA/133 abar m4096@0xfebd1000 port 0xfebd1280 irq 43 lpm-pol 3
>[    0.559064] ata5: DUMMY
>[    0.559260] ata6: DUMMY
>
>Please post your whole log, including both lines prefixed with "scsi" and
>"ata".
>
>As you can see, you should see, with your configuration,
>32 "scsi: hostX: ahci" prints,
>6  "ataX: SATA max ..." prints,
>26 "ataX: DUMMY" prints.
>
>
>If your operating system is using systemd (considering your gentoo address,
>this is not a given), you could run:
>
>$ systemd-analyze
>
>both with and without the kernel module option.
>
>You should be able to see a difference.
>
>Or if you don't have systemd, please just upload the full dmesg with and
>without the kernel module option, so that we can look at the timestamps.
>
>
>Kind regards,
>Niklas
Niklas Cassel May 6, 2024, 4:46 p.m. UTC | #11
On Sun, May 05, 2024 at 07:42:55PM +0000, Conrad Kostecki wrote:
> Hello Niklas,
> please forgive me, I wasn't able to answer earlier. I have to roll back here
> :-)

No worries :)


> I don't know, what I exactly did wrong, but I did now tested again PATCH v2
> and can confirm, it just works as it should.
> Tested on 6.8.9 and also 6.9-rc6 (with has PATCH v2 applied).
> 
> My ports are being correct masked and DUMMY is being correctly shown.
> So all fine and I am happy with the patch as it is now.
> 
> Sorry for the noise, I am really not sure, what I did wrong during first
> testing.

I'm glad that the patch/kernel module param works for you.
(Even if we can all agree that we would prefer not needing a kernel module
param at all :) )


Kind regards,
Niklas
diff mbox series

Patch

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 562302e2e57c..6548f10e61d9 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -666,6 +666,87 @@  static int mobile_lpm_policy = -1;
 module_param(mobile_lpm_policy, int, 0644);
 MODULE_PARM_DESC(mobile_lpm_policy, "Default LPM policy for mobile chipsets");
 
+static char *ahci_mask_port_map;
+module_param_named(mask_port_map, ahci_mask_port_map, charp, 0444);
+MODULE_PARM_DESC(mask_port_map,
+		 "32-bits port map masks to ignore controllers ports. "
+		 "Valid values are: "
+		 "\"<mask>\" to apply the same mask to all AHCI controller "
+		 "devices, and \"<pci_dev>=<mask>,<pci_dev>=<mask>,...\" to "
+		 "specify different masks for the controllers specified, "
+		 "where <pci_dev> is the PCI ID of an AHCI controller in the "
+		 "form \"domain:bus:dev.func\"");
+
+static void ahci_apply_port_map_mask(struct device *dev,
+				     struct ahci_host_priv *hpriv, char *mask_s)
+{
+	unsigned int mask;
+
+	if (kstrtouint(mask_s, 0, &mask)) {
+		dev_err(dev, "Invalid port map mask\n");
+		return;
+	}
+
+	hpriv->mask_port_map = mask;
+}
+
+static void ahci_get_port_map_mask(struct device *dev,
+				   struct ahci_host_priv *hpriv)
+{
+	char *param, *end, *str, *mask_s;
+	char *name;
+
+	if (!strlen(ahci_mask_port_map))
+		return;
+
+	str = kstrdup(ahci_mask_port_map, GFP_KERNEL);
+	if (!str)
+		return;
+
+	/* Handle single mask case */
+	if (!strchr(str, '=')) {
+		ahci_apply_port_map_mask(dev, hpriv, str);
+		goto free;
+	}
+
+	/*
+	 * Mask list case: parse the parameter to apply the mask only if
+	 * the device name matches.
+	 */
+	param = str;
+	end = param + strlen(param);
+	while (param && param < end && *param) {
+		name = param;
+		param = strchr(name, '=');
+		if (!param)
+			break;
+
+		*param = '\0';
+		param++;
+		if (param >= end)
+			break;
+
+		if (strcmp(dev_name(dev), name) != 0) {
+			param = strchr(param, ',');
+			if (param)
+				param++;
+			continue;
+		}
+
+		mask_s = param;
+		param = strchr(mask_s, ',');
+		if (param) {
+			*param = '\0';
+			param++;
+		}
+
+		ahci_apply_port_map_mask(dev, hpriv, mask_s);
+	}
+
+free:
+	kfree(str);
+}
+
 static void ahci_pci_save_initial_config(struct pci_dev *pdev,
 					 struct ahci_host_priv *hpriv)
 {
@@ -688,6 +769,10 @@  static void ahci_pci_save_initial_config(struct pci_dev *pdev,
 			  "Disabling your PATA port. Use the boot option 'ahci.marvell_enable=0' to avoid this.\n");
 	}
 
+	/* Handle port map masks passed as module parameter. */
+	if (ahci_mask_port_map)
+		ahci_get_port_map_mask(&pdev->dev, hpriv);
+
 	ahci_save_initial_config(&pdev->dev, hpriv);
 }