RE: [PATCH net-next v3 4/5] net: lan743x: Register SFP platform device for PCI11x1x

[Thangaraj.S]

Hi Jakub,
Thanks for the comments.

> -----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 4/5] net: lan743x: Register SFP platform
> device for PCI11x1x
> 
> 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.
> ---
> net: lan743x: Register SFP platform device for PCI11x1x
> 
> This commit registers an SFP platform device for PCI11x1x when SFP support is
> enabled, using the SFP software node as its firmware node and associating the
> I2C adapter with its own software node for module management.
> 
> > diff --git a/drivers/net/ethernet/microchip/Kconfig
> > b/drivers/net/ethernet/microchip/Kconfig
> > index a83db3c7404f..9857053dabf8 100644
> > --- a/drivers/net/ethernet/microchip/Kconfig
> > +++ b/drivers/net/ethernet/microchip/Kconfig
> > @@ -52,6 +52,7 @@ config LAN743X
> >       select PHYLINK
> >       select I2C_PCI1XXXX
> >       select GP_PCI1XXXX
> > +     select SFP
> 
> [Medium]
> Does this select need to inherit SFP's own dependencies?  In drivers/net/pse-
> pd/Kconfig the SFP symbol is declared as:
> 
>     config SFP
>         tristate "SFP cage support"
>         depends on NETDEVICES && PHYLIB
>         depends on HWMON || HWMON=n
>         ...
> 
> With HWMON=m and LAN743X=y, select SFP forces SFP=y, which violates the
> HWMON || HWMON=n constraint and produces an unmet direct
> dependencies warning.
> 
> For comparison, drivers/net/ethernet/wangxun/Kconfig uses:
> 
>     select HWMON if TXGBE=y
>     select SFP
> 
> Would adding either depends on HWMON || HWMON=n to LAN743X, or a
> conditional HWMON select, be preferable here?

[Thangaraj Samynathan]  Agreed. Will Fix in next revision.

