lan966x pci device: Add support for SFPs, PCI 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 issues in the PCI subsystem. It has to be seen as a continuation of the big picture series but related to this specific core part.

Patches 1 to 6 introduce and use fw_devlink_set_device() in already existing code. No functional change but the introduction of the fw_devlink_set_device() wrapper.

Patch 7 avoid a dangling pointer.

Patches 8 and 9 fix fw_devlink issues specific to PCI and the device-tree nodes created during enumeration.

[0] https://lore.kernel.org/all/[email protected]/

Best regards,
Hervé

Changes:

v8 -> v9
v8: https://lore.kernel.org/all/[email protected]/

- Patch 7 (new patch in v9)

    Clear fwnode->dev during root bridge node removal (avoid a dangling
    pointer).

- Patch 8 (patch 7 in v8)

    Clear fwnode->dev during PCI device node removal.

v7 -> v8
v7: https://lore.kernel.org/all/[email protected]/

Rebase on top of v7.2-rc1

- patches 4 and 5
Add 'Reviewed-by: Andy Shevchenko'

- Other Patches
No other changes

v6 -> v7

Rebase on top of v7.1-rc1

- Patch 1
Add Cc stable. Needed for patches 11 and 12

- Patches 2 to 5
No changes

- Patch 6
Add Cc stable. Needed for patch 13
Add 'Acked-by: Bjorn Helgaas'

- Patch 7
Fix a typo in commit log
Add Fixes tag + Cc stable
Add 'Acked-by: Bjorn Helgaas'

- Patch 8
Add Fixes tag + Cc stable
Add 'Acked-by: Bjorn Helgaas'
Older iterations:
Patches 6 to 13 in the big picture series
https://lore.kernel.org/all/[email protected]/

Herve Codina (9):
driver core: fw_devlink: Introduce fw_devlink_set_device()
drivers: core: Use fw_devlink_set_device()
pinctrl: cs42l43: Use fw_devlink_set_device()
cxl/test: Use device_set_node()
cxl/test: Use fw_devlink_set_device()
PCI: of: Use fw_devlink_set_device()
PCI: of: Clear fwnode->dev during root bridge node removal
PCI: of: Set fwnode device of newly created PCI device nodes
PCI: of: Remove fwnode_dev_initialized() call for a PCI root bridge
node

 drivers/base/core.c                      |  4 ++--
 drivers/pci/of.c                         | 12 ++++++++++--
 drivers/pinctrl/cirrus/pinctrl-cs42l43.c |  2 +-
 include/linux/fwnode.h                   |  6 ++++++
 tools/testing/cxl/test/cxl.c             |  4 ++--
 5 files changed, 21 insertions(+), 7 deletions(-)

2.54.0

Setting fwnode->dev is specific to fw_devlink.

In order to avoid having a direct 'fwnode->dev = dev;' in several place in the kernel, introduce fw_devlink_set_device() helper to perform this operation.

Having this helper allows to hide the fwnode devlink related stuff behind the helper.

---
Cc stable because used by other patches with Fixes + Cc stable

---
 include/linux/fwnode.h | 6 ++++++
 1 file changed, 6 insertions(+)

index 4e86e6990d28..31538f54b1ca 100644
@@ -256,4 +256,10 @@ void fw_devlink_purge_absent_suppliers(struct fwnode_handle *fwnode);
 void fw_devlink_refresh_fwnode(struct fwnode_handle *fwnode);
 bool fw_devlink_is_strict(void);
 
+static inline void fw_devlink_set_device(struct fwnode_handle *fwnode,
+					 struct device *dev)
+{
+	fwnode->dev = dev;
+}
+
 #endif

2.54.0

The code set directly fwnode->dev field.

Use the dedicated fw_devlink_set_device() helper to perform this operation.

---
 drivers/base/core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 4d026682944f..c205125344cf 100644
@@ -3750,7 +3750,7 @@ int device_add(struct device *dev)
 	 * device and the driver sync_state callback is called for this device.
 	 */
 	if (dev->fwnode && !dev->fwnode->dev) {
-		dev->fwnode->dev = dev;
+		fw_devlink_set_device(dev->fwnode, dev);
 		fw_devlink_link_device(dev);
 	}
 

