Re: [PATCH] net: macb: Fix tx_ptr_lock locking

[Sean Anderson <sean.anderson@linux.dev>]

On 8/28/25 12:00, Sean Anderson wrote:
> macb_start_xmit can be called with bottom-halves disabled (e.g.
> transmitting from softirqs) as well as with interrupts disabled (with
> netpoll). Because of this, all other functions taking tx_ptr_lock must
> disable IRQs, and macb_start_xmit must only re-enable IRQs if they
> were already enabled.
> 
> Fixes: 138badbc21a0 ("net: macb: use NAPI for TX completion path")
> Reported-by: Mike Galbraith <efault@gmx.de>
> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
> ---
> 
>  drivers/net/ethernet/cadence/macb_main.c | 25 ++++++++++++------------
>  1 file changed, 13 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 16d28a8b3b56..b0a8dfa341ea 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -1228,7 +1228,7 @@ static int macb_tx_complete(struct macb_queue *queue, int budget)
>  	int packets = 0;
>  	u32 bytes = 0;
>  
> -	spin_lock(&queue->tx_ptr_lock);
> +	spin_lock_irq(&queue->tx_ptr_lock);
>  	head = queue->tx_head;
>  	for (tail = queue->tx_tail; tail != head && packets < budget; tail++) {
>  		struct macb_tx_skb	*tx_skb;
> @@ -1291,7 +1291,7 @@ static int macb_tx_complete(struct macb_queue *queue, int budget)
>  	    CIRC_CNT(queue->tx_head, queue->tx_tail,
>  		     bp->tx_ring_size) <= MACB_TX_WAKEUP_THRESH(bp))
>  		netif_wake_subqueue(bp->dev, queue_index);
> -	spin_unlock(&queue->tx_ptr_lock);
> +	spin_unlock_irq(&queue->tx_ptr_lock);
>  
>  	return packets;
>  }
> @@ -1708,7 +1708,7 @@ static void macb_tx_restart(struct macb_queue *queue)
>  	struct macb *bp = queue->bp;
>  	unsigned int head_idx, tbqp;
>  
> -	spin_lock(&queue->tx_ptr_lock);
> +	spin_lock_irq(&queue->tx_ptr_lock);
>  
>  	if (queue->tx_head == queue->tx_tail)
>  		goto out_tx_ptr_unlock;
> @@ -1720,19 +1720,19 @@ static void macb_tx_restart(struct macb_queue *queue)
>  	if (tbqp == head_idx)
>  		goto out_tx_ptr_unlock;
>  
> -	spin_lock_irq(&bp->lock);
> +	spin_lock(&bp->lock);
>  	macb_writel(bp, NCR, macb_readl(bp, NCR) | MACB_BIT(TSTART));
> -	spin_unlock_irq(&bp->lock);
> +	spin_unlock(&bp->lock);
>  
>  out_tx_ptr_unlock:
> -	spin_unlock(&queue->tx_ptr_lock);
> +	spin_unlock_irq(&queue->tx_ptr_lock);
>  }
>  
>  static bool macb_tx_complete_pending(struct macb_queue *queue)
>  {
>  	bool retval = false;
>  
> -	spin_lock(&queue->tx_ptr_lock);
> +	spin_lock_irq(&queue->tx_ptr_lock);
>  	if (queue->tx_head != queue->tx_tail) {
>  		/* Make hw descriptor updates visible to CPU */
>  		rmb();
> @@ -1740,7 +1740,7 @@ static bool macb_tx_complete_pending(struct macb_queue *queue)
>  		if (macb_tx_desc(queue, queue->tx_tail)->ctrl & MACB_BIT(TX_USED))
>  			retval = true;
>  	}
> -	spin_unlock(&queue->tx_ptr_lock);
> +	spin_unlock_irq(&queue->tx_ptr_lock);
>  	return retval;
>  }
>  
> @@ -2308,6 +2308,7 @@ static netdev_tx_t macb_start_xmit(struct sk_buff *skb, struct net_device *dev)
>  	struct macb_queue *queue = &bp->queues[queue_index];
>  	unsigned int desc_cnt, nr_frags, frag_size, f;
>  	unsigned int hdrlen;
> +	unsigned long flags;
>  	bool is_lso;
>  	netdev_tx_t ret = NETDEV_TX_OK;
>  
> @@ -2368,7 +2369,7 @@ static netdev_tx_t macb_start_xmit(struct sk_buff *skb, struct net_device *dev)
>  		desc_cnt += DIV_ROUND_UP(frag_size, bp->max_tx_length);
>  	}
>  
> -	spin_lock_bh(&queue->tx_ptr_lock);
> +	spin_lock_irqsave(&queue->tx_ptr_lock, flags);
>  
>  	/* This is a hard error, log it. */
>  	if (CIRC_SPACE(queue->tx_head, queue->tx_tail,
> @@ -2392,15 +2393,15 @@ static netdev_tx_t macb_start_xmit(struct sk_buff *skb, struct net_device *dev)
>  	netdev_tx_sent_queue(netdev_get_tx_queue(bp->dev, queue_index),
>  			     skb->len);
>  
> -	spin_lock_irq(&bp->lock);
> +	spin_lock(&bp->lock);
>  	macb_writel(bp, NCR, macb_readl(bp, NCR) | MACB_BIT(TSTART));
> -	spin_unlock_irq(&bp->lock);
> +	spin_unlock(&bp->lock);
>  
>  	if (CIRC_SPACE(queue->tx_head, queue->tx_tail, bp->tx_ring_size) < 1)
>  		netif_stop_subqueue(dev, queue_index);
>  
>  unlock:
> -	spin_unlock_bh(&queue->tx_ptr_lock);
> +	spin_unlock_irqrestore(&queue->tx_ptr_lock, flags);
>  
>  	return ret;
>  }

Sorry, this should be [PATCH net].

--Sean