| Message ID | 20250507071315.394857-6-herve.codina@bootlin.com |
|---|---|
| State | New |
| Headers | show |
| Series | lan966x pci device: Add support for SFPs | expand |
On Wed, May 07, 2025 at 09:12:47AM +0200, Herve Codina wrote: > The simple-pm-bus drivers handles several simple bus. When it is used bus --> busses ? > with busses other than a compatible "simple-pm-bus", it don't populate > its child devices during its probe. > > This confuses fw_devlink and results in wrong or missing devlinks. > > 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. ... > if (match && match->data) { > if (of_property_match_string(np, "compatible", match->compatible) == 0) Side note, there is an fwnode_is_device_compatible() API for such cases. And IIRC there is also OF variant of it. > - return 0; > + goto populate; > else > return -ENODEV; > } ... > + if (pdev->dev.of_node) Why do you need this check? AFAICS it dups the one the call has already in it. > + of_platform_depopulate(&pdev->dev);
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/ > with busses other than a compatible "simple-pm-bus", it don't populate s/it don't/it doesn't/ > 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? > 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? > + 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? > 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? > + > 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) > --
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. 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; 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__); + 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)
The simple-pm-bus drivers handles several simple bus. When it is used with busses other than a compatible "simple-pm-bus", it don't populate its child devices during its probe. This confuses fw_devlink and results in wrong or missing devlinks. 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(-)