Skip to contentSkip to conversation

tg3 ring buffer allocation error handling

PATCH In review Matt Mackall 2 participants 4 messages Apr 14, 2004

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

Matt Mackall

author
#0 lore raw

PATCH 1/1 tg3 ring buffer allocation error handling

tg3 - handle ring buffer allocation failure

Try to allocate rx ring buffer and properly clean up and exit on failure.

 tiny-mpm/drivers/net/tg3.c |   21 ++++++++++++++-------
 1 files changed, 14 insertions(+), 7 deletions(-)
drivers/net/tg3.c renamed from drivers/net/tg3.c~tg3-oops +14 -7
diff -puN drivers/net/tg3.c~tg3-oops drivers/net/tg3.c
@@ -3092,7 +3092,7 @@ static void tg3_free_rings(struct tg3 *t
  * end up in the driver.  tp->{tx,}lock are held and thus
  * we may not sleep.
  */
-static void tg3_init_rings(struct tg3 *tp)
+static int tg3_init_rings(struct tg3 *tp)
 {
 	unsigned long start, end;
 	u32 i;

@@ -3151,18 +3151,23 @@ static void tg3_init_rings(struct tg3 *t
 
 	/* Now allocate fresh SKBs for each rx ring. */
 	for (i = 0; i < tp->rx_pending; i++) {
-		if (tg3_alloc_rx_skb(tp, RXD_OPAQUE_RING_STD,
-				     -1, i) < 0)
-			break;
+		if (tg3_alloc_rx_skb(tp, RXD_OPAQUE_RING_STD, -1, i) < 0) {
+			printk("tg3_alloc_rx_skb %d failed\n", i);
+			return -ENOMEM;
+		}
 	}
 
 	if (tp->tg3_flags & TG3_FLAG_JUMBO_ENABLE) {
 		for (i = 0; i < tp->rx_jumbo_pending; i++) {
 			if (tg3_alloc_rx_skb(tp, RXD_OPAQUE_RING_JUMBO,
-					     -1, i) < 0)
-				break;
+					     -1, i) < 0) {
+				printk("tg3_alloc_rx_skb %d failed\n", i);
+				return -ENOMEM;
+			}
 		}
 	}
+
+	return 0;
 }
 
 /*

@@ -4557,7 +4562,9 @@ static int tg3_reset_hw(struct tg3 *tp)
 	 * can only do this after the hardware has been
 	 * successfully reset.
 	 */
-	tg3_init_rings(tp);
+	err = tg3_init_rings(tp);
+	if (err)
+		return err;
 
 	/* This value is determined during the probe time DMA
 	 * engine test, tg3_test_dma.

_

Signature · 1 line
Matt Mackall : http://www.selenic.com : Linux development and consulting
#1 lore raw

On Wed, Apr 14, 2004 at 01:46:42PM -0500, Matt Mackall told us:

drivers/net/tg3.c static void tg3_init_rings(struct tg3 *t · line 3165

Quoted 8 lines
drivers/net/tg3.c renamed from drivers/net/tg3.c~tg3-oops +3 -2
 	if (tp->tg3_flags & TG3_FLAG_JUMBO_ENABLE) {
 		for (i = 0; i < tp->rx_jumbo_pending; i++) {
 			if (tg3_alloc_rx_skb(tp, RXD_OPAQUE_RING_JUMBO,
-					     -1, i) < 0)
-				break;
+					     -1, i) < 0) {
+				printk("tg3_alloc_rx_skb %d failed\n", i);
+				return -ENOMEM;

Please correct me if I'm wrong, but shouldn't the buffers from the irst allocation (RXD_OPAQUE_RING_STD) be freed (maybe via tg3_free_rings??) when we get an error here??

Sven

Quoted 7 lines
drivers/net/tg3.c renamed from drivers/net/tg3.c~tg3-oops +3 -0
+			}
 		}
 	}
+
+	return 0;
 }
Signature · 2 lines
Linux zion 2.6.5 #1 Sun Apr 4 19:56:55 CEST 2004 i686 athlon i386 GNU/Linux
 22:53:42  up 9 days, 23:37,  2 users,  load average: 0.00, 0.00, 0.00
author
#2 lore raw
Quoted 13 lines · Sven Schuster

On Wed, Apr 14, 2004 at 01:46:42PM -0500, Matt Mackall told us:

drivers/net/tg3.c static void tg3_init_rings(struct tg3 *t · line 3165

Quoted 8 lines
drivers/net/tg3.c renamed from drivers/net/tg3.c~tg3-oops +3 -2
 	if (tp->tg3_flags & TG3_FLAG_JUMBO_ENABLE) {
 		for (i = 0; i < tp->rx_jumbo_pending; i++) {
 			if (tg3_alloc_rx_skb(tp, RXD_OPAQUE_RING_JUMBO,
-					     -1, i) < 0)
-				break;
+					     -1, i) < 0) {
+				printk("tg3_alloc_rx_skb %d failed\n", i);
+				return -ENOMEM;

Please correct me if I'm wrong, but shouldn't the buffers from the irst allocation (RXD_OPAQUE_RING_STD) be freed (maybe via tg3_free_rings??) when we get an error here??

The buffers are freed in the module release path, which is invoked when we return failure here. Without this, we don't notice the allocation failure and instead blow up when we get traffic.

FYI, I last tested this on a mem=4M box, where it was very obvious that the 1M of ring buffers weren't leaking.

Signature · 1 line
Matt Mackall : http://www.selenic.com : Linux development and consulting
#3 lore raw

On Wed, Apr 14, 2004 at 04:45:10PM -0500, Matt Mackall told us:

Quoted 7 lines
Quoted 3 lines

Please correct me if I'm wrong, but shouldn't the buffers from the irst allocation (RXD_OPAQUE_RING_STD) be freed (maybe via tg3_free_rings??) when we get an error here??

The buffers are freed in the module release path, which is invoked when we return failure here. Without this, we don't notice the allocation failure and instead blow up when we get traffic.

So thanks for correcting me :-) should have taken a closer look at the tg3 source.

Sven

Quoted 6 lines

FYI, I last tested this on a mem=4M box, where it was very obvious that the 1M of ring buffers weren't leaking.

Signature · 1 line
Matt Mackall : http://www.selenic.com : Linux development and consulting
Signature · 2 lines
Linux zion 2.6.5 #1 Sun Apr 4 19:56:55 CEST 2004 i686 athlon i386 GNU/Linux
 00:01:49  up 10 days, 45 min,  2 users,  load average: 0.06, 0.05, 0.02