Re: [PATCH net] net: lan966x: don't free XDP buffers in fdma tx clear with budget 0

[Daniel Machon]

> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 (1) · Medium: 1 (1) · Low: 0
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [Medium] This patch adds a skip to lan966x_fdma_tx_clear_buf()
>   (drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c, line 359): `if…
> 
> Pre-existing issues:
> - [High] This problem wasn't introduced by this patch, but
>   lan966x_fdma_reload()…
> - [Medium] This problem wasn't introduced by this patch, but
>   lan966x_fdma_deinit()…
> 
> --
> 
> net: lan966x: don't free XDP buffers in fdma tx clear with budget 0
> 
> This patch makes lan966x_fdma_tx_clear_buf() skip completed XDP_TX and
> ndo_xdp_xmit DCBs when netpoll calls it with a budget of 0. That way the
> page pool and XDP return APIs are not called with hardirqs disabled. The
> skipped buffers are left for the next NAPI poll, and skbs are still freed
> via napi_consume_skb().
> 
> > diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
> > index 21668bbcc9063..1fbe910c5ec38 100644
> > --- a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
> > +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
> > @@ -353,6 +353,12 @@ static void lan966x_fdma_tx_clear_buf(struct lan966x *lan966x, int weight)
> >               if (!fdma_db_is_done(db))
> >                       continue;
> >
> > +             /* XDP and page pool APIs can't be used with a budget of 0.
> > +              * Leave these buffers for the next NAPI poll.
> > +              */
> > +             if (!weight && !dcb_buf->use_skb)
> > +                     continue;
> 
> [Severity: Medium]
> Can this starve netconsole transmits on the port that owns the FDMA NAPI?
> 
> skbs and XDP frames get slots from the same ring through
> lan966x_fdma_get_next_dcb(), which only checks the used bit:
> 
> lan966x_fdma_get_next_dcb() {
>         ...
>                 if (!dcb_buf->used &&
>                     !fdma_is_last(&tx->fdma, &tx->fdma.dcbs[i]))
>                         return i;
>         ...
> }
> 
> If heavy XDP_TX or ndo_xdp_xmit traffic fills every usable slot, a
> netpoll skb ends up here in lan966x_fdma_xmit():
> 
>         next_to_use = lan966x_fdma_get_next_dcb(tx);
>         if (next_to_use < 0) {
>                 netif_stop_queue(dev);
>                 return NETDEV_TX_BUSY;
>         }
> 
> __netpoll_send_skb() then polls between retries:
> 
> netpoll_poll_dev()->poll_one_napi()->napi->poll(napi, 0)
>   ->lan966x_fdma_tx_clear_buf(lan966x, 0)
> 
> With the new check, every completed XDP DCB is skipped. No slot is freed,
> clear stays false, and lan966x_fdma_wakeup_netdev() is never called.
> 
> Once the retries run out, the skb is put on npinfo->txq and handed to
> delayed work, and later messages queue behind it. In normal operation a
> later IRQ-driven NAPI poll frees the DCBs and output only comes late.
> 
> During a panic, with the other CPUs stopped and hardirqs off, would that
> work ever run? If not, the netconsole output is lost.
> 
> Before this patch, the budget-0 poll freed those slots, so netpoll could
> make progress.

Seems right, however, AFAICT, this is only an issue if the panic hits while the
ring is completely full of finished XDP frames. Freeing those frames from
netpoll is what napi.rst forbids: with a budget of 0 the driver cannot call any
XDP or page pool APIs, and doing so is the splat this patch fixes.

The only other way I see is to give XDP its own FDMA injection channel, so XDP
frames can never take the slots netpoll needs. lan966x has free injection
channels for that, but it is a rework for net-next, not a fix for net.

> 
> [Severity: High]
> This isn't a bug introduced by this patch, but can XDP_TX pages that TX
> DCBs still hold end up in the wrong page_pool when lan966x_fdma_reload()
> replaces the RX pool?
> 
> lan966x_fdma_reload() does:
> 
>         napi_synchronize(&lan966x->napi);
>         napi_disable(&lan966x->napi);
>         ...
>         err = lan966x_fdma_rx_alloc(&lan966x->rx);
>         ...
>         page_pool_destroy(page_pool);
> 
>         lan966x_fdma_wakeup_netdev(lan966x);
>         napi_enable(&lan966x->napi);
> 
> It never walks tx->dcbs_buf[]. Suppose an XDP_TX DCB posted by the last
> poll completes in hardware after napi_disable(). The FDMA IRQ's
> napi_schedule() then does nothing, and the DCB stays used.
> 
> The first poll after napi_enable() then runs this in
> lan966x_fdma_tx_clear_buf():
> 
>         page_pool_recycle_direct(rx->page_pool,
>                                  dcb_buf->data.page);
> 
> At that point rx->page_pool is the new pool. __page_pool_put_page() does
> not check which pool owns the page. So the old pool's page, with its old
> DMA mapping and order, goes into the new pool's cache.
> 
> If the MTU change raised page_order, could the new pool give this smaller
> page to the RX ring? There db_size is PAGE_SIZE << new_order, so the
> device could DMA a large frame past the end of the page.
> 
> Even if the order stays the same, the old pool's inflight count would
> never reach zero, so its shutdown stalls and the pool is leaked. And
> wouldn't the new pool's inflight count go negative when it is destroyed?
> 
> This can happen on these paths while XDP_TX traffic is flowing:
> 
> lan966x_port_change_mtu()->lan966x_fdma_change_mtu()
>   ->__lan966x_fdma_reload()->lan966x_fdma_reload()
> 
> lan966x_xdp_setup()->lan966x_fdma_reload_page_pool()
>   ->__lan966x_fdma_reload()->lan966x_fdma_reload()
> 
> This patch doesn't widen the window much. An IRQ-driven NAPI poll still
> reclaims those DCBs if it runs before napi_disable().
> 
> [Severity: Medium]
> This is a pre-existing issue, but does lan966x_fdma_deinit() leak the TX
> DCBs that are still marked used?
> 
> lan966x_fdma_deinit() {
>         ...
>         napi_synchronize(&lan966x->napi);
>         napi_disable(&lan966x->napi);
> 
>         lan966x_fdma_rx_free_pages(&lan966x->rx);
>         fdma_free_coherent(lan966x->dma_dev, &lan966x->rx.fdma);
>         page_pool_destroy(lan966x->rx.page_pool);
>         lan966x_fdma_tx_free(&lan966x->tx);
> }
> 
> lan966x_fdma_tx_free() only does:
> 
>         kfree(tx->dcbs_buf);
>         fdma_free_coherent(lan966x->dma_dev, &tx->fdma);
> 
> Entries that are still used are never released:
> 
> - skb and xdp_ndo buffers stay DMA-mapped
> - skbs are not consumed
> - xdp_frames are not returned
> - XDP_TX pages are not returned to the page pool
> 
> Those pages keep the destroyed pool inflight, so its shutdown stalls.
> 
> lan966x_remove() and the probe error path both call fdma_deinit() before
> lan966x_cleanup_ports(). After napi_disable() no poll can reclaim these
> entries.
> 
> This patch adds completed but deferred XDP buffers to what can still be
> outstanding here. The window is narrow, because an IRQ-driven poll
> normally frees them before napi_disable().
> 
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261029-lan966x-fdma-tx-clear-budget-v1-1-38c52d7aab8c%40microchip.com