* Re: [PATCH] ata: ahci: Make ahci_ignore_port() handle empty mask_port_map
2025-02-25 14:16 ` [PATCH] ata: ahci: Make ahci_ignore_port() handle empty mask_port_map Niklas Cassel
@ 2025-02-25 14:34 ` Marek Szyprowski
2025-02-26 2:08 ` Damien Le Moal
2025-02-26 10:27 ` Niklas Cassel
2 siblings, 0 replies; 4+ messages in thread
From: Marek Szyprowski @ 2025-02-25 14:34 UTC (permalink / raw)
To: Niklas Cassel, Damien Le Moal, Hans de Goede, Josua Mayer
Cc: Klaus Kudielka, linux-ide
On 25.02.2025 15:16, Niklas Cassel wrote:
> Commit 8c87215dd3a2 ("ata: libahci_platform: support non-consecutive port
> numbers") added a skip to ahci_platform_enable_phys() for ports that are
> not in mask_port_map.
>
> The code in ahci_platform_get_resources(), will currently set mask_port_map
> for each child "port" node it finds in the device tree.
>
> However, device trees that do not have any child "port" nodes will not have
> mask_port_map set, and for non-device tree platforms mask_port_map will
> only exist as a quirk for specific PCI device + vendor IDs, or as a kernel
> module parameter, but will not be set by default.
>
> Therefore, the common thing is that mask_port_map is only set if you do not
> want to use all ports (as defined by Offset 0Ch: PI – Ports Implemented
> register), but instead only want to use the ports in mask_port_map. If
> mask_port_map is not set, all ports are available.
>
> Thus, ahci_ignore_port() must be able to handle an empty mask_port_map.
>
> Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Closes: https://lore.kernel.org/linux-ide/10b31dd0-d0bb-4f76-9305-2195c3e17670@samsung.com/
> Co-developed-by: Damien Le Moal <dlemoal@kernel.org>
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> Fixes: 8c87215dd3a2 ("ata: libahci_platform: support non-consecutive port numbers")
> Fixes: 2c202e6c4f4d ("ata: libahci_platform: Do not set mask_port_map when not needed")
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
This fixes the issue I've observed.
Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
> drivers/ata/ahci.h | 8 ++++++--
> drivers/ata/libahci.c | 1 +
> 2 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
> index aea30df50c58..b2e0ef4efbdc 100644
> --- a/drivers/ata/ahci.h
> +++ b/drivers/ata/ahci.h
> @@ -386,8 +386,12 @@ struct ahci_host_priv {
> static inline bool ahci_ignore_port(struct ahci_host_priv *hpriv,
> unsigned int portid)
> {
> - return portid >= hpriv->nports ||
> - !(hpriv->mask_port_map & (1 << portid));
> + if (portid >= hpriv->nports)
> + return true;
> + /* mask_port_map not set means that all ports are available */
> + if (!hpriv->mask_port_map)
> + return false;
> + return !(hpriv->mask_port_map & (1 << portid));
> }
>
> extern int ahci_ignore_sss;
> diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
> index fdfa7b266218..e7ace4b10f15 100644
> --- a/drivers/ata/libahci.c
> +++ b/drivers/ata/libahci.c
> @@ -541,6 +541,7 @@ void ahci_save_initial_config(struct device *dev, struct ahci_host_priv *hpriv)
> hpriv->saved_port_map = port_map;
> }
>
> + /* mask_port_map not set means that all ports are available */
> if (hpriv->mask_port_map) {
> dev_warn(dev, "masking port_map 0x%lx -> 0x%lx\n",
> port_map,
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] ata: ahci: Make ahci_ignore_port() handle empty mask_port_map
2025-02-25 14:16 ` [PATCH] ata: ahci: Make ahci_ignore_port() handle empty mask_port_map Niklas Cassel
2025-02-25 14:34 ` Marek Szyprowski
@ 2025-02-26 2:08 ` Damien Le Moal
2025-02-26 10:27 ` Niklas Cassel
2 siblings, 0 replies; 4+ messages in thread
From: Damien Le Moal @ 2025-02-26 2:08 UTC (permalink / raw)
To: Niklas Cassel, Hans de Goede, Josua Mayer
Cc: Klaus Kudielka, Marek Szyprowski, linux-ide
On 2/25/25 11:16 PM, Niklas Cassel wrote:
> Commit 8c87215dd3a2 ("ata: libahci_platform: support non-consecutive port
> numbers") added a skip to ahci_platform_enable_phys() for ports that are
> not in mask_port_map.
>
> The code in ahci_platform_get_resources(), will currently set mask_port_map
> for each child "port" node it finds in the device tree.
>
> However, device trees that do not have any child "port" nodes will not have
> mask_port_map set, and for non-device tree platforms mask_port_map will
> only exist as a quirk for specific PCI device + vendor IDs, or as a kernel
> module parameter, but will not be set by default.
>
> Therefore, the common thing is that mask_port_map is only set if you do not
> want to use all ports (as defined by Offset 0Ch: PI – Ports Implemented
> register), but instead only want to use the ports in mask_port_map. If
> mask_port_map is not set, all ports are available.
>
> Thus, ahci_ignore_port() must be able to handle an empty mask_port_map.
>
> Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Closes: https://lore.kernel.org/linux-ide/10b31dd0-d0bb-4f76-9305-2195c3e17670@samsung.com/
> Co-developed-by: Damien Le Moal <dlemoal@kernel.org>
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> Fixes: 8c87215dd3a2 ("ata: libahci_platform: support non-consecutive port numbers")
> Fixes: 2c202e6c4f4d ("ata: libahci_platform: Do not set mask_port_map when not needed")
I think that it also would be a good idea to add:
Fixes: c9b5be909e65 ("ahci: Introduce ahci_ignore_port() helper")
So that this patch also gets picked up for backporting.
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> ---
> drivers/ata/ahci.h | 8 ++++++--
> drivers/ata/libahci.c | 1 +
> 2 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
> index aea30df50c58..b2e0ef4efbdc 100644
> --- a/drivers/ata/ahci.h
> +++ b/drivers/ata/ahci.h
> @@ -386,8 +386,12 @@ struct ahci_host_priv {
> static inline bool ahci_ignore_port(struct ahci_host_priv *hpriv,
> unsigned int portid)
> {
> - return portid >= hpriv->nports ||
> - !(hpriv->mask_port_map & (1 << portid));
> + if (portid >= hpriv->nports)
> + return true;
> + /* mask_port_map not set means that all ports are available */
> + if (!hpriv->mask_port_map)
> + return false;
> + return !(hpriv->mask_port_map & (1 << portid));
> }
>
> extern int ahci_ignore_sss;
> diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
> index fdfa7b266218..e7ace4b10f15 100644
> --- a/drivers/ata/libahci.c
> +++ b/drivers/ata/libahci.c
> @@ -541,6 +541,7 @@ void ahci_save_initial_config(struct device *dev, struct ahci_host_priv *hpriv)
> hpriv->saved_port_map = port_map;
> }
>
> + /* mask_port_map not set means that all ports are available */
> if (hpriv->mask_port_map) {
> dev_warn(dev, "masking port_map 0x%lx -> 0x%lx\n",
> port_map,
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] ata: ahci: Make ahci_ignore_port() handle empty mask_port_map
2025-02-25 14:16 ` [PATCH] ata: ahci: Make ahci_ignore_port() handle empty mask_port_map Niklas Cassel
2025-02-25 14:34 ` Marek Szyprowski
2025-02-26 2:08 ` Damien Le Moal
@ 2025-02-26 10:27 ` Niklas Cassel
2 siblings, 0 replies; 4+ messages in thread
From: Niklas Cassel @ 2025-02-26 10:27 UTC (permalink / raw)
To: Damien Le Moal, Hans de Goede, Josua Mayer, Niklas Cassel
Cc: Klaus Kudielka, Marek Szyprowski, linux-ide
On Tue, 25 Feb 2025 15:16:12 +0100, Niklas Cassel wrote:
> Commit 8c87215dd3a2 ("ata: libahci_platform: support non-consecutive port
> numbers") added a skip to ahci_platform_enable_phys() for ports that are
> not in mask_port_map.
>
> The code in ahci_platform_get_resources(), will currently set mask_port_map
> for each child "port" node it finds in the device tree.
>
> [...]
Applied to libata/linux.git (for-6.14), thanks!
[1/1] ata: ahci: Make ahci_ignore_port() handle empty mask_port_map
https://git.kernel.org/libata/linux/c/130ff5c8
Kind regards,
Niklas
^ permalink raw reply [flat|nested] 4+ messages in thread