[patch] tg3 ring buffer allocation error handling

[patch] tg3 ring buffer allocation error handling

[Matt Mackall]

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(-)

diff -puN drivers/net/tg3.c~tg3-oops drivers/net/tg3.c
--- tiny/drivers/net/tg3.c~tg3-oops	2004-03-20 20:08:42.000000000 -0600
+++ tiny-mpm/drivers/net/tg3.c	2004-03-20 20:08:42.000000000 -0600
@@ -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.

_

-- 
Matt Mackall : http://www.selenic.com : Linux development and consulting

Re: [patch] tg3 ring buffer allocation error handling

[Sven Schuster]

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

On Wed, Apr 14, 2004 at 01:46:42PM -0500, Matt Mackall told us:
>  	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

> +			}
>  		}
>  	}
> +
> +	return 0;
>  }
>  

-- 
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

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [patch] tg3 ring buffer allocation error handling

[Matt Mackall]

On Wed, Apr 14, 2004 at 10:59:59PM +0200, Sven Schuster wrote:
> On Wed, Apr 14, 2004 at 01:46:42PM -0500, Matt Mackall told us:
> >  	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.

-- 
Matt Mackall : http://www.selenic.com : Linux development and consulting

Re: [patch] tg3 ring buffer allocation error handling

[Sven Schuster]

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

On Wed, Apr 14, 2004 at 04:45:10PM -0500, Matt Mackall told us:
> > 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

> 
> FYI, I last tested this on a mem=4M box, where it was very obvious
> that the 1M of ring buffers weren't leaking.
> 
> -- 
> Matt Mackall : http://www.selenic.com : Linux development and consulting

-- 
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

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]