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

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

[Daniel Machon]

lan966x_fdma_tx_clear_buf() runs from the NAPI poll and frees the
buffers of completed tx DCBs with napi_consume_skb(),
page_pool_recycle_direct() and xdp_return_frame_bulk(), under
lan966x->tx_lock.

Before this patch, the lock is taken with spin_lock_irqsave(), so the
buffers are freed with hardirqs disabled. RX skbs are marked for
page_pool recycling, so when such an skb is forwarded back out and its
tx completion frees it here, the free path reaches __page_pool_put_page(),
which warns via lockdep_assert_no_hardirq().

Take tx_lock with a plain spin_lock(), as the other tx_lock users,
lan966x_port_xmit() and lan966x_fdma_xmit_xdpf(), already do.
tx_clear_buf() runs from the NAPI poll, so disabling hardirqs there is
not needed.

Found with lockdep enabled (CONFIG_PROVE_LOCKING), which the assert
depends on. Reproduced on a lan9668 board by redirecting received frames
out of another port with tc mirred.

Fixes: 11871aba1974 ("net: lan96x: Use page_pool API")
Signed-off-by: Daniel Machon <daniel.machon@microchip.com>
---
 drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

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)
 	struct lan966x_tx_dcb_buf *dcb_buf;
 	struct fdma *fdma = &tx->fdma;
 	struct xdp_frame_bulk bq;
-	unsigned long flags;
 	bool clear = false;
 	struct fdma_db *db;
 	int i;
 
 	xdp_frame_bulk_init(&bq);
 
-	spin_lock_irqsave(&lan966x->tx_lock, flags);
+	spin_lock(&lan966x->tx_lock);
 	for (i = 0; i < fdma->n_dcbs; ++i) {
 		dcb_buf = &tx->dcbs_buf[i];
 
@@ -388,7 +387,7 @@ static void lan966x_fdma_tx_clear_buf(struct lan966x *lan966x, int weight)
 	if (clear)
 		lan966x_fdma_wakeup_netdev(lan966x);
 
-	spin_unlock_irqrestore(&lan966x->tx_lock, flags);
+	spin_unlock(&lan966x->tx_lock);
 }
 
 static int lan966x_fdma_rx_check_frame(struct lan966x_rx *rx, u64 *src_port)

---
base-commit: 6430fb7f1cc4e329e386521b7b208df443de92f2
change-id: 20261023-lan966x-fdma-tx-clear-hardirq-43724a086d0d

Best regards,
-- 
Daniel Machon <daniel.machon@microchip.com>

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

[netdev-bot+sashiko]

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

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

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

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Mon, 26 Oct 2026 14:45:58 +0100 you wrote:
> lan966x_fdma_tx_clear_buf() runs from the NAPI poll and frees the
> buffers of completed tx DCBs with napi_consume_skb(),
> page_pool_recycle_direct() and xdp_return_frame_bulk(), under
> lan966x->tx_lock.
> 
> Before this patch, the lock is taken with spin_lock_irqsave(), so the
> buffers are freed with hardirqs disabled. RX skbs are marked for
> page_pool recycling, so when such an skb is forwarded back out and its
> tx completion frees it here, the free path reaches __page_pool_put_page(),
> which warns via lockdep_assert_no_hardirq().
> 
> [...]

Here is the summary with links:
  - [net] net: lan966x: don't disable hardirqs in fdma tx clear path
    https://git.kernel.org/netdev/net/c/b2e17432e7ee

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html