* Re: [net-next v24 3/7] net: mtip: Add buffers management functions to the L2 switch driver
2026-06-05 2:18 ` [net-next v24 3/7] net: mtip: Add buffers management functions to the L2 switch driver Jakub Kicinski
@ 2026-06-09 8:37 ` Łukasz Majewski
0 siblings, 0 replies; 2+ messages in thread
From: Łukasz Majewski @ 2026-06-09 8:37 UTC (permalink / raw)
To: Jakub Kicinski
Cc: andrew+netdev, davem, edumazet, pabeni, richardcochran, lgirdwood,
nathan, linux-kernel, netdev, krzk+dt, imx, linux-arm-kernel,
robh, Andrew Lunn
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
^ permalink raw reply [flat|nested] 2+ messages in thread