Skip to contentSkip to conversation

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

PATCH In review Steven J. Hill 2 participants 2 messages Jul 19, 2007

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

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

Fix long standing panic with regards to descriptors and locking.

---

drivers/net/mv643xx_eth.c +14 -9
diff -ur linux-2.6.22.1/drivers/net/mv643xx_eth.c linux-2.6.22.1-rci/drivers/net/mv643xx_eth.c
@@ -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;
#1 lore raw
Quoted 9 lines · Steven J. Hill

Fix long standing panic with regards to descriptors and locking.

---

diff -ur linux-2.6.22.1/drivers/net/mv643xx_eth.c linux-2.6.22.1-rci/drivers/net/mv643xx_eth.c

drivers/net/mv643xx_eth.c line 359

Quoted 10 lines
drivers/net/mv643xx_eth.c +0 -7
@@ -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.

drivers/net/mv643xx_eth.c line 364

Quoted 7 lines
drivers/net/mv643xx_eth.c +1 -1
@@ -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.

Quoted 7 lines
drivers/net/mv643xx_eth.c +2 -1
@@ -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) {
+		/*

drivers/net/mv643xx_eth.c line 1495

Quoted 10 lines
drivers/net/mv643xx_eth.c +10 -0
+		 * 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