Skip to contentSkip to conversation

lan966x pci device: Add support for SFPs, simple-bus part

PATCH Superseded Herve Codina 2 participants 6 messages May 11 – Jun 5 Driver core

v7 of 14 Superseded Latest is v14 →

lore
Share & tools
lore

Herve Codina

author
#0 lore raw

v7 cover lan966x pci device: Add support for SFPs, simple-bus part

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/[email protected]/

Best regards,
Hervé

Changes:

v6 -> v7

Rebase on top 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/[email protected]/

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(-)
Signature · 1 line
2.54.0
author
#1 lore raw

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

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 <[email protected]>
---
 drivers/bus/simple-pm-bus.c | 3 +++
 1 file changed, 3 insertions(+)
drivers/bus/simple-pm-bus.c +3 -0
index c920bd6fbaaf..4b2a793dfbd9 100644
@@ -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);
 }
Signature · 1 line
2.54.0
author
#2 lore raw

PATCH v7 2/2 bus: simple-pm-bus: Populate child nodes at probe

In reply to 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 <[email protected]>
---
 drivers/bus/simple-pm-bus.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)
drivers/bus/simple-pm-bus.c +12 -9
index 4b2a793dfbd9..d969bf19a4e9 100644
@@ -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)
Signature · 1 line
2.54.0
AI review
#3 lore raw

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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] The patch resolves fw_devlink corruption for transparent buses matched via `of_match_table`, but…

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The patch resolves fw_devlink corruption for transparent buses matched via of_match_table, but fails to apply this fix to transparent buses bound via driver_override, leaving devlinks broken in that path. --

commit 428668c33cfe17b55dfc398a9e43567a082e1c3e
Author: Herve Codina <[email protected]>

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

This commit modifies the simple-pm-bus driver to populate child nodes during probe for transparent buses matched via of_match_table. This prevents wrong or missing devlinks when fw_devlink assumes a bound driver will populate its child firmware nodes.

Quoted 8 lines
drivers/bus/simple-pm-bus.c +0 -0
index 4b2a793dfbd90..d969bf19a4e95 100644
@@ -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

[ ... ]

drivers/bus/simple-pm-bus.c static int simple_pm_bus_probe(struct platform_device *pdev) · line 56

Quoted 7 lines
drivers/bus/simple-pm-bus.c +1 -1
 	if (match && match->data) {
 		if (of_property_match_string(np, "compatible", match->compatible) == 0)
-			return 0;
+			goto populate;
 		else
 			return -ENODEV;
 	}

Does this leave fw_devlink corrupted when the driver is bound using driver_override?

Looking at simple_pm_bus_probe(), there is an early exit for driver_override just before this block:

	if (device_has_driver_override(&pdev->dev))
		return 0;

Since it still returns directly instead of jumping to the populate label, transparent buses bound via the driver_override sysfs attribute will skip populating their child nodes. This seems to trigger the same wrong supplier assignments from fw_devlink that this patch intended to fix.

drivers/bus/simple-pm-bus.c static void simple_pm_bus_remove(struct platform_device *pdev) · line 89

Quoted 12 lines
drivers/bus/simple-pm-bus.c +1 -1
@@ -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);

If the driver_override path in probe() is updated to populate children, should this remove() function also be updated to depopulate them?

Additionally, does checking device_has_driver_override() here risk unbalanced cleanup? If userspace modifies the driver_override sysfs attribute after probe() but before remove(), this check could evaluate differently during removal. Since of_platform_depopulate() safely ignores unpopulated buses, would it be better to call it unconditionally?

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
author
#4 lore raw

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

Hi all,

Quoted 37 lines · On Tue, 12 May 2026 22:12:30 +0000 [email protected] wrote:

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The patch resolves fw_devlink corruption for transparent buses matched via of_match_table, but fails to apply this fix to transparent buses bound via driver_override, leaving devlinks broken in that path. --

commit 428668c33cfe17b55dfc398a9e43567a082e1c3e
Author: Herve Codina <[email protected]>

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

This commit modifies the simple-pm-bus driver to populate child nodes during probe for transparent buses matched via of_match_table. This prevents wrong or missing devlinks when fw_devlink assumes a bound driver will populate its child firmware nodes.

Quoted 8 lines
drivers/bus/simple-pm-bus.c +0 -0
index 4b2a793dfbd90..d969bf19a4e95 100644
@@ -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

[ ... ]

drivers/bus/simple-pm-bus.c static int simple_pm_bus_probe(struct platform_device *pdev) · line 56

Quoted 7 lines
drivers/bus/simple-pm-bus.c +1 -1
 	if (match && match->data) {
 		if (of_property_match_string(np, "compatible", match->compatible) == 0)
-			return 0;
+			goto populate;
 		else
 			return -ENODEV;
 	}

Does this leave fw_devlink corrupted when the driver is bound using driver_override?

Looking at simple_pm_bus_probe(), there is an early exit for driver_override just before this block:

Quoted 7 lines
	if (device_has_driver_override(&pdev->dev))
		return 0;

Since it still returns directly instead of jumping to the populate label, transparent buses bound via the driver_override sysfs attribute will skip populating their child nodes. This seems to trigger the same wrong supplier assignments from fw_devlink that this patch intended to fix.

Well if driver_override is used fw_devlink issues related to simple-bus can be present.

The idea in the original driver was to do nothing when driver_override is used. This idea has been kept to avoid any regression.

Quoted 17 lines

drivers/bus/simple-pm-bus.c static void simple_pm_bus_remove(struct platform_device *pdev) · line 89

Quoted 13 lines
drivers/bus/simple-pm-bus.c +1 -1
@@ -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);

If the driver_override path in probe() is updated to populate children, should this remove() function also be updated to depopulate them?

Quoted 6 lines

Additionally, does checking device_has_driver_override() here risk unbalanced cleanup? If userspace modifies the driver_override sysfs attribute after probe() but before remove(), this check could evaluate differently during removal. Since of_platform_depopulate() safely ignores unpopulated buses, would it be better to call it unconditionally?

Not sure that calling it unconditionally should be done. The original driver behavior at remove() has been kept.

Maintainers, what is your feeling about this driver_override topic here with modification and in the original driver?

Best regards,
Hervé

author
#5 lore raw
In reply to Herve Codina

Hi all,

Quoted 9 lines · On Mon, 11 May 2026 17:58:58 +0200 Herve Codina <[email protected]>…

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.

Quoted 9 lines

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/[email protected]/

I haven't receive any comment on the series.

Is there anything I can do to move forward on this simple-bus driver?

Best regards,
Hervé