Re: [PATCH net v2 01/12] net/sonic: Add mutual exclusion for accessing shared state

[Eric Dumazet]

On 1/21/20 1:22 PM, Finn Thain wrote:
> The netif_stop_queue() call in sonic_send_packet() races with the
> netif_wake_queue() call in sonic_interrupt(). This causes issues
> like "NETDEV WATCHDOG: eth0 (macsonic): transmit queue 0 timed out".
> Fix this by disabling interrupts when accessing tx_skb[] and next_tx.
> Update a comment to clarify the synchronization properties.
> 
> Fixes: efcce839360f ("[PATCH] macsonic/jazzsonic network drivers update")
> Tested-by: Stan Johnson <userm57@xxxxxxxxx>
> Signed-off-by: Finn Thain <fthain@xxxxxxxxxxxxxxxxxxx>

> @@ -284,9 +287,16 @@ static irqreturn_t sonic_interrupt(int irq, void *dev_id)
>  	struct net_device *dev = dev_id;
>  	struct sonic_local *lp = netdev_priv(dev);
>  	int status;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&lp->lock, flags);


This is a hard irq handler, no need to block hard irqs.

spin_lock() here is enough.

> +
> +	status = SONIC_READ(SONIC_ISR) & SONIC_IMR_DEFAULT;
> +	if (!status) {
> +		spin_unlock_irqrestore(&lp->lock, flags);
>  
> -	if (!(status = SONIC_READ(SONIC_ISR) & SONIC_IMR_DEFAULT))
>  		return IRQ_NONE;
> +	}
>  
>  	do {
>  		if (status & SONIC_INT_PKTRX) {
> @@ -300,11 +310,12 @@ static irqreturn_t sonic_interrupt(int irq, void *dev_id)
>  			int td_status;
>  			int freed_some = 0;
>  
> -			/* At this point, cur_tx is the index of a TD that is one of:
> -			 *   unallocated/freed                          (status set   & tx_skb[entry] clear)
> -			 *   allocated and sent                         (status set   & tx_skb[entry] set  )
> -			 *   allocated and not yet sent                 (status clear & tx_skb[entry] set  )
> -			 *   still being allocated by sonic_send_packet (status clear & tx_skb[entry] clear)
> +			/* The state of a Transmit Descriptor may be inferred
> +			 * from { tx_skb[entry], td_status } as follows.
> +			 * { clear, clear } => the TD has never been used
> +			 * { set,   clear } => the TD was handed to SONIC
> +			 * { set,   set   } => the TD was handed back
> +			 * { clear, set   } => the TD is available for re-use
>  			 */
>  
>  			netif_dbg(lp, intr, dev, "%s: tx done\n", __func__);
> @@ -406,7 +417,12 @@ static irqreturn_t sonic_interrupt(int irq, void *dev_id)
>  		/* load CAM done */
>  		if (status & SONIC_INT_LCD)
>  			SONIC_WRITE(SONIC_ISR, SONIC_INT_LCD); /* clear the interrupt */
> -	} while((status = SONIC_READ(SONIC_ISR) & SONIC_IMR_DEFAULT));
> +
> +		status = SONIC_READ(SONIC_ISR) & SONIC_IMR_DEFAULT;
> +	} while (status);
> +
> +	spin_unlock_irqrestore(&lp->lock, flags);
> +
>  	return IRQ_HANDLED;