RFC: fix ethernet device initialization
PATCH 1/1 RFC: fix ethernet device initialization
People from time to time point out a wart in ethernet initialization:
The net_device is allocated and registered to the system in init_etherdev, which is usually one of the first things an ethernet driver probe function does. The net_device's final members are setup at some time between then and the exit of the probe function. There is never a clear point where the net device is available to the system for use.
Our API already supports a solution -- setup the device, then call register_netdev. The patch below adds a helper, alloc_etherdev, to eliminate duplicate code in drivers. Ethernet device initialization, after the patch, should now look like
dev = alloc_etherdev(sizeof(struct netdev_private)); ... initialize device ... ... set up net_device struct members ... rc = register_netdevice(dev); if (rc) /* handle error */ netif_start_queue(dev);
This makes the ethernet driver look and behave similar to other APIs in the kernel, and presents a nice and atomic present-this-netdev-to-the-system operation.
It should be noted that there is a net_device::init function. IIRC in the last discussion of this issue, ::init() was mentioned as a possible solution. I agree that init() can be used, but I do not like the idea of using init as a probe function, as the ISA drivers do it. This means the ethernet device has the potential of holdling rtnl_lock for a long time, and staying in the probe function, inside register_netdevice, for a long time. And... I agree that init() can be used as a constructor for filling in dev->xxx and dev->priv->xxx values, but I think doing so is pointless: you must fill in certain values before your "constructor" is called, just so your constructor has enough information to operate.
Patch description:
* Add alloc_etherdev, alloc_trdev, alloc_hippi_dev, alloc_fcdev, ...
* Use declarator macros to create init_etherdev, init_trdev, etc., and remove duplicate code.
* Move net_init EXPORT_SYMBOL from net/netsyms.c to net_init.c.
* Convert drivers/net/8139too.c to use alloc_etherdev, as an example.
Final word, mostly PCI drivers need this change. ISA drivers use the net_device::init constructor approach. IMHO they shouldn't stay in register_netdevice so long, but that is a wart. This patch fixes a bug, the huge span of time between init_etherdev and the indeterminant point in the future when the net device is ready for use.
Comments?
Signature · 3 lines
Jeff Garzik | "You see, in this world there's two kinds of Building 1024 | people, my friend: Those with loaded guns MandrakeSoft | and those who dig. You dig." --Blondie
Index: drivers/net/8139too.c
===================================================================
RCS file: /cvsroot/gkernel/linux_2_4/drivers/net/8139too.c,v retrieving revision 1.1.1.29
net/8139too.c +61 -45
@@ -647,11 +647,43 @@ (RX_DMA_BURST << RxCfgDMAShift); +static void __rtl8139_cleanup_dev (struct net_device *dev) +{ +struct rtl8139_private *tp; +struct pci_dev *pdev; + +assert (dev !=NULL ); +assert (dev->priv !=NULL ); + + tp = dev->priv; +assert (tp->pci_dev !=NULL ); + pdev = tp->pci_dev; + +#ifndef USE_IO_OPS +if (tp->mmio_addr) +iounmap (tp->mmio_addr); +#endif /* !USE_IO_OPS */ + +/* it's ok to call this even if we have no regions to free */ +pci_release_regions (pdev); + +#ifndef RTL8139_NDEBUG +/* poison memory before freeing */ +memset (dev,0xBC , +sizeof (struct net_device ) + +sizeof (struct rtl8139_private )); +#endif /* RTL8139_NDEBUG */ + +kfree (dev); + +pci_set_drvdata (pdev,NULL ); +} + +static int __devinit rtl8139_init_board (struct pci_dev *pdev, -struct net_device **dev_out, -void **ioaddr_out) +struct net_device **dev_out) { -void *ioaddr =NULL ; +void *ioaddr;struct net_device *dev;struct rtl8139_private *tp;u8 tmp8;
@@ -663,20 +695,19 @@DPRINTK ("ENTER\n" );assert (pdev !=NULL ); -assert (ioaddr_out !=NULL ); - *ioaddr_out =NULL ; *dev_out =NULL ; -/* dev zeroed in init_etherdev */ - dev =init_etherdev (NULL ,sizeof (*tp)); +/* dev and dev->priv zeroed in alloc_etherdev */ + dev =alloc_etherdev (sizeof (*tp));if (dev ==NULL ) { - printk (KERN_ERR PFX "unable to alloc new ethernet\n"); + printk (KERN_ERR PFX "Unable to alloc new net device\n");DPRINTK ("EXIT, returning -ENOMEM\n" );return -ENOMEM ; }SET_MODULE_OWNER (dev); tp = dev->priv; + tp->pci_dev = pdev;/* enable device (incl. PCI PM wakeup and hotplug setup) */ rc =pci_enable_device (pdev);
@@ -722,7 +753,7 @@ goto err_out; } - rc =pci_request_regions (pdev, dev->name); + rc =pci_request_regions (pdev,"8139too" );if (rc) goto err_out;
@@ -731,14 +762,17 @@#ifdef USE_IO_OPS ioaddr = (void *) pio_start; + dev->base_addr = pio_start;#else /* ioremap MMIO region */ ioaddr =ioremap (mmio_start, mmio_len);if (ioaddr ==NULL ) { printk (KERN_ERR PFX "cannot remap MMIO, aborting\n"); rc = -EIO ; - goto err_out_free_res; + goto err_out; } + dev->base_addr = (long ) ioaddr; + tp->mmio_addr = ioaddr;#endif /* USE_IO_OPS */ /* Soft reset the chip. */
@@ -766,12 +800,12 @@if ((tmp8 & Cfg1_PIO) ==0 ) { printk (KERN_ERR PFX "PIO not enabled, Cfg1=%02X, aborting\n", tmp8); rc = -EIO ; - goto err_out_iounmap; + goto err_out; }if ((tmp8 & Cfg1_MMIO) ==0 ) { printk (KERN_ERR PFX "MMIO not enabled, Cfg1=%02X, aborting\n", tmp8); rc = -EIO ; - goto err_out_iounmap; + goto err_out; }/* identify chip attached to board */
@@ -795,20 +829,11 @@ rtl_chip_info[tp->chipset].name);DPRINTK ("EXIT, returning 0\n" ); - *ioaddr_out = ioaddr; *dev_out = dev;return 0 ; -err_out_iounmap: -assert (ioaddr >0 ); -#ifndef USE_IO_OPS -iounmap (ioaddr); -err_out_free_res: -#endif /* !USE_IO_OPS */ -pci_release_regions (pdev); err_out: -unregister_netdev (dev); -kfree (dev); +__rtl8139_cleanup_dev (dev);DPRINTK ("EXIT, returning %d\n" , rc);return rc; }
@@ -820,7 +845,7 @@struct net_device *dev =NULL ;struct rtl8139_private *tp;int i, addr_len, option; -void *ioaddr =NULL ; +void *ioaddr;static int board_idx =-1 ;static int printed_version;u8 tmp;
@@ -837,13 +862,14 @@ printed_version =1 ; } - i =rtl8139_init_board (pdev, &dev, &ioaddr); + i =rtl8139_init_board (pdev, &dev);if (i <0 ) {DPRINTK ("EXIT, returning %d\n" , i);return i; } tp = dev->priv; + ioaddr = tp->mmio_addr;assert (ioaddr !=NULL );assert (dev !=NULL );
@@ -865,20 +891,22 @@ dev->watchdog_timeo =TX_TIMEOUT ; dev->irq = pdev->irq; - dev->base_addr = (unsigned long ) ioaddr;/* dev->priv/tp zeroed and aligned in init_etherdev */ tp = dev->priv;/* note: tp->chipset set in rtl8139_init_board */ tp->drv_flags = board_info[ent->driver_data].hw_flags; - tp->pci_dev = pdev; tp->mmio_addr = ioaddr;spin_lock_init (&tp->lock);init_waitqueue_head (&tp->thr_wait);init_MUTEX_LOCKED (&tp->thr_exited); + +/* dev is fully set up and ready to use now */ + i =register_netdev (dev); +if (i) goto err_out; -pci_set_drvdata (pdev, dev); +pci_set_drvdata (pdev, dev);printk (KERN_INFO "%s: %s at 0x%lx, " "%2.2x:%2.2x:%2.2x:%2.2x:%2.2x:%2.2x, "
@@ -954,6 +982,10 @@DPRINTK ("EXIT - returning 0\n" );return 0 ; + +err_out: +__rtl8139_cleanup_dev (dev); +return i; }
@@ -965,28 +997,12 @@DPRINTK ("ENTER\n" );assert (dev !=NULL ); - - np = (struct rtl8139_private *) (dev->priv); + np = dev->priv;assert (np !=NULL );unregister_netdev (dev); -#ifndef USE_IO_OPS -iounmap (np->mmio_addr); -#endif /* !USE_IO_OPS */ - -pci_release_regions (pdev); - -#ifndef RTL8139_NDEBUG -/* poison memory before freeing */ -memset (dev,0xBC , -sizeof (struct net_device ) + -sizeof (struct rtl8139_private )); -#endif /* RTL8139_NDEBUG */ - -kfree (dev); - -pci_set_drvdata (pdev,NULL ); +__rtl8139_cleanup_dev (dev);DPRINTK ("EXIT\n" ); }
Index: drivers/net/Makefile
===================================================================
RCS file: /cvsroot/gkernel/linux_2_4/drivers/net/Makefile,v retrieving revision 1.1.1.25.2.1
net/Makefile +3 -2
@@ -15,8 +15,9 @@ # All of the (potential) objects that export symbols. # This list comes from 'grep -l EXPORT_SYMBOL *.[hc]'. -export-objs := 8390.o arlan.o aironet4500_core.o aironet4500_card.o ppp_async.o \ - ppp_generic.o slhc.o pppox.o auto_irq.o +export-objs := 8390.o arlan.o aironet4500_core.o aironet4500_card.o \ + ppp_async.o ppp_generic.o slhc.o pppox.o auto_irq.o \ + net_init.o ifeq ($(CONFIG_TULIP),y) obj-y += tulip/tulip.o
Index: drivers/net/net_init.c
===================================================================
RCS file: /cvsroot/gkernel/linux_2_4/drivers/net/net_init.c,v retrieving revision 1.1.1.8
net/net_init.c +100 -102
320 lines not on this page · raw message
Index: include/linux/etherdevice.h
===================================================================
RCS file: /cvsroot/gkernel/linux_2_4/include/linux/etherdevice.h,v retrieving revision 1.1.1.10
linux/etherdevice.h +1 -0
@@ -39,6 +39,7 @@extern int eth_header_parse (struct sk_buff *skb,unsigned char *haddr);extern struct net_device *init_etherdev (struct net_device *,int ); +extern struct net_device *alloc_etherdev (unsigned int );static __inline__void eth_copy_and_sum (struct sk_buff *dest,unsigned char *src,int len,int base) {
Index: include/linux/fcdevice.h
===================================================================
RCS file: /cvsroot/gkernel/linux_2_4/include/linux/fcdevice.h,v retrieving revision 1.1.1.1
linux/fcdevice.h +1 -0
@@ -34,6 +34,7 @@//extern unsigned short fc_type_trans(struct sk_buff *skb, struct net_device *dev); extern struct net_device *init_fcdev (struct net_device *,int ); +extern struct net_device *alloc_fcdev (unsigned int );#endif
Index: include/linux/fddidevice.h
===================================================================
RCS file: /cvsroot/gkernel/linux_2_4/include/linux/fddidevice.h,v retrieving revision 1.1.1.2
linux/fddidevice.h +1 -0
@@ -35,6 +35,7 @@extern unsigned short fddi_type_trans (struct sk_buff *skb,struct net_device *dev);extern struct net_device *init_fddidev (struct net_device *,int ); +extern struct net_device *alloc_fddidev (unsigned int );#endif #endif /* _LINUX_FDDIDEVICE_H */
Index: include/linux/hippidevice.h
===================================================================
RCS file: /cvsroot/gkernel/linux_2_4/include/linux/hippidevice.h,v retrieving revision 1.1.1.1
linux/hippidevice.h +1 -0
@@ -52,6 +52,7 @@void hippi_setup (struct net_device *dev);extern struct net_device *init_hippi_dev (struct net_device *,int ); +extern struct net_device *alloc_hippi_dev (unsigned int );extern void unregister_hipdev (struct net_device *dev);#endif
Index: include/linux/trdevice.h
===================================================================
RCS file: /cvsroot/gkernel/linux_2_4/include/linux/trdevice.h,v retrieving revision 1.1.1.1
linux/trdevice.h +1 -0
@@ -34,6 +34,7 @@extern int tr_rebuild_header (struct sk_buff *skb);extern unsigned short tr_type_trans (struct sk_buff *skb,struct net_device *dev);extern struct net_device *init_trdev (struct net_device *,int ); +extern struct net_device *alloc_trdev (unsigned int );#endif
Index: net/netsyms.c
===================================================================
RCS file: /cvsroot/gkernel/linux_2_4/net/netsyms.c,v retrieving revision 1.1.1.19
netsyms.c +0 -24
@@ -432,33 +432,19 @@#endif /* CONFIG_INET */ #ifdef CONFIG_TR -EXPORT_SYMBOL (tr_setup);EXPORT_SYMBOL (tr_type_trans); -EXPORT_SYMBOL (register_trdev); -EXPORT_SYMBOL (unregister_trdev); -EXPORT_SYMBOL (init_trdev);#endif -#ifdef CONFIG_NET_FC -EXPORT_SYMBOL (register_fcdev); -EXPORT_SYMBOL (unregister_fcdev); -EXPORT_SYMBOL (init_fcdev); -#endif -/* Device callback registration */ EXPORT_SYMBOL (register_netdevice_notifier);EXPORT_SYMBOL (unregister_netdevice_notifier);/* support for loadable net drivers */ #ifdef CONFIG_NET -EXPORT_SYMBOL (init_etherdev);EXPORT_SYMBOL (loopback_dev);EXPORT_SYMBOL (register_netdevice);EXPORT_SYMBOL (unregister_netdevice); -EXPORT_SYMBOL (register_netdev); -EXPORT_SYMBOL (unregister_netdev);EXPORT_SYMBOL (netdev_state_change); -EXPORT_SYMBOL (ether_setup);EXPORT_SYMBOL (dev_new_index);EXPORT_SYMBOL (dev_get_by_index);EXPORT_SYMBOL (__dev_get_by_index);
@@ -469,8 +455,6 @@EXPORT_SYMBOL (eth_type_trans);#ifdef CONFIG_FDDI EXPORT_SYMBOL (fddi_type_trans); -EXPORT_SYMBOL (fddi_setup); -EXPORT_SYMBOL (init_fddidev);#endif /* CONFIG_FDDI */ #if 0 EXPORT_SYMBOL (eth_copy_and_sum);
@@ -511,8 +495,6 @@#ifdef CONFIG_HIPPI EXPORT_SYMBOL (hippi_type_trans); -EXPORT_SYMBOL (init_hippi_dev); -EXPORT_SYMBOL (unregister_hipdev);#endif #ifdef CONFIG_SYSCTL
@@ -522,12 +504,6 @@EXPORT_SYMBOL (sysctl_ip_default_ttl);#endif #endif - -#if defined(CONFIG_ATALK ) || defined(CONFIG_ATALK_MODULE ) -#include <linux/if_ltalk.h> -EXPORT_SYMBOL (ltalk_setup); -#endif -/* Packet scheduler modules want these. */ EXPORT_SYMBOL (qdisc_destroy);
Oh, it should be noted that since this is intended as a stable 2.4 series change. The patch does not change any existing APIs, only adds a function. Existing 2.4 drivers are free to continue using init_etherdev... This bug, which I fix, isn't causing oops AFAIK, just exporting ugliness to user space etc.
Signature · 3 lines
Jeff Garzik | "You see, in this world there's two kinds of Building 1024 | people, my friend: Those with loaded guns MandrakeSoft | and those who dig. You dig." --Blondie
Quoted 2 lines · In article <[email protected]> you wrote:
This bug, which I fix, isn't causing oops AFAIK, just exporting ugliness to user space etc.
It CAN and IS causing oopses. init_etherdev() causes /sbin/hotplug to be invoked, which in turn ifconfig up's the interface.
Several (if not all) drivers have an _up() function which cannot handle not-yet initialized device nodes.
Now we don't see this often as on UP systems, /sbin/hotplug is only started _after_ the probe function either sleeps or exits, and 95% of the probe functions don't sleep. (The ones that do are indeed reported to oops) For SMP systems, I guess not that many have cardbus or other hotplug NICs....
Greetings,
Arjan van de Ven
Quoted 11 lines · Jeff Garzik
Our API already supports a solution -- setup the device, then call
register_netdev. The patch below adds a helper,alloc_etherdev, to eliminate duplicate code in drivers. Ethernet device initialization, after the patch, should now look likedev =
alloc_etherdev(sizeof(struct netdev_private)); ... initialize device ...
... set upnet_devicestruct members ... rc =register_netdevice(dev);
if (rc) /* handle error */netif_start_queue(dev);
Think-o in my example: netif_start_queue occurs in dev->open(), not in the probe phase. Simply ignore that line in the example and you are ok.
Jeff
Signature · 3 lines
Jeff Garzik | "You see, in this world there's two kinds of Building 1024 | people, my friend: Those with loaded guns MandrakeSoft | and those who dig. You dig." --Blondie
Quoted 3 lines · Jeff Garzik
People from time to time point out a wart in ethernet initialization:
They sure do. You were away at the time, but I had a 94 file, 140k patch late last year which fixed all this. It's at
http://www.uow.edu.au/~andrewm/linux/netdevice.patch
and the design doc is at
http://www.uow.edu.au/~andrewm/linux/netdevice2.txt
Quoted 1 line
From a quick look, I think the only substantive difference
here is that my `prepare_etherdev()' function allocates and reserves the device's name (eth0), but prevents it from being available in netdevice namespace lookups. This was done because lots of drivers wanted to do:
init_etherdev(); (Replaced with prepare_etherdev())
printk("%s: something", dev->name);The changes to dev.c and net_init.c were fairly subtle and took some thinking about - we should revisit them if you want to go ahead with this.
The patch all worked OK, was back-compatible with unaltered drivers, and indeed altered all the drivers. But it kind of got lost. Too big, too late and dev_probe_lock() was there.
Now, Arjan says that this race is causing oopses. This surprises me, because current kernels have the the dev_probe_lock() hack which I put in. This fixes the problem for PCI and Cardbus drivers. The ISA drivers generally use the dev->init() technique which is not racy. There isn't a lot left over. Arjan? Which driver?
The other reason I'm surprised that it's causing oopses: most racy drivers do this:
xxx_probe()
{
init_etherdev();
<initialisation - takes 10s of milliseconds and can sleep>
dev->open = xxx_open;
return;
}So the vastly most probably failure mode if the race occurs is this: the interface is opened while dev->open is NULL. This won't oops. Sure, the interface is screwed because the open() routine hasn't been called, but it should hang in there. A subsequent close() of the interface *will* call dev->close, and I guess the driver is likely to get upset if its close() routine is called without a corresponding open().
Yes, we can fix this if we want, and kill off dev_probe_lock(). It'll only take a few days. Do we want? If not, we can extend the dev_probe_lock() thing to cover probes for other busses. USB, I guess.
-
Quoted 3 lines
It'll only take a few days. Do we want? If not, we can extend the
dev_probe_lock()thing to cover probes for other busses. USB, I guess.
cardbus.. usb.. insmod/rmmod
I'd like it fixed, but you have to convince DaveM
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Jeff" == Jeff Garzik <[email protected]> writes:
Jeff> People from time to time point out a wart in ethernet Jeff> initialization: The net_device is allocated and registered to Jeff> the system in init_etherdev, which is usually one of the first Jeff> things an ethernet driver probe function does. The net_device's Jeff> final members are setup at some time between then and the exit Jeff> of the probe function. There is never a clear point where the Jeff> net device is available to the system for use.
I don't like the way you declare all the code in obscure macros in there.
+#define DECLARE_CHG_MTU(suffix,low,high) \
+ static int suffix##_change_mtu(struct net_device *dev, int new_mtu) \
......
All it does is to make the code harder to read and debug for little/no gain.
Jes
Quoted 9 lines · Jes Sorensen
I don't like the way you declare all the code in obscure macros in there.
net/net_init.c +2
-0+#define DECLARE_CHG_MTU (suffix,low,high) \ + static int suffix##_change_mtu(struct net_device *dev, int new_mtu) \All it does is to make the code harder to read and debug for little/no gain.
I disagree, but you probably knew that when you saw the code :)
These macros are not used inside code, they declare entire functions. These functions are 100% duplicated across 2-4 protocols. Duplicated code means bugs in some portions of the code and no others, more difficult to maintain, etc. I even proved this point while developing the patch -- one of the functions was missing an EXPORT_xxx symbol. Using a standard macro automatically fixed this, a small oversight that had been in the kernel probably for over a year.
Signature · 3 lines
Jeff Garzik | "You see, in this world there's two kinds of Building 1024 | people, my friend: Those with loaded guns MandrakeSoft | and those who dig. You dig." --Blondie