Request_mem_region should be used with release_mem_region, not
release_resource.
The semantic match that finds this problem is as follows:
(http://coccinelle.lip6.fr/)
// <smpl>
@@
expression x,E;
@@
*x = request_mem_region(...)
... when != release_mem_region(x)
when != x = E
* release_resource(x);
// </smpl>
Signed-off-by: Julia Lawall <[email protected]>
---
drivers/watchdog/s3c2410_wdt.c | 19 ++++++++-----------
1 file changed, 8 insertions(+), 11 deletions(-)
diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
index 25b39bf..95ae53d 100644
--- a/drivers/watchdog/s3c2410_wdt.c
+++ b/drivers/watchdog/s3c2410_wdt.c
@@ -402,7 +402,6 @@ static inline void s3c2410wdt_cpufreq_deregister(void)
static int __devinit s3c2410wdt_probe(struct platform_device *pdev)
{
- struct resource *res;
struct device *dev;
unsigned int wtcon;
int started = 0;
@@ -416,20 +415,19 @@ static int __devinit s3c2410wdt_probe(struct platform_device *pdev)
/* get the memory region for the watchdog timer */
- res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- if (res == NULL) {
+ wdt_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (wdt_mem == NULL) {
dev_err(dev, "no memory resource specified\n");
return -ENOENT;
}
- size = resource_size(res);
- wdt_mem = request_mem_region(res->start, size, pdev->name);
- if (wdt_mem == NULL) {
+ size = resource_size(wdt_mem);
+ if (!request_mem_region(wdt_mem->start, size, pdev->name)) {
dev_err(dev, "failed to get memory region\n");
return -EBUSY;
}
- wdt_base = ioremap(res->start, size);
+ wdt_base = ioremap(wdt_mem->start, size);
if (wdt_base == NULL) {
dev_err(dev, "failed to ioremap() region\n");
ret = -EINVAL;
@@ -524,8 +522,8 @@ static int __devinit s3c2410wdt_probe(struct platform_device *pdev)
iounmap(wdt_base);
err_req:
- release_resource(wdt_mem);
- kfree(wdt_mem);
+ release_mem_region(wdt_mem->start, size);
+ wdt_mem = NULL;
return ret;
}
@@ -545,8 +543,7 @@ static int __devexit s3c2410wdt_remove(struct platform_device *dev)
iounmap(wdt_base);
- release_resource(wdt_mem);
- kfree(wdt_mem);
+ release_mem_region(wdt_mem->start, resource_size(wdt_mem));
wdt_mem = NULL;
return 0;
}
Julia Lawall wrote:
>
> Request_mem_region should be used with release_mem_region, not
> release_resource.
>
> The semantic match that finds this problem is as follows:
> (http://coccinelle.lip6.fr/)
>
> // <smpl>
> @@
> expression x,E;
> @@
> *x = request_mem_region(...)
> ... when != release_mem_region(x)
> when != x = E
> * release_resource(x);
> // </smpl>
>
> Signed-off-by: Julia Lawall <[email protected]>
>
> ---
> drivers/watchdog/s3c2410_wdt.c | 19 ++++++++-----------
> 1 file changed, 8 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/watchdog/s3c2410_wdt.c
b/drivers/watchdog/s3c2410_wdt.c
> index 25b39bf..95ae53d 100644
> --- a/drivers/watchdog/s3c2410_wdt.c
> +++ b/drivers/watchdog/s3c2410_wdt.c
> @@ -402,7 +402,6 @@ static inline void s3c2410wdt_cpufreq_deregister(void)
>
> static int __devinit s3c2410wdt_probe(struct platform_device *pdev)
> {
> - struct resource *res;
> struct device *dev;
> unsigned int wtcon;
> int started = 0;
> @@ -416,20 +415,19 @@ static int __devinit s3c2410wdt_probe(struct
> platform_device *pdev)
>
> /* get the memory region for the watchdog timer */
>
> - res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> - if (res == NULL) {
> + wdt_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (wdt_mem == NULL) {
Hmm...I think, 'res' is better for platform_get_resource().
Do we _really_ need to change the name?...
> dev_err(dev, "no memory resource specified\n");
> return -ENOENT;
> }
>
> - size = resource_size(res);
> - wdt_mem = request_mem_region(res->start, size, pdev->name);
> - if (wdt_mem == NULL) {
> + size = resource_size(wdt_mem);
> + if (!request_mem_region(wdt_mem->start, size, pdev->name)) {
If we keep the name 'res', don't need to change above.
> dev_err(dev, "failed to get memory region\n");
> return -EBUSY;
> }
>
> - wdt_base = ioremap(res->start, size);
> + wdt_base = ioremap(wdt_mem->start, size);
Same as above.
> if (wdt_base == NULL) {
> dev_err(dev, "failed to ioremap() region\n");
> ret = -EINVAL;
> @@ -524,8 +522,8 @@ static int __devinit s3c2410wdt_probe(struct
> platform_device *pdev)
> iounmap(wdt_base);
>
> err_req:
> - release_resource(wdt_mem);
> - kfree(wdt_mem);
> + release_mem_region(wdt_mem->start, size);
release_mem_region(res->start, size); ?
> + wdt_mem = NULL;
>
> return ret;
> }
> @@ -545,8 +543,7 @@ static int __devexit s3c2410wdt_remove(struct
> platform_device *dev)
>
> iounmap(wdt_base);
>
> - release_resource(wdt_mem);
> - kfree(wdt_mem);
> + release_mem_region(wdt_mem->start, resource_size(wdt_mem));
release_mem_region(res->start, resource_size(res)); ?
> wdt_mem = NULL;
> return 0;
> }
Thanks.
Best regards,
Kgene.
--
Kukjin Kim <[email protected]>, Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.
On Mon, 28 Feb 2011, Kukjin Kim wrote:
> Julia Lawall wrote:
> >
> > Request_mem_region should be used with release_mem_region, not
> > release_resource.
> >
> > The semantic match that finds this problem is as follows:
> > (http://coccinelle.lip6.fr/)
> >
> > // <smpl>
> > @@
> > expression x,E;
> > @@
> > *x = request_mem_region(...)
> > ... when != release_mem_region(x)
> > when != x = E
> > * release_resource(x);
> > // </smpl>
> >
> > Signed-off-by: Julia Lawall <[email protected]>
> >
> > ---
> > drivers/watchdog/s3c2410_wdt.c | 19 ++++++++-----------
> > 1 file changed, 8 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/watchdog/s3c2410_wdt.c
> b/drivers/watchdog/s3c2410_wdt.c
> > index 25b39bf..95ae53d 100644
> > --- a/drivers/watchdog/s3c2410_wdt.c
> > +++ b/drivers/watchdog/s3c2410_wdt.c
> > @@ -402,7 +402,6 @@ static inline void s3c2410wdt_cpufreq_deregister(void)
> >
> > static int __devinit s3c2410wdt_probe(struct platform_device *pdev)
> > {
> > - struct resource *res;
> > struct device *dev;
> > unsigned int wtcon;
> > int started = 0;
> > @@ -416,20 +415,19 @@ static int __devinit s3c2410wdt_probe(struct
> > platform_device *pdev)
> >
> > /* get the memory region for the watchdog timer */
> >
> > - res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > - if (res == NULL) {
> > + wdt_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + if (wdt_mem == NULL) {
>
> Hmm...I think, 'res' is better for platform_get_resource().
> Do we _really_ need to change the name?...
wdt_mem is a global variable. Is res a good name for a global variable?
One could say wdt_mem = res, if that seems better.
julia
> > dev_err(dev, "no memory resource specified\n");
> > return -ENOENT;
> > }
> >
> > - size = resource_size(res);
> > - wdt_mem = request_mem_region(res->start, size, pdev->name);
> > - if (wdt_mem == NULL) {
> > + size = resource_size(wdt_mem);
> > + if (!request_mem_region(wdt_mem->start, size, pdev->name)) {
>
> If we keep the name 'res', don't need to change above.
>
> > dev_err(dev, "failed to get memory region\n");
> > return -EBUSY;
> > }
> >
> > - wdt_base = ioremap(res->start, size);
> > + wdt_base = ioremap(wdt_mem->start, size);
>
> Same as above.
>
> > if (wdt_base == NULL) {
> > dev_err(dev, "failed to ioremap() region\n");
> > ret = -EINVAL;
> > @@ -524,8 +522,8 @@ static int __devinit s3c2410wdt_probe(struct
> > platform_device *pdev)
> > iounmap(wdt_base);
> >
> > err_req:
> > - release_resource(wdt_mem);
> > - kfree(wdt_mem);
> > + release_mem_region(wdt_mem->start, size);
>
> release_mem_region(res->start, size); ?
>
> > + wdt_mem = NULL;
> >
> > return ret;
> > }
> > @@ -545,8 +543,7 @@ static int __devexit s3c2410wdt_remove(struct
> > platform_device *dev)
> >
> > iounmap(wdt_base);
> >
> > - release_resource(wdt_mem);
> > - kfree(wdt_mem);
> > + release_mem_region(wdt_mem->start, resource_size(wdt_mem));
>
> release_mem_region(res->start, resource_size(res)); ?
>
> > wdt_mem = NULL;
> > return 0;
> > }
>
>
> Thanks.
>
> Best regards,
> Kgene.
> --
> Kukjin Kim <[email protected]>, Senior Engineer,
> SW Solution Development Team, Samsung Electronics Co., Ltd.
>
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
Hi Julia, Kukjin Kim,
> > Hmm...I think, 'res' is better for platform_get_resource().
> > Do we _really_ need to change the name?...
>
> wdt_mem is a global variable. Is res a good name for a global variable?
> One could say wdt_mem = res, if that seems better.
If you look at the code then you have:
static struct resource *wdt_mem;
static struct resource *wdt_irq;
and in the probe function you have:
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
...
wdt_mem = request_mem_region(res->start, size, pdev->name);
...
wdt_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
So doing Julia's:
wdt_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
...
if (!request_mem_region(wdt_mem->start, size, pdev->name)) {
...
wdt_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0)
seems the best solution to me (since you then have both resources being polulated via platform_get_resource(pdev,... ).
Kind regards,
Wim.
On Fri, 18 Mar 2011, Wim Van Sebroeck wrote:
> Hi Julia, Kukjin Kim,
>
> > > Hmm...I think, 'res' is better for platform_get_resource().
> > > Do we _really_ need to change the name?...
> >
> > wdt_mem is a global variable. Is res a good name for a global variable?
> > One could say wdt_mem = res, if that seems better.
>
> If you look at the code then you have:
> static struct resource *wdt_mem;
> static struct resource *wdt_irq;
>
> and in the probe function you have:
> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> ...
> wdt_mem = request_mem_region(res->start, size, pdev->name);
> ...
> wdt_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>
> So doing Julia's:
> wdt_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> ...
> if (!request_mem_region(wdt_mem->start, size, pdev->name)) {
> ...
> wdt_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0)
>
> seems the best solution to me (since you then have both resources being polulated via platform_get_resource(pdev,... ).
Actually, I later realized that one can also use the result of
request_mem_region in place of res. Even though it is not the same
structure, it has the same start and end field information, which is the
only information that is used. But I haven't had a chance to send a patch
that follows that strategy. So if it is preferred to keep res, then that
can be done.
julia
Hi Julia,
> Actually, I later realized that one can also use the result of
> request_mem_region in place of res. Even though it is not the same
> structure, it has the same start and end field information, which is the
> only information that is used. But I haven't had a chance to send a patch
> that follows that strategy. So if it is preferred to keep res, then that
> can be done.
If you look at the driver then you will see that it will use wdt_base to control the device:
static void __iomem *wdt_base;
...
res=platform_get_resource(pdev, IORESOURCE_MEM, 0);
...
wdt_base = ioremap(res->start, size);
So the memory resource is only used for:
1) requesting the memory region
2) ioremapping the memory so that we can use the registers.
My opinion: res can go out so that wdt_mem and wdt_irq get similar platform resource info.
Kind regards,
Wim.