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

[netdev-bot+sashiko@kernel.org]

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()?

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