Re: [net-next v24 3/7] net: mtip: Add buffers management functions to the L2 switch driver

Re: [net-next v24 3/7] net: mtip: Add buffers management functions to the L2 switch driver

[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.
---
net: mtip: Add buffers management functions to the L2 switch driver

This patch fills in mtip_alloc_buffers() and mtip_free_buffers() for the
MTIP L2 switch driver, replacing the previously empty stubs with real
page_pool setup/teardown and tx_bounce buffer allocation/free.

> diff --git a/drivers/net/ethernet/freescale/mtipsw/mtipl2sw.c b/drivers/net/ethernet/freescale/mtipsw/mtipl2sw.c
> index 1d1005a42306..5b5229ee411d 100644
> --- a/drivers/net/ethernet/freescale/mtipsw/mtipl2sw.c
> +++ b/drivers/net/ethernet/freescale/mtipsw/mtipl2sw.c
> @@ -892,11 +892,101 @@ static void mtip_get_drvinfo(struct net_device *dev,
>  
>  static void mtip_free_buffers(struct net_device *dev)
>  {
> +	struct mtip_ndev_priv *priv = netdev_priv(dev);
> +	struct switch_enet_private *fep = priv->fep;
> +	int i;
> +
> +	for (i = 0; i < RX_RING_SIZE; i++) {
> +		if (!fep->page[i])
> +			continue;
> +
> +		page_pool_put_full_page(fep->page_pool,
> +					fep->page[i], false);
> +		fep->page[i] = NULL;
> +	}
> +
> +	page_pool_destroy(fep->page_pool);
> +	fep->page_pool = NULL;
> +
> +	for (i = 0; i < TX_RING_SIZE; i++) {
> +		kfree(fep->tx_bounce[i]);
> +		fep->tx_bounce[i] = NULL;
> +	}
> +}

[High]
Now that mtip_free_buffers() is no longer an empty stub, can the
mtip_open() error path tear down buffers that another active port is
still using?

Looking at mtip_open():

	if (fep->usage_count == 0) {
		...
		ret = mtip_alloc_buffers(dev);
		if (ret)
			goto mtip_alloc_buffers_err;
	}
	...
	ret = mtip_mii_probe(dev);
	if (ret)
		goto mtip_mii_probe_err;
	...
 mtip_mii_probe_err:
	mtip_free_buffers(dev);
 mtip_alloc_buffers_err:
	if (fep->usage_count == 0) {
		netif_napi_del(&fep->napi);
		clk_disable(fep->clk_ipg);
	}
	return ret;

mtip_alloc_buffers() only runs when fep->usage_count == 0, so the
page_pool, fep->page[] and fep->tx_bounce[] are shared across switch
ports and only allocated on first open. mtip_close() reflects this and
only calls mtip_free_buffers() when fep->usage_count == 1.

When a second port is opened (fep->usage_count > 0) and mtip_mii_probe()
fails, the mtip_mii_probe_err label still calls mtip_free_buffers()
unconditionally, which destroys the page_pool and frees the tx_bounce
buffers that the first, still-active port is using.

Should the mtip_mii_probe_err teardown also be guarded by
fep->usage_count == 0 (or the labels reordered) so buffers are only
freed for the last user, similar to mtip_close()?

After this change the previously harmless call becomes a real teardown,
so the surviving port's NAPI path (mtip_switch_rx() and the later
mtip_update_cbd() refill) would dereference a freed fep->page_pool and
NULL fep->page[] / fep->tx_bounce[] entries whenever the second port's
PHY probe fails while the first port is up.

>  
> +static int mtip_create_page_pool(struct switch_enet_private *fep, int size)
> +{
[ ... ]
> +}
>  
>  static int mtip_alloc_buffers(struct net_device *dev)
>  {
[ ... ]
>  	return 0;
> +
> + err:
> +	mtip_free_buffers(dev);
> +	return -ENOMEM;
>  }
>

Re: [net-next v24 3/7] net: mtip: Add buffers management functions to the L2 switch driver

[Łukasz Majewski]

Hi Jakub,

> 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: mtip: Add buffers management functions to the L2 switch driver
> 
> This patch fills in mtip_alloc_buffers() and mtip_free_buffers() for
> the MTIP L2 switch driver, replacing the previously empty stubs with
> real page_pool setup/teardown and tx_bounce buffer allocation/free.
> 
> > diff --git a/drivers/net/ethernet/freescale/mtipsw/mtipl2sw.c
> > b/drivers/net/ethernet/freescale/mtipsw/mtipl2sw.c index
> > 1d1005a42306..5b5229ee411d 100644 ---
> > a/drivers/net/ethernet/freescale/mtipsw/mtipl2sw.c +++
> > b/drivers/net/ethernet/freescale/mtipsw/mtipl2sw.c @@ -892,11
> > +892,101 @@ static void mtip_get_drvinfo(struct net_device *dev, 
> >  static void mtip_free_buffers(struct net_device *dev)
> >  {
> > +	struct mtip_ndev_priv *priv = netdev_priv(dev);
> > +	struct switch_enet_private *fep = priv->fep;
> > +	int i;
> > +
> > +	for (i = 0; i < RX_RING_SIZE; i++) {
> > +		if (!fep->page[i])
> > +			continue;
> > +
> > +		page_pool_put_full_page(fep->page_pool,
> > +					fep->page[i], false);
> > +		fep->page[i] = NULL;
> > +	}
> > +
> > +	page_pool_destroy(fep->page_pool);
> > +	fep->page_pool = NULL;
> > +
> > +	for (i = 0; i < TX_RING_SIZE; i++) {
> > +		kfree(fep->tx_bounce[i]);
> > +		fep->tx_bounce[i] = NULL;
> > +	}
> > +}  
> 
> [High]
> Now that mtip_free_buffers() is no longer an empty stub, can the
> mtip_open() error path tear down buffers that another active port is
> still using?
> 
> Looking at mtip_open():
> 
> 	if (fep->usage_count == 0) {
> 		...
> 		ret = mtip_alloc_buffers(dev);
> 		if (ret)
> 			goto mtip_alloc_buffers_err;
> 	}
> 	...
> 	ret = mtip_mii_probe(dev);
> 	if (ret)
> 		goto mtip_mii_probe_err;
> 	...
>  mtip_mii_probe_err:
> 	mtip_free_buffers(dev);
>  mtip_alloc_buffers_err:
> 	if (fep->usage_count == 0) {
> 		netif_napi_del(&fep->napi);
> 		clk_disable(fep->clk_ipg);
> 	}
> 	return ret;
> 
> mtip_alloc_buffers() only runs when fep->usage_count == 0, so the
> page_pool, fep->page[] and fep->tx_bounce[] are shared across switch
> ports and only allocated on first open. mtip_close() reflects this and
> only calls mtip_free_buffers() when fep->usage_count == 1.
> 
> When a second port is opened (fep->usage_count > 0) and
> mtip_mii_probe() fails, the mtip_mii_probe_err label still calls
> mtip_free_buffers() unconditionally, which destroys the page_pool and
> frees the tx_bounce buffers that the first, still-active port is
> using.
> 
> Should the mtip_mii_probe_err teardown also be guarded by
> fep->usage_count == 0 (or the labels reordered) so buffers are only
> freed for the last user, similar to mtip_close()?
> 
> After this change the previously harmless call becomes a real
> teardown, so the surviving port's NAPI path (mtip_switch_rx() and the
> later mtip_update_cbd() refill) would dereference a freed
> fep->page_pool and NULL fep->page[] / fep->tx_bounce[] entries
> whenever the second port's PHY probe fails while the first port is up.
> 
> >  
> > +static int mtip_create_page_pool(struct switch_enet_private *fep,
> > int size) +{  
> [ ... ]
> > +}
> >  
> >  static int mtip_alloc_buffers(struct net_device *dev)
> >  {  
> [ ... ]
> >  	return 0;
> > +
> > + err:
> > +	mtip_free_buffers(dev);
> > +	return -ENOMEM;
> >  }
> >  

Thank you for sending the AI review.

Please find my comments:

- I've gone through them and yes, there are a few issues found.
  However, those are IMHO not critical ones.

- Some issues are not valid at all - like concern what happens when the
  switch is run on multi processor chip - both imx287 and vf610 are
  single core chips.

- It seems like the AI review is "progressing" - i.e. there were no
  issues found with v23 for this particular patch. I do guess that the
  new Orc Mode from [1] has found it.

- I do guess that different AI agent would generate different review

- The aforementioned issues could be addressed with a fix patches -
  dragging along (and preparing for upstream) patch set with ~3400
  LOC (which is rejected by non-deterministic AI review) takes too much
  resources now.


To sum up:
----------

The MTIP driver for v6.6 kernel (YPRR Scarthgap) with and without
PREEMPT_RT for vf610 and imx287 as well as the v24 for net-next can be
found at [2].


Links:

[1] - https://netdev-ai.bots.linux.dev/ai-local.html
[2] - https://github.com/lmajewski/linux-imx28-l2switch/branches



-- 
Best regards,

Łukasz Majewski