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

[Jakub Kicinski <kuba@kernel.org>]

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.