Re: [patch] Real-Time Preemption, -RT-2.6.12-rc6-V0.7.48-00

[William Weston]

On Tue, 21 Jun 2005, Ingo Molnar wrote:

> i'm not sure it's related to the lockup - but there is a generic bug in 
> the driver, which in ns83820_tx_timeout() does:
> 
> 	local_irq_save(flags);
> 	...
> 	CALL do_tx_done()
> 
> 		spin_lock_irq(&dev->tx_lock);
> 		...
> 		spin_unlock_irq(&dev->tx_lock); // [1]
> 	...
> 	local_irq_restore(flags); // [2]
> 
> this leads to interrupts being enabled at [1], while the intention was 
> to enable them at [2]. To solve this bug we can do the tx-locking in 
> ns83820_tx_timeout(). (local_irq_disable() use in ns83820_tx_timeout() 
> was probably SMP-unsafe too, but needs a rare race.)
> 
> find the patch below - it's also included in the -50-05 -RT tree i just 
> uploaded. Can you confirm that you dont get the warnings in the -50-05 
> (and later) -RT kernels?
> 
> 	Ingo

-48-37 still gave the warnings on ns83820_tx_timeout(), but otherwise ran
for a day without any lockups or loss of network or keyboard.  No warnings 
so far with -50-06.  I'll keep you posted.


--ww <weston@xxxxxxxxx>


> Index: drivers/net/ns83820.c
> ===================================================================
> --- drivers/net/ns83820.c.orig
> +++ drivers/net/ns83820.c
> @@ -1013,8 +1013,6 @@ static void do_tx_done(struct net_device
>  	struct ns83820 *dev = PRIV(ndev);
>  	u32 cmdsts, tx_done_idx, *desc;
>  
> -	spin_lock_irq(&dev->tx_lock);
> -
>  	dprintk("do_tx_done(%p)\n", ndev);
>  	tx_done_idx = dev->tx_done_idx;
>  	desc = dev->tx_descs + (tx_done_idx * DESC_SIZE);
> @@ -1070,7 +1068,6 @@ static void do_tx_done(struct net_device
>  		netif_start_queue(ndev);
>  		netif_wake_queue(ndev);
>  	}
> -	spin_unlock_irq(&dev->tx_lock);
>  }
>  
>  static void ns83820_cleanup_tx(struct ns83820 *dev)
> @@ -1371,7 +1368,9 @@ static void ns83820_do_isr(struct net_de
>  	 * work has accumulated
>  	 */
>  	if ((ISR_TXDESC | ISR_TXIDLE | ISR_TXOK | ISR_TXERR) & isr) {
> +		spin_lock_irq(&dev->tx_lock);
>  		do_tx_done(ndev);
> +		spin_unlock_irq(&dev->tx_lock);
>  
>  		/* Disable TxOk if there are no outstanding tx packets.
>  		 */
> @@ -1456,7 +1455,7 @@ static void ns83820_tx_timeout(struct ne
>          u32 tx_done_idx, *desc;
>  	unsigned long flags;
>  
> -	local_irq_save(flags);
> +	spin_lock_irqsave(&dev->tx_lock, flags);
>  
>  	tx_done_idx = dev->tx_done_idx;
>  	desc = dev->tx_descs + (tx_done_idx * DESC_SIZE);
> @@ -1483,7 +1482,7 @@ static void ns83820_tx_timeout(struct ne
>  		ndev->name,
>  		tx_done_idx, dev->tx_free_idx, le32_to_cpu(desc[DESC_CMDSTS]));
>  
> -	local_irq_restore(flags);
> +	spin_unlock_irqrestore(&dev->tx_lock, flags);
>  }
>  
> 
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/