Fix kfree usage in various mtd map remove functions

The remove() functions in various mtd map drivers were incorrectly kfree()ing the struct resource that they had passed to release_resource(), and/or failing to free the *_flash_info structure. This looks like a typo which has gradually propogated by copy&paste into a number of these drivers.

Since mtd drivers are rarely or never used as modules (they typically provide root filesystems) it is unlikely that these code paths have ever been exercised. It also means that this patch has not been tested by the author.

This patch is against 2.6.21.

diff -Nur linux-arm-2.6.21/drivers/mtd/maps.orig/integrator-flash.c linux-arm-2.6.21/drivers/mtd/maps/integrator-flash.c
@@ -174,7 +174,6 @@
 
 		iounmap(info->map.virt);
 		release_resource(info->res);
-		kfree(info->res);
 
 		if (info->plat && info->plat->exit)
 			info->plat->exit();

diff -Nur linux-arm-2.6.21/drivers/mtd/maps.orig/ixp2000.c linux-arm-2.6.21/drivers/mtd/maps/ixp2000.c
@@ -129,10 +129,10 @@
 
 	kfree(info->partitions);
 
-	if (info->res) {
+	if (info->res)
 		release_resource(info->res);
-		kfree(info->res);
-	}
+
+	kfree(info);
 
 	if (plat->exit)
 		plat->exit();

diff -Nur linux-arm-2.6.21/drivers/mtd/maps.orig/ixp4xx.c linux-arm-2.6.21/drivers/mtd/maps/ixp4xx.c
@@ -172,10 +172,10 @@
 
 	kfree(info->partitions);
 
-	if (info->res) {
+	if (info->res)
 		release_resource(info->res);
-		kfree(info->res);
-	}
+
+	kfree(info);
 
 	if (plat->exit)
 		plat->exit();

Binary files linux-arm-2.6.21/drivers/mtd/maps.orig/ixp4xx.o and linux-arm-2.6.21/drivers/mtd/maps/ixp4xx.o differ

diff -Nur linux-arm-2.6.21/drivers/mtd/maps.orig/physmap.c linux-arm-2.6.21/drivers/mtd/maps/physmap.c
@@ -64,10 +64,10 @@
 	if (info->map.virt != NULL)
 		iounmap(info->map.virt);
 
-	if (info->res != NULL) {
+	if (info->res)
 		release_resource(info->res);
-		kfree(info->res);
-	}
+
+	kfree(info);
 
 	return 0;
 }

diff -Nur linux-arm-2.6.21/drivers/mtd/maps.orig/physmap_of.c linux-arm-2.6.21/drivers/mtd/maps/physmap_of.c
@@ -107,10 +107,10 @@
 	if (info->map.virt != NULL)
 		iounmap(info->map.virt);
 
-	if (info->res != NULL) {
+	if (info->res)
 		release_resource(info->res);
-		kfree(info->res);
-	}
+
+	kfree(info);
 
 	return 0;
 }

the struct

the *_flash_info

copy&paste into a number

root filesystems)

that this patch

Good chap !!

They could be tested by building as modules then insmod/rmmod-ing repeatedly, whilst observing kernel memory usage, before and after fix.

Regards,
MikeW

The other issue that I noticed was the use of release_resource() instead of release_mem_region(). I'm not at all sure what should be going on here, but as far as I can see from kernel/resource.c these functions are not equivalent, and the "Linux Device Drivers" book shows release_mem_region() used to reverse the effect of request_mem_region(). I think that release_resource() should be called when (for example) a hotpluggable device is unplugged and the resource goes away, whereas release_mem_region() should be called when a driver no longer needs use of that area of memory. It's complex to understand the source because the same 'struct resource' is used to describe a physical resource and also a reservation/allocation of a portion of a physical resource to a driver.

I could submit a patch that replaces the release_resource() calls with release_mem_region(), but I would not be happy doing so unless someone with a bit more of a clue than me said it was the right thing to do.

Does anyone know of any documentation for any of these 'resource' functions?

See for example:
drivers/mtd/maps/ixp4xx.c function int ixp4xx_flash_remove

Phil.

The API manages device registers in I/O space or I/O memory space. The driver allocations show up in e.g. /proc/iomem and prevent drivers from trying to control the same physical device hardware. If your 'resource' request fails then the driver load fails - or your driver has to try an alternative address.

The pairings are:
request_region/request_mem_region, release_region/release_mem_region
but release_resource is generic so can be used as release.. or release_mem..

The existing calls you describe above sound OK but perhaps the author should have commented more clearly.

Best regards,
MikeW

Phil Endecott <spam_from_linux_mtd <at> chezphil.org> writes:

....

root filesystems)

that this patch

....

PS a little worrying that people are committing patches without testing them properly. Hope it's just an exception !

MikeW