| 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) > --
Hi Andy, On Thu, 8 May 2025 17:27:52 +0300 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > 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 ? Yes sure. > > > 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. fwnode_device_is_compatible() checked for all compatible string. I mean, if we have compatible = "foo,custom-bus", "simple-bus"; fwnode_device_is_compatible() checking against "simple-bus" returns true. Here, we want "simple-bus" as the first position in the compatible string. In other word, we want to match the more specific compatible string as mentioned in the comment. > > > - 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_populate() was called only if an OF node is present. I want to call of_platform_depopulate() on removal also only if an OF node is present. I don't see the other call that duplicated this check. Can you clarify? > > > + of_platform_depopulate(&pdev->dev); > Best regards, Hervé
On Mon, May 19, 2025 at 01:58:18PM +0200, Herve Codina wrote: > On Thu, 8 May 2025 17:27:52 +0300 > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Wed, May 07, 2025 at 09:12:47AM +0200, Herve Codina wrote: ... > > > 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. > > fwnode_device_is_compatible() checked for all compatible string. I mean, if > we have compatible = "foo,custom-bus", "simple-bus"; > fwnode_device_is_compatible() checking against "simple-bus" returns true. > > Here, we want "simple-bus" as the first position in the compatible string. > In other word, we want to match the more specific compatible string as > mentioned in the comment. I admit I'm not an expert in DT, but why is the compatibility position dependent? ... > > > + if (pdev->dev.of_node) > > > > Why do you need this check? AFAICS it dups the one the call has already in it. > > of_platform_populate() was called only if an OF node is present. > I want to call of_platform_depopulate() on removal also only if an OF node > is present. > > I don't see the other call that duplicated this check. > > Can you clarify? The of_...() is already NULL-aware (AFAICS), why do you need the duplicated check? > > > + of_platform_depopulate(&pdev->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é
Hi Andy, On Mon, 19 May 2025 15:06:33 +0300 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Mon, May 19, 2025 at 01:58:18PM +0200, Herve Codina wrote: > > On Thu, 8 May 2025 17:27:52 +0300 > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > > On Wed, May 07, 2025 at 09:12:47AM +0200, Herve Codina wrote: > > ... > > > > > 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. > > > > fwnode_device_is_compatible() checked for all compatible string. I mean, if > > we have compatible = "foo,custom-bus", "simple-bus"; > > fwnode_device_is_compatible() checking against "simple-bus" returns true. > > > > Here, we want "simple-bus" as the first position in the compatible string. > > In other word, we want to match the more specific compatible string as > > mentioned in the comment. > > I admit I'm not an expert in DT, but why is the compatibility position > dependent? > > ... > > > > > + if (pdev->dev.of_node) > > > > > > Why do you need this check? AFAICS it dups the one the call has already in it. > > > > of_platform_populate() was called only if an OF node is present. > > I want to call of_platform_depopulate() on removal also only if an OF node > > is present. > > > > I don't see the other call that duplicated this check. > > > > Can you clarify? > > The of_...() is already NULL-aware (AFAICS), why do you need the duplicated > check? Oh, I see. I was focused on previous of_device_get_match_data() call. My bad. Indeed, you're right, I can call directly of_platform_depopulate() without checking pdev->dev.of_node before the call. The check is already present in of_platform_depopulate() itself. I will do that in the next iteration. Thanks for pointing out. 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(-)