The simple-pm-bus driver handles several simple busses. When it is used
with busses other than a compatible "simple-pm-bus", it doesn'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(-)

diff --git a/drivers/bus/simple-pm-bus.c b/drivers/bus/simple-pm-bus.c
index d8e029e7e53f..cfa05f9bd226 100644
--- a/drivers/bus/simple-pm-bus.c
+++ b/drivers/bus/simple-pm-bus.c
@@ -42,14 +42,15 @@ 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.
+	 * need to have their child nodes populated. So, don't need to do
+	 * anything more except populate child nodes during this probe(). 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 +65,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 +80,15 @@ 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);
+	of_platform_depopulate(&pdev->dev);
+
+	if (!data)
+		pm_runtime_disable(&pdev->dev);
 }
 
 static int simple_pm_bus_runtime_suspend(struct device *dev)
-- 
2.49.0

On Fri, Jun 13, 2025 at 03:47:45PM +0200, Herve Codina wrote:
> The simple-pm-bus driver handles several simple busses. When it is used
> with busses other than a compatible "simple-pm-bus", it doesn'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.

IIRC, skipping child nodes was because there were problems with 
letting the driver handle 'simple-bus'. How does this avoid that now?

The root of_platform_populate() that created the simple-bus device that 
gets us to the probe here will continue descending into child nodes. 
Meanwhile, the probe here is also descending into those same child 
nodes. Best case, that's just redundant. Worst case, won't you still 
have the same problem if the first of_platform_populate() creates the 
devices first?

Rob

Hi Rob,

On Fri, 27 Jun 2025 10:52:00 -0500
Rob Herring <robh@kernel.org> wrote:

> On Fri, Jun 13, 2025 at 03:47:45PM +0200, Herve Codina wrote:
> > The simple-pm-bus driver handles several simple busses. When it is used
> > with busses other than a compatible "simple-pm-bus", it doesn'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.  
> 
> IIRC, skipping child nodes was because there were problems with 
> letting the driver handle 'simple-bus'. How does this avoid that now?

I don't know about the specific issues related to those problems. Do you
have some pointers about them?

> 
> The root of_platform_populate() that created the simple-bus device that 
> gets us to the probe here will continue descending into child nodes. 
> Meanwhile, the probe here is also descending into those same child 
> nodes. Best case, that's just redundant. Worst case, won't you still 
> have the same problem if the first of_platform_populate() creates the 
> devices first?
> 

Maybe we could simply avoid of_platform_populate() to be recursive when a
device populate by of_platform_populate() is one of devices handled by
the simple-bus driver and let the simple-bus driver do the job.

of_platform_populate will handle the first level. It will populate children
of the node given to of_platform_populate() and the children of those
children will be populate by the simple-bus driver.

I could try a modification in that way. Do you think it could be a correct
solution?

Best regards,
Hervé

Hi Rob,

On Thu, 3 Jul 2025 09:33:02 +0200
Herve Codina <herve.codina@bootlin.com> wrote:

> Hi Rob,
> 
> On Fri, 27 Jun 2025 10:52:00 -0500
> Rob Herring <robh@kernel.org> wrote:
> 
> > On Fri, Jun 13, 2025 at 03:47:45PM +0200, Herve Codina wrote:  
> > > The simple-pm-bus driver handles several simple busses. When it is used
> > > with busses other than a compatible "simple-pm-bus", it doesn'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.    
> > 
> > IIRC, skipping child nodes was because there were problems with 
> > letting the driver handle 'simple-bus'. How does this avoid that now?  
> 
> I don't know about the specific issues related to those problems. Do you
> have some pointers about them?
> 
> > 
> > The root of_platform_populate() that created the simple-bus device that 
> > gets us to the probe here will continue descending into child nodes. 
> > Meanwhile, the probe here is also descending into those same child 
> > nodes. Best case, that's just redundant. Worst case, won't you still 
> > have the same problem if the first of_platform_populate() creates the 
> > devices first?
> >   
> 
> Maybe we could simply avoid of_platform_populate() to be recursive when a
> device populate by of_platform_populate() is one of devices handled by
> the simple-bus driver and let the simple-bus driver do the job.
> 
> of_platform_populate will handle the first level. It will populate children
> of the node given to of_platform_populate() and the children of those
> children will be populate by the simple-bus driver.
> 
> I could try a modification in that way. Do you think it could be a correct
> solution?
> 

I have started to look at this solution and it's going to be more complex
than than I thought.

Many MFD drivers uses a compatible of this kind (the same exist for bus
driver with "simple-bus"):
  compatible = "foo,bar", "simple-mfd";

Usually the last compatible string ("simple-mfd" here) is a last fallback
and the first string is the more specific one.

