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

[Robert Hancock <robert.hancock@calian.com>]

On Thu, 2025-08-28 at 12:00 -0400, 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);
> 

Hm, I think I used a non-IRQ lock here to avoid potentially disabling
interrupts for so long during TX completion processing. I don't think I
considered the netpoll case where start_xmit can be called with IRQs
disabled however. Not sure if there is a better solution to satisfy
that case without turning IRQs off entirely here?

>         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;
>  }
> --
> 2.35.1.1320.gc452695387.dirty
>