RE: [PATCH net-next v3 3/5] net: lan743x: Add support to software-nodes for SFP

[Thangaraj.S]

Hi Jakub,
Thanks for reviewing the patch.

> -----Original Message-----
> From: Jakub Kicinski <kuba@xxxxxxxxxx>
> Sent: Tuesday, May 12, 2026 7:39 AM
> To: Thangaraj Samynathan - I53494 <Thangaraj.S@xxxxxxxxxxxxx>
> Cc: Jakub Kicinski <kuba@xxxxxxxxxx>; netdev@xxxxxxxxxxxxxxx;
> andrew+netdev@xxxxxxx; davem@xxxxxxxxxxxxx; edumazet@xxxxxxxxxx;
> pabeni@xxxxxxxxxx; Bryan Whitehead - C21958
> <Bryan.Whitehead@xxxxxxxxxxxxx>; UNGLinuxDriver
> <UNGLinuxDriver@xxxxxxxxxxxxx>; linux@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH net-next v3 3/5] net: lan743x: Add support to software-
> nodes for SFP
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> This is an AI-generated review of your patch. The human sending this email
> has considered the AI review valid, or at least plausible.
> ---
> Subject: net: lan743x: Add support to software-nodes for SFP
> 
> This change registers a software-node hierarchy (GPIO/I2C/SFP/phylink) for
> the LAN743x so that SFP cages on PCI11x1x devices can be described to
> phylink/sfp. It wires the lan743x probe to the sibling pci1xxxx I2C and GPIO
> peripheral drivers by walking the PCIe topology and dereferencing their
> drvdata.
> 
> A few questions on the patch below.
> 
> > diff --git a/drivers/net/ethernet/microchip/Kconfig
> > b/drivers/net/ethernet/microchip/Kconfig
> > index ee046468652c..a83db3c7404f 100644
> > --- a/drivers/net/ethernet/microchip/Kconfig
> > +++ b/drivers/net/ethernet/microchip/Kconfig
> > @@ -50,6 +50,8 @@ config LAN743X
> >       select CRC16
> >       select CRC32
> >       select PHYLINK
> > +     select I2C_PCI1XXXX
> > +     select GP_PCI1XXXX
> >       help
> >         Support for the Microchip LAN743x and PCI11x1x families of PCI
> >         Express Ethernet devices
> 
> [High]
> Should LAN743X inherit the dependencies of the symbols it now selects?
> 
> GP_PCI1XXXX in drivers/misc/mchp_pci1xxxx/Kconfig carries:
> 
>     depends on PCI
>     depends on GPIOLIB
>     depends on NVMEM_SYSFS
> 
> and I2C_PCI1XXXX is sourced inside an "if I2C" block in drivers/i2c/Kconfig, so
> it has an implicit depends on I2C. The LAN743X entry here does not depend
> on I2C, GPIOLIB, or NVMEM_SYSFS. On configurations where any of those are
> disabled (randconfig, allmodconfig variants), would this produce unmet-direct-
> dependency warnings from sym_warn_unmet_dep() and potentially fail to link
> because the selected drivers' helpers are not built?
> 
> Would adding "depends on I2C && GPIOLIB && NVMEM_SYSFS" (or gating the
> SFP glue behind a nested Kconfig symbol) be appropriate here?

[Thangaraj Samynathan]  Gating the SFP behind a nested symbol should be the clean
good solution. Will implement the same in next revision.

