[PATCH v8 0/2] lan966x pci device: Add support for SFPs, simple-bus part

[PATCH v8 0/2] lan966x pci device: Add support for SFPs, simple-bus part

[Herve Codina]

Hi,

Previously, I sent a big picture series adding support for SFP ports
available on the LAN966x PCI device [0].

In this series patches touch several parts and sub-system in the kernel.
Reviews have be done and it makes sense to split the series and send
parts separately.

This current series is the extraction of patches related to the
simple-bus driver. It has to be seen as a continuation of the big
picture series but related to this specific simple-bus part.

Patches 1 and 2 makes the simple-bus driver compatible with fw_devlink
expectation consisting in creating its child devices at probe.

[0] https://lore.kernel.org/all/20260325143555.451852-1-herve.codina@bootlin.com/

Best regards,
Hervé

Changes:

v7 -> v8
  v7: https://lore.kernel.org/all/20260511155902.34534-1-herve.codina@bootlin.com/

  Rebase on top of v7.2-rc1
  No nother changes

v6 -> v7

  Rebase on top of v7.1-rc1

  - Patch 1
    No changes

  - Patch 2
    Fix conflict due to device_has_driver_override() introduction.

Older iterations:
  Patches 4 and 5 in the big picture series
  https://lore.kernel.org/all/20260325143555.451852-1-herve.codina@bootlin.com/

Herve Codina (2):
  bus: simple-pm-bus: Remove child devices when the bus is unbound
  bus: simple-pm-bus: Populate child nodes at probe

 drivers/bus/simple-pm-bus.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

-- 
2.54.0

[PATCH v8 1/2] bus: simple-pm-bus: Remove child devices when the bus is unbound

[Herve Codina]

During its probe, the simple-pm-bus driver (simple-pm-bus compatible
variant) populates its child devices but it doesn't touch them during
its removal.

This leads to bus child devices present and bound to drivers whereas the
bus itself is unbound.

Be consistent and remove child devices when the bus itself is unbound.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 drivers/bus/simple-pm-bus.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/bus/simple-pm-bus.c b/drivers/bus/simple-pm-bus.c
index c920bd6fbaaf..4b2a793dfbd9 100644
--- a/drivers/bus/simple-pm-bus.c
+++ b/drivers/bus/simple-pm-bus.c
@@ -83,6 +83,9 @@ static void simple_pm_bus_remove(struct platform_device *pdev)
 
 	dev_dbg(&pdev->dev, "%s\n", __func__);
 
+	if (pdev->dev.of_node)
+		of_platform_depopulate(&pdev->dev);
+
 	pm_runtime_disable(&pdev->dev);
 }
 
-- 
2.54.0

[PATCH v8 2/2] bus: simple-pm-bus: Populate child nodes at probe

[Herve Codina]

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 | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/drivers/bus/simple-pm-bus.c b/drivers/bus/simple-pm-bus.c
index 4b2a793dfbd9..d969bf19a4e9 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,7 +80,7 @@ static void simple_pm_bus_remove(struct platform_device *pdev)
 {
 	const void *data = of_device_get_match_data(&pdev->dev);
 
-	if (device_has_driver_override(&pdev->dev) || data)
+	if (device_has_driver_override(&pdev->dev))
 		return;
 
 	dev_dbg(&pdev->dev, "%s\n", __func__);
@@ -86,7 +88,8 @@ static void simple_pm_bus_remove(struct platform_device *pdev)
 	if (pdev->dev.of_node)
 		of_platform_depopulate(&pdev->dev);
 
-	pm_runtime_disable(&pdev->dev);
+	if (!data)
+		pm_runtime_disable(&pdev->dev);
 }
 
 static int simple_pm_bus_runtime_suspend(struct device *dev)
-- 
2.54.0

Re: [PATCH v8 1/2] bus: simple-pm-bus: Remove child devices when the bus is unbound

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The bus device is left in the RPM_ACTIVE state with its clocks permanently enabled upon removal, as `pm_runtime_disable()` cancels the asynchronous idle request triggered by child device removal.
--

commit db0675ba90c5bdf94a4790c06b36a7b5f8dff5e4
Author: Herve Codina <herve.codina@bootlin.com>

bus: simple-pm-bus: Remove child devices when the bus is unbound

During probe, the simple-pm-bus driver populates child devices but does not
remove them during its removal. This commit explicitly depopulates child
devices when the bus is unbound to keep the device state consistent.

