Re: [PATCH net] net: lan966x: don't disable hardirqs in fdma tx clear path

[Daniel Machon]

> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 1 · Medium: 0 · 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).
> 
> - [High] The patch only fixes the normal NAPI case.
> 
> --
> 
> net: lan966x: don't disable hardirqs in fdma tx clear path
> 
> This patch changes lan966x_fdma_tx_clear_buf() to take tx_lock with a
> plain spin_lock() instead of spin_lock_irqsave(). The aim is to stop
> freeing page_pool backed buffers with hardirqs disabled. That currently
> triggers the lockdep_assert_no_hardirq() warning in
> __page_pool_put_page().
> 
> > diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
> > index a7940eca5df31..a92bd0590b460 100644
> > --- a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
> > +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
> > @@ -336,14 +336,13 @@ static void lan966x_fdma_tx_clear_buf(struct lan966x *lan966x, int weight)
> 
> [ ... ]
> 
> >       xdp_frame_bulk_init(&bq);
> >
> > -     spin_lock_irqsave(&lan966x->tx_lock, flags);
> > +     spin_lock(&lan966x->tx_lock);
> 
> [Severity: High]
> Does this cover the netpoll case? The commit message says:
> 
>   tx_clear_buf() runs from the NAPI poll, so disabling hardirqs there is
>   not needed.
> 
> However, netpoll can call the NAPI poll with budget 0 after the caller
> has already disabled hardirqs:
> 
> netpoll_send_skb()
>   local_irq_save(flags)
>   __netpoll_send_skb()
>     netpoll_poll_dev()
>       poll_napi()
>         poll_one_napi()
>           napi->poll(napi, 0)
>             lan966x_fdma_napi_poll(weight == 0)
>               lan966x_fdma_tx_clear_buf(lan966x, 0)
> 
> lan966x_fdma_napi_poll() calls lan966x_fdma_tx_clear_buf() whatever the
> weight is. tx_clear_buf() then does the XDP and page pool frees without
> checking weight:
> 
>                         if (dcb_buf->xdp_ndo)
>                                 xdp_return_frame_bulk(dcb_buf->data.xdpf, &bq);
>                         else
>                                 page_pool_recycle_direct(rx->page_pool,
>                                                          dcb_buf->data.page);
>         ...
>         xdp_flush_frame_bulk(&bq);
> 
> Consider netconsole on the port that owns the FDMA NAPI (fdma_ndev, which
> does not set IFF_DISABLE_NETPOLL), with completed XDP_TX or ndo_xdp_xmit
> frames in the TX ring. In that case, won't page_pool_recycle_direct() and
> xdp_flush_frame_bulk() still hit lockdep_assert_no_hardirq() in
> __page_pool_put_page()?

This patch fixes the normal NAPI path, where the driver itself disabled
hardirqs around the frees. Fixing the netpoll case would be checking the budget,
which seems to be unrelated to this fix.

> The bulk path always goes through page_pool_recycle_ring_bulk(). The
> direct path does too once the alloc cache is full. That function takes
> the producer lock with spin_lock_bh(), and netpoll from process context
> is not in softirq.
> 
> Can spin_unlock_bh() then run __local_bh_enable_ip() with IRQs disabled?
> That could run pending softirqs or turn IRQs back on inside netpoll's
> local_irq_save() section.
> 
> Documentation/networking/napi.rst says:
> 
>   skb Tx processing should happen regardless of the budget, but if
>   the argument is 0 driver cannot call any XDP (or page pool) APIs.
> 
> The skb branch looks fine, because napi_consume_skb(skb, 0) falls back to
> dev_consume_skb_any(). Could the XDP and page pool part of TX completion
> be skipped when weight is 0, as other page_pool drivers do?
> 
> The missing budget check is older than this patch. Even so, it means the
> warning this patch is meant to fix can still be triggered from
> lan966x_fdma_tx_clear_buf().
> 
> [ ... ]
> 
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261026-lan966x-fdma-tx-clear-hardirq-v1-1-5f70f026a2e4%40microchip.com