Re: [net-next,v20,2/7] net: mtip: The L2 switch driver for imx287

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

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
net: mtip: The L2 switch driver for imx287

This patch adds support for the More Than IP L2 switch embedded in the
imx287 SoC. It creates a two-port switch driver that can be used
interchangeably with the FEC driver.

> diff --git a/drivers/net/ethernet/freescale/mtipsw/mtipl2sw.c b/drivers/net/ethernet/freescale/mtipsw/mtipl2sw.c
> new file mode 100644
> index 000000000000..03c5163d6508
> --- /dev/null
> +++ b/drivers/net/ethernet/freescale/mtipsw/mtipl2sw.c

[ ... ]

> +static void mtip_netif_stop_queues(struct switch_enet_private *fep)
> +{
> +	for (int i = 0; i < SWITCH_EPORT_NUMBER; i++)
> +		netif_stop_queue(fep->ndev[0]);
                                      ^^^

Should this be fep->ndev[i] instead of fep->ndev[0]? The loop iterates
with variable i but always uses index 0, so port 1's queue is never
stopped.

> +}
> +
> +static void mtip_netif_wake_queues(struct switch_enet_private *fep)
> +{
> +	for (int i = 0; i < SWITCH_EPORT_NUMBER; i++)
> +		netif_wake_queue(fep->ndev[0]);
                                      ^^^

Same here - should this be fep->ndev[i]?

> +}

[ ... ]

> +static int mtip_mii_probe(struct net_device *dev)
> +{
> +	struct mtip_ndev_priv *priv = netdev_priv(dev);
> +	struct switch_enet_private *fep = priv->fep;
> +	struct phy_device *phy_dev = NULL;
> +	int port_idx = priv->portnum - 1;
> +
> +	if (fep->phy_np[port_idx]) {
> +		phy_dev = of_phy_connect(dev, fep->phy_np[port_idx],
> +					 &mtip_adjust_link, 0,
> +					 fep->phy_interface[port_idx]);
> +		if (!phy_dev) {
> +			netdev_err(dev, "Unable to connect to phy\n");
> +			return -ENODEV;
> +		}
> +	}
> +
> +	phy_set_max_speed(phy_dev, 100);
                          ^^^^^^^

If fep->phy_np[port_idx] is NULL (no phy-handle in DT for this port),
the if block is skipped and phy_dev remains NULL. Can this cause a NULL
pointer dereference in phy_set_max_speed()?

> +	fep->phy_dev[port_idx] = phy_dev;

[ ... ]

> +static int mtip_parse_of(struct switch_enet_private *fep,
> +			 struct device_node *np)
> +{
> +	struct device_node *p;
> +	unsigned int port_num;
> +	int ret = 0;
> +
> +	p = of_get_child_by_name(np, "ethernet-ports");
> +
> +	for_each_available_child_of_node_scoped(p, port) {
> +		if (of_property_read_u32(port, "reg", &port_num))
> +			continue;
> +
> +		if (port_num > SWITCH_EPORT_NUMBER) {
                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

This only validates the upper bound. If the device tree contains
reg = <0>, would fep->mac[port_num - 1] access fep->mac[-1]? Perhaps
the check should also verify port_num is at least 1?

> +			dev_err(&fep->pdev->dev,
> +				"%s: The switch supports up to %d ports!\n",
> +				__func__, SWITCH_EPORT_NUMBER);
> +			goto of_get_err;
> +		}
> +
> +		fep->n_ports = port_num;
> +		ret = of_get_mac_address(port, &fep->mac[port_num - 1][0]);

[ ... ]

> +	ret = mtip_mii_init(fep, pdev);
> +	if (ret) {
> +		dev_err(&pdev->dev, "%s: Cannot init phy bus (%d)!\n", __func__,
> +			ret);
> +		goto dma_free_coherent_memory;
> +	}
> +
> +	ret = mtip_ndev_init(fep, pdev);
> +	if (ret) {
> +		dev_err(&pdev->dev, "%s: Failed to create virtual ndev (%d)\n",
> +			__func__, ret);
> +		goto mdiobus_free_memory;
> +	}

[ ... ]

> + mdiobus_free_memory:
> +	mdiobus_free(fep->mii_bus);

mtip_mii_init() calls of_mdiobus_register() which registers the bus.
Should mdiobus_unregister() be called here before mdiobus_free()?
Compare with mtip_mii_remove() which does call mdiobus_unregister().

> + dma_free_coherent_memory:
> +	dma_free_coherent(&fep->pdev->dev, PAGE_SIZE, fep->rx_bd_base,
> +			  fep->bd_dma);
> +	fep->rx_bd_base = NULL;
> +	fep->tx_bd_base = NULL;
> +
> +	return ret;
> +}
> +
> +static void mtip_sw_remove(struct platform_device *pdev)
> +{
> +	struct switch_enet_private *fep = platform_get_drvdata(pdev);
> +
> +	mtip_ndev_cleanup(fep);
> +
> +	mtip_mii_remove(fep);
> +
> +	timer_delete_sync(&fep->timer_mgnt);
> +	platform_set_drvdata(pdev, NULL);
> +}

mtip_sw_probe() allocates DMA memory with dma_alloc_coherent() in
mtip_switch_dma_init(), but mtip_sw_remove() does not appear to free it.
Is this a memory leak on driver removal?
-- 
pw-bot: cr