Message-ID: <add760cc-1011-4f12-aa54-7334a2e7245d@kernel.org>
Date: Wed, 27 Nov 2024 21:04:36 +0900
From: Damien Le Moal <dlemoal@...nel.org>
To: Josua Mayer <josua@...id-run.com>, Niklas Cassel <cassel@...nel.org>,
 Hans de Goede <hdegoede@...hat.com>
Cc: Jon Nettleton <jon@...id-run.com>,
 Mikhail Anikin <mikhail.anikin@...id-run.com>,
 Yazan Shhady <yazan.shhady@...id-run.com>,
 Rabeeh Khoury <rabeeh@...id-run.com>,
 "linux-ide@...r.kernel.org" <linux-ide@...r.kernel.org>,
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH RFC] ata: libahci_platform: support non-consecutive port
 numbers

On 11/27/24 20:52, Josua Mayer wrote:
> Am 25.11.24 um 02:12 schrieb Damien Le Moal:
> 
>> 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@...id-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.
> The check is superfluous, since the following loop will print a warning
> and ignore any child with port number greater than AHCI_MAX_PORTS.
> The check merely protected against dynamically allocating greater than
> AHCI_MAX_PORTS.
>>
>>> -
>>> -	/*
>>> -	 * 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.
> This is already handled in else case of if (child_nodes)
>>
>>> -
>>> -	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...
> The port number is being used as index into the target_pwrs and phys arrays,
> which is why those arrays must allocate at least to the highest port id.
> A better way to save memory is by cleaning out this semantic,
> e.g. by dynamically allocating a structure of id, phy and supply for each port.
>>
>> 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.
> This would make the already complex function more complex and less readable.
> I prefer to reduce corner cases rather than adding extras.

It would not. Simply implement a helper function that scans the OF IDs and
return a mask of ports and max ID. With that, the modifications of
ahci_platform_get_resources() would be very minimal.

>> 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

Powered by blists - more mailing lists