Re: sis900: eth0: NULL pointer encountered in Rx ring, skipping

[Eric Lemoine <eric.lemoine@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 1064 bytes --]

On Tue, 18 Jan 2005 11:21:39 +0100, Lennert Buytenhek
<buytenh@wantstofly.org> wrote:
> On Tue, Jan 18, 2005 at 11:13:58AM +0100, Lennert Buytenhek wrote:
> 
> > I can make one of my machines, which has an on-board sis900 NIC, lose
> > network connectivity within seconds by doing something like
> > "nc -l -p 6666 > /dev/null" and then blasting some data towards it.  When
> > this happens, syslog fills up with these messages:
> >
> >       eth0: NULL pointer encountered in Rx ring, skipping
> 
> Just before it starts spewing these it says:
> 
>         eth0: Memory squeeze,deferring packet.
> 
> So apparently it just doesn't deal with OOM very well.

There are dev_alloc_skb()'s failing due to memory shortage. I think
anyhow that the sis9000 driver doesnt handle "memory squeeze"
situations right. Why not throwing current rx packet away and reuse
ring entry instead of leaving holes (null skb's) in the ring. I would
suggest a patch to the sis900 driver around that attached.

Warning: patch compiling but untested - dont have hardware handy.  

-- 
Eric

[-- Attachment #2: sis900.patch --]
[-- Type: application/octet-stream, Size: 3038 bytes --]

===== drivers/net/sis900.c 1.56 vs edited =====
--- 1.56/drivers/net/sis900.c	2004-08-25 09:15:20 +02:00
+++ edited/drivers/net/sis900.c	2005-01-19 21:48:27 +01:00
@@ -1650,7 +1650,7 @@
 			/* reset buffer descriptor state */
 			sis_priv->rx_ring[entry].cmdsts = RX_BUF_SIZE;
 		} else {
-			struct sk_buff * skb;
+			struct sk_buff * skb, *new_skb;
 
 			/* This situation should never happen, but due to
 			   some unknow bugs, it is possible that
@@ -1662,11 +1662,30 @@
 				break;
 			}
 
+			skb = sis_priv->rx_skbuff[entry];
+
+			if ((new_skb = dev_alloc_skb(RX_BUF_SIZE)) == NULL) {
+				printk(KERN_INFO "%s: memory shortage, rx packet dropped\n",
+					net_dev->name);
+
+				sis_priv->rx_ring[entry].cmdsts = RX_BUF_SIZE;
+				sis_priv->stats.rx_dropped++;
+				sis_priv->dirty_rx++; /* dirty no longer needed? */
+				break; /* next */
+			}
+
 			pci_unmap_single(sis_priv->pci_dev, 
 				sis_priv->rx_ring[entry].bufptr, RX_BUF_SIZE, 
 				PCI_DMA_FROMDEVICE);
+
+			new_skb->dev = net_dev;
+			sis_priv->rx_skbuff[entry] = new_skb;
+			sis_priv->rx_ring[entry].cmdsts = RX_BUF_SIZE;
+                	sis_priv->rx_ring[entry].bufptr = 
+				pci_map_single(sis_priv->pci_dev, new_skb->tail, 
+					RX_BUF_SIZE, PCI_DMA_FROMDEVICE);
+
 			/* give the socket buffer to upper layers */
-			skb = sis_priv->rx_skbuff[entry];
 			skb_put(skb, rx_size);
 			skb->protocol = eth_type_trans(skb, net_dev);
 			netif_rx(skb);
@@ -1678,29 +1697,6 @@
 			sis_priv->stats.rx_bytes += rx_size;
 			sis_priv->stats.rx_packets++;
 
-			/* refill the Rx buffer, what if there is not enought memory for
-			   new socket buffer ?? */
-			if ((skb = dev_alloc_skb(RX_BUF_SIZE)) == NULL) {
-				/* not enough memory for skbuff, this makes a "hole"
-				   on the buffer ring, it is not clear how the
-				   hardware will react to this kind of degenerated
-				   buffer */
-				printk(KERN_INFO "%s: Memory squeeze,"
-				       "deferring packet.\n",
-				       net_dev->name);
-				sis_priv->rx_skbuff[entry] = NULL;
-				/* reset buffer descriptor state */
-				sis_priv->rx_ring[entry].cmdsts = 0;
-				sis_priv->rx_ring[entry].bufptr = 0;
-				sis_priv->stats.rx_dropped++;
-				break;
-			}
-			skb->dev = net_dev;
-			sis_priv->rx_skbuff[entry] = skb;
-			sis_priv->rx_ring[entry].cmdsts = RX_BUF_SIZE;
-                	sis_priv->rx_ring[entry].bufptr = 
-				pci_map_single(sis_priv->pci_dev, skb->tail, 
-					RX_BUF_SIZE, PCI_DMA_FROMDEVICE);
 			sis_priv->dirty_rx++;
 		}
 		sis_priv->cur_rx++;
@@ -1708,6 +1704,7 @@
 		rx_status = sis_priv->rx_ring[entry].cmdsts;
 	} // while
 
+#if 0 /* no need to refill skbs are added to the rx ring as packets are received */
 	/* refill the Rx buffer, what if the rate of refilling is slower than 
 	   consuming ?? */
 	for (;sis_priv->cur_rx - sis_priv->dirty_rx > 0; sis_priv->dirty_rx++) {
@@ -1735,6 +1732,7 @@
 					RX_BUF_SIZE, PCI_DMA_FROMDEVICE);
 		}
 	}
+#endif
 	/* re-enable the potentially idle receive state matchine */
 	outl(RxENA | inl(ioaddr + cr), ioaddr + cr );