bug in /net/core/dev.c

Hi Alan/Dave
Here is a better version of the patch I think it cuts about 15 lines out of dev.c & I think logically it is more bulletproof than my previous attempt.

(See attached file: dev.c.2.4.5.v2.patch)

Hi Dave,

On 390 it is very easy to hotplug devices under VM.

We do it all the time using the channel device layer on s/390, so users can reconfigure devices if they misconfigure them.

If it can be registered it should be able to be unregistered.

Cornelia Huck in our group in boeblingen noticed the bug too.

Please respond to "David S. Miller" <[email protected]>

To: Denis Joseph Barrow/Germany/Contr/IBM@IBMDE
cc: [email protected]
Subject: Re: bug in /net/core/dev.c

will

done.

When will devices built statically into the kernel ever be unregister'd?

Later,
David S. Miller
[email protected]

Hi,
I found this bug in dev.c

It happens if register_netdevice is called before net_dev_init which can happen from init functions,
when device drivers are compiled into the kernel.

The dev->refcnt will go up to 2 ( it should be 1) unregister_netdevice will usually loop forever
waiting for the refcnt to drop to 1 when an attempt to unregister is done.

The printk in the bootmessages early initialization of device <blah> is deferred is evidence of this behaviour occuring.

The small patch is below ,hope it is okay for you.

@@ -20,6 +20,10 @@
  *              Pekka Riikonen <[email protected]>
  *
  *	Changes:
+ *              D.J. Barrow     :       Fixed bug where dev->refcnt gets set to 2 
+ *                                      if register_netdev gets called before
+ *                                      net_dev_init & also removed a few lines
+ *                                      of code in the process.
  *		Alan Cox	:	device private ioctl copies fields back.
  *		Alan Cox	:	Transmit queue code does relevant stunts to
  *					keep the queue safe.

@@ -2382,6 +2386,7 @@
  *	will not get the same name.
  */
 
+int net_dev_init(void);
 int register_netdevice(struct net_device *dev)
 {
 	struct net_device *d, **dp;

@@ -2396,48 +2401,8 @@
 	dev->fastpath_lock=RW_LOCK_UNLOCKED;
 #endif
 
-	if (dev_boot_phase) {
-#ifdef CONFIG_NET_DIVERT
-		ret = alloc_divert_blk(dev);
-		if (ret)
-			return ret;
-#endif /* CONFIG_NET_DIVERT */
-		
-		/* This is NOT bug, but I am not sure, that all the
-		   devices, initialized before netdev module is started
-		   are sane. 
-
-		   Now they are chained to device boot list
-		   and probed later. If a module is initialized
-		   before netdev, but assumes that dev->init
-		   is really called by register_netdev(), it will fail.
-
-		   So that this message should be printed for a while.
-		 */
-		printk(KERN_INFO "early initialization of device %s is deferred\n", dev->name);
-
-		/* Check for existence, and append to tail of chain */
-		for (dp=&dev_base; (d=*dp) != NULL; dp=&d->next) {
-			if (d == dev || strcmp(d->name, dev->name) == 0) {
-				return -EEXIST;
-			}
-		}
-		dev->next = NULL;
-		write_lock_bh(&dev_base_lock);
-		*dp = dev;
-		dev_hold(dev);
-		write_unlock_bh(&dev_base_lock);
-
-		/*
-		 *	Default initial state at registry is that the
-		 *	device is present.
-		 */
-
-		set_bit(__LINK_STATE_PRESENT, &dev->state);
-
-		return 0;
-	}
-
+	if (dev_boot_phase)
+		net_dev_init();
 #ifdef CONFIG_NET_DIVERT
 	ret = alloc_divert_blk(dev);
 	if (ret)

@@ -2679,7 +2644,9 @@
 {
 	struct net_device *dev, **dp;
 	int i;
-
+	
+	if(!dev_boot_phase)
+		return 0;
 #ifdef CONFIG_NET_SCHED
 	pktsched_init();
 #endif

I still think it's bogus. Those "early init of device" messages should only be occurring for specific legacy ISA device drivers, and for nothing else.

You need to find a way to link in the S390 network drivers after the net/ module so that the drivers run their init after net_dev_init() runs.

Sure, this refcount thing is a bug, and I will look at your patch, but you need to make your normal net devices get init'd after net_dev_init() runs.

Later,
David S. Miller
[email protected]

Tricky.

net_dev_init() is in net/core/dev.c which is linked into net/core/core.o which is linked into net/network.o which is linked into vmlinux as part of $(NETWORKS).

drivers/s390/net creates drivers/s390/net/s390-net.o which is linked into drivers/s390/io.o which is part of $(CORE_FILES), not $(DRIVERS).

$(NETWORKS) is linked _after_ both $(CORE_FILES) and $(DRIVERS).

Have I mentioned recently how much I hate the "link order depends on Makefile compile order, except where fudged by special case Makefiles" method?

David, how do any network drivers that need net_dev_init() work when all $(DRIVERS) come before $(NETWORKS)? This is a generic problem, not s390 specific.

AFAICT net_dev_init() needs to be promoted ahead of $(DRIVERS), either as minimal code or by moving all of $(NETWORKS). Then moving drivers/s390/io.o to $(DRIVERS) instead of $(CORE_FILES) will fix the problem.

Then I don't understand why none of the statically built in drivers in any of my kernels (on any platform I use, sparc64, i386, etc.) make that "early initialization of device XXX deferred" message get spit out.

Actually after some studying I do understand :-) net_dev_init() is not invoked via net/* initialization. My bad. It gets invoked from drivers/block/genhd.c:device_init() so that is the ordering dependency and why things work for non-s390 network devices now.

So, if the s390 folks move their stuff into the appropriate spot it will work. In fact, I personally like to see the s390 net devices under drivers/net/s390 anyways. They'll get free maintenance from myself and Jeff Garzik in this way as I rarely look int drivers/${PLATFORM} type directories unless I'm doing a tree-wide grep. :-)

Later,
David S. Miller
[email protected]

Leave s390 separate for the moment, all the other architectures work the same way.

Minimal (and totally untested) patch to compile s390/net as part of the other network drivers follows - if it's good enough for acorn, it's good enough for s390. The method is as ugly as hell but it is the least possible change for 2.4, major redesign will have to wait for 2.5. Patch against 2.4.6-pre2.

Index: 6-pre2.1/drivers/s390/Makefile

@@ -4,7 +4,7 @@
 
 O_TARGET := io.o
 
-subdir-y := block char misc net
+subdir-y := block char misc
 subdir-m := $(subdir-y)
 obj-y := $(foreach dir,$(subdir-y),$(dir)/s390-$(dir).o)
 

@@ -12,3 +12,5 @@ obj-y += s390io.o s390mach.o s390dyn.o i
 export-objs += ccwcache.o idals.o s390dyn.o s390io.o
 
 include $(TOPDIR)/Rules.make
+
+# the NET subdir is included from drivers/net now

Index: 6-pre2.1/drivers/net/Makefile

@@ -214,6 +214,12 @@ subdir-y	+= ../acorn/net
 obj-y		+= ../acorn/net/acorn-net.o
 endif
 
+ifeq ($(CONFIG_ARCH_S390),y)
+mod-subdirs	+= ../s390/net
+subdir-y	+= ../s390/net
+obj-y		+= ../s390/net/s390-net.o
+endif
+
 #
 # HIPPI adapters
 #