RE: [PATCH net-next v3 5/5] net: lan743x: Add PCS/XPCS support for SFP on PCI11x1x

[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 5/5] net: lan743x: Add PCS/XPCS support for
> SFP on 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: Add PCS/XPCS support for SFP on PCI11x1x
> 
> Adds a dedicated PCS mdiobus and an XPCS instance driven through the
> existing SGMII register access helpers, integrates the xpcs with phylink via
> mac_select_pcs, and extends phylink support to cover SGMII and 2500BASE-X
> for SFP modules on PCI11x1x.
> 
> > diff --git a/drivers/net/ethernet/microchip/lan743x_main.c
> > b/drivers/net/ethernet/microchip/lan743x_main.c
> > index 88a2d11552f8..485fbc678481 100644
> > --- a/drivers/net/ethernet/microchip/lan743x_main.c
> > +++ b/drivers/net/ethernet/microchip/lan743x_main.c
> 
> [ ... ]
> 
> > @@ -3806,6 +3845,42 @@ static int lan743x_hardware_init(struct
> lan743x_adapter *adapter,
> >       return 0;
> >  }
> >
> > +static int lan743x_pcs_mdiobus_init(struct lan743x_adapter *adapter)
> > +{
> > +     struct phylink_pcs *pcs;
> > +     int ret;
> > +
> > +     adapter->pcs_mdiobus = devm_mdiobus_alloc(&adapter->pdev->dev);
> > +     if (!(adapter->pcs_mdiobus)) {
> > +             ret = -ENOMEM;
> > +             goto return_error;
> > +     }
> > +
> > +     adapter->pcs_mdiobus->priv = (void *)adapter;
> > +     adapter->pcs_mdiobus->read_c45 = pci11x1x_pcs_read;
> > +     adapter->pcs_mdiobus->write_c45 = pci11x1x_pcs_write;
> > +     adapter->pcs_mdiobus->name = "lan743x-pcs-mdiobus-c45";
> > +     netif_dbg(adapter, drv, adapter->netdev, "lan743x-pcs-mdiobus-
> c45\n");
> > +     sn printf(adapter->pcs_mdiobus->id, MII_BUS_ID_SIZE,
> > + "pci-pcs-%s", pci_name(adapter->pdev));
> > +
> > +     if (!adapter->phy_interface)
> > +             lan743x_phy_interface_select(adapter);
> > +
> > +     pcs = xpcs_create_pcs_mdiodev(adapter->pcs_mdiobus, 0);
> 
> [Critical]
> Is this pcs_mdiobus ever registered?  The bus is obtained from
> devm_mdiobus_alloc() and then handed straight to
> xpcs_create_pcs_mdiodev() without a
> mdiobus_register()/devm_mdiobus_register() call in between, so it stays in
> MDIOBUS_ALLOCATED state.
> 
> bus->mdio_lock is only initialized by __mdiobus_register():
> 
>     drivers/net/phy/mdio_bus_provider.c:__mdiobus_register() {
>         ...
>         mutex_init(&bus->mdio_lock);
>         ...
>     }
> 
> mdiobus_alloc_size() does not initialize it. xpcs_create_pcs_mdiodev() flows
> into xpcs_create() -> xpcs_init_id() -> xpcs_identify() and each
> C45 read grabs bus->mdio_lock via mutex_lock().
> 
> Can this end up locking an uninitialized mutex during probe on
> CONFIG_DEBUG_MUTEXES or lockdep kernels, and silently corrupting state
> otherwise?
> 
> Similarly, mdio_device_create() sets mdiodev->dev.parent = &bus->dev on a
> struct device that has never been through device_register(), which other xpcs
> consumers (txgbe_mdio_pcs_init, ksz9477_pcs_create,
> sja1105_mdiobus_pcs_register) avoid by registering the bus first.

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

> 
> > +     if (IS_ERR(pcs)) {
> > +             netdev_err(adapter->netdev, "failed to create xpcs\n");
> > +             ret = PTR_ERR(pcs);
> > +             goto return_error;
> > +     }
> > +
> > +     adapter->xpcs = pcs;
> > +     return 0;
> > +
> > +return_error:
> > +     mdiobus_free(adapter->pcs_mdiobus);
> > +     return ret;
> > +}
> 
> [Critical, Critical]
> Can this error path dereference a NULL pointer and also double-free the bus?
> 
> When devm_mdiobus_alloc() returns NULL, adapter->pcs_mdiobus is NULL
> and the code still jumps to return_error, which calls mdiobus_free():
> 
>     drivers/net/phy/mdio_bus_provider.c:mdiobus_free() {
>         if (bus->state == MDIOBUS_ALLOCATED) {
>             kfree(bus);
>             return;
>         }
>         ...
>     }
> 
> There is no NULL guard, so the OOM path dereferences NULL.
> 
> On the xpcs-create failure path, the bus was allocated by
> devm_mdiobus_alloc(), which registers devm_mdiobus_free() as a devres
> destructor that calls mdiobus_free() at device teardown
> (drivers/net/phy/mdio_devres.c). Calling mdiobus_free() here manually frees
> the bus (state == MDIOBUS_ALLOCATED takes the kfree(bus) path), and then
> devres runs again on the same pointer.
> 
> Would it be simpler to drop the return_error cleanup entirely and just return
> the error, letting devres free the bus?
> 
[Thangaraj Samynathan] Agreed. Will be fixed in next revision.
 
> > +
> >  static int lan743x_mdiobus_init(struct lan743x_adapter *adapter)  {
> >       int ret;
> > @@ -3927,6 +4002,11 @@ static int lan743x_pcidev_probe(struct pci_dev
> *pdev,
> >       if (ret)
> >               goto cleanup_hardware;
> >
> > +     if (adapter->is_sfp_support_en) {
> > +             ret = lan743x_pcs_mdiobus_init(adapter);
> > +             if (ret)
> > +                     goto cleanup_hardware;
> > +     }
> 
> [High]
> Is cleanup_hardware the right target here?
> 
> lan743x_mdiobus_init() was just called above and has already invoked
> mdiobus_register(adapter->mdiobus), so adapter->mdiobus is in state
> MDIOBUS_REGISTERED. Jumping to cleanup_hardware skips
> cleanup_mdiobus
> -> lan743x_mdiobus_cleanup() -> mdiobus_unregister(adapter->mdiobus).
> 
> The bus is then torn down only by the devm_mdiobus_free devres destructor,
> which reaches mdiobus_free() on a still-registered bus and
> trips:
> 
>     drivers/net/phy/mdio_bus_provider.c:mdiobus_free() {
>         ...
>         WARN(bus->state != MDIOBUS_UNREGISTERED,
>              "%s: not in UNREGISTERED state\n", bus->id);
>         ...
>     }
> 
> device_del() on the bus device is never called. Should this instead goto
> cleanup_mdiobus?

[Thangaraj Samynathan] Agreed, Will be fixed in next revision.
 
> 
> [Medium]
> On PCI11x1x, pci11x1x_strap_get_status() can leave is_sfp_support_en true
> while is_pcs_en is false (SFP strap set, SGMII strap cleared) with only a
> netif_err().  In that case lan743x_hardware_init() takes the SGMII-disabled
> path and writes SGMII_CTL with SGMII_CTL_SGMII_POWER_DN_ set and
> SGMII_CTL_SGMII_ENABLE_ cleared.
> 
> Then lan743x_pcidev_probe() still enters this branch because
> is_sfp_support_en is true, invoking lan743x_pcs_mdiobus_init() which drives
> xpcs_create_pcs_mdiodev() -> xpcs_identify() -> multiple C45 reads through
> pci11x1x_pcs_read() -> lan743x_sgmii_read():
> 
>     drivers/net/ethernet/microchip/lan743x_main.c:lan743x_sgmii_read() {
>         ...
>         lan743x_csr_write(adapter, SGMII_ACC, mmd_access);
>         ret = lan743x_sgmii_wait_till_not_busy(adapter);
>         ...
>     }
> 
> With the SGMII block powered down, can the busy bit ever clear, or will every
> read hit the 1-second readx_poll_timeout, producing an extremely slow probe
> and a bogus xpcs identification?

[Thangaraj Samynathan - I53494] This will be fixed by the patch 2/5 fix that forces
is_sfp_support_en = false when the invalid combination (SFP strap set without
SGMII strap) is detected, ensuring lan743x_pcs_mdiobus_init() is never entered
when the SGMII block is powered down.

> 
> >       adapter->netdev->netdev_ops = &lan743x_netdev_ops;
> >       adapter->netdev->ethtool_ops = &lan743x_ethtool_ops;
> >       adapter->netdev->features = NETIF_F_SG | NETIF_F_TSO |

Regards,
Thangaraj Samynathan