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

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

[Łukasz Majewski]

Hi Jakub,

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

Could you provide some guidelines about how to proceed?

The series was sent shortly before the net-next being open (and then it
was closed because of the new merge window).

Now it is open again, so maybe you would find some more time for this
patch set discussion?

Thanks in advance.

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