[PATCH] tg3: fix error handling in tg3_open

[PATCH] tg3: fix error handling in tg3_open

[Akinobu Mita]

If one of the request_irq calls fails in tg3_open, all interrupts
allocated with request_irq should be freed before returning error.

But it accidentally attempts to free the same invalid IRQ repeatedly.

Cc: "David S. Miller" <davem@davemloft.net>
Cc: Matt Carlson <mcarlson@broadcom.com>
Cc: Michael Chan <mchan@broadcom.com>
Cc: netdev@vger.kernel.org
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
 drivers/net/ethernet/broadcom/tg3.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
index 161cbbb..13c4054 100644
--- a/drivers/net/ethernet/broadcom/tg3.c
+++ b/drivers/net/ethernet/broadcom/tg3.c
@@ -9674,8 +9674,10 @@ static int tg3_open(struct net_device *dev)
 		struct tg3_napi *tnapi = &tp->napi[i];
 		err = tg3_request_irq(tp, i);
 		if (err) {
-			for (i--; i >= 0; i--)
+			for (i--; i >= 0; i--) {
+				tnapi = &tp->napi[i];
 				free_irq(tnapi->irq_vec, tnapi);
+			}
 			break;
 		}
 	}
-- 
1.7.4.4

Re: [PATCH] tg3: fix error handling in tg3_open

[Matt Carlson]

On Thu, Nov 03, 2011 at 03:36:43AM -0700, Akinobu Mita wrote:
> If one of the request_irq calls fails in tg3_open, all interrupts
> allocated with request_irq should be freed before returning error.
> 
> But it accidentally attempts to free the same invalid IRQ repeatedly.

Hi Akinobu.  I have a similar patch under internal review at this very
minute.  The patch I have does a minor additional cleanup.  (It moves
the 'goto err_out2' inside the 'if (err)' block.)  Is it O.K.
with you to add your SOB to my patch instead?

> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Matt Carlson <mcarlson@broadcom.com>
> Cc: Michael Chan <mchan@broadcom.com>
> Cc: netdev@vger.kernel.org
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> ---
>  drivers/net/ethernet/broadcom/tg3.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
> index 161cbbb..13c4054 100644
> --- a/drivers/net/ethernet/broadcom/tg3.c
> +++ b/drivers/net/ethernet/broadcom/tg3.c
> @@ -9674,8 +9674,10 @@ static int tg3_open(struct net_device *dev)
>  		struct tg3_napi *tnapi = &tp->napi[i];
>  		err = tg3_request_irq(tp, i);
>  		if (err) {
> -			for (i--; i >= 0; i--)
> +			for (i--; i >= 0; i--) {
> +				tnapi = &tp->napi[i];
>  				free_irq(tnapi->irq_vec, tnapi);
> +			}
>  			break;
>  		}
>  	}
> -- 
> 1.7.4.4
> 
> 

Re: [PATCH] tg3: fix error handling in tg3_open

[Akinobu Mita]

2011/11/4 Matt Carlson <mcarlson@broadcom.com>:
> On Thu, Nov 03, 2011 at 03:36:43AM -0700, Akinobu Mita wrote:
>> If one of the request_irq calls fails in tg3_open, all interrupts
>> allocated with request_irq should be freed before returning error.
>>
>> But it accidentally attempts to free the same invalid IRQ repeatedly.
>
> Hi Akinobu.  I have a similar patch under internal review at this very
> minute.  The patch I have does a minor additional cleanup.  (It moves
> the 'goto err_out2' inside the 'if (err)' block.)  Is it O.K.
> with you to add your SOB to my patch instead?

Sounds good.  Could you send the patch after finishing your review?