edd: fix error paths in module_init

This patch fixes error handlings when kzalloc() or edd_device_register() failed in module_init. It needs to clean registered edd_devices before return error.

Also this patch fixes return value of module_init. module_init should not return positive value.

Signed-off-by Akinobu Mita <[email protected]>

Cc Matt Domsch <[email protected]>

---
 drivers/firmware/edd.c |   30 ++++++++++++++++++------------
 1 file changed, 18 insertions(+), 12 deletions(-)

Index: 2.6-git/drivers/firmware/edd.c
===================================================================

drivers/firmware/edd.c +9 -8

 	edd_dev_set_info(edev, i);
 	edev->kobj.kset = edd_kset;
 	error = kobject_init_and_add(&edev->kobj, &edd_ktype, NULL,

@@ -744,8 +743,8 @@ static inline int edd_num_devices(void)
 static int __init
 edd_init(void)
 {
-	unsigned int i;
-	int rc=0;
+	int i;
+	int rc;
 	struct edd_device *edev;
 
 	printk(KERN_INFO "BIOS EDD facility v%s %s, %d devices found\n",

@@ -753,29 +752,36 @@ edd_init(void)
 
 	if (!edd_num_devices()) {
 		printk(KERN_INFO "EDD information not available.\n");
-		return 1;
+		return -ENODEV;
 	}
 
 	edd_kset = kset_create_and_add("edd", NULL, firmware_kobj);
 	if (!edd_kset)
 		return -ENOMEM;
 
-	for (i = 0; i < edd_num_devices() && !rc; i++) {
+	for (i = 0; i < edd_num_devices(); i++) {
 		edev = kzalloc(sizeof (*edev), GFP_KERNEL);
-		if (!edev)
-			return -ENOMEM;
+		if (!edev) {
+			rc = -ENOMEM;
+			goto out;
+		}
 
 		rc = edd_device_register(edev, i);
 		if (rc) {
 			kfree(edev);
-			break;
+			goto out;
 		}
 		edd_devices[i] = edev;
 	}
 
-	if (rc)

Quoted 6 lines · Akinobu Mita

This patch fixes error handlings when kzalloc() or edd_device_register() failed in module_init. It needs to clean registered edd_devices before return error.

Also this patch fixes return value of module_init. module_init should not return positive value.

Thanks for these. You caught me on holiday; I'll take a more thorough look when I'm back next week.

Quoted 6 lines

drivers/firmware/edd.c +0 -0

@@ -718,8 +718,7 @@ edd_device_register(struct edd_device *e
 {
 	int error;
 

drivers/firmware/edd.c edd_device_register(struct edd_device *e · line 721

Quoted 3 lines

drivers/firmware/edd.c +1 -2

-	if (!edev)
-		return 1;
+	BUG_ON(!edev);

Wouldn't WARN_ON() and return failure be sufficient? I hate crashing the system when loading a driver if I can avoid it.

Quoted 32 lines

drivers/firmware/edd.c +7 -6

@@ -753,29 +752,36 @@ edd_init(void)
 
 	if (!edd_num_devices()) {
 		printk(KERN_INFO "EDD information not available.\n");
-		return 1;
+		return -ENODEV;
 	}
 
 	edd_kset = kset_create_and_add("edd", NULL, firmware_kobj);
 	if (!edd_kset)
 		return -ENOMEM;
 
-	for (i = 0; i < edd_num_devices() && !rc; i++) {
+	for (i = 0; i < edd_num_devices(); i++) {
 		edev = kzalloc(sizeof (*edev), GFP_KERNEL);
-		if (!edev)
-			return -ENOMEM;
+		if (!edev) {
+			rc = -ENOMEM;
+			goto out;
+		}
 
 		rc = edd_device_register(edev, i);
 		if (rc) {
 			kfree(edev);
-			break;
+			goto out;
 		}
 		edd_devices[i] = edev;
 	}
 
-	if (rc)

drivers/firmware/edd.c edd_init(void) · line 784

Quoted 10 lines

drivers/firmware/edd.c +8 -2

-		kset_unregister(edd_kset);
-	return rc;
+	return 0;
+out:
+	while (--i >= 0)
+		edd_device_unregister(edd_devices[i]);
+
+	kset_unregister(edd_kset);
+
+  	return rc;

I didn't really like my initial approach, but the question was: when you hit a failure, do you try to back completely out (unregister everything that had successfully registered until now), or do you leave the things that have succeeded, and only fail the current and future devices? For my purposes, having even the first device be reported, even if the others couldn't be, is useful. Hence why I didn't undo all the registrations on failure.