Re: [PATCH v2 05/26] bus: simple-pm-bus: Populate child nodes at probe

[Herve Codina <herve.codina@bootlin.com>]

Hi Rafael,

On Fri, 16 May 2025 21:22:20 +0200
"Rafael J. Wysocki" <rafael@kernel.org> wrote:

> On Wed, May 7, 2025 at 9:13 AM Herve Codina <herve.codina@bootlin.com> wrote:
> >
> > The simple-pm-bus drivers handles several simple bus. When it is used  
> 
> s/drivers/driver/ (I think)
> s/simple bus/simple busses/

Will be fixed.

> 
> > with busses other than a compatible "simple-pm-bus", it don't populate  
> 
> s/it don't/it doesn't/

Will be fixed.

> 
> > its child devices during its probe.
> >
> > This confuses fw_devlink and results in wrong or missing devlinks.  
> 
> Well, fair enough, but doesn't it do that for a reason?

I think devlink is confused just because "simple-bus" or similar handled
by this driver didn't follow the devlink rule: "Child nodes should be
created during parent probe".

Suppose the following:
   foo@0 {
	compatible = "vendor,foo"

	bar@0 {
		compatible = "simple-bus";

		baz@100 {
			compatible = "vendor,baz"
		};
	};
   };

The foo driver probe() calls from of_platform_default_populate() to create
the bar device.

The bar is create and thanks to its compatible string, the simple-bus
probe() is called. Without my modification, the baz device was not created
during the simple-bus probe().

of_platform_default_populate() called from foo probe() creates the baz
device thanks to the recursive of_platform_bus_create() call.

This leads the baz device created outside the bar probe() call.
This "out of bus probe()" device creation confuses devlink.

> 
> > Once a driver is bound to a device and the probe() has been called,
> > device_links_driver_bound() is called.
> >
> > This function performs operation based on the following assumption:
> >     If a child firmware node of the bound device is not added as a
> >     device, it will never be added.
> >
> > Among operations done on fw_devlinks of those "never be added" devices,
> > device_links_driver_bound() changes their supplier.
> >
> > With devices attached to a simple-bus compatible device, this change
> > leads to wrong devlinks where supplier of devices points to the device
> > parent (i.e. simple-bus compatible device) instead of the device itself
> > (i.e. simple-bus child).
> >
> > When the device attached to the simple-bus is removed, because devlinks
> > are not correct, its consumers are not removed first.
> >
> > In order to have correct devlinks created, make the simple-pm-bus driver
> > compliant with the devlink assumption and create its child devices
> > during its probe.
> >
> > Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> > ---
> >  drivers/bus/simple-pm-bus.c | 23 ++++++++++++++---------
> >  1 file changed, 14 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/bus/simple-pm-bus.c b/drivers/bus/simple-pm-bus.c
> > index d8e029e7e53f..93c6ba605d7a 100644
> > --- a/drivers/bus/simple-pm-bus.c
> > +++ b/drivers/bus/simple-pm-bus.c
> > @@ -42,14 +42,14 @@ 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
> > -        * have their child nodes populated automatically.  So, don't need to
> > -        * do anything more. We only match with the device if this driver is
> > -        * the most specific match because we don't want to incorrectly bind to
> > -        * a device that has a more specific driver.
> > +        * have their child nodes populated automatically. So, don't need to
> > +        * do anything more except populate child nodes.  
> 
> The above part of the comment has become hard to grasp after the
> change.  In particular, why populate child notes if they are populated
> automatically?

What do you thing about:
	/*
	 * These are transparent bus devices (not simple-pm-bus matches) that
	 * have their child nodes be populated automatically. So, don't need to
	 * do anything more except populate child nodes. We only match with the
	 * device if this driver is the most specific match because we don't
	 * want to incorrectly bind to a device that has a more specific driver.
	 */

> 
> > + We only match with the
> > +        * device if this driver is the most specific match because we don't
> > +        * want to incorrectly bind to a device that has a more specific driver.
> >          */
> >         if (match && match->data) {
> >                 if (of_property_match_string(np, "compatible", match->compatible) == 0)
> > -                       return 0;
> > +                       goto populate;  
> 
> Doesn't this interfere with anything, like the automatic population of
> child nodes mentioned in the comment?

I don't think so.

Device population is protected against multiple calls with OF_POPULATED_BUS
flag:
  https://elixir.bootlin.com/linux/v6.15-rc6/source/drivers/of/platform.c#L349

> 
> >                 else
> >                         return -ENODEV;
> >         }
> > @@ -64,13 +64,14 @@ static int simple_pm_bus_probe(struct platform_device *pdev)
> >
> >         dev_set_drvdata(&pdev->dev, bus);
> >
> > -       dev_dbg(&pdev->dev, "%s\n", __func__);
> > -
> >         pm_runtime_enable(&pdev->dev);
> >
> > +populate:
> >         if (np)
> >                 of_platform_populate(np, NULL, lookup, &pdev->dev);
> >
> > +       dev_dbg(&pdev->dev, "%s\n", __func__);  
> 
> So how to distinguish between devices that only have child nodes
> populated and the ones that have drvdata set?

Hum, I don't see your point.
Can you clarify ?

> 
> > +
> >         return 0;
> >  }
> >
> > @@ -78,12 +79,16 @@ static void simple_pm_bus_remove(struct platform_device *pdev)
> >  {
> >         const void *data = of_device_get_match_data(&pdev->dev);
> >
> > -       if (pdev->driver_override || data)
> > +       if (pdev->driver_override)
> >                 return;
> >
> >         dev_dbg(&pdev->dev, "%s\n", __func__);
> >
> > -       pm_runtime_disable(&pdev->dev);
> > +       if (pdev->dev.of_node)
> > +               of_platform_depopulate(&pdev->dev);
> > +
> > +       if (!data)
> > +               pm_runtime_disable(&pdev->dev);
> >  }
> >
> >  static int simple_pm_bus_runtime_suspend(struct device *dev)
> > --  

Thanks for your feedback.

Best regards,
Hervé