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

Hi Jakub,
Thanks for reviewing the patch.

-----Original Message-----
From: Jakub Kicinski <kuba@kernel.org>
Sent: Tuesday, May 12, 2026 7:39 AM
To: Thangaraj Samynathan - I53494 <Thangaraj.S@microchip.com>
Cc: Jakub Kicinski <kuba@kernel.org>; netdev@vger.kernel.org;
andrew+netdev@lunn.ch; davem@davemloft.net; edumazet@google.com;
pabeni@redhat.com; Bryan Whitehead - C21958
[off-list ref]; UNGLinuxDriver
[off-list ref]; linux@armlinux.org.uk; linux-
kernel@vger.kernel.org
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.

quoted

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

[ ... ]

quoted

@@ -3806,6 +3845,42 @@ static int lan743x_hardware_init(struct

lan743x_adapter *adapter,

quoted

      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");

quoted

+     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..

quoted

+     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.
 

quoted

+
 static int lan743x_mdiobus_init(struct lan743x_adapter *adapter)  {
      int ret;

@@ -3927,6 +4002,11 @@ static int lan743x_pcidev_probe(struct pci_dev

*pdev,

quoted

      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.

quoted

      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