Re: [PATCH] ata: libahci_platform: use of_platform_device_create() return value

On 7/9/26 10:59, Rosen Penev wrote:
> On Wed, Jul 8, 2026 at 6:50 PM <[email protected]> wrote:
>>
>> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>> - [High] Using the return value of `of_platform_device_create()` breaks target regulator fetching during driver rebind or probe deferral, leading to unpowered SATA ports.
>> --
>>
>> --- Patch [1]: [PATCH] ata: libahci_platform: use of_platform_device_create() return value ---
>> commit b10e7693c196a90e6a36f9889202414e7cb20fc9
>> Author: Rosen Penev <[email protected]>
>>
>> ata: libahci_platform: use of_platform_device_create() return value
>>
>> This commit updates ahci_platform_get_resources() to use the return value
>> of of_platform_device_create() directly, removing the subsequent call to
>> of_find_device_by_node() to avoid needing to free the reference.
>>
>>> --- a/drivers/ata/libahci_platform.c
>>> +++ b/drivers/ata/libahci_platform.c
>>> @@ -617,10 +617,7 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
>>>                       mask_port_map |= BIT(port);
>>>
>>>  #ifdef CONFIG_OF_ADDRESS
>>> -                     of_platform_device_create(child, NULL, NULL);
>>> -
>>> -                     port_dev = of_find_device_by_node(child);
>>> -
>>> +                     port_dev = of_platform_device_create(child, NULL, NULL);
>>>                       if (port_dev) {
>>>                               rc = ahci_platform_get_regulator(hpriv, port,
>>>                                                               &port_dev->dev);
>>
>> [Severity: High]
>> Does this change break regulator fetching during a probe deferral?
>>
>> When of_platform_device_create() successfully creates the device on the first
>> probe attempt, it sets the OF_POPULATED flag on the child device node.
>>
>> If a subsequent initialization step triggers a probe deferral (for instance,
>> returning -EPROBE_DEFER), the probe is aborted. However, the driver's cleanup
>> path does not depopulate the platform device, so the OF_POPULATED flag remains
>> set on the child node.
>>
>> During the second probe attempt, of_platform_device_create() sees the
>> OF_POPULATED flag and returns NULL.
>>
>> Because port_dev is now NULL, the block above is bypassed, and
>> ahci_platform_get_regulator() is silently skipped. This leaves the SATA port
>> without its required target power regulator.
> Welp. So much for that.

Indeed. Not a great API... So we would need a pattern like:

		if (of_node_check_flag(child, OF_POPULATED))
			port_dev = of_find_device_by_node(child);
		else
			port_dev = of_platform_device_create(child, NULL, NULL);

Which kind of makes the patch not needed. Let's drop it ?

-- 
Damien Le Moal
Western Digital Research