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