> 
> > diff --git a/drivers/net/ethernet/microchip/lan743x_main.c
> > b/drivers/net/ethernet/microchip/lan743x_main.c
> > index 867310dbe9ba..b90b35cef2e6 100644
> > --- a/drivers/net/ethernet/microchip/lan743x_main.c
> > +++ b/drivers/net/ethernet/microchip/lan743x_main.c
> > @@ -112,6 +113,91 @@ static void lan743x_pci_cleanup(struct
> > lan743x_adapter *adapter)
> > +static void *pci1xxxx_perif_drvdata_get(struct lan743x_adapter *adapter,
> > +                                     u16 perif_id) {
> > +     struct pci_dev *pdev = adapter->pdev;
> > +     struct pci_bus *perif_bus;
> > +     struct pci_dev *perif_dev;
> > +     struct pci_dev *br_dev;
> > +     struct pci_bus *br_bus;
> > +     struct pci_dev *dev;
> [ ... ]
> > +     br_bus = br_dev->bus;
> > +     list_for_each_entry(dev, &br_bus->devices, bus_list) {
> > +             if (dev->vendor == PCI1XXXX_VENDOR_ID &&
> > +                 (dev->device & ~PCI1XXXX_DEV_MASK) ==
> PCI1XXXX_BR_PERIF_ID) {
> > +                     perif_bus = dev->subordinate;
> > +                     list_for_each_entry(perif_dev, &perif_bus->devices,
> > +                                         bus_list) {
> > +                             if (perif_dev->vendor == PCI1XXXX_VENDOR_ID &&
> > +                                 (perif_dev->device & ~PCI1XXXX_DEV_MASK) ==
> > +                                  perif_id)
> > +                                     return pci_get_drvdata(perif_dev);
> > +                     }
> > +             }
> > +     }
> 
> [High]
> Is it safe to walk br_bus->devices and perif_bus->devices without holding
> pci_bus_sem?
> 
> Both pci_device_add() and pci_destroy_dev() take
> down_write(&pci_bus_sem) when mutating pci_bus->devices, and
> pci_walk_bus() / other in-tree consumers either wrap the iteration in
> down_read(&pci_bus_sem) or assert lockdep_assert_held(&pci_bus_sem). If
> a PCI hotplug or sibling bind/unbind happens concurrently here, could the
> traversal corrupt, loop, or step through a pci_dev that is being freed?
> 
> Relatedly, pci_get_drvdata(perif_dev) is returned directly with no reference
> acquired on the peer device or its driver. Can the following sequence occur?
> 
>     lan743x probe stores adapter->i2c_adap = &i2c_drvdata->adap
>     user unbinds i2c-mchp-pci1xxxx via sysfs
>     i2c_drvdata (and its embedded i2c_adapter) is freed
>     SFP EEPROM access via the software-node fwnode still dereferences it
> 
> Would using pci_get_device() (or pci_dev_get() under pci_bus_sem) plus a
> device_link_add() from the peer device to the lan743x consumer be a more
> robust pairing, or alternatively routing this coupling through the auxiliary bus?
> 
[Thangaraj Samynathan] Agreed. Will be fixed in next revision.
 
> > +static int pci1xxxx_i2c_adapter_get(struct lan743x_adapter *adapter)
> > +{
> > +     struct pci1xxxx_i2c *i2c_drvdata;
> > +
> > +     i2c_drvdata = pci1xxxx_perif_drvdata_get(adapter,
> PCI1XXXX_PERIF_I2C_ID);
> > +     if (!i2c_drvdata)
> > +             return -EPROBE_DEFER;
> > +
> > +     adapter->i2c_adap = &i2c_drvdata->adap;
> 
> [Medium]
> The cast here relies on struct pci1xxxx_i2c being textually redeclared in
> lan743x_main.h, identical to the authoritative definition in
> drivers/i2c/busses/i2c-mchp-pci1xxxx.c. Nothing ties the two copies together
> at build time (no shared header, no BUILD_BUG_ON on sizeof or offsetof), and
> there is no runtime check that perif_dev->driver is the expected pci_driver
> before the drvdata is cast.
> 
> If the i2c driver later reorders or resizes a field, would &i2c_drvdata->adap
> silently read from a wrong offset? And if the peer PCI function is bound to vfio-
> pci or any other driver,
> pci_get_drvdata() returns an unrelated pointer which is then dereferenced as
> struct pci1xxxx_i2c. Would it make sense to add a narrow accessor exported
> by the peripheral driver, e.g. something like pci1xxxx_i2c_get_adapter(pdev),
> so the layout is not duplicated across subsystems?
> 
> The same question applies to the redeclarations of struct
> auxiliary_device_wrapper, struct gp_aux_data_type and struct
> aux_bus_device in lan743x_main.h.

  Thanks for the review comment. This will be fixed in the next revision.

  The redeclared structs will be removed from lan743x_main.h. Instead of
  casting pci_get_drvdata() to private peripheral structs, we will use
  standard kernel subsystem APIs.

  For the I2C adapter, device_find_child() with i2c_verify_adapter() as
  the match function will be used to locate the registered struct i2c_adapter
  without needing struct pci1xxxx_i2c layout knowledge. This pattern is
  already used in drivers/media/pci/mgb4/mgb4_core.c.

  For the GPIO aux device, device_find_child() with a match on
  to_auxiliary_dev(dev)->name == "gp_gpio" will replace the magic [1]
  index, avoiding the need for struct aux_bus_device layout knowledge.
> 
> > +static int pci1xxxx_gpio_dev_get(struct lan743x_adapter *adapter) {
> > +     struct aux_bus_device *aux_bus;
> > +     struct device *gpio_dev;
> > +
> > +     aux_bus = pci1xxxx_perif_drvdata_get(adapter,
> PCI1XXXX_PERIF_GPIO_ID);
> > +     if (!aux_bus)
> > +             return -EPROBE_DEFER;
> > +
> > +     gpio_dev = &aux_bus->aux_device_wrapper[1]->aux_dev.dev;
> 
> [Medium]
> Is the hardcoded index [1] here guaranteed by anything?
> 
> In drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gp.c the wrapper array has
> slot 0 = gp_otp_e2p and slot 1 = gp_gpio today, but that ordering is an internal
> detail of the GP driver and is not enforced by a shared type, a name check, or
> a documented contract. If the GP driver ever reorders the registration, fails
> partway so slot 1 is unpopulated, or adds a new slot, would this dereference
> pick up the wrong aux_dev (or a NULL)?
> 
> Would it be preferable to look up the GPIO aux device by name (e.g.
> matching aux_dev.name == "gp_gpio") or expose a named accessor from the
> GP driver?

Agreed, Will be fixed in next revision
> 
> > @@ -2890,6 +2976,96 @@ static int lan743x_rx_open(struct lan743x_rx
> > *rx)
> > +static int lan743x_swnodes_register(struct lan743x_adapter *adapter)
> > +{
> > +     struct pci_dev *pdev = adapter->pdev;
> > +     struct lan743x_sw_nodes *nodes;
> > +     struct software_node *swnodes;
> > +     int ret;
> > +     u32 id;
> > +
> > +     nodes = kzalloc_obj(*nodes);
> > +     if (!nodes)
> > +             return -ENOMEM;
> > +
> > +     adapter->nodes = nodes;
> > +
> > +     ret = pci1xxxx_gpio_dev_get(adapter);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     ret = pci1xxxx_i2c_adapter_get(adapter);
> > +     if (ret < 0)
> > +             return ret;
> 
> [High]
> Does this leak the lan743x_sw_nodes allocation on error?
> 
> adapter->nodes is set unconditionally before the two peripheral lookups,
> both of which commonly return -EPROBE_DEFER while the sibling pci1xxxx
> drivers are still probing. On that error, lan743x_swnodes_register() returns
> without freeing nodes and without clearing adapter->nodes.
> 
> The caller in lan743x_phylink_create() handles the failure as:
> 
>     ret = lan743x_swnodes_register(adapter);
>     if (ret) {
>         netdev_err(netdev, "failed to register software nodes\n");
>         return ret;
>     }
> 
> without calling lan743x_swnodes_unregister(). The probe error path
> (lan743x_pcidev_probe) then jumps to cleanup_mdiobus, which does not
> reach lan743x_destroy_phylink() (the only caller of
> lan743x_swnodes_unregister). Each -EPROBE_DEFER retry would allocate a
> fresh struct and discard the previous one.
> 
> The same concern applies to the software_node_register_node_group()
> return at the end of this function: its failure is returned to a caller that does
> not call lan743x_swnodes_unregister().
> 
> Would cleaning up in lan743x_swnodes_register() itself on every error path
> (kfree(nodes); adapter->nodes = NULL) be appropriate?

Agreed. Will be fixed in next revision.
> 
> > +     nodes->gpio_props[0] = PROPERTY_ENTRY_STRING("pinctrl-names",
> "default");
> > +     swnodes[SWNODE_GPIO] = NODE_PROP(nodes->gpio_name,
> > + nodes->gpio_props);
> [ ... ]
> > +     nodes->i2c_props[0] = PROPERTY_ENTRY_STRING("pinctrl-names",
> "default");
> > +     swnodes[SWNODE_I2C] = NODE_PROP(nodes->i2c_name,
> > + nodes->i2c_props);
> [ ... ]
> > +     nodes->phylink_props[0] = PROPERTY_ENTRY_STRING("managed",
> > +                                                     "in-band-status");
> > +     nodes->phylink_props[1] = PROPERTY_ENTRY_REF_ARRAY("sfp",
> > +                                                        nodes->sfp_ref);
> > +     swnodes[SWNODE_PHYLINK] = NODE_PROP(nodes->phylink_name,
> > +                                         nodes->phylink_props);
> 
> [High]
> Do the property_entry arrays need a trailing NULL sentinel?
> 
> The struct definition in lan743x_main.h sizes them to the exact number of
> used entries:
> 
>     struct property_entry gpio_props[1];
>     struct property_entry i2c_props[1];
>     struct property_entry sfp_props[8];    /* 7 used + terminator */
>     struct property_entry phylink_props[2];
> 
> The swnode code in drivers/base/swnode.c iterates with
> 
>     for (; prop->name; prop++)
> 
> in property_entry_get() and property_entries_dup(), which relies on a zeroed
> terminator. Only sfp_props[8] includes such a slot.
> 
> Because gpio_props, i2c_props, sfp_props and phylink_props are laid out
> contiguously in struct lan743x_sw_nodes, when iteration walks past
> gpio_props[0] or i2c_props[0] it would land on the next array's entries (for
> example i2c_props[0].name = "pinctrl-names", then sfp_props entries
> "compatible"/"sff,sfp", "i2c-bus", ...), only terminating at sfp_props[7]. The
> GPIO and I2C software nodes could silently acquire SFP properties.
> 
> For phylink_props[2], iteration would continue into i2c_ref[0], whose first field
> swnode is a non-NULL pointer; would that pointer then be interpreted as
> prop->name and passed to strcmp() during lookups for names other than
> "managed"/"sfp", producing an out-of-bounds read of kernel memory?
> 
> Would sizing each array with +1 for a zero terminator address this?

Agreed. Will be fixed in next revision.

> 
> > @@ -3134,7 +3310,9 @@ static const struct phylink_mac_ops
> > lan743x_phylink_mac_ops = {  static int lan743x_phylink_create(struct
> > lan743x_adapter *adapter)  {
> >       struct net_device *netdev = adapter->netdev;
> > +     struct fwnode_handle *fwnode = NULL;
> >       struct phylink *pl;
> > +     int ret;
> [ ... ]
> > +     if (adapter->is_sfp_support_en) {
> > +             ret = lan743x_swnodes_register(adapter);
> > +             if (ret) {
> > +                     netdev_err(netdev, "failed to register software nodes\n");
> > +                     return ret;
> > +             }
> > +             fwnode = software_node_fwnode(adapter->nodes-
> >group[SWNODE_PHYLINK]);
> > +             if (!fwnode) {
> > +                     lan743x_swnodes_unregister(adapter);
> > +                     return -ENODEV;
> > +             }
> > +     }
> >
> > +     pl = phylink_create(&adapter->phylink_config, fwnode,
> > +                         adapter->phy_interface,
> > + &lan743x_phylink_mac_ops);
> >       if (IS_ERR(pl)) {
> >               netdev_err(netdev, "Could not create phylink (%pe)\n",
> > pl);
> > +             lan743x_swnodes_unregister(adapter);
> >               return PTR_ERR(pl);
> >       }
> 
> Thanks for reading.
Thanks,
Thangaraj Samynathan