Re: [PATCH] fix error handling in bus_add_device

[Hannes Reinecke <hare@suse.de>]

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

Greg KH wrote:
> On Wed, May 18, 2005 at 09:19:37AM +0200, Hannes Reinecke wrote:
>>Greg KH wrote:
>>>On Thu, May 12, 2005 at 04:19:24PM +0200, Hannes Reinecke wrote:
>>>>Hi Greg,
>>>>
>>>>this patch fixes the error handling in bus_add_device() and
>>>>device_attach(). Previously it was 'interesting'.
>>>>And totally confusing to boot.
>>>I agree, that's why it has been rewritten in the -mm tree :)
>>>
>>>Anyway, your patch doesn't take into account that device_attach()'s
>>>return value is tested in the bus_rescan_devices_helper(), so if you
>>>change the return value, that also needs to be changed.
>>>
>>>But even in the -mm tree, the bus_add_devices() function has not had the
>>>error handling added to it that you provided, is there any devices that
>>>you are seeing that need this?
>>>
>>Not yet :-)
>>
>>I'm just doing some cleanups here which me and Kay Sievers will be
>>exploiting in the near future.
>>My main point is:
>>either we do an error check in bus_add_device and return a proper
>>status, or we don't and fix bus_add_device to be of type 'void'.
>>And as some functions called by bus_add_device may fail I thought it
>>reasonable to evaluate the return status properly.
>>Unless you tell me that bus_add_device is a fire-and-forget procedure
>>and we don't care at all for any failures. But then we should at least
>>set the type of bus_add_device() to 'void'.
>>You're the maintainer, you have to decide :-).
>>I don't care either way, I just want to have it consistent.
>>
>>But you're correct about the bus_rescan_devices_helper. Fixed and new
>>patch attached.
> 
> Ok, I agree that we should have error checks in there.  Now, could you
> make your patch against the latest -mm tree instead due to all of the
> changes involved in that area in my trees?  That way I can apply it :)
> 
Whee, innovations.
Which your patches to -mm the whole thing is even easier and now
actually looks quite sane.
New patch attached.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke			hare@suse.de
SuSE Linux AG				S390 & zSeries
Maxfeldstraße 5				+49 911 74053 688
90409 Nürnberg				http://www.suse.de

[-- Attachment #2: driver-core-bus_add_device-error-handling --]
[-- Type: text/plain, Size: 1857 bytes --]

From: Hannes Reinecke <hare@suse.de>
Subject: Fix error handling in bus_add_device()

The error handling in bus_add_device() and device_attach() is simply
non-existing. This patch propagates any error from device_attach to
the upper layers to allow for a proper recovery.

--- linux-2.6.12-rc4-mm2/drivers/base/bus.c.orig	2005-05-18 10:26:50.000000000 +0200
+++ linux-2.6.12-rc4-mm2/drivers/base/bus.c	2005-05-18 10:36:08.000000000 +0200
@@ -270,11 +270,14 @@ int bus_add_device(struct device * dev)
 
 	if (bus) {
 		pr_debug("bus %s: add device %s\n", bus->name, dev->bus_id);
-		device_attach(dev);
+		error = device_attach(dev);
 		klist_add_tail(&bus->klist_devices, &dev->knode_bus);
-		device_add_attrs(bus, dev);
-		sysfs_create_link(&bus->devices.kobj, &dev->kobj, dev->bus_id);
-		sysfs_create_link(&dev->kobj, &dev->bus->subsys.kset.kobj, "bus");
+		if (error >= 0)
+			error = device_add_attrs(bus, dev);
+		if (!error) {
+			sysfs_create_link(&bus->devices.kobj, &dev->kobj, dev->bus_id);
+			sysfs_create_link(&dev->kobj, &dev->bus->subsys.kset.kobj, "bus");
+		}
 	}
 	return error;
 }
@@ -394,7 +397,7 @@ static int bus_rescan_devices_helper(str
 {
 	int *count = data;
 
-	if (!dev->driver && device_attach(dev))
+	if (!dev->driver && (device_attach(dev) > 0))
 		(*count)++;
 
 	return 0;
--- linux-2.6.12-rc4-mm2/drivers/base/dd.c.orig	2005-05-18 10:29:27.000000000 +0200
+++ linux-2.6.12-rc4-mm2/drivers/base/dd.c	2005-05-18 10:39:45.000000000 +0200
@@ -119,7 +119,8 @@ static int __device_attach(struct device
  *	driver_probe_device() for each pair. If a compatible
  *	pair is found, break out and return.
  *
- *	Returns 1 if the device was bound to a driver; 0 otherwise.
+ *	Returns 1 if the device was bound to a driver;
+ *	0 if no matching device was found; error code otherwise.
  */
 int device_attach(struct device * dev)
 {