Skip to contentSkip to conversation

niu: panic on reset

PATCH In review [email protected] 3 participants 4 messages Sep 4, 2008 – Sep 10, 2008

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 niu: panic on reset

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.

---
drivers/net/niu.c | 16 ++++++++++++++++

1 files changed, 16 insertions(+), 0 deletions(-)
drivers/net/niu.c +16 -0
index e4765b7..422d7e5 100644
@@ -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

maintainer
#1 lore raw

From: [email protected]
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. ]

Quoted 5 lines

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.

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.

#2 lore raw
Quoted 14 lines · David Miller

From: [email protected]
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. ]

Quoted 4 lines

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.

Quoted 8 lines
Quoted 3 lines

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

Quoted 9 lines

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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

maintainer
#3 lore raw

From: Matheos Worku <[email protected]>
Date: Wed, 10 Sep 2008 10:01:42 -0700

Quoted 4 lines

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.