@@ -3925,7 +3925,7 @@ void device_del(struct device *dev)
 	device_unlock(dev);
 
 	if (dev->fwnode && dev->fwnode->dev == dev)
-		dev->fwnode->dev = NULL;
+		fw_devlink_set_device(dev->fwnode, NULL);
 
 	/* Notify clients of device removal.  This call must come
 	 * before dpm_sysfs_remove().

2.54.0

The code set directly fwnode->dev field.

Use the dedicated fw_devlink_set_device() helper to perform this operation.

---
 drivers/pinctrl/cirrus/pinctrl-cs42l43.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 8990fab0446c..563070aba3d8 100644
@@ -576,7 +576,7 @@ static int cs42l43_pin_probe(struct platform_device *pdev)
 				return ret;
 
 			if (!child->dev)
-				child->dev = priv->dev;
+				fw_devlink_set_device(child, priv->dev);
 			fwnode = child;
 		}
 	}

2.54.0

The code set directly dev->fwnode.

Use the dedicated helper to perform this operation.

---
 tools/testing/cxl/test/cxl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index ef92dd35e030..69da0727362b 100644
@@ -1528,7 +1528,7 @@ static void mock_companion(struct acpi_device *adev, struct device *dev)
 {
 	device_initialize(&adev->dev);
 	fwnode_init(&adev->fwnode, NULL);
-	dev->fwnode = &adev->fwnode;
+	device_set_node(dev, &adev->fwnode);
 	adev->fwnode.dev = dev;
 }
 

2.54.0

The code set directly fwnode.dev field.

Use the dedicated fw_devlink_set_device() helper to perform this operation.

---
 tools/testing/cxl/test/cxl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 69da0727362b..95e39dfc6ec9 100644
@@ -1529,7 +1529,7 @@ static void mock_companion(struct acpi_device *adev, struct device *dev)
 	device_initialize(&adev->dev);
 	fwnode_init(&adev->fwnode, NULL);
 	device_set_node(dev, &adev->fwnode);
-	adev->fwnode.dev = dev;
+	fw_devlink_set_device(&adev->fwnode, dev);
 }
 
 #ifndef SZ_64G

2.54.0

The code set directly fwnode.dev field.

Use the dedicated fw_devlink_set_device() helper to perform this operation.

---
Cc stable because used by other patches with Fixes + Cc stable

---
 drivers/pci/of.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 8b18c4ba845c..ee9eb384b377 100644
@@ -803,7 +803,7 @@ void of_pci_make_host_bridge_node(struct pci_host_bridge *bridge)
 	 * bus. Avoid any new device creation.
 	 */
 	of_node_set_flag(np, OF_POPULATED);
-	np->fwnode.dev = &bridge->dev;
+	fw_devlink_set_device(&np->fwnode, &bridge->dev);
 	fwnode_dev_initialized(&np->fwnode, true);
 
 	ret = of_changeset_apply(cset);

2.54.0

Hi,

This issue is relevant and clearing np->fwnode.dev in the error path will be take into account in the next iteration.

Hervé

During the of_pci_make_host_bridge_node() call, an OF node is created dynamically and its fwnode device (fwnode->dev) is set to the PCI root bridge device using the fw_devlink_set_device(&np->fwnode, &bridge->dev) call.

On removal, of_pci_remove_host_bridge_node() is called and calls device_remove_of_node() which in turn set to NULL the related dev->fwnode.

Later in the removal sequence, device_del() is called and runs its cleanup logic:

    if (dev->fwnode && dev->fwnode->dev == dev)
        fw_devlink_set_device(dev->fwnode, NULL);

Because dev->fwnode has been cleared earlier, fw_devlink_set_device() is not called and leaves fwnode->dev unchanged. This fwnode device (fwnode->dev) becomes an dangling pointer.

If any reference to the OF node is held after this removal, the pointer is still accessible using the OF node (np->fwnode.dev) but points to a freed area.

Avoid this dangling fwnode->dev pointer by clearing it in of_pci_remove_host_bridge_node().

