Re: [PATCH RFC] ata: libahci_platform: support non-consecutive port numbers

[Damien Le Moal <dlemoal@kernel.org>]

On 11/22/24 12:05 AM, Josua Mayer wrote:
> So far ahci_platform relied on number of child nodes in firmware to
> allocate arrays and expected port numbers to start from 0 without holes.
> This number of ports is then set in private structure for use when
> configuring phys and regulators.
> 
> Some platforms may not use every port of an ahci controller.
> E.g. SolidRUN CN9130 Clearfog uses only port 1 but not port 0, leading
> to the following errors during boot:
> [    1.719476] ahci f2540000.sata: invalid port number 1
> [    1.724562] ahci f2540000.sata: No port enabled
> 
> Remove from ahci_host_priv the property nports which only makes sense
> when enabled ports are consecutive. It is replaced with AHCI_MAX_PORTS
> and checks for hpriv->mask_port_map, which indicates each port that is
> enabled.
> 
> Update ahci_host_priv properties target_pwrs and phys from dynamically
> allocated arrays to statically allocated to size AHCI_MAX_PORTS.
> 
> Update ahci_platform_get_resources to ignore holes in the port numbers
> and enable ports defined in firmware by their reg property only.
> 
> When firmware does not define children it is assumed that there is
> exactly one port, using index 0.
> 
> I marked this RFC because it was only tested with Linux v6.1, Marvell
> fork, CN9130 Clearfog Pro which has only port number 1 in device-tree.
> Further I am not completely sure if it has severe side-effects on
> other platforms.
> I plan to submit it again after testing on v6.13-rc1, but do welcome
> feedback in the meantime, particularly whether this idea of supporting
> non-consecutive ports is acceptable.
> 
> Signed-off-by: Josua Mayer <josua@solid-run.com>

[...]


> @@ -539,41 +544,7 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
>  		hpriv->f_rsts = flags & AHCI_PLATFORM_RST_TRIGGER;
>  	}
>  
> -	/*
> -	 * Too many sub-nodes most likely means having something wrong with
> -	 * the firmware.
> -	 */
>  	child_nodes = of_get_child_count(dev->of_node);
> -	if (child_nodes > AHCI_MAX_PORTS) {
> -		rc = -EINVAL;
> -		goto err_out;
> -	}

Why remove this check ? Your platform may not need ti, but it is still valid
for others.

> -
> -	/*
> -	 * If no sub-node was found, we still need to set nports to
> -	 * one in order to be able to use the
> -	 * ahci_platform_[en|dis]able_[phys|regulators] functions.
> -	 */
> -	if (child_nodes)
> -		hpriv->nports = child_nodes;
> -	else
> -		hpriv->nports = 1;

Same here.

> -
> -	hpriv->phys = devm_kcalloc(dev, hpriv->nports, sizeof(*hpriv->phys), GFP_KERNEL);
> -	if (!hpriv->phys) {
> -		rc = -ENOMEM;
> -		goto err_out;
> -	}
> -	/*
> -	 * We cannot use devm_ here, since ahci_platform_put_resources() uses
> -	 * target_pwrs after devm_ have freed memory
> -	 */
> -	hpriv->target_pwrs = kcalloc(hpriv->nports, sizeof(*hpriv->target_pwrs), GFP_KERNEL);
> -	if (!hpriv->target_pwrs) {
> -		rc = -ENOMEM;
> -		goto err_out;
> -	}

And for platforms that actually have a valid nports with no ID holes, the above
is OK and uses less memory...

Why not simply adding code that checks the ID of the child nodes ? If there are
no ID holes, then nothing need to change. If there are holes, then
hpriv->nports can be set to the highest ID + 1 and you can set
hpriv->mask_port_map as you go. With just that, you should get everything
working with far less changes than you have here.

>  	if (child_nodes) {
>  		for_each_child_of_node_scoped(dev->of_node, child) {
>  			u32 port;
> @@ -587,7 +558,7 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
>  				goto err_out;
>  			}
>  
> -			if (port >= hpriv->nports) {
> +			if (port >= AHCI_MAX_PORTS) {
>  				dev_warn(dev, "invalid port number %d\n", port);
>  				continue;
>  			}
> @@ -625,6 +596,8 @@ 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;
> 
> ---
> base-commit: adc218676eef25575469234709c2d87185ca223a
> change-id: 20241121-ahci-nonconsecutive-ports-a8911b3255a7
> 
> Best regards,


-- 
Damien Le Moal
Western Digital Research