MTD/pxa: patch for error return value method of pxa2xx-flash.c
PATCH 1/3 MTD/pxa: patch for error return value method of pxa2xx-flash.c
This patch is to re-implement the 'pxa2xx-flash.c' error return value method of probe() and remove() function,the old return way can arouse the 'info' memory leak risk.
--- drivers/mtd/maps/pxa2xx-flash.c | 47 ++++++++++++++++++++++++++++---------- 1 files changed, 34 insertions(+), 13 deletions(-)
drivers/mtd/maps/pxa2xx-flash.c +34 -13
@@ -60,12 +60,21 @@ static int __init pxa2xx_flash_probe(struct platform_device *pdev)int ret =0 ; res =platform_get_resource (pdev,IORESOURCE_MEM ,0 ); -if (!res) -return -ENODEV ; +if (!res) { + ret = -ENODEV ; + goto fail0; + } + +if (!request_mem_region (res->start,resource_size (res), pdev->name)) { + ret = -EBUSY ; + goto fail0; + } info =kzalloc (sizeof (struct pxa2xx_flash_info ),GFP_KERNEL ); -if (!info) -return -ENOMEM ; +if (!info) { + ret = -ENOMEM ; + goto fail1; + } info->map.name = (char *) flash->name; info->map.bankwidth = flash->width;
@@ -78,13 +87,17 @@ static int __init pxa2xx_flash_probe(struct platform_device *pdev)if (!info->map.virt) {printk (KERN_WARNING "Failed to ioremap %s\n" , info->map.name); -return -ENOMEM ; + ret = -ENOMEM ; + goto fail2; } info->map.cached =ioremap_cached (info->map.phys, info->map.size); -if (!info->map.cached) +if (!info->map.cached) {printk (KERN_WARNING "Failed to ioremap cached %s\n" , info->map.name); + ret = -ENOMEM ; + goto fail3; + } info->map.inval_cache = pxa2xx_map_inval_cache;simple_map_init (&info->map);
@@ -97,10 +110,8 @@ static int __init pxa2xx_flash_probe(struct platform_device *pdev) info->mtd =do_map_probe (flash->map_name, &info->map);if (!info->mtd) { -iounmap ((void *)info->map.virt); -if (info->map.cached) -iounmap (info->map.cached); -return -EIO ; + ret = -EIO ; + goto fail4; } info->mtd->owner =THIS_MODULE ;
@@ -124,13 +135,21 @@ static int __init pxa2xx_flash_probe(struct platform_device *pdev)platform_set_drvdata (pdev, info);return 0 ; + +fail4:iounmap (info->map.cached); +fail3:iounmap (info->map.virt); +fail2:kfree (info); +fail1:release_mem_region (res->start,resource_size (res)); +fail0: +return ret; } static int __devexit pxa2xx_flash_remove(struct platform_device *dev) {struct pxa2xx_flash_info *info =platform_get_drvdata (dev); +struct resource *res; -platform_set_drvdata (dev,NULL ); + res =platform_get_resource (pdev,IORESOURCE_MEM ,0 );#ifdef CONFIG_MTD_PARTITIONS if (info->nr_parts)
@@ -141,10 +160,12 @@ static int __devexit pxa2xx_flash_remove(struct platform_device *dev)map_destroy (info->mtd);iounmap (info->map.virt); -if (info->map.cached) -iounmap (info->map.cached); +iounmap (info->map.cached);kfree (info->parts);kfree (info); +release_mem_region (res->start,resource_size (res)); +platform_set_drvdata (dev,NULL ); +return 0 ; }
Signature · 1 line
1.6.3.3
PATCH 1/3 MTD/pxa: patch for error return value method of pxa2xx-flash.c
2010/6/5 Wan ZongShun <[email protected]>:
Quoted 15 lines
This patch is to re-implement the '
pxa2xx-flash.c' error return value method ofprobe()andremove()function,the old return way can arouse the 'info' memory leak risk.Signed-off-by Wan ZongShun <[email protected]>---
?drivers/mtd/maps/pxa2xx-flash.c | ? 47 ++++++++++++++++++++++++++++---------- ?1 files changed, 34 insertions(+), 13 deletions(-)drivers/mtd/maps/pxa2xx-flash.c +0
-0
drivers/mtd/maps/pxa2xx-flash.c static int __init pxa2xx_flash_probe(struct platform_device *pdev) · line 66
Quoted 9 lines
drivers/mtd/maps/pxa2xx-flash.c +4
-2@@ -60,12 +60,21 @@ static int __init pxa2xx_flash_probe(struct platform_device *pdev) ? ? ? ?int ret = 0; ? ? ? ?res = platform_get_resource(pdev, IORESOURCE_MEM, 0); - ? ? ? if (!res) - ? ? ? ? ? ? ? return -ENODEV; + ? ? ? if (!res) { + ? ? ? ? ? ? ? ret = -ENODEV; + ? ? ? ? ? ? ? goto fail0; + ? ? ? }
Can just return -ENODEV, why bother goto here?
drivers/mtd/maps/pxa2xx-flash.c static int __init pxa2xx_flash_probe(struct platform_device *pdev) · line 72
Quoted 6 lines
drivers/mtd/maps/pxa2xx-flash.c +5
-0+ + ? ? ? if (!request_mem_region(res->start, resource_size(res), pdev->name)) { + ? ? ? ? ? ? ? ret = -EBUSY; + ? ? ? ? ? ? ? goto fail0; + ? ? ? }
Same here, nothing to release, can just return -EBUSY here. Provided I always doubt the necessity of request_mem_region() here since the resource should have been claimed when the platform device is registered, this is possibly going to claim a sub-region of that resource. But that's another story.
Quoted 18 lines
drivers/mtd/maps/pxa2xx-flash.c +6
-3? ? ? ?info = kzalloc(sizeof(struct pxa2xx_flash_info), GFP_KERNEL); - ? ? ? if (!info) - ? ? ? ? ? ? ? return -ENOMEM; + ? ? ? if (!info) { + ? ? ? ? ? ? ? ret = -ENOMEM; + ? ? ? ? ? ? ? goto fail1; + ? ? ? } ? ? ? ?info->map.name = (char *) flash->name; ? ? ? ?info->map.bankwidth = flash->width;@@ -78,13 +87,17 @@ static int __init pxa2xx_flash_probe(struct platform_device *pdev) ? ? ? ?if (!info->map.virt) { ? ? ? ? ? ? ? ?printk(KERN_WARNING "Failed to ioremap %s\n", ? ? ? ? ? ? ? ? ? ? ? info->map.name); - ? ? ? ? ? ? ? return -ENOMEM; + ? ? ? ? ? ? ? ret = -ENOMEM; + ? ? ? ? ? ? ? goto fail2; ? ? ? ?}
Quoted 6 lines
? ? ? ?info->map.cached =
? ? ? ? ? ? ? ?ioremap_cached(info->map.phys,info->map.size);
- ? ? ? if (!info->map.cached)
+ ? ? ? if (!info->map.cached) {
? ? ? ? ? ? ? ?printk(KERN_WARNING "Failed to ioremap cached %s\n", ? ? ? ? ? ? ? ? ? ? ?info->map.name);
This might not be an error at all, it's a warning. The cached mapping is for performance reason on some platforms, it should be working all right without it.
Quoted 7 lines
+ ? ? ? ? ? ? ? ret =
-ENOMEM;
+ ? ? ? ? ? ? ? goto fail3;
+ ? ? ? }
? ? ? ?info->map.inval_cache =pxa2xx_map_inval_cache; ? ? ? ?simple_map_init(&info->map);
drivers/mtd/maps/pxa2xx-flash.c static int __init pxa2xx_flash_probe(struct platform_device *pdev) · line 113
Quoted 8 lines
drivers/mtd/maps/pxa2xx-flash.c +1
-4@@ -97,10 +110,8 @@ static int __init pxa2xx_flash_probe(struct platform_device *pdev) ? ? ? ?info->mtd = do_map_probe(flash->map_name, &info->map); ? ? ? ?if (!info->mtd) { - ? ? ? ? ? ? ? iounmap((void *)info->map.virt); - ? ? ? ? ? ? ? if (info->map.cached) - ? ? ? ? ? ? ? ? ? ? ? iounmap(info->map.cached); - ? ? ? ? ? ? ? return -EIO; + ? ? ? ? ? ? ? ret = -EIO;
See above.
Quoted 13 lines
drivers/mtd/maps/pxa2xx-flash.c +6
-0+ ? ? ? ? ? ? ? goto fail4; ? ? ? ?} ? ? ? ?info->mtd->owner = THIS_MODULE;@@ -124,13 +135,21 @@ static int __init pxa2xx_flash_probe(struct platform_device *pdev) ? ? ? ?platform_set_drvdata(pdev, info); ? ? ? ?return 0; + +fail4:iounmap (info->map.cached); +fail3:iounmap (info->map.virt); +fail2:kfree (info); +fail1:release_mem_region (res->start,resource_size (res));
drivers/mtd/maps/pxa2xx-flash.c static int __init pxa2xx_flash_probe(struct platform_device *pdev) · line 133
Quoted 10 lines
drivers/mtd/maps/pxa2xx-flash.c +3
-1+fail0: + ? ? ? return ret; ?} ?static int __devexit pxa2xx_flash_remove(struct platform_device *dev) ?{ ? ? ? ?struct pxa2xx_flash_info *info = platform_get_drvdata(dev); + ? ? ? struct resource *res; - ? ? ? platform_set_drvdata(dev, NULL);
What's wrong with the above statement? And normally there is some benefit of setting drive data to be NULL, as the driver data could be used concurrently during removal (i.e. an interrupt handler if the IRQ is not already disabled). Though relying on this behavior to safely remove a device is _not_ by anyway recommended.
drivers/mtd/maps/pxa2xx-flash.c static int __devexit pxa2xx_flash_remove(struct platform_device *dev) · line 163
Quoted 11 lines
drivers/mtd/maps/pxa2xx-flash.c +1
-0+ ? ? ? res = platform_get_resource(pdev, IORESOURCE_MEM, 0);?#ifdef
CONFIG_MTD_PARTITIONS
? ? ? ?if (info->nr_parts)drivers/mtd/maps/pxa2xx-flash.c +1
-2@@ -141,10 +160,12 @@ static int __devexit pxa2xx_flash_remove(struct platform_device *dev) ? ? ? ?map_destroy(info->mtd); ? ? ? ?iounmap(info->map.virt); - ? ? ? if (info->map.cached) - ? ? ? ? ? ? ? iounmap(info->map.cached); + ? ? ? iounmap(info->map.cached);
See above, the map.cached is supposed to be optional and can only be removed if it's mapped, no force here.
Quoted 11 lines
drivers/mtd/maps/pxa2xx-flash.c +3
-1? ? ? ?kfree(info->parts); ? ? ? ?kfree(info); + ? ? ? release_mem_region(res->start, resource_size(res)); + ? ? ? platform_set_drvdata(dev, NULL); + ? ? ? ?return 0; ?} --1.6.3.3
PATCH 1/3 MTD/pxa: patch for error return value method of pxa2xx-flash.c
Hi Eric,
Thanks for your review.
2010/6/5 Eric Miao <[email protected]>:
Quoted 35 lines
2010/6/5 Wan ZongShun <[email protected]>:
Quoted 15 lines
This patch is to re-implement the '
pxa2xx-flash.c' error return value method ofprobe()andremove()function,the old return way can arouse the 'info' memory leak risk.Signed-off-by Wan ZongShun <[email protected]>---
?drivers/mtd/maps/pxa2xx-flash.c | ? 47 ++++++++++++++++++++++++++++---------- ?1 files changed, 34 insertions(+), 13 deletions(-)drivers/mtd/maps/pxa2xx-flash.c +0
-0drivers/mtd/maps/pxa2xx-flash.c static int __init pxa2xx_flash_probe(struct platform_device *pdev) · line 66
Quoted 9 lines
drivers/mtd/maps/pxa2xx-flash.c +4
-2@@ -60,12 +60,21 @@ static int __init pxa2xx_flash_probe(struct platform_device *pdev) ? ? ? ?int ret = 0; ? ? ? ?res = platform_get_resource(pdev, IORESOURCE_MEM, 0); - ? ? ? if (!res) - ? ? ? ? ? ? ? return -ENODEV; + ? ? ? if (!res) { + ? ? ? ? ? ? ? ret = -ENODEV; + ? ? ? ? ? ? ? goto fail0; + ? ? ? }Can just return
-ENODEV, why bother goto here?drivers/mtd/maps/pxa2xx-flash.c static int __init pxa2xx_flash_probe(struct platform_device *pdev) · line 72
Quoted 6 lines
drivers/mtd/maps/pxa2xx-flash.c +5
-0+ + ? ? ? if (!request_mem_region(res->start, resource_size(res), pdev->name)) { + ? ? ? ? ? ? ? ret = -EBUSY; + ? ? ? ? ? ? ? goto fail0; + ? ? ? }
Quoted 5 lines
Same here, nothing to release, can just return
-EBUSYhere. Provided I always doubt the necessity ofrequest_mem_region()here since the resource should have been claimed when the platform device is registered, this is possibly going to claim a sub-region of that resource. But that's another story.
I agree with you.
So, keep it be there or remove it?
Could Keeping it make a more compatibility code?
Quoted 22 lines
Quoted 22 lines
drivers/mtd/maps/pxa2xx-flash.c +6
-3? ? ? ?info = kzalloc(sizeof(struct pxa2xx_flash_info), GFP_KERNEL); - ? ? ? if (!info) - ? ? ? ? ? ? ? return -ENOMEM; + ? ? ? if (!info) { + ? ? ? ? ? ? ? ret = -ENOMEM; + ? ? ? ? ? ? ? goto fail1; + ? ? ? } ? ? ? ?info->map.name = (char *) flash->name; ? ? ? ?info->map.bankwidth = flash->width;@@ -78,13 +87,17 @@ static int __init pxa2xx_flash_probe(struct platform_device *pdev) ? ? ? ?if (!info->map.virt) { ? ? ? ? ? ? ? ?printk(KERN_WARNING "Failed to ioremap %s\n", ? ? ? ? ? ? ? ? ? ? ? info->map.name); - ? ? ? ? ? ? ? return -ENOMEM; + ? ? ? ? ? ? ? ret = -ENOMEM; + ? ? ? ? ? ? ? goto fail2; ? ? ? ?}? ? ? ?info->map.cached =
? ? ? ? ? ? ? ?ioremap_cached(info->map.phys,info->map.size);
- ? ? ? if (!info->map.cached)
Quoted 8 lines
Quoted 3 lines
+ ? ? ? if (!info->map.cached) {
? ? ? ? ? ? ? ?printk(KERN_WARNING "Failed to ioremap cached %s\n", ? ? ? ? ? ? ? ? ? ? ?info->map.name);This might not be an error at all, it's a warning. The cached mapping is for performance reason on some platforms, it should be working all right without it.
Thanks, I just have a doubt regarding this condition judement, so thanks for your comments.
Quoted 41 lines
Quoted 8 lines
+ ? ? ? ? ? ? ? ret =
-ENOMEM;
+ ? ? ? ? ? ? ? goto fail3;
+ ? ? ? }
? ? ? ?info->map.inval_cache =pxa2xx_map_inval_cache; ? ? ? ?simple_map_init(&info->map);drivers/mtd/maps/pxa2xx-flash.c static int __init pxa2xx_flash_probe(struct platform_device *pdev) · line 113
Quoted 8 lines
drivers/mtd/maps/pxa2xx-flash.c +1
-4@@ -97,10 +110,8 @@ static int __init pxa2xx_flash_probe(struct platform_device *pdev) ? ? ? ?info->mtd = do_map_probe(flash->map_name, &info->map); ? ? ? ?if (!info->mtd) { - ? ? ? ? ? ? ? iounmap((void *)info->map.virt); - ? ? ? ? ? ? ? if (info->map.cached) - ? ? ? ? ? ? ? ? ? ? ? iounmap(info->map.cached); - ? ? ? ? ? ? ? return -EIO; + ? ? ? ? ? ? ? ret = -EIO;See above.
Quoted 22 lines
drivers/mtd/maps/pxa2xx-flash.c +9
-0+ ? ? ? ? ? ? ? goto fail4; ? ? ? ?} ? ? ? ?info->mtd->owner = THIS_MODULE;@@ -124,13 +135,21 @@ static int __init pxa2xx_flash_probe(struct platform_device *pdev) ? ? ? ?platform_set_drvdata(pdev, info); ? ? ? ?return 0; + +fail4:iounmap (info->map.cached); +fail3:iounmap (info->map.virt); +fail2:kfree (info); +fail1:release_mem_region (res->start,resource_size (res)); +fail0: + ? ? ? return ret; ?} ?static int __devexit pxa2xx_flash_remove(struct platform_device *dev) ?{ ? ? ? ?struct pxa2xx_flash_info *info = platform_get_drvdata(dev); + ? ? ? struct resource *res;
Quoted 7 lines
drivers/mtd/maps/pxa2xx-flash.c static int __init pxa2xx_flash_probe(struct platform_device *pdev) · line 133
Quoted 1 line
drivers/mtd/maps/pxa2xx-flash.c +0
-1- ? ? ? platform_set_drvdata(dev, NULL);What's wrong with the above statement? And normally there is some benefit of setting drive data to be NULL, as the driver data could be used concurrently during removal (i.e. an interrupt handler if the IRQ is not already disabled). Though relying on this behavior to safely remove a device is _not_ by anyway recommended.
I got it,so, setting the drive data to be NULL should be as early as possible.
Quoted 28 lines
drivers/mtd/maps/pxa2xx-flash.c static int __devexit pxa2xx_flash_remove(struct platform_device *dev) · line 163
Quoted 12 lines
drivers/mtd/maps/pxa2xx-flash.c +1
-0+ ? ? ? res = platform_get_resource(pdev, IORESOURCE_MEM, 0);?#ifdef
CONFIG_MTD_PARTITIONS
? ? ? ?if (info->nr_parts)drivers/mtd/maps/pxa2xx-flash.c +1
-2@@ -141,10 +160,12 @@ static int __devexit pxa2xx_flash_remove(struct platform_device *dev) ? ? ? ?map_destroy(info->mtd); ? ? ? ?iounmap(info->map.virt); - ? ? ? if (info->map.cached) - ? ? ? ? ? ? ? iounmap(info->map.cached); + ? ? ? iounmap(info->map.cached);See above, the map.cached is supposed to be optional and can only be removed if it's mapped, no force here.
Quoted 11 lines
drivers/mtd/maps/pxa2xx-flash.c +3
-1? ? ? ?kfree(info->parts); ? ? ? ?kfree(info); + ? ? ? release_mem_region(res->start, resource_size(res)); + ? ? ? platform_set_drvdata(dev, NULL); + ? ? ? ?return 0; ?} --1.6.3.3
Signature · 10 lines
*linux-arm-kernel mailing list mail addr:linux-arm-kernel at lists.infradead.org you can subscribe by: http://lists.infradead.org/mailman/listinfo/linux-arm-kernel * linux-arm-NUC900 mailing list mail addr:NUC900 at googlegroups.com main web: https://groups.google.com/group/NUC900 you can subscribe it by sending me mail: mcuos.com at gmail.com
PATCH 1/3 MTD/pxa: patch for error return value method of pxa2xx-flash.c
Hi Eric,
This following piece of code:
if (!info->map.virt) {
printk(KERN_WARNING "Failed to ioremap %s\n",
info->map.name);
ret = -ENOMEM;
goto fail2;
}Shouldn't the 'KERN_ERR' in stead of the 'KERN_WARNING '? Because the 'info->map.virt == NULL' can arouse the serious crash.
2010/6/5 Wan ZongShun <[email protected]>:
Quoted 180 lines
Hi Eric,
Thanks for your review.
2010/6/5 Eric Miao <[email protected]>:
Quoted 35 lines
2010/6/5 Wan ZongShun <[email protected]>:
Quoted 15 lines
This patch is to re-implement the '
pxa2xx-flash.c' error return value method ofprobe()andremove()function,the old return way can arouse the 'info' memory leak risk.Signed-off-by Wan ZongShun <[email protected]>---
?drivers/mtd/maps/pxa2xx-flash.c | ? 47 ++++++++++++++++++++++++++++---------- ?1 files changed, 34 insertions(+), 13 deletions(-)drivers/mtd/maps/pxa2xx-flash.c +0
-0drivers/mtd/maps/pxa2xx-flash.c static int __init pxa2xx_flash_probe(struct platform_device *pdev) · line 66
Quoted 9 lines
drivers/mtd/maps/pxa2xx-flash.c +4
-2@@ -60,12 +60,21 @@ static int __init pxa2xx_flash_probe(struct platform_device *pdev) ? ? ? ?int ret = 0; ? ? ? ?res = platform_get_resource(pdev, IORESOURCE_MEM, 0); - ? ? ? if (!res) - ? ? ? ? ? ? ? return -ENODEV; + ? ? ? if (!res) { + ? ? ? ? ? ? ? ret = -ENODEV; + ? ? ? ? ? ? ? goto fail0; + ? ? ? }Can just return
-ENODEV, why bother goto here?drivers/mtd/maps/pxa2xx-flash.c static int __init pxa2xx_flash_probe(struct platform_device *pdev) · line 72
Quoted 6 lines
drivers/mtd/maps/pxa2xx-flash.c +5
-0+ + ? ? ? if (!request_mem_region(res->start, resource_size(res), pdev->name)) { + ? ? ? ? ? ? ? ret = -EBUSY; + ? ? ? ? ? ? ? goto fail0; + ? ? ? }Quoted 5 lines
Same here, nothing to release, can just return
-EBUSYhere. Provided I always doubt the necessity ofrequest_mem_region()here since the resource should have been claimed when the platform device is registered, this is possibly going to claim a sub-region of that resource. But that's another story.I agree with you.
So, keep it be there or remove it?Could Keeping it make a more compatibility code?
Quoted 22 lines
Quoted 22 lines
drivers/mtd/maps/pxa2xx-flash.c +6
-3? ? ? ?info = kzalloc(sizeof(struct pxa2xx_flash_info), GFP_KERNEL); - ? ? ? if (!info) - ? ? ? ? ? ? ? return -ENOMEM; + ? ? ? if (!info) { + ? ? ? ? ? ? ? ret = -ENOMEM; + ? ? ? ? ? ? ? goto fail1; + ? ? ? } ? ? ? ?info->map.name = (char *) flash->name; ? ? ? ?info->map.bankwidth = flash->width;@@ -78,13 +87,17 @@ static int __init pxa2xx_flash_probe(struct platform_device *pdev) ? ? ? ?if (!info->map.virt) { ? ? ? ? ? ? ? ?printk(KERN_WARNING "Failed to ioremap %s\n", ? ? ? ? ? ? ? ? ? ? ? info->map.name); - ? ? ? ? ? ? ? return -ENOMEM; + ? ? ? ? ? ? ? ret = -ENOMEM; + ? ? ? ? ? ? ? goto fail2; ? ? ? ?}? ? ? ?info->map.cached =
? ? ? ? ? ? ? ?ioremap_cached(info->map.phys,info->map.size);
- ? ? ? if (!info->map.cached)Quoted 8 lines
Quoted 3 lines
+ ? ? ? if (!info->map.cached) {
? ? ? ? ? ? ? ?printk(KERN_WARNING "Failed to ioremap cached %s\n", ? ? ? ? ? ? ? ? ? ? ?info->map.name);This might not be an error at all, it's a warning. The cached mapping is for performance reason on some platforms, it should be working all right without it.
Thanks, I just have a doubt regarding this condition judement, so thanks for your comments.
Quoted 41 lines
Quoted 8 lines
+ ? ? ? ? ? ? ? ret =
-ENOMEM;
+ ? ? ? ? ? ? ? goto fail3;
+ ? ? ? }
? ? ? ?info->map.inval_cache =pxa2xx_map_inval_cache; ? ? ? ?simple_map_init(&info->map);drivers/mtd/maps/pxa2xx-flash.c static int __init pxa2xx_flash_probe(struct platform_device *pdev) · line 113
Quoted 8 lines
drivers/mtd/maps/pxa2xx-flash.c +1
-4@@ -97,10 +110,8 @@ static int __init pxa2xx_flash_probe(struct platform_device *pdev) ? ? ? ?info->mtd = do_map_probe(flash->map_name, &info->map); ? ? ? ?if (!info->mtd) { - ? ? ? ? ? ? ? iounmap((void *)info->map.virt); - ? ? ? ? ? ? ? if (info->map.cached) - ? ? ? ? ? ? ? ? ? ? ? iounmap(info->map.cached); - ? ? ? ? ? ? ? return -EIO; + ? ? ? ? ? ? ? ret = -EIO;See above.
Quoted 22 lines
drivers/mtd/maps/pxa2xx-flash.c +9
-0+ ? ? ? ? ? ? ? goto fail4; ? ? ? ?} ? ? ? ?info->mtd->owner = THIS_MODULE;@@ -124,13 +135,21 @@ static int __init pxa2xx_flash_probe(struct platform_device *pdev) ? ? ? ?platform_set_drvdata(pdev, info); ? ? ? ?return 0; + +fail4:iounmap (info->map.cached); +fail3:iounmap (info->map.virt); +fail2:kfree (info); +fail1:release_mem_region (res->start,resource_size (res)); +fail0: + ? ? ? return ret; ?} ?static int __devexit pxa2xx_flash_remove(struct platform_device *dev) ?{ ? ? ? ?struct pxa2xx_flash_info *info = platform_get_drvdata(dev); + ? ? ? struct resource *res;Quoted 7 lines
drivers/mtd/maps/pxa2xx-flash.c static int __init pxa2xx_flash_probe(struct platform_device *pdev) · line 133
Quoted 1 line
drivers/mtd/maps/pxa2xx-flash.c +0
-1- ? ? ? platform_set_drvdata(dev, NULL);What's wrong with the above statement? And normally there is some benefit of setting drive data to be NULL, as the driver data could be used concurrently during removal (i.e. an interrupt handler if the IRQ is not already disabled). Though relying on this behavior to safely remove a device is _not_ by anyway recommended.
I got it,so, setting the drive data to be NULL should be as early as possible.
Quoted 7 lines
Quoted 7 lines
drivers/mtd/maps/pxa2xx-flash.c +1
-0+ ? ? ? res = platform_get_resource(pdev, IORESOURCE_MEM, 0);?#ifdef
CONFIG_MTD_PARTITIONS
? ? ? ?if (info->nr_parts)drivers/mtd/maps/pxa2xx-flash.c +0
-0@@ -141,10 +160,12 @@ static int __devexit pxa2xx_flash_remove(struct platform_device *dev)Quoted 21 lines
drivers/mtd/maps/pxa2xx-flash.c static int __devexit pxa2xx_flash_remove(struct platform_device *dev) · line 163
Quoted 5 lines
drivers/mtd/maps/pxa2xx-flash.c +1
-2? ? ? ?map_destroy(info->mtd); ? ? ? ?iounmap(info->map.virt); - ? ? ? if (info->map.cached) - ? ? ? ? ? ? ? iounmap(info->map.cached); + ? ? ? iounmap(info->map.cached);See above, the map.cached is supposed to be optional and can only be removed if it's mapped, no force here.
Quoted 11 lines
drivers/mtd/maps/pxa2xx-flash.c +3
-1? ? ? ?kfree(info->parts); ? ? ? ?kfree(info); + ? ? ? release_mem_region(res->start, resource_size(res)); + ? ? ? platform_set_drvdata(dev, NULL); + ? ? ? ?return 0; ?} --1.6.3.3
--
*linux-arm-kernel mailing list
mail addr:linux-arm-kernel at lists.infradead.org
you can subscribe by:
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel* linux-arm-NUC900 mailing list
mail addr:NUC900 at googlegroups.com
main web: https://groups.google.com/group/NUC900
you can subscribe it by sending me mail:
mcuos.com at gmail.com
Signature · 10 lines
*linux-arm-kernel mailing list mail addr:linux-arm-kernel at lists.infradead.org you can subscribe by: http://lists.infradead.org/mailman/listinfo/linux-arm-kernel * linux-arm-NUC900 mailing list mail addr:NUC900 at googlegroups.com main web: https://groups.google.com/group/NUC900 you can subscribe it by sending me mail: mcuos.com at gmail.com
PATCH 1/3 MTD/pxa: patch for error return value method of pxa2xx-flash.c
Quoted 11 lines · Wan ZongShun
Hi Eric,
This following piece of code:
? ? ? ?if (!info->map.virt) {
? ? ? ? ? ? ? ?printk(KERN_WARNING "Failed to ioremap %s\n", ? ? ? ? ? ? ? ? ? ? ?info->map.name);
? ? ? ? ? ? ? ?ret =-ENOMEM;
? ? ? ? ? ? ? ?goto fail2;
? ? ? ?}
Quoted 3 lines
Shouldn't ?the '
KERN_ERR' in stead of the 'KERN_WARNING'? Because the ?'info->map.virt == NULL' can arouse the serious crash.
The thing is you need to figure out if it is mandatory because we have another info->map.cached just in case. Disaster is both can not be mapped.
PATCH 1/3 MTD/pxa: patch for error return value method of pxa2xx-flash.c
In reply to Wan ZongShunQuoted 40 lines · Wan ZongShun
Hi Eric,
Thanks for your review.
2010/6/5 Eric Miao <[email protected]>:
Quoted 35 lines
2010/6/5 Wan ZongShun <[email protected]>:
Quoted 15 lines
This patch is to re-implement the '
pxa2xx-flash.c' error return value method ofprobe()andremove()function,the old return way can arouse the 'info' memory leak risk.Signed-off-by Wan ZongShun <[email protected]>---
?drivers/mtd/maps/pxa2xx-flash.c | ? 47 ++++++++++++++++++++++++++++---------- ?1 files changed, 34 insertions(+), 13 deletions(-)drivers/mtd/maps/pxa2xx-flash.c +0
-0drivers/mtd/maps/pxa2xx-flash.c static int __init pxa2xx_flash_probe(struct platform_device *pdev) · line 66
Quoted 9 lines
drivers/mtd/maps/pxa2xx-flash.c +4
-2@@ -60,12 +60,21 @@ static int __init pxa2xx_flash_probe(struct platform_device *pdev) ? ? ? ?int ret = 0; ? ? ? ?res = platform_get_resource(pdev, IORESOURCE_MEM, 0); - ? ? ? if (!res) - ? ? ? ? ? ? ? return -ENODEV; + ? ? ? if (!res) { + ? ? ? ? ? ? ? ret = -ENODEV; + ? ? ? ? ? ? ? goto fail0; + ? ? ? }Can just return
-ENODEV, why bother goto here?Quoted 6 lines
drivers/mtd/maps/pxa2xx-flash.c +5
-0+ + ? ? ? if (!request_mem_region(res->start, resource_size(res), pdev->name)) { + ? ? ? ? ? ? ? ret = -EBUSY; + ? ? ? ? ? ? ? goto fail0; + ? ? ? }
Quoted 9 lines
Quoted 5 lines
Same here, nothing to release, can just return
-EBUSYhere. Provided I always doubt the necessity ofrequest_mem_region()here since the resource should have been claimed when the platform device is registered, this is possibly going to claim a sub-region of that resource. But that's another story.I agree with you.
So, keep it be there or remove it?
I'd recommend not to add it in this patch. You can have another patch to add it if it's necessary.
Quoted 86 lines
Could Keeping it make a more compatibility code?
Quoted 22 lines
Quoted 22 lines
drivers/mtd/maps/pxa2xx-flash.c +6
-3? ? ? ?info = kzalloc(sizeof(struct pxa2xx_flash_info), GFP_KERNEL); - ? ? ? if (!info) - ? ? ? ? ? ? ? return -ENOMEM; + ? ? ? if (!info) { + ? ? ? ? ? ? ? ret = -ENOMEM; + ? ? ? ? ? ? ? goto fail1; + ? ? ? } ? ? ? ?info->map.name = (char *) flash->name; ? ? ? ?info->map.bankwidth = flash->width;@@ -78,13 +87,17 @@ static int __init pxa2xx_flash_probe(struct platform_device *pdev) ? ? ? ?if (!info->map.virt) { ? ? ? ? ? ? ? ?printk(KERN_WARNING "Failed to ioremap %s\n", ? ? ? ? ? ? ? ? ? ? ? info->map.name); - ? ? ? ? ? ? ? return -ENOMEM; + ? ? ? ? ? ? ? ret = -ENOMEM; + ? ? ? ? ? ? ? goto fail2; ? ? ? ?}? ? ? ?info->map.cached =
? ? ? ? ? ? ? ?ioremap_cached(info->map.phys,info->map.size);
- ? ? ? if (!info->map.cached)Quoted 8 lines
Quoted 3 lines
+ ? ? ? if (!info->map.cached) {
? ? ? ? ? ? ? ?printk(KERN_WARNING "Failed to ioremap cached %s\n", ? ? ? ? ? ? ? ? ? ? ?info->map.name);This might not be an error at all, it's a warning. The cached mapping is for performance reason on some platforms, it should be working all right without it.
Thanks, I just have a doubt regarding this condition judement, so thanks for your comments.
Quoted 41 lines
Quoted 8 lines
+ ? ? ? ? ? ? ? ret =
-ENOMEM;
+ ? ? ? ? ? ? ? goto fail3;
+ ? ? ? }
? ? ? ?info->map.inval_cache =pxa2xx_map_inval_cache; ? ? ? ?simple_map_init(&info->map);drivers/mtd/maps/pxa2xx-flash.c static int __init pxa2xx_flash_probe(struct platform_device *pdev) · line 113
Quoted 8 lines
drivers/mtd/maps/pxa2xx-flash.c +1
-4@@ -97,10 +110,8 @@ static int __init pxa2xx_flash_probe(struct platform_device *pdev) ? ? ? ?info->mtd = do_map_probe(flash->map_name, &info->map); ? ? ? ?if (!info->mtd) { - ? ? ? ? ? ? ? iounmap((void *)info->map.virt); - ? ? ? ? ? ? ? if (info->map.cached) - ? ? ? ? ? ? ? ? ? ? ? iounmap(info->map.cached); - ? ? ? ? ? ? ? return -EIO; + ? ? ? ? ? ? ? ret = -EIO;See above.
Quoted 22 lines
drivers/mtd/maps/pxa2xx-flash.c +9
-0+ ? ? ? ? ? ? ? goto fail4; ? ? ? ?} ? ? ? ?info->mtd->owner = THIS_MODULE;@@ -124,13 +135,21 @@ static int __init pxa2xx_flash_probe(struct platform_device *pdev) ? ? ? ?platform_set_drvdata(pdev, info); ? ? ? ?return 0; + +fail4:iounmap (info->map.cached); +fail3:iounmap (info->map.virt); +fail2:kfree (info); +fail1:release_mem_region (res->start,resource_size (res)); +fail0: + ? ? ? return ret; ?} ?static int __devexit pxa2xx_flash_remove(struct platform_device *dev) ?{ ? ? ? ?struct pxa2xx_flash_info *info = platform_get_drvdata(dev); + ? ? ? struct resource *res;Quoted 7 lines
drivers/mtd/maps/pxa2xx-flash.c static int __init pxa2xx_flash_probe(struct platform_device *pdev) · line 133
Quoted 1 line
drivers/mtd/maps/pxa2xx-flash.c +0
-1- ? ? ? platform_set_drvdata(dev, NULL);What's wrong with the above statement? And normally there is some benefit of setting drive data to be NULL, as the driver data could be used concurrently during removal (i.e. an interrupt handler if the IRQ is not already disabled). Though relying on this behavior to safely remove a device is _not_ by anyway recommended.
Quoted 2 lines
I got it,so, setting the drive data to be NULL should be as early as possible.
Normally yes. But I don't think that's a guarantee of a safe removal though.