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

["Łukasz Majewski" <lukasz.majewski@mailbox.org>]

Hi Jakub,

> This is an AI-generated review

AI is going to take the "Maintainer's" job :D

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

Ech.... I will fix it.

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

The same here.

> > +}  
> 
> [ ... ]
> 
> > +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()?

No. The mtip_mii_probe() is only called in mtip_open(), which is called
only when netdevice's DT node is correctly parsed in the
mtip_sw_probe().

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

In the:
./Documentation/devicetree/bindings/net/nxp,imx28-mtip-switch.yaml

one can have only ethernet-ports set to [12].

In the current tree - some switches start numbering from 0, other from
1.

I would opt for keeping things as they are now...

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

+1

The mdiobus_unregister() is missing.

I will add it.

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

Yes. The dma_free_coherent() is missing.

I will add it for v21.

-- 
Best regards,

Łukasz Majewski