pmcmsp-flash.c: correct clean up?

PATCH Roel Kluin 2 participants 5 messages Jul 29, 2009 – Aug 11, 2009 Memory technology devices (MTD)

Share & tools

Roel Kluin

author

2009-07-29 18:50 UTC #0 lore raw

Hi,

I noted that cleanup_msp_flash() attempted to determine the size of msp_flash with `sizeof(msp_flash) / sizeof(struct mtd_info **)' this will not work since msp_flash is not an array.

Also I made some changes to since init_msp_flash() to clean up after errors. Do you have problems with this?

Thanks,

Roel

drivers/mtd/maps/pmcmsp-flash.c +52 ~~-17~~

index 4768bd5..6133147 100644
@@ -50,7 +50,7 @@ static int fcnt;
 
 static int __init init_msp_flash(void)
 {
-	int i, j;
+	int i, j, ret = -ENOMEM;
 	int offset, coff;
 	char *env;
 	int pcnt;

@@ -75,14 +75,16 @@ static int __init init_msp_flash(void)
 	printk(KERN_NOTICE "Found %d PMC flash devices\n", fcnt);
 
 	msp_flash = kmalloc(fcnt * sizeof(struct map_info *), GFP_KERNEL);
+	if (!msp_flash)
+		return -ENOMEM;
+
 	msp_parts = kmalloc(fcnt * sizeof(struct mtd_partition *), GFP_KERNEL);
+	if (!msp_parts)
+		goto free_msp_flash;
+
 	msp_maps = kcalloc(fcnt, sizeof(struct mtd_info), GFP_KERNEL);
-	if (!msp_flash || !msp_parts || !msp_maps) {
-		kfree(msp_maps);
-		kfree(msp_parts);
-		kfree(msp_flash);
-		return -ENOMEM;
-	}
+	if (!msp_maps)
+		goto free_msp_parts;
 
 	/* loop over the flash devices, initializing each */
 	for (i = 0; i < fcnt; i++) {

@@ -100,13 +102,17 @@ static int __init init_msp_flash(void)
 
 		msp_parts[i] = kcalloc(pcnt, sizeof(struct mtd_partition),
 				       GFP_KERNEL);
+		if (!msp_parts[i])
+			goto free_in_loop;
 
 		/* now initialize the devices proper */
 		flash_name[5] = '0' + i;
 		env = prom_getenv(flash_name);
 
-		if (sscanf(env, "%x:%x", &addr, &size) < 2)
-			return -ENXIO;
+		if (sscanf(env, "%x:%x", &addr, &size) < 2) {
+			ret = -ENXIO;
+			goto free_msp_parts_arr;
+		}
 		addr = CPHYSADDR(addr);
 
 		printk(KERN_NOTICE

@@ -123,12 +129,19 @@ static int __init init_msp_flash(void)
 		if (size > CONFIG_MSP_FLASH_MAP_LIMIT)
 			size = CONFIG_MSP_FLASH_MAP_LIMIT;
 		msp_maps[i].virt = ioremap(addr, size);
+
+		if (msp_maps[i].virt == NULL) {
+			ret = -ENXIO;
+			goto free_msp_parts_arr;
+		}
+
 		msp_maps[i].bankwidth = 1;
-		msp_maps[i].name = strncpy(kmalloc(7, GFP_KERNEL),
-					flash_name, 7);
+		msp_maps[i].name = kmalloc(7, GFP_KERNEL);
+		if (!msp_maps[i].name)
+			goto iounmap_virt;
+
+		msp_maps[i].name = strncpy(msp_maps[i].name, flash_name, 7);
 
-		if (msp_maps[i].virt == NULL)
-			return -ENXIO;
 
 		for (j = 0; j < pcnt; j++) {
 			part_name[5] = '0' + i;

@@ -136,8 +149,10 @@ static int __init init_msp_flash(void)
 
 			env = prom_getenv(part_name);
 
-			if (sscanf(env, "%x:%x:%n", &offset, &size, &coff) < 2)
-				return -ENXIO;
+			if (sscanf(env, "%x:%x:%n", &offset, &size, &coff) < 2) {
+				ret = -ENXIO;
+				goto free_msp_maps_name;
+			}
 
 			msp_parts[i][j].size = size;
 			msp_parts[i][j].offset = offset;

@@ -152,18 +167,38 @@ static int __init init_msp_flash(void)
 			add_mtd_partitions(msp_flash[i], msp_parts[i], pcnt);
 		} else {
 			printk(KERN_ERR "map probe failed for flash\n");
-			return -ENXIO;
+			ret = -ENXIO;
+			goto free_msp_maps_name;
 		}
 	}
 
 	return 0;
+
+	while (i--) {
+                del_mtd_partitions(msp_flash[i]);
+                map_destroy(msp_flash[i]);
+free_msp_maps_name:
+                kfree(msp_maps[i].name);
+iounmap_virt:
+                iounmap((void *)msp_maps[i].virt);
+free_msp_parts_arr:
+                kfree(msp_parts[i]);
+free_in_loop:
+	}
+
+	kfree(msp_maps);
+free_msp_parts:
+	kfree(msp_parts);
+free_msp_flash:
+	kfree(msp_flash);
+	return ret;
 }
 
 static void __exit cleanup_msp_flash(void)
 {
 	int i;
 
-	for (i = 0; i < sizeof(msp_flash) / sizeof(struct mtd_info **); i++) {
+	for (i = 0; i < fcnt; i++) {
 		del_mtd_partitions(msp_flash[i]);
 		map_destroy(msp_flash[i]);
 		iounmap((void *)msp_maps[i].virt);

Roel Kluin

author

2009-08-02 08:20 UTC #1 lore raw

Op 29-07-09 20:52, Roel Kluin schreef:

Quoted 8 lines

Hi,
I noted that cleanup_msp_flash() attempted to determine the size of msp_flash with `sizeof(msp_flash) / sizeof(struct mtd_info **)' this will not work since msp_flash is not an array.
Also I made some changes to since init_msp_flash() to clean up after errors. Do you have problems with this?

