| 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);
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(-)