Patch: linux-2.5.58/drivers/base/bus.c ignored pre-existing devices

Patch: linux-2.5.58/drivers/base/bus.c ignored pre-existing devices

[Adam J. Richter]

[-- Attachment #1: Type: text/plain, Size: 2010 bytes --]

Hi Patrick,

	device_attach() in linux-2.5.5[78]/drivers/base/bus.c has a
bug.  Any device that is detected before its driver is loaded is
forgotten.

	I noticed this bug when my keyboard and mouse stopped working
and thought that it might be an interrupt routing problem, but Ed
Tomlinson emailed me with a critical observation that I hadn't
noticed: if you unplug the USB devices and plug them back in, they
start to work.  Many thanks to Ed for saving me probably several hours
of bug tracking.  I suppose this is a pretty good example Eric Raymond's
mixed metaphor "given enough eyeballs, all bugs are shallow."

	It's also a good example of the benefits of parts of
drivers/base.  If this bug had only occured in the PCI hot plugging
code, it's unlikely it would have been noticed, but because the code
was common, the bug was noticed much more quickly (both my USB keyboard
and CardBus networking card were no longer binding to their devices).

	Anyhow, getting back to business, the problem arose due to a
botched change in drivers/base/bus.c that apprently was intended to
allow a driver->probe function to return an error other than -ENODEV
and thereby cause the whole binding process to abort.  At least that's
what I think the extra code was inteded to do.  If not, I could shrink
the code by making device_attach return void.

	Here is the patch.  Note that the line numbers will be off
because I have other changes in my bus.c (kmalloc and
dma_alloc_consistent preallocations for drivers that request them,
which I posted before).  Please integrate (or fix the bug some other
way) and forward to Linus.  It will avoid repititious bug reports
if we this bug is gone from Linus's last current release when he
leaves for vacaction.

-- 
Adam J. Richter     __     ______________   575 Oroville Road
adam@yggdrasil.com     \ /                  Milpitas, California 95035
+1 408 309-6081         | g g d r a s i l   United States of America
                         "Free Software For The Rest Of Us."

[-- Attachment #2: bus.diff --]
[-- Type: text/plain, Size: 817 bytes --]

--- linux-2.5.58/drivers/base/bus.c	2003-01-13 21:58:25.000000000 -0800
+++ linux/drivers/base/bus.c	2003-01-15 02:09:48.000000000 -0800
@@ -283,28 +326,28 @@
  *	for each pair. If a compatible pair is found, break out and return.
  */
 static int device_attach(struct device * dev)
 {
  	struct bus_type * bus = dev->bus;
 	struct list_head * entry;
-	int error = 0;
 
 	if (dev->driver) {
 		device_bind_driver(dev);
 		return 0;
 	}
 
 	if (!bus->match)
 		return 0;
 
 	list_for_each(entry,&bus->drivers.list) {
 		struct device_driver * drv = to_drv(entry);
-		if (!(error = bus_match(dev,drv)))
-			break;
+		int error = bus_match(dev,drv);
+		if (error != 0 && error != -ENODEV)
+			return error;
 	}
-	return error;
+	return 0;
 }
 
 
 /**
  *	driver_attach - try to bind driver to devices.
  *	@drv:	driver.

Re: Patch: linux-2.5.58/drivers/base/bus.c ignored pre-existing devices

[Patrick Mochel]

> 	device_attach() in linux-2.5.5[78]/drivers/base/bus.c has a
> bug.  Any device that is detected before its driver is loaded is
> forgotten.

> 	Anyhow, getting back to business, the problem arose due to a
> botched change in drivers/base/bus.c that apprently was intended to
> allow a driver->probe function to return an error other than -ENODEV
> and thereby cause the whole binding process to abort.  At least that's
> what I think the extra code was inteded to do.  If not, I could shrink
> the code by making device_attach return void.

The extra code was an attempt at properly handling failure of ->probe(),
and to make sure that an error from ->probe() (e.g. -ENOMEM) is propogated
up.

The cause of the problem you're seeing is that if a device wasn't bound to 
a driver, -ENODEV was returned, causing it to be removed from the bus's 
list of devices. 

To remedy this, I've changed the semantics of bus_match() to return the 
following:

* 1	if device was bound to a driver.
* 0	if it wasn't 
* <0	if drv->probe() returned an error. 

This allows the caller to know if binding happened, as well as bubble the 
error up. 

Greg notified me of this yesterday, and I sent him the following patch. I 
haven't pushed it upward yet, since he hasn't had a chance to test it. If 
you get a chance, please try it and let me know if fixes your problem..

Thanks,

	-pat

===== drivers/base/bus.c 1.38 vs edited =====
--- 1.38/drivers/base/bus.c	Mon Jan 13 10:34:12 2003
+++ edited/drivers/base/bus.c	Tue Jan 14 16:41:22 2003
@@ -256,22 +256,27 @@
  *	
  *	If we find a match, we call @drv->probe(@dev) if it exists, and 
  *	call attach() above.
+ *
+ *	If the deivce is bound to the driver, we return 1. If the bus
+ *	reports that they do not match (bus->match() returns FALSE), we
+ *	return 0. Otherwise, we return the error that drv->probe() 
+ *	returns.
  */
 static int bus_match(struct device * dev, struct device_driver * drv)
 {
-	int error = -ENODEV;
+	int ret = 0;
 	if (dev->bus->match(dev,drv)) {
 		dev->driver = drv;
 		if (drv->probe) {
-			if ((error = drv->probe(dev))) {
+			if ((ret = drv->probe(dev))) {
 				dev->driver = NULL;
-				return error;
+				return ret;
 			}
 		}
 		device_bind_driver(dev);
-		error = 0;
+		ret = 1;
 	}
-	return error;
+	return ret;
 }
 
 
@@ -298,8 +303,11 @@
 
 	list_for_each(entry,&bus->drivers.list) {
 		struct device_driver * drv = to_drv(entry);
-		if (!(error = bus_match(dev,drv)))
+		if ((error = bus_match(dev,drv))) {
+			if (error > 1)
+				error = 0;
 			break;
+		}
 	}
 	return error;
 }
@@ -322,6 +330,7 @@
 {
 	struct bus_type * bus = drv->bus;
 	struct list_head * entry;
+	int error = 0;
 
 	if (!bus->match)
 		return 0;
@@ -329,8 +338,12 @@
 	list_for_each(entry,&bus->devices.list) {
 		struct device * dev = container_of(entry,struct device,bus_list);
 		if (!dev->driver) {
-			if (!bus_match(dev,drv) && dev->driver)
-				devclass_add_device(dev);
+			if ((error = bus_match(dev,drv))) {
+				if (error > 0)
+					error = devclass_add_device(dev);
+				else
+					break;
+			}
 		}
 	}
 	return 0;
@@ -396,7 +409,8 @@
 		if ((error = device_attach(dev)))
 			list_del_init(&dev->bus_list);
 		up_write(&dev->bus->subsys.rwsem);
-		sysfs_create_link(&bus->devices.kobj,&dev->kobj,dev->bus_id);
+		if (!error)
+			sysfs_create_link(&bus->devices.kobj,&dev->kobj,dev->bus_id);
 	}
 	return error;
 }

Re: Patch: linux-2.5.58/drivers/base/bus.c ignored pre-existing devices

[Adam J. Richter]

On 2003-01-15, Patrick Mochel wrote:
>The extra code was an attempt at properly handling failure of ->probe(),
>and to make sure that an error from ->probe() (e.g. -ENOMEM) is propogated
>up.
>
>The cause of the problem you're seeing is that if a device wasn't bound to 
>a driver, -ENODEV was returned, causing it to be removed from the bus's 
>list of devices. 
>
>To remedy this, I've changed the semantics of bus_match() to return the 
>following:
>
>* 1     if device was bound to a driver.
>* 0     if it wasn't 
>* <0    if drv->probe() returned an error. 
>
>This allows the caller to know if binding happened, as well as bubble the 
>error up.

	I have't tried your patch, but, from reading it, I infer that
your patch would not work with drivers that do further matching
in their probe function and return -ENODEV, which should be handled
as if match() function failed.  I suggest you use my patch.

Adam J. Richter     __     ______________   575 Oroville Road
adam@yggdrasil.com     \ /                  Milpitas, California 95035
+1 408 309-6081         | g g d r a s i l   United States of America
                         "Free Software For The Rest Of Us."