Linus Torvalds wrote:
On Wed, 31 Jan 2007, Francois Romieu wrote:
Call chain:
-> rtl8169_init_one
-> register_netdev (dev_open starts to race...)
-> rtl8169_init_phy
-> rtl8169_set_speed
-> tp->set_speed
-> mod_timer(&tp->timer, ...) (if netif_running() is true)
As netif_running() is true just before dev->open() is issued and the
timer is initialized during dev->open, mod_timer() meets an uninitialized
tp->timer and oopses.
Doesn't this basically mean that *any* use of "rtl8169_set_speed()" is
buggy?
No, just the first use, after which the one-time initialization occurs.
Anyway, I'm going to wait for somebody smarter than me to ACK this patch.
Jeff?
I would rather have something more like the attached patch, which
initializes the timer with the rest of the private-struct
initialization. Just like most other net drivers do.
And Herbert Xu wrote:
Does rtl8169_init_phy need to occur after register_netdev? Normally
register_netdev should be the very last thing in a probe routine.
Quite correct.
So... anybody wanna test my patch (didn't compile it, but it looks
right) and confirm that it fixes things?
Jeff
diff --git a/drivers/net/r8169.c b/drivers/net/r8169.c
index 577babd..ce66b2a 100644
--- a/drivers/net/r8169.c
+++ b/drivers/net/r8169.c
@@ -1369,11 +1369,7 @@ static inline void rtl8169_request_timer(struct
net_device *dev)
(tp->phy_version >= RTL_GIGA_PHY_VER_H))
return;
- init_timer(timer);
- timer->expires = jiffies + RTL8169_PHY_TIMEOUT;
- timer->data = (unsigned long)(dev);
- timer->function = rtl8169_phy_timer;
- add_timer(timer);
+ mod_timer(timer, jiffies + RTL8169_PHY_TIMEOUT);
}
#ifdef CONFIG_NET_POLL_CONTROLLER
@@ -1686,6 +1682,10 @@ rtl8169_init_one(struct pci_dev *pdev, const struct
pci_device_id *ent)
tp->mmio_addr = ioaddr;
tp->align = rtl_cfg_info[ent->driver_data].align;
+ init_timer(&tp->timer);
+ tp->timer.data = (unsigned long)(dev);
+ tp->timer.function = rtl8169_phy_timer;
+
spin_lock_init(&tp->lock);
rc = register_netdev(dev);
