* [PATCH 1/2] drivers/watchdog/s3c2410_wdt.c: Convert release_resource to release_region/release_mem_region
@ 2011-02-26 16:34 Julia Lawall
2011-02-28 10:07 ` Kukjin Kim
0 siblings, 1 reply; 6+ messages in thread
From: Julia Lawall @ 2011-02-26 16:34 UTC (permalink / raw)
To: linux-arm-kernel
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 <julia@diku.dk>
---
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;
}
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 1/2] drivers/watchdog/s3c2410_wdt.c: Convert release_resource to release_region/release_mem_region
2011-02-26 16:34 [PATCH 1/2] drivers/watchdog/s3c2410_wdt.c: Convert release_resource to release_region/release_mem_region Julia Lawall
@ 2011-02-28 10:07 ` Kukjin Kim
2011-02-28 10:12 ` Julia Lawall
0 siblings, 1 reply; 6+ messages in thread
From: Kukjin Kim @ 2011-02-28 10:07 UTC (permalink / raw)
To: linux-arm-kernel
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 <julia@diku.dk>
>
> ---
> 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 <kgene.kim@samsung.com>, Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] drivers/watchdog/s3c2410_wdt.c: Convert release_resource to release_region/release_mem_region
2011-02-28 10:07 ` Kukjin Kim
@ 2011-02-28 10:12 ` Julia Lawall
2011-03-18 8:11 ` Wim Van Sebroeck
0 siblings, 1 reply; 6+ messages in thread
From: Julia Lawall @ 2011-02-28 10:12 UTC (permalink / raw)
To: linux-arm-kernel
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 <julia@diku.dk>
> >
> > ---
> > 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 <kgene.kim@samsung.com>, 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 majordomo at vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] drivers/watchdog/s3c2410_wdt.c: Convert release_resource to release_region/release_mem_region
2011-02-28 10:12 ` Julia Lawall
@ 2011-03-18 8:11 ` Wim Van Sebroeck
2011-03-18 8:35 ` Julia Lawall
0 siblings, 1 reply; 6+ messages in thread
From: Wim Van Sebroeck @ 2011-03-18 8:11 UTC (permalink / raw)
To: linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] drivers/watchdog/s3c2410_wdt.c: Convert release_resource to release_region/release_mem_region
2011-03-18 8:11 ` Wim Van Sebroeck
@ 2011-03-18 8:35 ` Julia Lawall
2011-03-18 9:12 ` Wim Van Sebroeck
0 siblings, 1 reply; 6+ messages in thread
From: Julia Lawall @ 2011-03-18 8:35 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] drivers/watchdog/s3c2410_wdt.c: Convert release_resource to release_region/release_mem_region
2011-03-18 8:35 ` Julia Lawall
@ 2011-03-18 9:12 ` Wim Van Sebroeck
0 siblings, 0 replies; 6+ messages in thread
From: Wim Van Sebroeck @ 2011-03-18 9:12 UTC (permalink / raw)
To: linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-03-18 9:12 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-02-26 16:34 [PATCH 1/2] drivers/watchdog/s3c2410_wdt.c: Convert release_resource to release_region/release_mem_region Julia Lawall
2011-02-28 10:07 ` Kukjin Kim
2011-02-28 10:12 ` Julia Lawall
2011-03-18 8:11 ` Wim Van Sebroeck
2011-03-18 8:35 ` Julia Lawall
2011-03-18 9:12 ` Wim Van Sebroeck
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).