lan966x pci device: Add support for SFPs, core part
Subsystems (8)
v7 cover lan966x pci device: Add support for SFPs, core 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 driver core subsystem. It fixes devlink issues when a device-tree overlay is applied and avoid a warning when a device is removed.
It has to be seen as a continuation of the big picture series but related to this specific core part.
Patches 1 and 2 fixes fw_devlink when it is used with overlay. Those patches were previously sent by Saravana [1].
I rebased them on top of v7.1-rc1 and I added a call to driver_deferred_probe_trigger() in Saravana's patch (patch 2) to ensure that probes are retried after the modification performed on the dangling consumers. This allows to fix issues reported by Matti and Geert [2] with the previous iteration patches.
Patch 3 avoids a warning on device removal.
Those 3 patches seem ready to land and so having them extracted in this current series will help having them applied.
[0] https://lore.kernel.org/all/[email protected]/
[1] https://lore.kernel.org/lkml/[email protected]/
[2] https://lore.kernel.org/all/[email protected]/
Best regards,
Hervé
Changes:
v6 -> v7
Rebase on top v7.1-rc1
- Patch 1
Fix conflict due to commit f72e77c33e4b ("device property: Make modifications of fwnode "flags" thread safe")
Add 'Acked-by: Rob Herring'
- Patch 2
Use fwnode_test_flag() to test fwnode flags value Add 'Acked-by: Rob Herring'
- Patche 3
No changes
Older iterations:
Patches 1 to 3 in the big picture series
https://lore.kernel.org/all/[email protected]/
Herve Codina (1):
driver core: Avoid warning when removing a device while its supplier
is unbinding
Saravana Kannan (2):
Revert "treewide: Fix probing of devices in DT overlays"
of: dynamic: Fix overlayed devices not probing because of fw_devlink
drivers/base/core.c | 86 ++++++++++++++++++++++++++++++++++----- drivers/bus/imx-weim.c | 6 --- drivers/i2c/i2c-core-of.c | 5 --- drivers/of/dynamic.c | 1 - drivers/of/overlay.c | 15 +++++++ drivers/of/platform.c | 5 --- drivers/spi/spi.c | 5 --- include/linux/fwnode.h | 1 + 8 files changed, 92 insertions(+), 32 deletions(-)
Signature · 1 line
2.54.0
PATCH v7 1/3 Revert "treewide: Fix probing of devices in DT overlays"
From: Saravana Kannan <[email protected]>
This reverts commit 1a50d9403fb90cbe4dea0ec9fd0351d2ecbd8924.
While the commit fixed fw_devlink overlay handling for one case, it broke it for another case. So revert it and redo the fix in a separate patch.
[Herve: Fix conflicts due to f72e77c33e4b ("device property: Make modifications of fwnode "flags" thread safe")]
--- drivers/bus/imx-weim.c | 6 ------ drivers/i2c/i2c-core-of.c | 5 ----- drivers/of/dynamic.c | 1 - drivers/of/platform.c | 5 ----- drivers/spi/spi.c | 5 ----- 5 files changed, 22 deletions(-)
drivers/bus/imx-weim.c +0 -6
@@ -327,12 +327,6 @@ static int of_weim_notify(struct notifier_block *nb, unsigned long action, "Failed to setup timing for '%pOF'\n", rd->dn);if (!of_node_check_flag (rd->dn,OF_POPULATED )) { -/* -* Clear the flag before adding the device so that -* fw_devlink doesn't skip adding consumers to this -* device. -*/ -fwnode_clear_flag (&rd->dn->fwnode,FWNODE_FLAG_NOT_DEVICE );if (!of_platform_device_create (rd->dn,NULL , &pdev->dev)) { dev_err(&pdev->dev, "Failed to create child device '%pOF'\n",
drivers/i2c/i2c-core-of.c +0 -5
@@ -176,11 +176,6 @@ static int of_i2c_notify(struct notifier_block *nb, unsigned long action,return NOTIFY_OK ; } -/* -* Clear the flag before adding the device so that fw_devlink -* doesn't skip adding consumers to this device. -*/ -fwnode_clear_flag (&rd->dn->fwnode,FWNODE_FLAG_NOT_DEVICE ); client =of_i2c_register_device (adap, rd->dn);if (IS_ERR (client)) { dev_err(&adap->dev, "failed to create client for '%pOF'\n",
drivers/of/dynamic.c +0 -1
@@ -225,7 +225,6 @@ static void __of_attach_node(struct device_node *np) np->sibling = np->parent->child; np->parent->child = np;of_node_clear_flag (np,OF_DETACHED ); -fwnode_set_flag (&np->fwnode,FWNODE_FLAG_NOT_DEVICE );raw_spin_unlock_irqrestore (&devtree_lock, flags);
drivers/of/platform.c +0 -5
@@ -744,11 +744,6 @@ static int of_platform_notify(struct notifier_block *nb,if (of_node_check_flag (rd->dn,OF_POPULATED ))return NOTIFY_OK ; -/* -* Clear the flag before adding the device so that fw_devlink -* doesn't skip adding consumers to this device. -*/ -fwnode_clear_flag (&rd->dn->fwnode,FWNODE_FLAG_NOT_DEVICE );/* pdev_parent may be NULL when no bus platform device */ pdev_parent =of_find_device_by_node (parent); pdev = of_platform_device_create(rd->dn, NULL,
drivers/spi/spi.c +0 -5
@@ -5003,11 +5003,6 @@ static int of_spi_notify(struct notifier_block *nb, unsigned long action,return NOTIFY_OK ; } -/* -* Clear the flag before adding the device so that fw_devlink -* doesn't skip adding consumers to this device. -*/ -fwnode_clear_flag (&rd->dn->fwnode,FWNODE_FLAG_NOT_DEVICE ); spi =of_register_spi_device (ctlr, rd->dn);put_device (&ctlr->dev);
Signature · 1 line
2.54.0
Re: [PATCH v7 1/3] Revert "treewide: Fix probing of devices in DT overlays"
Quoted 16 lines · Herve Codina
From: Saravana Kannan <[email protected]>
This reverts commit
1a50d9403fb90cbe4dea0ec9fd0351d2ecbd8924.While the commit fixed
fw_devlinkoverlay handling for one case, it broke it for another case. So revert it and redo the fix in a separate patch.Fixes 1a50d9403fb9 ("treewide: Fix probing of devices in DT overlays")Reported-by Herve Codina <[email protected]>Closes https://lore.kernel.org/lkml/CAMuHMdXEnSD4rRJ-o90x4OprUacN_rJgyo8x6=9F9rZ+-KzjOg@mail.gmail.com/ open hereSigned-off-by Saravana Kannan <[email protected]>
Quoted 6 lines
[Herve: Fix conflicts due to
f72e77c33e4b("device property: Make modifications of fwnode "flags" thread safe")]Signed-off-by Herve Codina <[email protected]>Acked-by Mark Brown <[email protected]>
PATCH v7 2/3 of: dynamic: Fix overlayed devices not probing because of fw_devlink
In reply to Herve CodinaFrom: Saravana Kannan <[email protected]>
When an overlay is applied, if the target device has already probed successfully and bound to a device, then some of the fw_devlink logic that ran when the device was probed needs to be rerun. This allows newly created dangling consumers of the overlayed device tree nodes to be moved to become consumers of the target device.
--- drivers/base/core.c | 83 +++++++++++++++++++++++++++++++++++++----- drivers/of/overlay.c | 15 ++++++++ include/linux/fwnode.h | 1 + 3 files changed, 90 insertions(+), 9 deletions(-)
drivers/base/core.c +74 -9
@@ -235,6 +235,79 @@ static void __fw_devlink_pickup_dangling_consumers(struct fwnode_handle *fwnode,__fw_devlink_pickup_dangling_consumers (child, new_sup); } +static void fw_devlink_pickup_dangling_consumers (struct device *dev) +{ +struct fwnode_handle *child; + +guard (mutex)(&fwnode_link_lock); + +fwnode_for_each_available_child_node (dev->fwnode, child) +__fw_devlink_pickup_dangling_consumers (child, dev->fwnode); +__fw_devlink_link_to_consumers (dev); +} + +/** +* fw_devlink_refresh_fwnode - Recheck the tree under this firmware node +* @fwnode: The fwnode under which the fwnode tree has changed +* +* This function is mainly meant to adjust the supplier/consumer dependencies +* after a fwnode tree overlay has occurred. +*/ +void fw_devlink_refresh_fwnode (struct fwnode_handle *fwnode) +{ +struct device *dev; + +/* +* Find the closest ancestor fwnode that has been converted to a device +* that can bind to a driver (bus device). +*/ +fwnode_handle_get (fwnode); +do { +if (fwnode_test_flag (fwnode,FWNODE_FLAG_NOT_DEVICE )) +continue ; + + dev =get_dev_from_fwnode (fwnode); +if (!dev) +continue ; + +if (dev->bus) +break ; + +put_device (dev); + }while ((fwnode =fwnode_get_next_parent (fwnode))); + +/* +* If none of the ancestor fwnodes have (yet) been converted to a device +* that can bind to a driver, there's nothing to fix up. +*/ +if (!fwnode) +return ; + +WARN (device_is_bound (dev) && dev->links.status !=DL_DEV_DRIVER_BOUND , +"Don't multithread overlaying and probing the same device!\n" ); + +/* +* If the device has already bound to a driver, then we need to redo +* some of the work that was done after the device was bound to a +* driver. If the device hasn't bound to a driver, running things too +* soon would incorrectly pick up consumers that it shouldn't. +*/ +if (dev->links.status ==DL_DEV_DRIVER_BOUND ) { +fw_devlink_pickup_dangling_consumers (dev); +/* +* Some of dangling consumers could have been put previously in +* the deferred probe list due to the unavailability of their +* suppliers. Those consumers have been picked up and some of +* their suppliers links have been updated. Time to re-try their +* probe sequence. +*/ +driver_deferred_probe_trigger (); + } + +put_device (dev); +fwnode_handle_put (fwnode); +} +static DEFINE_MUTEX (device_links_lock);DEFINE_STATIC_SRCU (device_links_srcu);
@@ -1312,16 +1385,8 @@ void device_links_driver_bound(struct device *dev)* child firmware node. */ if (dev->fwnode && dev->fwnode->dev == dev) { -struct fwnode_handle *child; -fwnode_links_purge_suppliers (dev->fwnode); - -guard (mutex)(&fwnode_link_lock); - -fwnode_for_each_available_child_node (dev->fwnode, child) -__fw_devlink_pickup_dangling_consumers (child, - dev->fwnode); -__fw_devlink_link_to_consumers (dev); +fw_devlink_pickup_dangling_consumers (dev); }device_remove_file (dev, &dev_attr_waiting_for_supplier);
drivers/of/overlay.c +15 -0
@@ -185,6 +185,15 @@ static int overlay_notify(struct overlay_changeset *ovcs,return 0 ; } +static void overlay_fw_devlink_refresh (struct overlay_changeset *ovcs) +{ +for (int i =0 ; i < ovcs->count; i++) { +struct device_node *np = ovcs->fragments[i].target; + +fw_devlink_refresh_fwnode (of_fwnode_handle (np)); + } +} +/* * The values of properties in the "/__symbols__" node are paths in * the ovcs->overlay_root. When duplicating the properties, the paths
@@ -951,6 +960,12 @@ static int of_overlay_apply(struct overlay_changeset *ovcs,pr_err ("overlay apply changeset entry notify error %d\n" , ret);/* notify failure is not fatal, continue */ +/* +* Needs to happen after changeset notify to give the listeners a chance +* to finish creating all the devices they need to create. +*/ +overlay_fw_devlink_refresh (ovcs); + ret_tmp =overlay_notify (ovcs,OF_OVERLAY_POST_APPLY );if (ret_tmp)if (!ret)
include/linux/fwnode.h +1 -0
@@ -250,6 +250,7 @@ int fwnode_link_add(struct fwnode_handle *con, struct fwnode_handle *sup, u8 flags);void fwnode_links_purge (struct fwnode_handle *fwnode);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 );#endif
Signature · 1 line
2.54.0
[email protected]
AI reviewRe: [PATCH v7 2/3] of: dynamic: Fix overlayed devices not probing because of fw_devlink
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] Missing `dev->fwnode` validation in `fw_devlink_pickup_dangling_consumers()` leads to a kernel…
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Missing dev->fwnode validation in fw_devlink_pickup_dangling_consumers() leads to a kernel panic if the device's fwnode is NULL. --
commit eebf7e6f50a0aa0439d5a1099141444a70ec8219
Author: Saravana Kannan <[email protected]>
of: dynamic: Fix overlayed devices not probing because of fw_devlink
When an overlay is applied to an already probed device, fw_devlink logic is rerun. This allows newly created dangling consumers of overlayed device tree nodes to become consumers of the target device.
Quoted 8 lines
drivers/base/core.c +0
-0@@ -235,6 +235,79 @@ static void __fw_devlink_pickup_dangling_consumers(struct fwnode_handle *fwnode,__fw_devlink_pickup_dangling_consumers (child, new_sup); }
drivers/base/core.c static void __fw_devlink_pickup_dangling_consumers(struct fwnode_handle *fwnode, · line 247
Quoted 10 lines
drivers/base/core.c +10
-0+static void fw_devlink_pickup_dangling_consumers (struct device *dev) +{ +struct fwnode_handle *child; + +guard (mutex)(&fwnode_link_lock); + +fwnode_for_each_available_child_node (dev->fwnode, child) +__fw_devlink_pickup_dangling_consumers (child, dev->fwnode); +__fw_devlink_link_to_consumers (dev); +}
Is it possible for dev->fwnode to be NULL here?
The original logic in device_links_driver_bound() was protected by a dev->fwnode check. The newly added caller fw_devlink_refresh_fwnode() appears to call this without checking if dev->fwnode is still valid:
fw_devlink_refresh_fwnode()
dev = get_dev_from_fwnode(fwnode);
...
if (dev->links.status == DL_DEV_DRIVER_BOUND) {
fw_devlink_pickup_dangling_consumers(dev);If dev->fwnode is NULL, such as when it is cleared by a concurrent device_remove_of_node() before device_del() runs, the nested call to __fw_devlink_link_to_consumers() will dereference it:
__fw_devlink_link_to_consumers(struct device *dev)
{
struct fwnode_handle *fwnode = dev->fwnode;
...
list_for_each_entry_safe(link, tmp, &fwnode->consumers, s_hook) {Could this lead to a crash when &fwnode->consumers is evaluated? Would it make sense to add an explicit check for !dev->fwnode at the start of fw_devlink_pickup_dangling_consumers() to mirror the structural safety previously provided in device_links_driver_bound()?
[ ... ]
Quoted 18 lines
drivers/base/core.c +1
-9@@ -1312,16 +1385,8 @@ void device_links_driver_bound(struct device *dev)* child firmware node. */ if (dev->fwnode && dev->fwnode->dev == dev) { -struct fwnode_handle *child; -fwnode_links_purge_suppliers (dev->fwnode); - -guard (mutex)(&fwnode_link_lock); - -fwnode_for_each_available_child_node (dev->fwnode, child) -__fw_devlink_pickup_dangling_consumers (child, - dev->fwnode); -__fw_devlink_link_to_consumers (dev); +fw_devlink_pickup_dangling_consumers (dev); }device_remove_file (dev, &dev_attr_waiting_for_supplier);
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
Re: [PATCH v7 2/3] of: dynamic: Fix overlayed devices not probing because of fw_devlink
Hi all,
Quoted 23 lines · On Tue, 12 May 2026 21:16:43 +0000 [email protected] wrote:
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Missingdev->fwnodevalidation infw_devlink_pickup_dangling_consumers()leads to a kernel panic if the device's fwnode is NULL. --commit
eebf7e6f50a0aa0439d5a1099141444a70ec8219
Author: Saravana Kannan <[email protected]>of: dynamic: Fix overlayed devices not probing because of
fw_devlinkWhen an overlay is applied to an already probed device,
fw_devlinklogic is rerun. This allows newly created dangling consumers of overlayed device tree nodes to become consumers of the target device.Quoted 8 lines
drivers/base/core.c +0
-0@@ -235,6 +235,79 @@ static void __fw_devlink_pickup_dangling_consumers(struct fwnode_handle *fwnode,__fw_devlink_pickup_dangling_consumers (child, new_sup); }
Quoted 12 lines
drivers/base/core.c static void __fw_devlink_pickup_dangling_consumers(struct fwnode_handle *fwnode, · line 247
Quoted 10 lines
drivers/base/core.c +10
-0+static void fw_devlink_pickup_dangling_consumers (struct device *dev) +{ +struct fwnode_handle *child; + +guard (mutex)(&fwnode_link_lock); + +fwnode_for_each_available_child_node (dev->fwnode, child) +__fw_devlink_pickup_dangling_consumers (child, dev->fwnode); +__fw_devlink_link_to_consumers (dev); +}Is it possible for
dev->fwnodeto be NULL here?
Well dev->fwnode is checked for NULL in fwnode_for_each_available_child_node() iterator.
https://elixir.bootlin.com/linux/v7.1-rc3/source/include/linux/property.h#L175
https://elixir.bootlin.com/linux/v7.1-rc3/source/drivers/base/property.c#L840
Adding an additional check for dev->fwnode is not needed.
Best regards,
Hervé
Quoted 47 lines
The original logic in
device_links_driver_bound()was protected by adev->fwnodecheck. The newly added callerfw_devlink_refresh_fwnode()appears to call this without checking ifdev->fwnodeis still valid:
fw_devlink_refresh_fwnode()
dev =get_dev_from_fwnode(fwnode);
...if (dev->links.status == DL_DEV_DRIVER_BOUND) { fw_devlink_pickup_dangling_consumers(dev);If
dev->fwnodeis NULL, such as when it is cleared by a concurrentdevice_remove_of_node()beforedevice_del()runs, the nested call to__fw_devlink_link_to_consumers()will dereference it:
__fw_devlink_link_to_consumers(struct device *dev){ struct fwnode_handle *fwnode = dev->fwnode; ... list_for_each_entry_safe(link, tmp, &fwnode->consumers, s_hook) {Could this lead to a crash when
&fwnode->consumersis evaluated? Would it make sense to add an explicit check for !dev->fwnode at the start offw_devlink_pickup_dangling_consumers()to mirror the structural safety previously provided indevice_links_driver_bound()?[ ... ]
Quoted 18 lines
drivers/base/core.c +1
-9@@ -1312,16 +1385,8 @@ void device_links_driver_bound(struct device *dev)* child firmware node. */ if (dev->fwnode && dev->fwnode->dev == dev) { -struct fwnode_handle *child; -fwnode_links_purge_suppliers (dev->fwnode); - -guard (mutex)(&fwnode_link_lock); - -fwnode_for_each_available_child_node (dev->fwnode, child) -__fw_devlink_pickup_dangling_consumers (child, - dev->fwnode); -__fw_devlink_link_to_consumers (dev); +fw_devlink_pickup_dangling_consumers (dev); }device_remove_file (dev, &dev_attr_waiting_for_supplier);
Signature · 3 lines
Hervé Codina, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
PATCH v7 3/3 driver core: Avoid warning when removing a device while its supplier is unbinding
In reply to Herve CodinaDuring driver removal, the following warning can appear:
WARNING: CPU: 1 PID: 139 at drivers/base/core.c:1497 __device_links_no_driver+0xcc/0xfc
...
Call trace:
__device_links_no_driver+0xcc/0xfc (P)
device_links_driver_cleanup+0xa8/0xf0
device_release_driver_internal+0x208/0x23c
device_links_unbind_consumers+0xe0/0x108
device_release_driver_internal+0xec/0x23c
device_links_unbind_consumers+0xe0/0x108
device_release_driver_internal+0xec/0x23c
device_links_unbind_consumers+0xe0/0x108
device_release_driver_internal+0xec/0x23c
driver_detach+0xa0/0x12c
bus_remove_driver+0x6c/0xbc
driver_unregister+0x30/0x60
pci_unregister_driver+0x20/0x9c
lan966x_pci_driver_exit+0x18/0xa90 [lan966x_pci]This warning is triggered when a consumer is removed because the links status of its supplier is not DL_DEV_DRIVER_BOUND and the link flag DL_FLAG_SYNC_STATE_ONLY is not set.
The topology in terms of consumers/suppliers used was the following (consumer ---> supplier):
i2c -----------> OIC ----> PCI device
| ^
| |
+---> pinctrl ---+When the PCI device is removed, the OIC (interrupt controller) has to be removed. In order to remove the OIC, pinctrl and i2c need to be removed and to remove pinctrl, i2c need to be removed. The removal order is:
1) i2c
2) pinctrl
3) OIC
4) PCI device
In details, the removal sequence is the following (with 0000:01:00.0 the PCI device):
driver_detach: call device_release_driver_internal(0000:01:00.0)...
device_links_busy(0000:01:00.0):
links->status = DL_DEV_UNBINDING
device_links_unbind_consumers(0000:01:00.0):
0000:01:00.0--oic link->status = DL_STATE_SUPPLIER_UNBIND
call device_release_driver_internal(oic)...
device_links_busy(oic):
links->status = DL_DEV_UNBINDING
device_links_unbind_consumers(oic):
oic--pinctrl link->status = DL_STATE_SUPPLIER_UNBIND
call device_release_driver_internal(pinctrl)...
device_links_busy(pinctrl):
links->status = DL_DEV_UNBINDING
device_links_unbind_consumers(pinctrl):
pinctrl--i2c link->status = DL_STATE_SUPPLIER_UNBIND
call device_release_driver_internal(i2c)...
device_links_busy(i2c): links->status = DL_DEV_UNBINDING
__device_links_no_driver(i2c)...
pinctrl--i2c link->status is DL_STATE_SUPPLIER_UNBIND
oic--i2c link->status is DL_STATE_ACTIVE
oic--i2c link->supplier->links.status is DL_DEV_UNBINDINGThe warning is triggered by the i2c removal because the OIC (supplier) links status is not DL_DEV_DRIVER_BOUND. Its links status is indeed set to DL_DEV_UNBINDING.
It is perfectly legit to have the links status set to DL_DEV_UNBINDING in that case. Indeed we had started to unbind the OIC which triggered the consumer unbinding and didn't finish yet when the i2c is unbound.
Avoid the warning when the supplier links status is set to DL_DEV_UNBINDING and thus support this removal sequence without any warnings.
--- drivers/base/core.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/base/core.c +2 -1
@@ -1500,7 +1500,8 @@ static void __device_links_no_driver(struct device *dev)if (link->supplier->links.status ==DL_DEV_DRIVER_BOUND ) {WRITE_ONCE (link->status,DL_STATE_AVAILABLE ); }else { -WARN_ON (!device_link_test (link,DL_FLAG_SYNC_STATE_ONLY )); +WARN_ON (link->supplier->links.status !=DL_DEV_UNBINDING && + !device_link_test (link,DL_FLAG_SYNC_STATE_ONLY ));WRITE_ONCE (link->status,DL_STATE_DORMANT ); } }
Signature · 1 line
2.54.0