| Message ID | 4C09F15E.9020408@gmail.com |
|---|---|
| State | New, archived |
| Headers | show |
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. > 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. > + 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. > + 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 >
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 >> >
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
>
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.
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.
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; + } + + 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; }