fix error handling in bus_add_device

PATCH In review Hannes Reinecke 2 participants 5 messages May 12, 2005 – May 18, 2005 Driver core

lore

Share & tools

lore

Hannes Reinecke

author

2005-05-12 14:19 UTC #0 lore raw

PATCH 1/1 fix error handling in bus_add_device

Hi Greg,

this patch fixes the error handling in bus_add_device() and device_attach(). Previously it was 'interesting'. And totally confusing to boot.

Please apply.

Cheers,

Hannes

Signature · 4 lines

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

From: Hannes Reinecke <[email protected]>
Subject: Fix error handling in bus_add_device()

The error handling in bus_add_device() and device_attach() is simply non-existing. This patch updates both function to align with the default driver core convention to return '0' on success and an error code otherwise. Note that '-ENODEV' is not an error for device_attach and driver_probe_device as it is quite possible that no matching device was found.

Signed-off-by Kay Sievers <[email protected]>

drivers/base/bus.c +14 -8

diff -pur linux-2.6.12-rc4.orig/drivers/base/bus.c linux-2.6.12-rc4/drivers/base/bus.c
@@ -312,11 +312,11 @@ int device_attach(struct device * dev)
 {
  	struct bus_type * bus = dev->bus;
 	struct list_head * entry;
-	int error;
+	int error = -ENODEV;
 
 	if (dev->driver) {
 		device_bind_driver(dev);
-		return 1;
+		return 0;
 	}
 
 	if (bus->match) {

@@ -325,7 +325,7 @@ int device_attach(struct device * dev)
 			error = driver_probe_device(drv, dev);
 			if (!error)
 				/* success, driver matched */
-				return 1;
+				return 0;
 			if (error != -ENODEV && error != -ENXIO)
 				/* driver matched but the probe failed */
 				printk(KERN_WARNING

@@ -334,7 +334,7 @@ int device_attach(struct device * dev)
 		}
 	}
 
-	return 0;
+	return error;
 }

@@ -460,11 +460,17 @@ int bus_add_device(struct device * dev)
 		down_write(&dev->bus->subsys.rwsem);
 		pr_debug("bus %s: add device %s\n", bus->name, dev->bus_id);
 		list_add_tail(&dev->bus_list, &dev->bus->devices.list);
-		device_attach(dev);
+		error = device_attach(dev);
 		up_write(&dev->bus->subsys.rwsem);
-		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 || error == -ENODEV)
+			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");
+		} else {
+			pr_debug("bus %s: attach device %s failed with %d\n", bus->name, dev->bus_id, error);
+			put_bus(bus);
+		}
 	}
 	return error;
 }

Greg KH

2005-05-18 05:50 UTC #1 lore raw

Quoted 5 lines · Hannes Reinecke

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?

thanks,

greg k-h

Hannes Reinecke

author

2005-05-18 07:21 UTC #2 lore raw

Quoted 9 lines · Greg KH

Quoted 5 lines · Hannes Reinecke
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 :)

Quoted 8 lines

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.

Cheers,

Hannes

Signature · 4 lines

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

From: Hannes Reinecke <[email protected]>
Subject: Fix error handling in bus_add_device()

Signed-off-by Kay Sievers <[email protected]>

drivers/base/bus.c +15 -9

diff -pur linux-2.6.12-rc4.orig/drivers/base/bus.c linux-2.6.12-rc4/drivers/base/bus.c
@@ -312,11 +312,11 @@ int device_attach(struct device * dev)
 {
  	struct bus_type * bus = dev->bus;
 	struct list_head * entry;
-	int error;
+	int error = -ENODEV;
 
 	if (dev->driver) {
 		device_bind_driver(dev);
-		return 1;
+		return 0;
 	}
 
 	if (bus->match) {

@@ -325,7 +325,7 @@ int device_attach(struct device * dev)
 			error = driver_probe_device(drv, dev);
 			if (!error)
 				/* success, driver matched */
-				return 1;
+				return 0;
 			if (error != -ENODEV && error != -ENXIO)
 				/* driver matched but the probe failed */
 				printk(KERN_WARNING

@@ -334,7 +334,7 @@ int device_attach(struct device * dev)
 		}
 	}
 
-	return 0;
+	return error;
 }

@@ -460,11 +460,17 @@ int bus_add_device(struct device * dev)
 		down_write(&dev->bus->subsys.rwsem);
 		pr_debug("bus %s: add device %s\n", bus->name, dev->bus_id);
 		list_add_tail(&dev->bus_list, &dev->bus->devices.list);
-		device_attach(dev);
+		error = device_attach(dev);
 		up_write(&dev->bus->subsys.rwsem);
-		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 || error == -ENODEV)
+			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");
+		} else {
+			pr_debug("bus %s: attach device %s failed with %d\n", bus->name, dev->bus_id, error);
+			put_bus(bus);
+		}
 	}
 	return error;
 }

@@ -588,7 +594,7 @@ static int bus_rescan_devices_helper(str
 {
 	int *count = data;
 
-	if (!dev->driver && device_attach(dev))
+	if (!dev->driver && !device_attach(dev))
 		(*count)++;
 
 	return 0;

Greg KH

2005-05-18 07:27 UTC #3 lore raw

Quoted 33 lines · Hannes Reinecke

Quoted 9 lines · Greg KH
Quoted 5 lines · Hannes Reinecke
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 :)
Quoted 8 lines
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.

Quoted 2 lines

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 :)

thanks,

greg k-h

Hannes Reinecke

author

2005-05-18 08:42 UTC #4 lore raw

Quoted 33 lines · Greg KH

Quoted 32 lines · Hannes Reinecke
Quoted 8 lines · Greg KH
Quoted 5 lines · Hannes Reinecke
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 :)
Quoted 8 lines
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.

Quoted 7 lines

Quoted 2 lines
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

Signature · 4 lines

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

From: Hannes Reinecke <[email protected]>
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.

drivers/base/bus.c renamed from drivers/base/bus.c.orig +8 -5

@@ -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;

drivers/base/dd.c renamed from drivers/base/dd.c.orig +2 -1

@@ -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)
 {