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

[Jakub Kicinski <kuba@kernel.org>]

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.

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

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

[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?

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