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

On Wed, Jul 8, 2026 at 7:00 PM Damien Le Moal <[email protected]> wrote:
>
> 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 ?
Yes.
>
> --
> Damien Le Moal
> Western Digital Research