In the problematic case, "foo,bar" has a specific driver and the driver
performs some operations at probe() but doesn't call of_platform_populate()
and relies on the core to do the device creations (recursively) based on
the "simple,mfd" string present in the compatible property.

Some other calls of_platform_populate() in they probe (which I think is
correct) and in that case, the child device creation can be done at two
location: specific driver probe() and core.

You pointed out that the core could create devices before the specific
driver is probed. In that case, some of existing drivers calling
of_platform_populate() are going to have issues.

I can try to modify existing MFD and bus drivers (compatible fallback to
simple-mfd, simple-bus, ...) in order to have them call of_platform_populate()
in they probe() and after all problematic drivers are converted, the
recursive creation of devices done in the core could be removed.

Before starting in that way, can you confirm that this is the right
direction?

Best regards,
Hervé

On Fri, Jul 4, 2025 at 3:57 AM Herve Codina <herve.codina@bootlin.com> wrote:
>
> Hi Rob,
>
> On Thu, 3 Jul 2025 09:33:02 +0200
> Herve Codina <herve.codina@bootlin.com> wrote:
>
> > Hi Rob,
> >
> > On Fri, 27 Jun 2025 10:52:00 -0500
> > Rob Herring <robh@kernel.org> wrote:
> >
> > > On Fri, Jun 13, 2025 at 03:47:45PM +0200, Herve Codina wrote:
> > > > The simple-pm-bus driver handles several simple busses. When it is used
> > > > with busses other than a compatible "simple-pm-bus", it doesn'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.
> > >
> > > IIRC, skipping child nodes was because there were problems with
> > > letting the driver handle 'simple-bus'. How does this avoid that now?
> >
> > I don't know about the specific issues related to those problems. Do you
> > have some pointers about them?
> >
> > >
> > > The root of_platform_populate() that created the simple-bus device that
> > > gets us to the probe here will continue descending into child nodes.
> > > Meanwhile, the probe here is also descending into those same child
> > > nodes. Best case, that's just redundant. Worst case, won't you still
> > > have the same problem if the first of_platform_populate() creates the
> > > devices first?
> > >
> >
> > Maybe we could simply avoid of_platform_populate() to be recursive when a
> > device populate by of_platform_populate() is one of devices handled by
> > the simple-bus driver and let the simple-bus driver do the job.
> >
> > of_platform_populate will handle the first level. It will populate children
> > of the node given to of_platform_populate() and the children of those
> > children will be populate by the simple-bus driver.
> >
> > I could try a modification in that way. Do you think it could be a correct
> > solution?
> >
>
> I have started to look at this solution and it's going to be more complex
> than than I thought.
>
> Many MFD drivers uses a compatible of this kind (the same exist for bus
> driver with "simple-bus"):
>   compatible = "foo,bar", "simple-mfd";
>
> Usually the last compatible string ("simple-mfd" here) is a last fallback
> and the first string is the more specific one.
>
> In the problematic case, "foo,bar" has a specific driver and the driver
> performs some operations at probe() but doesn't call of_platform_populate()
> and relies on the core to do the device creations (recursively) based on
> the "simple,mfd" string present in the compatible property.
>
> Some other calls of_platform_populate() in they probe (which I think is
> correct) and in that case, the child device creation can be done at two
> location: specific driver probe() and core.
>
> You pointed out that the core could create devices before the specific
> driver is probed. In that case, some of existing drivers calling
> of_platform_populate() are going to have issues.
>
> I can try to modify existing MFD and bus drivers (compatible fallback to
> simple-mfd, simple-bus, ...) in order to have them call of_platform_populate()
> in they probe() and after all problematic drivers are converted, the
> recursive creation of devices done in the core could be removed.

The problem is how does a bus driver know if there is a specific MFD
driver or not? It doesn't. The MFD driver could be a module and loaded
any time later. We'd really need some sort of unbind the generic
driver and re-bind to a more specific driver when and if that driver
appears. We could perhaps have a list of devices with a driver because
in theory that should be a short list as the (broken) promise of
simple-mfd is the child nodes have no dependency on the parent node
which implies the parent doesn't have a driver. The specific
compatible is there in case that assumption turns out wrong.

Rob

Hi Rob,

On Mon, 14 Jul 2025 12:44:22 -0500
Rob Herring <robh@kernel.org> wrote:

