[PATCH 1/3] MTD/pxa: patch for error return value method of pxa2xx-flash.c
Wan ZongShun
mcuos.com at gmail.com
Sat Jun 5 03:53:25 EDT 2010
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 <mcuos.com at gmail.com>:
> Hi Eric,
>
> Thanks for your review.
>
> 2010/6/5 Eric Miao <eric.y.miao at gmail.com>:
>> 2010/6/5 Wan ZongShun <mcuos.com at gmail.com>:
>>> 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.
>>>
>>> Signed-off-by :Wan ZongShun <mcuos.com at gmail.com>
>>>
>>> ---
>>> drivers/mtd/maps/pxa2xx-flash.c | 47 ++++++++++++++++++++++++++++----------
>>> 1 files changed, 34 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/mtd/maps/pxa2xx-flash.c b/drivers/mtd/maps/pxa2xx-flash.c
>>> index dd90880..1d2583f 100644
>>> --- a/drivers/mtd/maps/pxa2xx-flash.c
>>> +++ b/drivers/mtd/maps/pxa2xx-flash.c
>>> @@ -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?
>>
>>> +
>>> + 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.
>
> I agree with you.
> So, keep it be there or remove it?
>
> Could Keeping it make a more compatibility code?
>
>
>>
>>> 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);
>>
>>
>> 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.
>
>>
>>> + 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;
>>
>> See above.
>>
>>> + 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);
>>
>> 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.
>
>>
>>> + 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);
>>
>> See above, the map.cached is supposed to be optional and can only
>> be removed if it's mapped, no force here.
>>
>>> 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
>
--
*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
More information about the linux-arm-kernel
mailing list