Re: [PATCH v7 2/2] bus: simple-pm-bus: Populate child nodes at probe

Hi all,

On Tue, 12 May 2026 22:12:30 +0000
[email protected] wrote:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] The patch resolves fw_devlink corruption for transparent buses matched via `of_match_table`, but fails to apply this fix to transparent buses bound via `driver_override`, leaving devlinks broken in that path.
> --
> 
> commit 428668c33cfe17b55dfc398a9e43567a082e1c3e
> Author: Herve Codina <[email protected]>
> 
> bus: simple-pm-bus: Populate child nodes at probe
> 
> This commit modifies the simple-pm-bus driver to populate child nodes
> during probe for transparent buses matched via of_match_table. This prevents
> wrong or missing devlinks when fw_devlink assumes a bound driver will
> populate its child firmware nodes.
> 
> > diff --git a/drivers/bus/simple-pm-bus.c b/drivers/bus/simple-pm-bus.c
> > index 4b2a793dfbd90..d969bf19a4e95 100644
> > --- a/drivers/bus/simple-pm-bus.c
> > +++ b/drivers/bus/simple-pm-bus.c
> > @@ -42,14 +42,15 @@ static int simple_pm_bus_probe(struct platform_device *pdev)
> >  	match = of_match_device(dev->driver->of_match_table, dev);
> >  	/*
> >  	 * These are transparent bus devices (not simple-pm-bus matches) that  
> [ ... ]
> >  	if (match && match->data) {
> >  		if (of_property_match_string(np, "compatible", match->compatible) == 0)
> > -			return 0;
> > +			goto populate;
> >  		else
> >  			return -ENODEV;
> >  	}  
> 
> Does this leave fw_devlink corrupted when the driver is bound using
> driver_override?
> 
> Looking at simple_pm_bus_probe(), there is an early exit for driver_override
> just before this block:
> 
> 	if (device_has_driver_override(&pdev->dev))
> 		return 0;
> 
> Since it still returns directly instead of jumping to the populate label,
> transparent buses bound via the driver_override sysfs attribute will skip
> populating their child nodes. This seems to trigger the same wrong
> supplier assignments from fw_devlink that this patch intended to fix.

Well if driver_override is used fw_devlink issues related to simple-bus can
be present.

The idea in the original driver was to do nothing when driver_override is
used. This idea has been kept to avoid any regression.

> 
> > @@ -78,7 +80,7 @@ static void simple_pm_bus_remove(struct platform_device *pdev)
> >  {
> >  	const void *data = of_device_get_match_data(&pdev->dev);
> >  
> > -	if (device_has_driver_override(&pdev->dev) || data)
> > +	if (device_has_driver_override(&pdev->dev))
> >  		return;
> >  
> >  	dev_dbg(&pdev->dev, "%s\n", __func__);
> > @@ -86,7 +88,8 @@ static void simple_pm_bus_remove(struct platform_device *pdev)
> >  	if (pdev->dev.of_node)
> >  		of_platform_depopulate(&pdev->dev);  
> 
> If the driver_override path in probe() is updated to populate children,
> should this remove() function also be updated to depopulate them?
> 
> Additionally, does checking device_has_driver_override() here risk unbalanced
> cleanup? If userspace modifies the driver_override sysfs attribute after
> probe() but before remove(), this check could evaluate differently during
> removal. Since of_platform_depopulate() safely ignores unpopulated buses,
> would it be better to call it unconditionally?
> 

Not sure that calling it unconditionally should be done.
The original driver behavior at remove() has been kept.

Maintainers, what is your feeling about this driver_override topic here
with modification and in the original driver?

Best regards,
Hervé