> On Fri, Jul 4, 2025 at 3:57 AM Herve Codina <herve.codina@bootlin.com> wrote:
> >
> > Hi Rob,
> >
> > On Thu, 3 Jul 2025 09:33:02 +0200
> > Herve Codina <herve.codina@bootlin.com> wrote:
> >  
> > > Hi Rob,
> > >
> > > On Fri, 27 Jun 2025 10:52:00 -0500
> > > Rob Herring <robh@kernel.org> wrote:
> > >  
> > > > On Fri, Jun 13, 2025 at 03:47:45PM +0200, Herve Codina wrote:  
> > > > > The simple-pm-bus driver handles several simple busses. When it is used
> > > > > with busses other than a compatible "simple-pm-bus", it doesn'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.  
> > > >
> > > > IIRC, skipping child nodes was because there were problems with
> > > > letting the driver handle 'simple-bus'. How does this avoid that now?  
> > >
> > > I don't know about the specific issues related to those problems. Do you
> > > have some pointers about them?
> > >  
> > > >
> > > > The root of_platform_populate() that created the simple-bus device that
> > > > gets us to the probe here will continue descending into child nodes.
> > > > Meanwhile, the probe here is also descending into those same child
> > > > nodes. Best case, that's just redundant. Worst case, won't you still
> > > > have the same problem if the first of_platform_populate() creates the
> > > > devices first?
> > > >  
> > >
> > > Maybe we could simply avoid of_platform_populate() to be recursive when a
> > > device populate by of_platform_populate() is one of devices handled by
> > > the simple-bus driver and let the simple-bus driver do the job.
> > >
> > > of_platform_populate will handle the first level. It will populate children
> > > of the node given to of_platform_populate() and the children of those
> > > children will be populate by the simple-bus driver.
> > >
> > > I could try a modification in that way. Do you think it could be a correct
> > > solution?
> > >  
> >
> > I have started to look at this solution and it's going to be more complex
> > than than I thought.
> >
> > Many MFD drivers uses a compatible of this kind (the same exist for bus
> > driver with "simple-bus"):
> >   compatible = "foo,bar", "simple-mfd";
> >
> > Usually the last compatible string ("simple-mfd" here) is a last fallback
> > and the first string is the more specific one.
> >
> > In the problematic case, "foo,bar" has a specific driver and the driver
> > performs some operations at probe() but doesn't call of_platform_populate()
> > and relies on the core to do the device creations (recursively) based on
> > the "simple,mfd" string present in the compatible property.
> >
> > Some other calls of_platform_populate() in they probe (which I think is
> > correct) and in that case, the child device creation can be done at two
> > location: specific driver probe() and core.
> >
> > You pointed out that the core could create devices before the specific
> > driver is probed. In that case, some of existing drivers calling
> > of_platform_populate() are going to have issues.
> >
> > I can try to modify existing MFD and bus drivers (compatible fallback to
> > simple-mfd, simple-bus, ...) in order to have them call of_platform_populate()
> > in they probe() and after all problematic drivers are converted, the
> > recursive creation of devices done in the core could be removed.  
> 
> The problem is how does a bus driver know if there is a specific MFD
> driver or not? It doesn't. The MFD driver could be a module and loaded
> any time later. We'd really need some sort of unbind the generic
> driver and re-bind to a more specific driver when and if that driver
> appears. We could perhaps have a list of devices with a driver because
> in theory that should be a short list as the (broken) promise of
> simple-mfd is the child nodes have no dependency on the parent node
> which implies the parent doesn't have a driver. The specific
> compatible is there in case that assumption turns out wrong.
> 

Hum, I see.

In my use case, I don't use MFD drivers but only simple-bus compatible.
I think your point is also relevant with simple-bus. Indeed how does a
parent bus driver know if there is a specific bus driver that handles
the child simple-bus compatible one in case of 'simple-bus' used as
fallback.

Related to your proposal related to the "list of devices with a driver",
what do you mean? I don't see how to set this kind of list. Can you give
me some pointers?

If I understood the discussion, the issue seems that 'simple-bus' can't
populate unconditionally children at his probe. The possible recursion
in creating devices done by of_platform_populate() should be kept and
'simple-bus' should rely on that.

The other solution that fixes my use case is to use an other compatible
string. Would you accept a new compatible string: "simple-platform-bus"?

In simple-pm-bus.c, this compatible would populate children at probe.
In fact, it will act the same way as 'simple-pm-bus' without looking at
clocks nor handling pm_runtime.

Best regards,
Hervé

On Fri, Jun 13, 2025 at 03:47:45PM +0200, Herve Codina wrote:
> The simple-pm-bus driver handles several simple busses. When it is used
> with busses other than a compatible "simple-pm-bus", it doesn'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)
> -			return 0;
> +			goto populate;
>  		else
>  			return -ENODEV;
>  	}

A nit: seems that now we don't need to keep a symmetry in the branches and
hence the redundant 'else' can be dropped

		if (of_property_match_string(np, "compatible", match->compatible) != 0)
			return -ENODEV;
		goto populate;
	}

But this might be out of the scope here and can be done later on. In my opinion
it will make code slightly easier to follow.


-- 
With Best Regards,
Andy Shevchenko