From: "Łukasz Majewski" <lukasz.majewski@mailbox•org>
To: Jakub Kicinski <kuba@kernel•org>
Cc: andrew+netdev@lunn•ch, davem@davemloft•net, edumazet@google•com,
pabeni@redhat•com, richardcochran@gmail•com, lgirdwood@gmail•com,
nathan@kernel•org, linux-kernel@vger•kernel.org,
netdev@vger•kernel.org, krzk+dt@kernel•org, imx@lists•linux.dev,
linux-arm-kernel@lists•infradead.org, robh@kernel•org,
Andrew Lunn <andrew@lunn•ch>
Subject: Re: [net-next v24 3/7] net: mtip: Add buffers management functions to the L2 switch driver
Date: Tue, 9 Jun 2026 10:37:04 +0200 [thread overview]
Message-ID: <20260609103704.443c0b57@wsk> (raw)
In-Reply-To: <20260605021853.3594553-1-kuba@kernel.org>
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
next prev parent reply other threads:[~2026-06-09 8:37 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-06-01 11:24 [net-next v24 0/7] net: mtip: Add support for MTIP imx287 L2 switch driver Lukasz Majewski
2026-06-01 11:24 ` [net-next v24 1/7] dt-bindings: net: Add MTIP L2 switch description Lukasz Majewski
2026-06-01 11:24 ` [net-next v24 2/7] net: mtip: The L2 switch driver for imx287 Lukasz Majewski
2026-06-05 2:18 ` Jakub Kicinski
2026-06-01 11:24 ` [net-next v24 3/7] net: mtip: Add buffers management functions to the L2 switch driver Lukasz Majewski
2026-06-05 2:18 ` Jakub Kicinski
2026-06-09 8:37 ` Łukasz Majewski [this message]
2026-06-01 11:24 ` [net-next v24 4/7] net: mtip: Add net_device_ops " Lukasz Majewski
2026-06-05 2:18 ` Jakub Kicinski
2026-06-01 11:24 ` [net-next v24 5/7] net: mtip: Add mtip_switch_{rx|tx} " Lukasz Majewski
2026-06-05 2:18 ` Jakub Kicinski
2026-06-01 11:24 ` [net-next v24 6/7] net: mtip: Extend the L2 switch driver with management operations Lukasz Majewski
2026-06-05 2:18 ` Jakub Kicinski
2026-06-01 11:24 ` [net-next v24 7/7] net: mtip: Extend the L2 switch driver for imx287 with bridge operations Lukasz Majewski
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20260609103704.443c0b57@wsk \
--to=lukasz.majewski@mailbox$(echo .)org \
--cc=andrew+netdev@lunn$(echo .)ch \
--cc=andrew@lunn$(echo .)ch \
--cc=davem@davemloft$(echo .)net \
--cc=edumazet@google$(echo .)com \
--cc=imx@lists$(echo .)linux.dev \
--cc=krzk+dt@kernel$(echo .)org \
--cc=kuba@kernel$(echo .)org \
--cc=lgirdwood@gmail$(echo .)com \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
--cc=linux-kernel@vger$(echo .)kernel.org \
--cc=nathan@kernel$(echo .)org \
--cc=netdev@vger$(echo .)kernel.org \
--cc=pabeni@redhat$(echo .)com \
--cc=richardcochran@gmail$(echo .)com \
--cc=robh@kernel$(echo .)org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox