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

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

[Jakub Kicinski]

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

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

[Łukasz Majewski]

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

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

[Andrew Lunn]

On Wed, Jan 28, 2026 at 10:41:25PM +0100, Łukasz Majewski wrote:
> Hi Jakub,
> 
> > This is an AI-generated review
> 
> AI is going to take the "Maintainer's" job :D

We had a bit of a discussion about this.

Our observations so far is that AI finds different sorts of issues,
which human reviewer often don't spot. It also does get things wrong,
so we spend some time sanity checking its reports. So overall, it
might actually increase the workload, but also increase the quality of
the code which gets committed.

	Andrew

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

[Łukasz Majewski]

Hi Andrew,

> On Wed, Jan 28, 2026 at 10:41:25PM +0100, Łukasz Majewski wrote:
> > Hi Jakub,
> >   
> > > This is an AI-generated review  
> > 
> > AI is going to take the "Maintainer's" job :D  
> 
> We had a bit of a discussion about this.
> 
> Our observations so far is that AI finds different sorts of issues,
> which human reviewer often don't spot. It also does get things wrong,
> so we spend some time sanity checking its reports. So overall, it
> might actually increase the workload, but also increase the quality of
> the code which gets committed.
> 

Frankly - from the MTIP L2 switch patch set review - the AI review does
a _really_ good job ...

It finds non trivial issues - with very deep insights to function's
call dependencies.

> 	Andrew



-- 
Best regards,

Łukasz Majewski