Skip to contentSkip to conversation

ata: libahci_platform: Do not set mask_port_map when not needed

PATCH Applied Damien Le Moal 4 participants 5 messages Feb 7, 2025 – Feb 25, 2025

Subsystems (2)
libata libata SATA AHCI platform devices support
lore
Share & tools
lore

Damien Le Moal

author maintainer
#0 lore raw

PATCH v2 1/1 ata: libahci_platform: Do not set mask_port_map when not needed

Commit 8c87215dd3a2 ("ata: libahci_platform: support non-consecutive port numbers") modified ahci_platform_get_resources() to allow identifying the ports of a controller that are defined as child nodes of the controller node in order to support non-consecutive port numbers (as defined by the platform device tree).

However, this commit also erroneously sets bit 0 of hpriv->mask_port_map when the platform devices tree does not define port child nodes, to match the fact that the temporary default number of ports used in that case is 1 (which is also consistent with the fact that only index 0 of hpriv->phys[] is initialized with the call to ahci_platform_get_phy(). But doing so causes ahci_platform_init_host() to initialize and probe only the first port, even if this function determines that the controller has in fact multiple ports using the capability register of the controller (through a call to ahci_nr_ports()). This can be seen with the ahci_mvebu driver (Armada 385 SoC) with the second port declared as "dummy":

ahci-mvebu f10a8000.sata: masking port_map 0x3 -> 0x1
ahci-mvebu f10a8000.sata: AHCI vers 0001.0000, 32 command slots, 6 Gbps, platform mode ahci-mvebu f10a8000.sata: 1/2 ports implemented (port mask 0x1) ahci-mvebu f10a8000.sata: flags: 64bit ncq sntf led only pmp fbs pio slum part sxs scsi host0: ahci-mvebu
scsi host1: ahci-mvebu
ata1: SATA max UDMA/133 mmio [mem 0xf10a8000-0xf10a9fff] port 0x100 irq 40 lpm-pol 0
ata2: DUMMY

Fix this issue by removing setting bit 0 of hpriv->mask_port_map when the platform device tree does not define port child nodes.

Reported-by Klaus Kudielka <[email protected]>
Fixes 8c87215dd3a2 ("ata: libahci_platform: support non-consecutive port numbers")
Tested-by Klaus Kudielka <[email protected]>
Signed-off-by Damien Le Moal <[email protected]>

---
Changes from v1:
- Improved commit message (fixed typos, added example and removed cc-stable tag).

 drivers/ata/libahci_platform.c | 2 --
 1 file changed, 2 deletions(-)
drivers/ata/libahci_platform.c +0 -2
index 53b2c7719dc5..91d44302eac9 100644
@@ -651,8 +651,6 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
 		 * If no sub-node was found, keep this for device tree
 		 * compatibility
 		 */
-		hpriv->mask_port_map |= BIT(0);
-
 		rc = ahci_platform_get_phy(hpriv, 0, dev, dev->of_node);
 		if (rc)
 			goto err_out;
Signature · 1 line
2.48.1
Acked-by on v2 1/1
#1 lore raw

Am 08.02.25 um 00:29 schrieb Damien Le Moal:

Quoted 47 lines

Commit 8c87215dd3a2 ("ata: libahci_platform: support non-consecutive port numbers") modified ahci_platform_get_resources() to allow identifying the ports of a controller that are defined as child nodes of the controller node in order to support non-consecutive port numbers (as defined by the platform device tree).

However, this commit also erroneously sets bit 0 of hpriv->mask_port_map when the platform devices tree does not define port child nodes, to match the fact that the temporary default number of ports used in that case is 1 (which is also consistent with the fact that only index 0 of hpriv->phys[] is initialized with the call to ahci_platform_get_phy(). But doing so causes ahci_platform_init_host() to initialize and probe only the first port, even if this function determines that the controller has in fact multiple ports using the capability register of the controller (through a call to ahci_nr_ports()). This can be seen with the ahci_mvebu driver (Armada 385 SoC) with the second port declared as "dummy":

ahci-mvebu f10a8000.sata: masking port_map 0x3 -> 0x1
ahci-mvebu f10a8000.sata: AHCI vers 0001.0000, 32 command slots, 6 Gbps, platform mode ahci-mvebu f10a8000.sata: 1/2 ports implemented (port mask 0x1) ahci-mvebu f10a8000.sata: flags: 64bit ncq sntf led only pmp fbs pio slum part sxs scsi host0: ahci-mvebu
scsi host1: ahci-mvebu
ata1: SATA max UDMA/133 mmio [mem 0xf10a8000-0xf10a9fff] port 0x100 irq 40 lpm-pol 0
ata2: DUMMY

Fix this issue by removing setting bit 0 of hpriv->mask_port_map when the platform device tree does not define port child nodes.

Reported-by Klaus Kudielka <[email protected]>
Fixes 8c87215dd3a2 ("ata: libahci_platform: support non-consecutive port numbers")
Tested-by Klaus Kudielka <[email protected]>
Signed-off-by Damien Le Moal <[email protected]>

---
Changes from v1:
- Improved commit message (fixed typos, added example and removed cc-stable tag).

 drivers/ata/libahci_platform.c | 2 --
 1 file changed, 2 deletions(-)
drivers/ata/libahci_platform.c +0 -0
index 53b2c7719dc5..91d44302eac9 100644

drivers/ata/libahci_platform.c struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev, · line 656

Quoted 8 lines
drivers/ata/libahci_platform.c +0 -2
@@ -651,8 +651,6 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
 		 * If no sub-node was found, keep this for device tree
 		 * compatibility
 		 */
-		hpriv->mask_port_map |= BIT(0);
-
 		rc = ahci_platform_get_phy(hpriv, 0, dev, dev->of_node);
 		if (rc)
 			goto err_out;

After testing on Armada 388 Helios-4 board with and without port subnodes, I can confirm that before my patch and without port subnodes both controller ports were fully operational.

Therefore I believe restoring the original behaviour here is correct.

Acked-by Josua Mayer <[email protected]>
maintainer Applied
#2 lore raw
In reply to Damien Le Moal
Quoted 18 lines · Damien Le Moal

Commit 8c87215dd3a2 ("ata: libahci_platform: support non-consecutive port numbers") modified ahci_platform_get_resources() to allow identifying the ports of a controller that are defined as child nodes of the controller node in order to support non-consecutive port numbers (as defined by the platform device tree).

However, this commit also erroneously sets bit 0 of hpriv->mask_port_map when the platform devices tree does not define port child nodes, to match the fact that the temporary default number of ports used in that case is 1 (which is also consistent with the fact that only index 0 of hpriv->phys[] is initialized with the call to ahci_platform_get_phy(). But doing so causes ahci_platform_init_host() to initialize and probe only the first port, even if this function determines that the controller has in fact multiple ports using the capability register of the controller (through a call to ahci_nr_ports()). This can be seen with the ahci_mvebu driver (Armada 385 SoC) with the second port declared as "dummy":

Quoted 1 line

[...]

Applied to libata/linux.git (for-6.14), thanks!

[1/1] ata: libahci_platform: Do not set mask_port_map when not needed

      https://git.kernel.org/libata/linux/c/2c202e6c

Kind regards,
Niklas

#3 lore raw
In reply to Damien Le Moal

Hi Damien,

Quoted 27 lines · Damien Le Moal

Commit 8c87215dd3a2 ("ata: libahci_platform: support non-consecutive port numbers") modified ahci_platform_get_resources() to allow identifying the ports of a controller that are defined as child nodes of the controller node in order to support non-consecutive port numbers (as defined by the platform device tree).

However, this commit also erroneously sets bit 0 of hpriv->mask_port_map when the platform devices tree does not define port child nodes, to match the fact that the temporary default number of ports used in that case is 1 (which is also consistent with the fact that only index 0 of hpriv->phys[] is initialized with the call to ahci_platform_get_phy(). But doing so causes ahci_platform_init_host() to initialize and probe only the first port, even if this function determines that the controller has in fact multiple ports using the capability register of the controller (through a call to ahci_nr_ports()). This can be seen with the ahci_mvebu driver (Armada 385 SoC) with the second port declared as "dummy":

ahci-mvebu f10a8000.sata: masking port_map 0x3 -> 0x1
ahci-mvebu f10a8000.sata: AHCI vers 0001.0000, 32 command slots, 6 Gbps, platform mode ahci-mvebu f10a8000.sata: 1/2 ports implemented (port mask 0x1) ahci-mvebu f10a8000.sata: flags: 64bit ncq sntf led only pmp fbs pio slum part sxs scsi host0: ahci-mvebu
scsi host1: ahci-mvebu
ata1: SATA max UDMA/133 mmio [mem 0xf10a8000-0xf10a9fff] port 0x100 irq 40 lpm-pol 0
ata2: DUMMY

Quoted 7 lines

Fix this issue by removing setting bit 0 of hpriv->mask_port_map when the platform device tree does not define port child nodes.

Reported-by Klaus Kudielka <[email protected]>
Fixes 8c87215dd3a2 ("ata: libahci_platform: support non-consecutive port numbers")
Tested-by Klaus Kudielka <[email protected]>
Signed-off-by Damien Le Moal <[email protected]>

This patch landed yesterday in linux-next as commit 2c202e6c4f4d ("ata:
libahci_platform: Do not set mask_port_map when not needed"). Unfortunately it breaks SATA operation on legacy ARM Exynos5250 Arndale board (arch/arm/boot/dts/samsung/exynos5250-arndale.dts). I wonder if this patch is really correct as if I understand code correctly, it disables support for any legacy of-based, single port.

Here is the log from that board:

[    0.937868] libata version 3.00 loaded.
[    2.322611] ahci-dwc 122f0000.sata: supply ahci not found, using dummy regulator
[    2.329389] ahci-dwc 122f0000.sata: supply phy not found, using dummy regulator
[    2.336745] ahci-dwc 122f0000.sata: supply target not found, using dummy regulator
[    2.344254] ahci-dwc 122f0000.sata: forcing port_map 0x0 -> 0x1 [    2.349517] ahci-dwc 122f0000.sata: AHCI vers 0001.0300, 32 command slots, 6 Gbps, platform mode
[    2.358128] ahci-dwc 122f0000.sata: 1/1 ports implemented (port mask 0x1)
[    2.364921] ahci-dwc 122f0000.sata: flags: ncq sntf pm led clo only pmp pio slum part ccc apst
[    2.385620] ata1: SATA max UDMA/133 mmio [mem 0x122f0000-0x122f01fe] port 0x100 irq 101 lpm-pol 0
[    2.716379] ata1: SATA link down (SStatus 0 SControl 300)

with the $subject patch reverted:

[    0.938268] libata version 3.00 loaded.
[    2.283918] ahci-dwc 122f0000.sata: supply ahci not found, using dummy regulator
[    2.290714] ahci-dwc 122f0000.sata: supply phy not found, using dummy regulator
[    2.298064] ahci-dwc 122f0000.sata: supply target not found, using dummy regulator
[    2.312275] ahci-dwc 122f0000.sata: forcing port_map 0x0 -> 0x1 [    2.316847] ahci-dwc 122f0000.sata: masking port_map 0x1 -> 0x1 [    2.322791] ahci-dwc 122f0000.sata: AHCI vers 0001.0300, 32 command slots, 6 Gbps, platform mode
[    2.331464] ahci-dwc 122f0000.sata: 1/1 ports implemented (port mask 0x1)
[    2.338258] ahci-dwc 122f0000.sata: flags: ncq sntf pm led clo only pmp pio slum part ccc apst
[    2.359896] ata1: SATA max UDMA/133 mmio [mem 0x122f0000-0x122f01fe] port 0x100 irq 101 lpm-pol 0
[    2.687374] ata1: SATA link up 3.0 Gbps (SStatus 123 SControl 300) [    2.744673] ata1.00: HPA detected: current 117229295, native 117231408 [    2.757119] ata1.00: ATA-8: Corsair CSSD-F60GB2, 1.1, max UDMA/133 [    2.772062] ata1.00: 117229295 sectors, multi 1: LBA48 NCQ (depth 32) [    2.834520] ata1.00: configured for UDMA/133

Quoted 13 lines

---
Changes from v1:
- Improved commit message (fixed typos, added example and removed cc-stable tag).

  drivers/ata/libahci_platform.c | 2 --
  1 file changed, 2 deletions(-)
drivers/ata/libahci_platform.c +0 -0
index 53b2c7719dc5..91d44302eac9 100644

drivers/ata/libahci_platform.c struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev, · line 656

Quoted 8 lines
drivers/ata/libahci_platform.c +0 -2
@@ -651,8 +651,6 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
  		 * If no sub-node was found, keep this for device tree
  		 * compatibility
  		 */
-		hpriv->mask_port_map |= BIT(0);
-
  		rc = ahci_platform_get_phy(hpriv, 0, dev, dev->of_node);
  		if (rc)
  			goto err_out;

Best regards

Signature · 2 lines
Marek Szyprowski, PhD
Samsung R&D Institute Poland
maintainer
#4 lore raw

Hello Marek,

Quoted 16 lines · Marek Szyprowski

with the $subject patch reverted:

[    0.938268] libata version 3.00 loaded.
[    2.283918] ahci-dwc 122f0000.sata: supply ahci not found, using dummy regulator
[    2.290714] ahci-dwc 122f0000.sata: supply phy not found, using dummy regulator
[    2.298064] ahci-dwc 122f0000.sata: supply target not found, using dummy regulator
[    2.312275] ahci-dwc 122f0000.sata: forcing port_map 0x0 -> 0x1 [    2.316847] ahci-dwc 122f0000.sata: masking port_map 0x1 -> 0x1 [    2.322791] ahci-dwc 122f0000.sata: AHCI vers 0001.0300, 32 command slots, 6 Gbps, platform mode
[    2.331464] ahci-dwc 122f0000.sata: 1/1 ports implemented (port mask 0x1)
[    2.338258] ahci-dwc 122f0000.sata: flags: ncq sntf pm led clo only

Quoted 8 lines

pmp pio slum part ccc apst
[    2.359896] ata1: SATA max UDMA/133 mmio [mem 0x122f0000-0x122f01fe] port 0x100 irq 101 lpm-pol 0
[    2.687374] ata1: SATA link up 3.0 Gbps (SStatus 123 SControl 300) [    2.744673] ata1.00: HPA detected: current 117229295, native 117231408 [    2.757119] ata1.00: ATA-8: Corsair CSSD-F60GB2, 1.1, max UDMA/133 [    2.772062] ata1.00: 117229295 sectors, multi 1: LBA48 NCQ (depth 32) [    2.834520] ata1.00: configured for UDMA/133

Could you please test this patch:
https://lore.kernel.org/linux-ide/[email protected]/T/#u
and see if it helps.

Kind regards,
Niklas