[PATCH 1/1] niu: panic on reset

[PATCH 1/1] niu: panic on reset

[Santwona.Behera]

The reset_task function in the niu driver does not reset the hardware 
fully. The buffer addresses and pointers are not being reset/updated in 
the hardware and that leads to panic on reset.

Signed-off-by: santwona.behera@sun.com <santwona.behera@sun.com>
---
 drivers/net/niu.c |   16 ++++++++++++++++
 1 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/drivers/net/niu.c b/drivers/net/niu.c
index e4765b7..422d7e5 100644
--- a/drivers/net/niu.c
+++ b/drivers/net/niu.c
@@ -6006,11 +6006,27 @@ static void niu_reset_task(struct work_struct *work)

        niu_stop_hw(np);

+       spin_unlock_irqrestore(&np->lock, flags);
+
+       niu_free_channels(np);
+
+       err = niu_alloc_channels(np);
+       if (err) {
+               dev_err(np->device, PFX "%s:Failed to alloc niu channels\n",
+                       np->dev->name);
+               return;
+       }
+
+       spin_lock_irqsave(&np->lock, flags);
+
        err = niu_init_hw(np);
        if (!err) {
                np->timer.expires = jiffies + HZ;
                add_timer(&np->timer);
                niu_netif_start(np);
+       } else {
+               dev_err(np->device, PFX "%s:Failed to init hw\n",
+                       np->dev->name);
        }

        spin_unlock_irqrestore(&np->lock, flags);
--
1.5.2

Re: [PATCH 1/1] niu: panic on reset

[David Miller]

From: Santwona.Behera@Sun.COM
Date: Thu, 04 Sep 2008 16:55:16 -0700

[ Please learn how to hit the return key every 80 characters or so,
  long lines are very difficult to read on text-only email clients
  like the one I use.  Expecting auto-formatting and other beautifications
  in email readers amongst kernel developers is a very bad idea. ]

> The reset_task function in the niu driver does not reset the
> hardware fully. The buffer addresses and pointers are not being
> reset/updated in the hardware and that leads to panic on reset.
>
> Signed-off-by: santwona.behera@sun.com <santwona.behera@sun.com>

This is way overkill just to fix this bug and it adds a new failure
mode for the driver which is a seriously negative aspect to this fix.

Instead, simply write a function which reloads the buffers addresses
properly instead of calling the functions to free and reallocate all
of these dynamic datastructures just to get those side effects.

Re: [PATCH 1/1] niu: panic on reset

[Matheos Worku]

David Miller wrote:

>From: Santwona.Behera@Sun.COM
>Date: Thu, 04 Sep 2008 16:55:16 -0700
>
>[ Please learn how to hit the return key every 80 characters or so,
>  long lines are very difficult to read on text-only email clients
>  like the one I use.  Expecting auto-formatting and other beautifications
>  in email readers amongst kernel developers is a very bad idea. ]
>
>  
>
>>The reset_task function in the niu driver does not reset the
>>hardware fully. The buffer addresses and pointers are not being
>>reset/updated in the hardware and that leads to panic on reset.
>>
>>Signed-off-by: santwona.behera@sun.com <santwona.behera@sun.com>
>>    
>>
>
>This is way overkill just to fix this bug and it adds a new failure
>mode for the driver which is a seriously negative aspect to this fix.
>  
>
Dave,

Thanks for the feedback.
Instead of creating a new function, Santwona was borrowing an existing
logic from niu_change_mtu function. Rather than introducing a new
function and possibly a new failure mode, we were trying to re-use existing
logic, reducing the probability of a new bug.

Regards,
Matheos

>Instead, simply write a function which reloads the buffers addresses
>properly instead of calling the functions to free and reallocate all
>of these dynamic datastructures just to get those side effects.
>--
>To unsubscribe from this list: send the line "unsubscribe netdev" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html
>  
>

Re: [PATCH 1/1] niu: panic on reset

[David Miller]

From: Matheos Worku <Matheos.Worku@Sun.COM>
Date: Wed, 10 Sep 2008 10:01:42 -0700

> Instead of creating a new function, Santwona was borrowing an existing
> logic from niu_change_mtu function. Rather than introducing a new
> function and possibly a new failure mode, we were trying to re-use existing
> logic, reducing the probability of a new bug.

But in essence the patch does add a new failure mode.  It now can fail
if a memory allocation cannot succeed at the current point in time.

That was not true previously, and if you just fix up the buffer pointers
or whatever state to be reset, you won't have this possible failure mode
either.

We should fix this correctly.