My former patch had whitespace issues.

Roel

drivers/mtd/maps/pmcmsp-flash.c +53 ~~-17~~

index 4768bd5..52812a9 100644
@@ -50,7 +50,7 @@ static int fcnt;
 
 static int __init init_msp_flash(void)
 {
-	int i, j;
+	int i, j, ret = -ENOMEM;
 	int offset, coff;
 	char *env;
 	int pcnt;

@@ -75,14 +75,16 @@ static int __init init_msp_flash(void)
 	printk(KERN_NOTICE "Found %d PMC flash devices\n", fcnt);
 
 	msp_flash = kmalloc(fcnt * sizeof(struct map_info *), GFP_KERNEL);
+	if (!msp_flash)
+		return -ENOMEM;
+
 	msp_parts = kmalloc(fcnt * sizeof(struct mtd_partition *), GFP_KERNEL);
+	if (!msp_parts)
+		goto free_msp_flash;
+
 	msp_maps = kcalloc(fcnt, sizeof(struct mtd_info), GFP_KERNEL);
-	if (!msp_flash || !msp_parts || !msp_maps) {
-		kfree(msp_maps);
-		kfree(msp_parts);
-		kfree(msp_flash);
-		return -ENOMEM;
-	}
+	if (!msp_maps)
+		goto free_msp_parts;
 
 	/* loop over the flash devices, initializing each */
 	for (i = 0; i < fcnt; i++) {

@@ -100,13 +102,17 @@ static int __init init_msp_flash(void)
 
 		msp_parts[i] = kcalloc(pcnt, sizeof(struct mtd_partition),
 				       GFP_KERNEL);
+		if (!msp_parts[i])
+			goto free_in_loop;
 
 		/* now initialize the devices proper */
 		flash_name[5] = '0' + i;
 		env = prom_getenv(flash_name);
 
-		if (sscanf(env, "%x:%x", &addr, &size) < 2)
-			return -ENXIO;
+		if (sscanf(env, "%x:%x", &addr, &size) < 2) {
+			ret = -ENXIO;
+			goto free_msp_parts_arr;
+		}
 		addr = CPHYSADDR(addr);
 
 		printk(KERN_NOTICE

@@ -123,12 +129,19 @@ static int __init init_msp_flash(void)
 		if (size > CONFIG_MSP_FLASH_MAP_LIMIT)
 			size = CONFIG_MSP_FLASH_MAP_LIMIT;
 		msp_maps[i].virt = ioremap(addr, size);
+
+		if (msp_maps[i].virt == NULL) {
+			ret = -ENXIO;
+			goto free_msp_parts_arr;
+		}
+
 		msp_maps[i].bankwidth = 1;
-		msp_maps[i].name = strncpy(kmalloc(7, GFP_KERNEL),
-					flash_name, 7);
+		msp_maps[i].name = kmalloc(7, GFP_KERNEL);
+		if (!msp_maps[i].name)
+			goto iounmap_virt;
+
+		msp_maps[i].name = strncpy(msp_maps[i].name, flash_name, 7);
 
-		if (msp_maps[i].virt == NULL)
-			return -ENXIO;
 
 		for (j = 0; j < pcnt; j++) {
 			part_name[5] = '0' + i;

@@ -136,8 +149,11 @@ static int __init init_msp_flash(void)
 
 			env = prom_getenv(part_name);
 
-			if (sscanf(env, "%x:%x:%n", &offset, &size, &coff) < 2)
-				return -ENXIO;
+			if (sscanf(env, "%x:%x:%n", &offset, &size,
+						&coff) < 2) {
+				ret = -ENXIO;
+				goto free_msp_maps_name;
+			}
 
 			msp_parts[i][j].size = size;
 			msp_parts[i][j].offset = offset;

@@ -152,18 +168,38 @@ static int __init init_msp_flash(void)
 			add_mtd_partitions(msp_flash[i], msp_parts[i], pcnt);
 		} else {
 			printk(KERN_ERR "map probe failed for flash\n");
-			return -ENXIO;
+			ret = -ENXIO;
+			goto free_msp_maps_name;
 		}
 	}
 
 	return 0;
+
+	while (i--) {
+		del_mtd_partitions(msp_flash[i]);
+		map_destroy(msp_flash[i]);
+free_msp_maps_name:
+		kfree(msp_maps[i].name);
+iounmap_virt:
+		iounmap((void *)msp_maps[i].virt);
+free_msp_parts_arr:
+		kfree(msp_parts[i]);
+free_in_loop:
+	}
+
+	kfree(msp_maps);
+free_msp_parts:
+	kfree(msp_parts);
+free_msp_flash:
+	kfree(msp_flash);
+	return ret;
 }
 
 static void __exit cleanup_msp_flash(void)
 {
 	int i;
 
-	for (i = 0; i < sizeof(msp_flash) / sizeof(struct mtd_info **); i++) {
+	for (i = 0; i < fcnt; i++) {
 		del_mtd_partitions(msp_flash[i]);
 		map_destroy(msp_flash[i]);
 		iounmap((void *)msp_maps[i].virt);

Artem Bityutskiy

2009-08-09 15:37 UTC #2 lore raw

In reply to Roel Kluin

Quoted 119 lines · Roel Kluin

Hi,

I noted that cleanup_msp_flash() attempted to determine the size of msp_flash with `sizeof(msp_flash) / sizeof(struct mtd_info **)' this will not work since msp_flash is not an array.

Also I made some changes to since init_msp_flash() to clean up after errors. Do you have problems with this?

Thanks,

Roel

drivers/mtd/maps/pmcmsp-flash.c +34 ~~-16~~

index 4768bd5..6133147 100644
@@ -50,7 +50,7 @@ static int fcnt;
 
  static int __init init_msp_flash(void)
  {
-	int i, j;
+	int i, j, ret = -ENOMEM;
  	int offset, coff;
  	char *env;
  	int pcnt;

@@ -75,14 +75,16 @@ static int __init init_msp_flash(void)
  	printk(KERN_NOTICE "Found %d PMC flash devices\n", fcnt);
 
  	msp_flash = kmalloc(fcnt * sizeof(struct map_info *), GFP_KERNEL);
+	if (!msp_flash)
+		return -ENOMEM;
+
  	msp_parts = kmalloc(fcnt * sizeof(struct mtd_partition *), GFP_KERNEL);
+	if (!msp_parts)
+		goto free_msp_flash;
+
  	msp_maps = kcalloc(fcnt, sizeof(struct mtd_info), GFP_KERNEL);
-	if (!msp_flash || !msp_parts || !msp_maps) {
-		kfree(msp_maps);
-		kfree(msp_parts);
-		kfree(msp_flash);
-		return -ENOMEM;
-	}
+	if (!msp_maps)
+		goto free_msp_parts;
 
  	/* loop over the flash devices, initializing each */
  	for (i = 0; i<  fcnt; i++) {

@@ -100,13 +102,17 @@ static int __init init_msp_flash(void)
 
  		msp_parts[i] = kcalloc(pcnt, sizeof(struct mtd_partition),
  				       GFP_KERNEL);
+		if (!msp_parts[i])
+			goto free_in_loop;
 
  		/* now initialize the devices proper */
  		flash_name[5] = '0' + i;
  		env = prom_getenv(flash_name);
 
-		if (sscanf(env, "%x:%x",&addr,&size)<  2)
-			return -ENXIO;
+		if (sscanf(env, "%x:%x",&addr,&size)<  2) {
+			ret = -ENXIO;
+			goto free_msp_parts_arr;
+		}
  		addr = CPHYSADDR(addr);
 
  		printk(KERN_NOTICE

@@ -123,12 +129,19 @@ static int __init init_msp_flash(void)
  		if (size>  CONFIG_MSP_FLASH_MAP_LIMIT)
  			size = CONFIG_MSP_FLASH_MAP_LIMIT;
  		msp_maps[i].virt = ioremap(addr, size);
+
+		if (msp_maps[i].virt == NULL) {
+			ret = -ENXIO;
+			goto free_msp_parts_arr;
+		}
+
  		msp_maps[i].bankwidth = 1;
-		msp_maps[i].name = strncpy(kmalloc(7, GFP_KERNEL),
-					flash_name, 7);
+		msp_maps[i].name = kmalloc(7, GFP_KERNEL);
+		if (!msp_maps[i].name)
+			goto iounmap_virt;
+
+		msp_maps[i].name = strncpy(msp_maps[i].name, flash_name, 7);
 
-		if (msp_maps[i].virt == NULL)
-			return -ENXIO;
 
  		for (j = 0; j<  pcnt; j++) {
  			part_name[5] = '0' + i;

@@ -136,8 +149,10 @@ static int __init init_msp_flash(void)
 
  			env = prom_getenv(part_name);
 
-			if (sscanf(env, "%x:%x:%n",&offset,&size,&coff)<  2)
-				return -ENXIO;
+			if (sscanf(env, "%x:%x:%n",&offset,&size,&coff)<  2) {
+				ret = -ENXIO;
+				goto free_msp_maps_name;
+			}
 
  			msp_parts[i][j].size = size;
  			msp_parts[i][j].offset = offset;

@@ -152,18 +167,38 @@ static int __init init_msp_flash(void)
  			add_mtd_partitions(msp_flash[i], msp_parts[i], pcnt);
  		} else {
  			printk(KERN_ERR "map probe failed for flash\n");
-			return -ENXIO;
+			ret = -ENXIO;
+			goto free_msp_maps_name;
  		}
  	}
 
  	return 0;
+
+	while (i--) {

drivers/mtd/maps/pmcmsp-flash.c static int __init init_msp_flash(void) · line 187

Quoted 10 lines

drivers/mtd/maps/pmcmsp-flash.c +10 -0

+                del_mtd_partitions(msp_flash[i]);
+                map_destroy(msp_flash[i]);
+free_msp_maps_name:
+                kfree(msp_maps[i].name);
+iounmap_virt:
+                iounmap((void *)msp_maps[i].virt);
+free_msp_parts_arr:
+                kfree(msp_parts[i]);
+free_in_loop:
+	}

I think jumping into the middle of a loop is not nice. Would it be better to free the recources belonging to the current interation inside the allocation loop, and then jump to the beginning of the freeing loop instead?

Signature · 2 lines

Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Roel Kluin

author

2009-08-10 08:12 UTC #3 lore raw

This also adds cleaning up after errors in init_msp_flash().

Also cleanup_msp_flash() attempts to determine the size of msp_flash with `sizeof(msp_flash) / sizeof(struct mtd_info **)' This will not work since msp_flash is not an array.

Signed-off-by Roel Kluin <[email protected]>

---

Quoted 4 lines

I think jumping into the middle of a loop is not nice. Would it be better to free the recources belonging to the current interation inside the allocation loop, and then jump to the beginning of the freeing loop instead?

Done,

thanks.

drivers/mtd/maps/pmcmsp-flash.c +59 ~~-17~~

index 4768bd5..c8fd8da 100644
@@ -50,7 +50,7 @@ static int fcnt;
 
 static int __init init_msp_flash(void)
 {
-	int i, j;
+	int i, j, ret = -ENOMEM;
 	int offset, coff;
 	char *env;
 	int pcnt;

@@ -75,14 +75,16 @@ static int __init init_msp_flash(void)
 	printk(KERN_NOTICE "Found %d PMC flash devices\n", fcnt);
 
 	msp_flash = kmalloc(fcnt * sizeof(struct map_info *), GFP_KERNEL);
+	if (!msp_flash)
+		return -ENOMEM;
+
 	msp_parts = kmalloc(fcnt * sizeof(struct mtd_partition *), GFP_KERNEL);
+	if (!msp_parts)
+		goto free_msp_flash;
+
 	msp_maps = kcalloc(fcnt, sizeof(struct mtd_info), GFP_KERNEL);
-	if (!msp_flash || !msp_parts || !msp_maps) {
-		kfree(msp_maps);
-		kfree(msp_parts);
-		kfree(msp_flash);
-		return -ENOMEM;
-	}
+	if (!msp_maps)
+		goto free_msp_parts;
 
 	/* loop over the flash devices, initializing each */
 	for (i = 0; i < fcnt; i++) {

@@ -100,13 +102,18 @@ static int __init init_msp_flash(void)
 
 		msp_parts[i] = kcalloc(pcnt, sizeof(struct mtd_partition),
 				       GFP_KERNEL);
+		if (!msp_parts[i])
+			goto cleanup_loop;
 
 		/* now initialize the devices proper */
 		flash_name[5] = '0' + i;
 		env = prom_getenv(flash_name);
 
-		if (sscanf(env, "%x:%x", &addr, &size) < 2)
-			return -ENXIO;
+		if (sscanf(env, "%x:%x", &addr, &size) < 2) {
+			ret = -ENXIO;
+			kfree(msp_parts[i]);
+			goto cleanup_loop;
+		}
 		addr = CPHYSADDR(addr);
 
 		printk(KERN_NOTICE

@@ -122,13 +129,23 @@ static int __init init_msp_flash(void)
 		 */
 		if (size > CONFIG_MSP_FLASH_MAP_LIMIT)
 			size = CONFIG_MSP_FLASH_MAP_LIMIT;
+
 		msp_maps[i].virt = ioremap(addr, size);
+		if (msp_maps[i].virt == NULL) {
+			ret = -ENXIO;
+			kfree(msp_parts[i]);
+			goto cleanup_loop;
+		}
+
 		msp_maps[i].bankwidth = 1;
-		msp_maps[i].name = strncpy(kmalloc(7, GFP_KERNEL),
-					flash_name, 7);
+		msp_maps[i].name = kmalloc(7, GFP_KERNEL);
+		if (!msp_maps[i].name) {
+			iounmap(msp_maps[i].virt);
+			kfree(msp_parts[i]);
+			goto cleanup_loop;
+		}
 
-		if (msp_maps[i].virt == NULL)
-			return -ENXIO;
+		msp_maps[i].name = strncpy(msp_maps[i].name, flash_name, 7);
 
 		for (j = 0; j < pcnt; j++) {
 			part_name[5] = '0' + i;

@@ -136,8 +153,14 @@ static int __init init_msp_flash(void)
 
 			env = prom_getenv(part_name);
 
-			if (sscanf(env, "%x:%x:%n", &offset, &size, &coff) < 2)
-				return -ENXIO;
+			if (sscanf(env, "%x:%x:%n", &offset, &size,
+						&coff) < 2) {
+				ret = -ENXIO;
+				kfree(msp_maps[i].name);
+				iounmap(msp_maps[i].virt);
+				kfree(msp_parts[i]);
+				goto cleanup_loop;
+			}
 
 			msp_parts[i][j].size = size;
 			msp_parts[i][j].offset = offset;

@@ -152,18 +175,37 @@ static int __init init_msp_flash(void)
 			add_mtd_partitions(msp_flash[i], msp_parts[i], pcnt);
 		} else {
 			printk(KERN_ERR "map probe failed for flash\n");
-			return -ENXIO;
+			ret = -ENXIO;
+			kfree(msp_maps[i].name);
+			iounmap(msp_maps[i].virt);
+			kfree(msp_parts[i]);
+			goto cleanup_loop;
 		}
 	}
 
 	return 0;
+
+cleanup_loop:
+	while (i--) {
+		del_mtd_partitions(msp_flash[i]);
+		map_destroy(msp_flash[i]);
+		kfree(msp_maps[i].name);
+		iounmap(msp_maps[i].virt);
+		kfree(msp_parts[i]);
+	}
+	kfree(msp_maps);
+free_msp_parts:
+	kfree(msp_parts);
+free_msp_flash:
+	kfree(msp_flash);
+	return ret;
 }
 
 static void __exit cleanup_msp_flash(void)
 {
 	int i;
 
-	for (i = 0; i < sizeof(msp_flash) / sizeof(struct mtd_info **); i++) {
+	for (i = 0; i < fcnt; i++) {
 		del_mtd_partitions(msp_flash[i]);
 		map_destroy(msp_flash[i]);
 		iounmap((void *)msp_maps[i].virt);

Artem Bityutskiy

2009-08-11 11:28 UTC #4 lore raw

Quoted 7 lines · Roel Kluin

This also adds cleaning up after errors in init_msp_flash().
Also cleanup_msp_flash() attempts to determine the size of msp_flash with `sizeof(msp_flash) / sizeof(struct mtd_info **)' This will not work since msp_flash is not an array.
Signed-off-by Roel Kluin <[email protected]>

Taken to l2-mtd-2.6.git, thanks.

Signature · 2 lines

Best Regards,
Artem Bityutskiy (Артём Битюцкий)