> --- a/drivers/bus/simple-pm-bus.c
> +++ b/drivers/bus/simple-pm-bus.c
> @@ -83,6 +83,9 @@ static void simple_pm_bus_remove(struct platform_device *pdev)
>  
>  	dev_dbg(&pdev->dev, "%s\n", __func__);
>  
> +	if (pdev->dev.of_node)
> +		of_platform_depopulate(&pdev->dev);
> +
>  	pm_runtime_disable(&pdev->dev);

[Severity: High]
Will this sequence leave the bus device active and leak clock enable counts?

When of_platform_depopulate() removes the child devices, the parent bus
device's child_count drops to zero, triggering an asynchronous
pm_request_idle() on the parent.

Because pm_runtime_disable() is called immediately afterwards, it will invoke
__pm_runtime_barrier() and synchronously cancel any pending asynchronous PM
requests.

If the idle request is cancelled before it executes, does the bus bypass
simple_pm_bus_runtime_suspend() and remain in the RPM_ACTIVE state with its
clocks permanently enabled? When devres later frees the clock structures via
clk_bulk_put_all(), would the active clock enable counts be leaked, causing
a warning? Should there be a synchronous runtime PM put or suspend operation
before disabling runtime PM?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260630102717.413501-1-herve.codina@bootlin.com?part=1

Re: [PATCH v8 2/2] bus: simple-pm-bus: Populate child nodes at probe

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Time-of-Check to Time-of-Use (TOCTOU) race with the mutable driver_override sysfs attribute causes state mismatch and resource leaks.
- [High] The driver leaks clock enable counts during removal because pm_runtime_disable() is called without synchronously suspending the device.
--

commit 8879d2bb547d47c5c484809ff15676d13cfec9a8
Author: Herve Codina <herve.codina@bootlin.com>

bus: simple-pm-bus: Populate child nodes at probe

The simple-pm-bus driver didn't populate child devices during probe when used
with busses other than a compatible "simple-pm-bus", confusing fw_devlink.
This commit explicitly populates child nodes during probe for those compatible
devices to ensure proper devlink creation and consumer removal ordering.

> diff --git a/drivers/bus/simple-pm-bus.c b/drivers/bus/simple-pm-bus.c
> index 4b2a793dfbd90..d969bf19a4e95 100644
> --- a/drivers/bus/simple-pm-bus.c
> +++ b/drivers/bus/simple-pm-bus.c

[ ... ]

> @@ -78,7 +80,7 @@ static void simple_pm_bus_remove(struct platform_device *pdev)
>  {
>  	const void *data = of_device_get_match_data(&pdev->dev);
>  
> -	if (device_has_driver_override(&pdev->dev) || data)
> +	if (device_has_driver_override(&pdev->dev))

[Severity: High]
This isn't a bug introduced by this patch, but is it safe to rely on
device_has_driver_override() here during removal?

The driver_override sysfs attribute is mutable. If it is changed by a user
after probe() but before remove(), the control flow in remove() will skip
of_platform_depopulate() and pm_runtime_disable(), causing state mismatches.

If the override is cleared after being set during probe, remove() might
attempt to depopulate children it never populated and disable PM runtime it
never enabled.

Could this rely on the driver's own internal state (like dev_get_drvdata())
instead of checking a mutable sysfs attribute?

>  		return;
>  
>  	dev_dbg(&pdev->dev, "%s\n", __func__);
> @@ -86,7 +88,8 @@ static void simple_pm_bus_remove(struct platform_device *pdev)
>  	if (pdev->dev.of_node)
>  		of_platform_depopulate(&pdev->dev);
>  
> -	pm_runtime_disable(&pdev->dev);
> +	if (!data)
> +		pm_runtime_disable(&pdev->dev);

[Severity: High]
This is a pre-existing issue, but does calling pm_runtime_disable() here
without synchronously suspending the device leak clock enable counts?

of_platform_depopulate() removes the children, which may asynchronously drop
the bus's PM usage counter. Calling pm_runtime_disable() immediately
afterward cancels any pending async suspend requests.

If the parent bus remains RPM_ACTIVE when pm_runtime_disable() completes,
devm will free the clock structures without disabling them, permanently
leaking the clock references.

Should the driver synchronously suspend the device before disabling runtime PM?

>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260630102717.413501-1-herve.codina@bootlin.com?part=2