* [PATCH 1/3] MTD/pxa: patch for error return value method of pxa2xx-flash.c
@ 2010-06-05 7:38 ` Wan ZongShun
0 siblings, 0 replies; 12+ messages in thread
From: Wan ZongShun @ 2010-06-05 7:38 UTC (permalink / raw)
To: linux-arm-kernel
Hi Eric,
Thanks for your review.
2010/6/5 Eric Miao <eric.y.miao@gmail.com>:
> 2010/6/5 Wan ZongShun <mcuos.com@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@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
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH 1/3] MTD/pxa: patch for error return value method of pxa2xx-flash.c
2010-06-05 7:38 ` Wan ZongShun
@ 2010-06-05 7:53 ` Wan ZongShun
-1 siblings, 0 replies; 12+ messages in thread
From: Wan ZongShun @ 2010-06-05 7:53 UTC (permalink / raw)
To: Eric Miao; +Cc: linux-mtd, David Woodhouse, linux-arm-kernel
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@gmail.com>:
> Hi Eric,
>
> Thanks for your review.
>
> 2010/6/5 Eric Miao <eric.y.miao@gmail.com>:
>> 2010/6/5 Wan ZongShun <mcuos.com@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@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@lists.infradead.org
> you can subscribe by:
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
> * linux-arm-NUC900 mailing list
> mail addr:NUC900@googlegroups.com
> main web: https://groups.google.com/group/NUC900
> you can subscribe it by sending me mail:
> mcuos.com@gmail.com
>
--
*linux-arm-kernel mailing list
mail addr:linux-arm-kernel@lists.infradead.org
you can subscribe by:
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
* linux-arm-NUC900 mailing list
mail addr:NUC900@googlegroups.com
main web: https://groups.google.com/group/NUC900
you can subscribe it by sending me mail:
mcuos.com@gmail.com
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH 1/3] MTD/pxa: patch for error return value method of pxa2xx-flash.c
@ 2010-06-05 7:53 ` Wan ZongShun
0 siblings, 0 replies; 12+ messages in thread
From: Wan ZongShun @ 2010-06-05 7:53 UTC (permalink / raw)
To: linux-arm-kernel
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@gmail.com>:
> Hi Eric,
>
> Thanks for your review.
>
> 2010/6/5 Eric Miao <eric.y.miao@gmail.com>:
>> 2010/6/5 Wan ZongShun <mcuos.com@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@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
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH 1/3] MTD/pxa: patch for error return value method of pxa2xx-flash.c
2010-06-05 7:53 ` Wan ZongShun
@ 2010-06-05 7:59 ` Eric Miao
-1 siblings, 0 replies; 12+ messages in thread
From: Eric Miao @ 2010-06-05 7:59 UTC (permalink / raw)
To: Wan ZongShun; +Cc: linux-mtd, David Woodhouse, linux-arm-kernel
On Sat, Jun 5, 2010 at 3:53 PM, Wan ZongShun <mcuos.com@gmail.com> wrote:
> 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.
>
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.
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH 1/3] MTD/pxa: patch for error return value method of pxa2xx-flash.c
@ 2010-06-05 7:59 ` Eric Miao
0 siblings, 0 replies; 12+ messages in thread
From: Eric Miao @ 2010-06-05 7:59 UTC (permalink / raw)
To: linux-arm-kernel
On Sat, Jun 5, 2010 at 3:53 PM, Wan ZongShun <mcuos.com@gmail.com> wrote:
> 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.
>
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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] MTD/pxa: patch for error return value method of pxa2xx-flash.c
2010-06-05 7:38 ` Wan ZongShun
@ 2010-06-05 7:58 ` Eric Miao
-1 siblings, 0 replies; 12+ messages in thread
From: Eric Miao @ 2010-06-05 7:58 UTC (permalink / raw)
To: Wan ZongShun; +Cc: linux-mtd, David Woodhouse, linux-arm-kernel
On Sat, Jun 5, 2010 at 3:38 PM, Wan ZongShun <mcuos.com@gmail.com> wrote:
> Hi Eric,
>
> Thanks for your review.
>
> 2010/6/5 Eric Miao <eric.y.miao@gmail.com>:
>> 2010/6/5 Wan ZongShun <mcuos.com@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@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?
>
I'd recommend not to add it in this patch. You can have another patch
to add it if it's necessary.
> 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.
>
Normally yes. But I don't think that's a guarantee of a safe removal though.
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH 1/3] MTD/pxa: patch for error return value method of pxa2xx-flash.c
@ 2010-06-05 7:58 ` Eric Miao
0 siblings, 0 replies; 12+ messages in thread
From: Eric Miao @ 2010-06-05 7:58 UTC (permalink / raw)
To: linux-arm-kernel
On Sat, Jun 5, 2010 at 3:38 PM, Wan ZongShun <mcuos.com@gmail.com> wrote:
> Hi Eric,
>
> Thanks for your review.
>
> 2010/6/5 Eric Miao <eric.y.miao@gmail.com>:
>> 2010/6/5 Wan ZongShun <mcuos.com@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@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?
>
I'd recommend not to add it in this patch. You can have another patch
to add it if it's necessary.
> 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.
>
Normally yes. But I don't think that's a guarantee of a safe removal though.
^ permalink raw reply [flat|nested] 12+ messages in thread