[PATCH] net: sunplus: fix tx_poll spin_lock missing irqsave in NAPI context

[PATCH] net: sunplus: fix tx_poll spin_lock missing irqsave in NAPI context

[Andrew Gaylard]

spl2sw_tx_poll() runs in NAPI (softirq) context and acquires
comm->tx_lock with plain spin_lock(). The hardirq handler
spl2sw_ethernet_interrupt() acquires the same lock, so a hardirq on
the same CPU while the NAPI poll holds the lock would deadlock with
IRQs disabled. Use spin_lock_irqsave/spin_unlock_irqrestore.

Signed-off-by: Andrew Gaylard <ag@ffroot.co.za>
---
 drivers/net/ethernet/sunplus/spl2sw_int.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/sunplus/spl2sw_int.c b/drivers/net/ethernet/sunplus/spl2sw_int.c
index a37c9a4c281f..7e07cd941434 100644
--- a/drivers/net/ethernet/sunplus/spl2sw_int.c
+++ b/drivers/net/ethernet/sunplus/spl2sw_int.c
@@ -147,7 +147,7 @@ int spl2sw_tx_poll(struct napi_struct *napi, int budget)
 	u32 cmd;
 	int i;
 
-	spin_lock(&comm->tx_lock);
+	spin_lock_irqsave(&comm->tx_lock, flags);
 
 	tx_done_pos = comm->tx_done_pos;
 	while (((tx_done_pos != comm->tx_pos) || (comm->tx_desc_full == 1)) && budget_left) {
@@ -196,7 +196,7 @@ int spl2sw_tx_poll(struct napi_struct *napi, int budget)
 				if (netif_queue_stopped(comm->ndev[i]))
 					netif_wake_queue(comm->ndev[i]);
 
-	spin_unlock(&comm->tx_lock);
+	spin_unlock_irqrestore(&comm->tx_lock, flags);
 
 	spin_lock_irqsave(&comm->int_mask_lock, flags);
 	mask = readl(comm->l2sw_reg_base + L2SW_SW_INT_MASK_0);
-- 
2.53.0

Re: [PATCH] net: sunplus: fix tx_poll spin_lock missing irqsave in NAPI context

[Jakub Kicinski]

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: sunplus: fix tx_poll spin_lock missing irqsave in NAPI context

This converts the comm->tx_lock acquisition in spl2sw_tx_poll() from
spin_lock()/spin_unlock() to spin_lock_irqsave()/spin_unlock_irqrestore(),
reusing the existing flags variable. The stated reason is a possible
same-CPU deadlock against the hardirq handler.

> diff --git a/drivers/net/ethernet/sunplus/spl2sw_int.c b/drivers/net/ethernet/sunplus/spl2sw_int.c
> index a37c9a4c281f0..7e07cd9414347 100644
> --- a/drivers/net/ethernet/sunplus/spl2sw_int.c
> +++ b/drivers/net/ethernet/sunplus/spl2sw_int.c

>     spl2sw_tx_poll() runs in NAPI (softirq) context and acquires
>     comm->tx_lock with plain spin_lock(). The hardirq handler
>     spl2sw_ethernet_interrupt() acquires the same lock, so a hardirq on
>     the same CPU while the NAPI poll holds the lock would deadlock with
>     IRQs disabled. Use spin_lock_irqsave/spin_unlock_irqrestore.

[Severity: Medium]
Is the claim that "The hardirq handler spl2sw_ethernet_interrupt() acquires
the same lock" accurate?

Looking at spl2sw_ethernet_interrupt() in
drivers/net/ethernet/sunplus/spl2sw_int.c, every acquisition there is of
comm->int_mask_lock, not comm->tx_lock:

	if (status & MAC_INT_TX) {
		/* Disable TX interrupts. */
		spin_lock(&comm->int_mask_lock);
		mask = readl(comm->l2sw_reg_base + L2SW_SW_INT_MASK_0);
		mask |= MAC_INT_TX;
		writel(mask, comm->l2sw_reg_base + L2SW_SW_INT_MASK_0);
		spin_unlock(&comm->int_mask_lock);
		...
			napi_schedule(&comm->tx_napi);
	}

The other acquirers of comm->tx_lock are spl2sw_ethernet_start_xmit() and
spl2sw_ethernet_tx_timeout() in drivers/net/ethernet/sunplus/spl2sw_driver.c,
both in non-hardirq context and both already using spin_lock_irqsave().
With spl2sw_tx_poll() that appears to be the complete set of acquirers, and
none of them run in hardirq context.

Does that mean the plain spin_lock() in the softirq-only holder was already
correct, and the described same-CPU self-deadlock cannot occur?

The declaration in drivers/net/ethernet/sunplus/spl2sw_define.h also seems to
draw the distinction between the two locks:

	spinlock_t tx_lock;		/* spinlock for accessing tx buffer */

> @@ -147,7 +147,7 @@ int spl2sw_tx_poll(struct napi_struct *napi, int budget)
>  	u32 cmd;
>  	int i;
>  
> -	spin_lock(&comm->tx_lock);
> +	spin_lock_irqsave(&comm->tx_lock, flags);
>  
>  	tx_done_pos = comm->tx_done_pos;
>  	while (((tx_done_pos != comm->tx_pos) || (comm->tx_desc_full == 1)) && budget_left) {

[Severity: Medium]
With no hardirq contender for comm->tx_lock, does this change simply extend
the hard-IRQ-disabled window over the whole TX reclaim path?

The section now covered with interrupts off in spl2sw_tx_poll() is up to a
full NAPI budget of descriptors, each doing:

		dma_unmap_single(&comm->pdev->dev, skbinfo->mapping, skbinfo->len,
				 DMA_TO_DEVICE);
		skbinfo->mapping = 0;
		dev_kfree_skb_irq(skbinfo->skb);

followed by the netif_wake_queue() loop over MAX_NETDEV_NUM devices.

If the intent is only to make the tx_lock users look consistent with
spl2sw_ethernet_start_xmit() and spl2sw_ethernet_tx_timeout(), could the
changelog say that and drop the deadlock description, along with the "fix"
in the subject?

> -	spin_unlock(&comm->tx_lock);
> +	spin_unlock_irqrestore(&comm->tx_lock, flags);
>  
>  	spin_lock_irqsave(&comm->int_mask_lock, flags);
>  	mask = readl(comm->l2sw_reg_base + L2SW_SW_INT_MASK_0);

[Severity: Low]
One note on tagging: a patch described as fixing an IRQs-disabled deadlock
would normally carry a Fixes: tag, and git blame shows the plain
spin_lock(&comm->tx_lock) here came in with fd3040b9394c ("net: ethernet:
Add driver for Sunplus SP7021").

Given that spl2sw_ethernet_interrupt() only ever takes comm->int_mask_lock,
would such a tag point at a bug that was never there? Rewording the
changelog as a cleanup instead of a fix seems like the way to avoid the
question entirely.

Re: [PATCH] net: sunplus: fix tx_poll spin_lock missing irqsave in NAPI context

[Jakub Kicinski]

On Thu, 20 Aug 2026 15:01:59 +0200 Andrew Gaylard wrote:
> spl2sw_tx_poll() runs in NAPI (softirq) context and acquires
> comm->tx_lock with plain spin_lock(). The hardirq handler
> spl2sw_ethernet_interrupt() acquires the same lock, so a hardirq on
> the same CPU while the NAPI poll holds the lock would deadlock with
> IRQs disabled. Use spin_lock_irqsave/spin_unlock_irqrestore.

Please explain how the issue was found and how the fix was tested.
I mean - this is an ancient driver, if you don't have a way to test the
fix you should not be sending the patch.
-- 
pw-bot: cr

Re: [PATCH] net: sunplus: fix tx_poll spin_lock missing irqsave in NAPI context

[Andrew Gaylard]

Jakub Kicinski <kuba@kernel.org> writes:

> On Thu, 20 Aug 2026 15:01:59 +0200 Andrew Gaylard wrote:
>> spl2sw_tx_poll() runs in NAPI (softirq) context and acquires
>> comm->tx_lock with plain spin_lock(). The hardirq handler
>> spl2sw_ethernet_interrupt() acquires the same lock, so a hardirq on
>> the same CPU while the NAPI poll holds the lock would deadlock with
>> IRQs disabled. Use spin_lock_irqsave/spin_unlock_irqrestore.
>
> Please explain how the issue was found and how the fix was tested.
> I mean - this is an ancient driver, if you don't have a way to test the
> fix you should not be sending the patch.

I'm testing on a Tibbo LTTP3G2 board:
https://tibbo.com/store/tps/ltpp3g2.html

This work is part of getting it to boot, getting it stable, adding a
couple of missing drivers, and sending the changes upstream.

I found the issue using lockdep. At least, I though I did, but I've not
been able to reproduce it, so I must have been mistaken.

Apologies.  Please drop this patch.

-- 
Andrew