---
 drivers/pci/of.c | 1 +
 1 file changed, 1 insertion(+)

index ee9eb384b377..8dd558a490bb 100644
@@ -743,6 +743,7 @@ void of_pci_remove_host_bridge_node(struct pci_host_bridge *bridge)
 	if (!np || !of_node_check_flag(np, OF_DYNAMIC))
 		return;
 
+	fw_devlink_set_device(&np->fwnode, NULL);
 	device_remove_of_node(&bridge->bus->dev);
 	device_remove_of_node(&bridge->dev);
 	of_changeset_revert(np->data);

2.54.0

This issue is relevant and clearing np->fwnode.dev in the error path will be take into account in the next iteration.

This scenario should not exist but anyway, I will add a new patch in the series in order to have a more robust code checking for a NULL np->data in both of_pci_remove_host_bridge_node() and of_pci_remove_node().

Hervé

Device-tree node can be created when CONFIG_PCI_DYNAMIC_OF_NODES. Those nodes are created and filled based on PCI core information but the fwnode device field is not set.

When later an overlay is applied, this confuses fw_devlink. Indeed, without any device attached to the node, fw_devlink considers that this node will never become a device. When this node is pointed as a supplier, devlink looks at its ancestors in order to find a node with a device that could be used as the supplier.

In the PCI use case, this leads to links that wrongly use the PCI root bridge device as the supplier instead of the expected PCI device.

Setting the fwnode device to the device of the PCI device allows devlink to use this device as a supplier and so, correct links are created.

---
 drivers/pci/of.c | 8 ++++++++
 1 file changed, 8 insertions(+)

index 8dd558a490bb..ca83006b7c7e 100644
@@ -657,6 +657,7 @@ void of_pci_remove_node(struct pci_dev *pdev)
 	if (!np || !of_node_check_flag(np, OF_DYNAMIC))
 		return;
 
+	fw_devlink_set_device(&np->fwnode, NULL);
 	device_remove_of_node(&pdev->dev);
 	of_changeset_revert(np->data);
 	of_changeset_destroy(np->data);

@@ -709,6 +710,13 @@ void of_pci_make_dev_node(struct pci_dev *pdev)
 	if (ret)
 		goto out_free_node;
 
+	/*
+	 * Set the fwnode device in order to have fw_devlink creating links
+	 * pointing to this PCI device instead of walking up to the PCI host
+	 * bridge.
+	 */
+	fw_devlink_set_device(&np->fwnode, &pdev->dev);
+
 	ret = of_changeset_apply(cset);
 	if (ret)
 		goto out_free_node;

2.54.0

During the instantiation of devices described by a device-tree overlay applied on a PCI device, devlink displays the following kind of debug messages instead of creating the expected links:
'Not linking xxxx - might never become dev'

Without those expected links, the device removal order cannot be correct.

Those debug traces are printed by fw_devlink_create_devlink(). In our use case, they are all printed because the supplier of the link has at least one of its ancestor with its fwnode flag FWNODE_FLAG_INITIALIZED set.

The culprit ancestor is the PCI root bridge.

The fwnode related to the PCI root bridge is created dynamically by the of_pci_make_host_bridge_node() function. During this creation fwnode_dev_initialized() is called which set the FWNODE_FLAG_INITIALIZED flag.

Calling fwnode_dev_initialized() tells devlink that the device related to this node is handled out of the driver core. This is not correct in our case. Indeed the device related to this firmware node is handled using driver core mechanisms and is fully compliant devlink expectations.

Simply remove the fwnode_dev_initialized() call. With that done, the devlink debug messages are no more displayed and links that were missing are correctly created.

---
 drivers/pci/of.c | 1 -
 1 file changed, 1 deletion(-)

index ca83006b7c7e..cb2470ca592f 100644
@@ -813,7 +813,6 @@ void of_pci_make_host_bridge_node(struct pci_host_bridge *bridge)
 	 */
 	of_node_set_flag(np, OF_POPULATED);
 	fw_devlink_set_device(&np->fwnode, &bridge->dev);
-	fwnode_dev_initialized(&np->fwnode, true);
 
 	ret = of_changeset_apply(cset);
 	if (ret)

2.54.0