Re: [PATCH net-next v3 4/5] net: lan743x: Register SFP platform device for 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: 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?

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

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

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