> 
> >       help
> >         Support for the Microchip LAN743x and PCI11x1x families of PCI
> >         Express Ethernet devices
> > diff --git a/drivers/net/ethernet/microchip/lan743x_main.c
> > b/drivers/net/ethernet/microchip/lan743x_main.c
> > index b90b35cef2e6..88a2d11552f8 100644
> > --- a/drivers/net/ethernet/microchip/lan743x_main.c
> > +++ b/drivers/net/ethernet/microchip/lan743x_main.c
> > @@ -3066,6 +3066,31 @@ static int lan743x_swnodes_register(struct
> lan743x_adapter *adapter)
> >       return software_node_register_node_group(nodes->group);
> >  }
> >
> > +static int lan743x_sfp_register(struct lan743x_adapter *adapter) {
> > +     struct pci_dev *pdev = adapter->pdev;
> > +     struct platform_device_info sfp_info;
> > +     struct platform_device *sfp_dev;
> > +
> > +     memset(&sfp_info, 0, sizeof(sfp_info));
> > +     sfp_info.parent = &adapter->pdev->dev;
> > +     sfp_info.fwnode = software_node_fwnode(adapter->nodes-
> >group[SWNODE_SFP]);
> > +     sfp_info.name = "sfp";
> > +     sfp_info.id = (pdev->bus->number << 8) | pdev->devfn;
> 
> [Medium]
> Should the platform device id incorporate pci_domain_nr(pdev->bus)?
> PCI bus numbers are only unique within a PCI domain, so on a multi-domain
> system two PCI11x1x devices in different domains that happen to share the
> same bus/devfn would both try to register as sfp.<same_id>.
> 
> The second platform_device_add() would then fail with -EEXIST from
> device_add() and SFP would not come up on that device.  Something
> like:
> 
>     sfp_info.id = (pci_domain_nr(pdev->bus) << 16) |
>                   (pdev->bus->number << 8) | pdev->devfn;
> 
> would avoid that collision.

[Thangaraj Samynathan] Agreed. Will be fixed in next revision.

> 
> > +     sfp_dev = platform_device_register_full(&sfp_info);
> > +     if (IS_ERR(sfp_dev)) {
> > +             netif_err(adapter, drv, adapter->netdev,
> > +                       "Failed to register SFP device\n");
> > +             return PTR_ERR(sfp_dev);
> > +     }
> > +
> > +     adapter->sfp_dev = sfp_dev;
> > +     netif_dbg(adapter, drv, adapter->netdev,
> > +               "SFP platform device registered");
> > +
> > +     return 0;
> > +}
> > +
> 
> [ ... ]
> 
> > @@ -3682,6 +3707,12 @@ static void lan743x_destroy_phylink(struct
> > lan743x_adapter *adapter)  static void lan743x_full_cleanup(struct
> > lan743x_adapter *adapter)  {
> >       unregister_netdev(adapter->netdev);
> > +     if (adapter->sfp_dev) {
> > +             platform_device_unregister(adapter->sfp_dev);
> > +             adapter->sfp_dev = NULL;
> > +     }
> > +     if (adapter->i2c_adap)
> > +             adapter->i2c_adap = NULL;
> 
> [High]
> Can this leave a dangling fwnode on the i2c_adap device?  The i2c_adap is
> fetched from a different PCI function (the pci1xxxx_i2c driver via
> pci1xxxx_i2c_adapter_get()) and outlives the lan743x instance.
> 
> In lan743x_pcidev_probe() the code does:
> 
>     adapter->i2c_adap->dev.fwnode =
>         software_node_fwnode(adapter->nodes->group[SWNODE_I2C]);
> 
> But on teardown only the local pointer is cleared:
> 
>     if (adapter->i2c_adap)
>         adapter->i2c_adap = NULL;
> 
> Meanwhile lan743x_destroy_phylink() calls
> lan743x_swnodes_unregister() which invokes
> software_node_unregister_node_group() and frees adapter->nodes.
> After that, i2c_adap->dev.fwnode still references the released swnode-
> backed fwnode.
> 
> Any subsequent access to i2c_adap->dev.fwnode (driver unload/rebind of
> pci1xxxx_i2c, sysfs reads, fwnode iteration, or fwnode ops
> dispatch) would then dereference freed memory.
> 
> Should the teardown also restore i2c_adap->dev.fwnode to what it was
> before the probe-side assignment, symmetric to the mutation done in
> lan743x_pcidev_probe()?
> 

[Thangaraj Samynathan] Agreed. Will be fixed in next revision. 

> >
> >       lan743x_destroy_phylink(adapter);
> >       lan743x_mdiobus_cleanup(adapter); @@ -3909,11 +3940,29 @@ static
> > int lan743x_pcidev_probe(struct pci_dev *pdev,
> >               goto cleanup_mdiobus;
> >       }
> >
> > +     if (adapter->is_sfp_support_en) {
> > +             adapter->i2c_adap->dev.fwnode =
> > +
> > + software_node_fwnode(adapter->nodes->group[SWNODE_I2C]);
> > +
> > +             ret = lan743x_sfp_register(adapter);
> > +             if (ret < 0) {
> > +                     netif_err(adapter, probe, netdev,
> > +                               "failed to sfp register (%d)\n", ret);
> > +                     goto cleanup_phylink;
> > +             }
> > +     }
> > +
> >       ret = register_netdev(adapter->netdev);
> >       if (ret < 0)
> > -             goto cleanup_phylink;
> > +             goto cleanup_sfp;
> >       return 0;
> >
> > +cleanup_sfp:
> > +     if (adapter->sfp_dev) {
> > +             platform_device_unregister(adapter->sfp_dev);
> > +             adapter->sfp_dev = NULL;
> > +     }
> > +
> >  cleanup_phylink:
> >       lan743x_destroy_phylink(adapter);
> >
Regards,
Thangaraj Samynathan