Re: [PATCH v2 05/26] bus: simple-pm-bus: Populate child nodes at probe
From: "Rafael J. Wysocki" <rafael@kernel.org>
Date: 2025-05-16 19:22:32
Also in:
imx, linux-acpi, linux-clk, linux-devicetree, linux-i2c, linux-pci, linux-spi, lkml
On Wed, May 7, 2025 at 9:13 AM Herve Codina [off-list ref] 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?
quoted hunk ↗ jump to hunk
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?
quoted hunk ↗ jump to hunk
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?
quoted hunk ↗ jump to hunk
+ 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) --