Hi Rafael,
On Fri, 16 May 2025 21:22:20 +0200
"Rafael J. Wysocki" <rafael@xxxxxxxxxx> wrote:
> On Wed, May 7, 2025 at 9:13 AM Herve Codina <herve.codina@xxxxxxxxxxx> 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@xxxxxxxxxxx>
> > ---
> > 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é
