[PATCH] Merge GT/MV642xx Support into MV643xx Driver [8/8]

[PATCH] Merge GT/MV642xx Support into MV643xx Driver [8/8]

[Steven J. Hill]

[-- Attachment #1.1: Type: text/plain, Size: 134 bytes --]

Fix long standing panic with regards to descriptors and locking.

Signed-off-by: Steven J. Hill <sjhill1@rockwellcollins.com>
---

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: 08-mv643xx-panic-lock-fix.patch --]
[-- Type: text/x-patch; name="08-mv643xx-panic-lock-fix.patch", Size: 1717 bytes --]

diff -ur linux-2.6.22.1/drivers/net/mv643xx_eth.c linux-2.6.22.1-rci/drivers/net/mv643xx_eth.c
--- linux-2.6.22.1/drivers/net/mv643xx_eth.c	2007-07-18 22:55:11.000000000 -0500
+++ linux-2.6.22.1-rci/drivers/net/mv643xx_eth.c	2007-07-18 22:39:37.000000000 -0500
@@ -350,13 +350,6 @@
 
 	while (mp->tx_desc_count > 0) {
 		spin_lock_irqsave(&mp->lock, flags);
-
-		/* tx_desc_count might have changed before acquiring the lock */
-		if (mp->tx_desc_count <= 0) {
-			spin_unlock_irqrestore(&mp->lock, flags);
-			return released;
-		}
-
 		tx_index = mp->tx_used_desc_q;
 		desc = &mp->p_tx_desc_area[tx_index];
 #ifdef CONFIG_GT64260
@@ -367,7 +360,7 @@
 
 		if (!force && (cmd_sts & ETH_BUFFER_OWNED_BY_DMA)) {
 			spin_unlock_irqrestore(&mp->lock, flags);
-			return released;
+			continue;
 		}
 
 		mp->tx_used_desc_q = (tx_index + 1) % mp->tx_ring_size;
@@ -1488,7 +1481,19 @@
 	BUG_ON(netif_queue_stopped(dev));
 	BUG_ON(skb == NULL);
 
-	if (mp->tx_ring_size - mp->tx_desc_count < MAX_DESCS_PER_SKB) {
+	if (mp->tx_ring_size - mp->tx_desc_count <= MAX_DESCS_PER_SKB) {
+		/*
+		 * We are completely out of TX descriptors, however,
+		 * if 'tx_used_desc_q' is zero, most likely the port
+		 * has been configured and is up, but there is no link.
+		 * We attempt to free a single descriptor to keep the
+		 * 'sendto' call and rest of the stack happy. If this
+		 * check is taken out, expect an error and kernel panic
+		 * from 'kernel/softirq.c:141' inside 'local_bh_enable'.
+		 */
+		if (mp->tx_used_desc_q == 0)
+			mv643xx_eth_free_all_tx_descs(dev);
+
 		printk(KERN_ERR "%s: transmit with queue full\n", dev->name);
 		netif_stop_queue(dev);
 		return 1;

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 254 bytes --]

Re: [PATCH] Merge GT/MV642xx Support into MV643xx Driver [8/8]

[Dale Farnsworth]

On Thu, Jul 19, 2007 at 05:02:04AM +0000, Steven J. Hill wrote:
> Fix long standing panic with regards to descriptors and locking.
> 
> Signed-off-by: Steven J. Hill <sjhill1@rockwellcollins.com>
> ---
> 
> diff -ur linux-2.6.22.1/drivers/net/mv643xx_eth.c linux-2.6.22.1-rci/drivers/net/mv643xx_eth.c
> --- linux-2.6.22.1/drivers/net/mv643xx_eth.c	2007-07-18 22:55:11.000000000 -0500
> +++ linux-2.6.22.1-rci/drivers/net/mv643xx_eth.c	2007-07-18 22:39:37.000000000 -0500
> @@ -350,13 +350,6 @@
>  
>  	while (mp->tx_desc_count > 0) {
>  		spin_lock_irqsave(&mp->lock, flags);
> -
> -		/* tx_desc_count might have changed before acquiring the lock */
> -		if (mp->tx_desc_count <= 0) {
> -			spin_unlock_irqrestore(&mp->lock, flags);
> -			return released;
> -		}
> -

The code you remove above is needed to protect against a nasty bug.
It was added in January and needs to stay in.

> @@ -367,7 +360,7 @@
>  
>  		if (!force && (cmd_sts & ETH_BUFFER_OWNED_BY_DMA)) {
>  			spin_unlock_irqrestore(&mp->lock, flags);
> -			return released;
> +			continue;
>  		}

No.  Continuing at that point results in the driver spinning, wasting
cpu while polling for the hardware to release the DMA descriptor.

> @@ -1488,7 +1481,19 @@
>  	BUG_ON(netif_queue_stopped(dev));
>  	BUG_ON(skb == NULL);
>  
> -	if (mp->tx_ring_size - mp->tx_desc_count < MAX_DESCS_PER_SKB) {
> +	if (mp->tx_ring_size - mp->tx_desc_count <= MAX_DESCS_PER_SKB) {
> +		/*
> +		 * We are completely out of TX descriptors, however,
> +		 * if 'tx_used_desc_q' is zero, most likely the port
> +		 * has been configured and is up, but there is no link.
> +		 * We attempt to free a single descriptor to keep the
> +		 * 'sendto' call and rest of the stack happy. If this
> +		 * check is taken out, expect an error and kernel panic
> +		 * from 'kernel/softirq.c:141' inside 'local_bh_enable'.
> +		 */
> +		if (mp->tx_used_desc_q == 0)
> +			mv643xx_eth_free_all_tx_descs(dev);

I don't understand the bug, but regardless, we need a better fix.
Freeing all the descriptors when the HW xmit queue is full is not
a solution.

-Dale