[PATCH 4/10] drivers/net/can/flexcan.c: add missing clk_put

[PATCH 4/10] drivers/net/can/flexcan.c: add missing clk_put

[Julia Lawall]

From: Julia Lawall <julia@diku.dk>

The failed_get label is used after the call to clk_get has succeeded, so it
should be moved up above the call to clk_put.

A simplified version of the semantic match that finds this problem is as
follows: (http://coccinelle.lip6.fr/)

// <smpl>
@r exists@
expression e1,e2;
statement S;
@@

e1 = clk_get@p1(...);
... when != e1 = e2
    when != clk_put(e1)
    when any
if (...) { ... when != clk_put(e1)
               when != if (...) { ... clk_put(e1) ... }
* return@p3 ...;
 } else S
// </smpl>

Signed-off-by: Julia Lawall <julia@diku.dk>

---
 drivers/net/can/flexcan.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index d499056..121739c 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -978,8 +978,8 @@ static int __devinit flexcan_probe(struct platform_device *pdev)
  failed_map:
 	release_mem_region(mem->start, mem_size);
  failed_req:
-	clk_put(clk);
  failed_get:
+	clk_put(clk);
  failed_clock:
 	return err;
 }

Re: [PATCH 4/10] drivers/net/can/flexcan.c: add missing clk_put

[Joe Perches]

On Wed, 2011-06-01 at 19:10 +0200, Julia Lawall wrote:
> The failed_get label is used after the call to clk_get has succeeded, so it
> should be moved up above the call to clk_put.
[]
> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
[]
> @@ -978,8 +978,8 @@ static int __devinit flexcan_probe(struct platform_device *pdev)
>   failed_map:
>  	release_mem_region(mem->start, mem_size);
>   failed_req:
> -	clk_put(clk);
>   failed_get:
> +	clk_put(clk);
>   failed_clock:
>  	return err;

If this is correct, it might be better to rename all the
uses of failed_req to failed_get and delete label failed_req.

Re: [PATCH 4/10] drivers/net/can/flexcan.c: add missing clk_put

[Julia Lawall]

On Wed, 1 Jun 2011, Joe Perches wrote:

> On Wed, 2011-06-01 at 19:10 +0200, Julia Lawall wrote:
> > The failed_get label is used after the call to clk_get has succeeded, so it
> > should be moved up above the call to clk_put.
> []
> > diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> []
> > @@ -978,8 +978,8 @@ static int __devinit flexcan_probe(struct platform_device *pdev)
> >   failed_map:
> >  	release_mem_region(mem->start, mem_size);
> >   failed_req:
> > -	clk_put(clk);
> >   failed_get:
> > +	clk_put(clk);
> >   failed_clock:
> >  	return err;
> 
> If this is correct, it might be better to rename all the
> uses of failed_req to failed_get and delete label failed_req.

As I replied to Walter Harms, I believe it is correct, because I don't see 
anything in the called functions that needs to be freed.  I can redo the 
patch to eliminate the label.

julia

Re: [PATCH 4/10] drivers/net/can/flexcan.c: add missing clk_put

[David Miller]

From: Julia Lawall <julia@diku.dk>
Date: Wed, 1 Jun 2011 22:08:46 +0200 (CEST)

> On Wed, 1 Jun 2011, Joe Perches wrote:
> 
>> On Wed, 2011-06-01 at 19:10 +0200, Julia Lawall wrote:
>> > The failed_get label is used after the call to clk_get has succeeded, so it
>> > should be moved up above the call to clk_put.
>> []
>> > diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
>> []
>> > @@ -978,8 +978,8 @@ static int __devinit flexcan_probe(struct platform_device *pdev)
>> >   failed_map:
>> >  	release_mem_region(mem->start, mem_size);
>> >   failed_req:
>> > -	clk_put(clk);
>> >   failed_get:
>> > +	clk_put(clk);
>> >   failed_clock:
>> >  	return err;
>> 
>> If this is correct, it might be better to rename all the
>> uses of failed_req to failed_get and delete label failed_req.
> 
> As I replied to Walter Harms, I believe it is correct, because I don't see 
> anything in the called functions that needs to be freed.  I can redo the 
> patch to eliminate the label.

Please do.

Re: [PATCH 4/10] drivers/net/can/flexcan.c: add missing clk_put

[Julia Lawall]

On Wed, 1 Jun 2011, David Miller wrote:

> From: Julia Lawall <julia@diku.dk>
> Date: Wed, 1 Jun 2011 22:08:46 +0200 (CEST)
> 
> > On Wed, 1 Jun 2011, Joe Perches wrote:
> > 
> >> On Wed, 2011-06-01 at 19:10 +0200, Julia Lawall wrote:
> >> > The failed_get label is used after the call to clk_get has succeeded, so it
> >> > should be moved up above the call to clk_put.
> >> []
> >> > diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> >> []
> >> > @@ -978,8 +978,8 @@ static int __devinit flexcan_probe(struct platform_device *pdev)
> >> >   failed_map:
> >> >  	release_mem_region(mem->start, mem_size);
> >> >   failed_req:
> >> > -	clk_put(clk);
> >> >   failed_get:
> >> > +	clk_put(clk);
> >> >   failed_clock:
> >> >  	return err;
> >> 
> >> If this is correct, it might be better to rename all the
> >> uses of failed_req to failed_get and delete label failed_req.
> > 
> > As I replied to Walter Harms, I believe it is correct, because I don't see 
> > anything in the called functions that needs to be freed.  I can redo the 
> > patch to eliminate the label.
> 
> Please do.

OK, I will do it tomorrow.

julia

Re: [PATCH 4/10] drivers/net/can/flexcan.c: add missing clk_put

[Julia Lawall]

From: Julia Lawall <julia@diku.dk>

The failed_get label is used after the call to clk_get has succeeded, so it
should be moved up above the call to clk_put.

The failed_req labels doesn't do anything different than failed_get, so
delete it.

A simplified version of the semantic match that finds this problem is as
follows: (http://coccinelle.lip6.fr/)

// <smpl>
@r exists@
expression e1,e2;
statement S;
@@

e1 = clk_get@p1(...);
... when != e1 = e2
    when != clk_put(e1)
    when any
if (...) { ... when != clk_put(e1)
               when != if (...) { ... clk_put(e1) ... }
* return@p3 ...;
 } else S
// </smpl>

Signed-off-by: Julia Lawall <julia@diku.dk>

---
 drivers/net/can/flexcan.c |    5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index d499056..1767811 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -923,7 +923,7 @@ static int __devinit flexcan_probe(struct platform_device *pdev)
 	mem_size = resource_size(mem);
 	if (!request_mem_region(mem->start, mem_size, pdev->name)) {
 		err = -EBUSY;
-		goto failed_req;
+		goto failed_get;
 	}
 
 	base = ioremap(mem->start, mem_size);
@@ -977,9 +977,8 @@ static int __devinit flexcan_probe(struct platform_device *pdev)
 	iounmap(base);
  failed_map:
 	release_mem_region(mem->start, mem_size);
- failed_req:
-	clk_put(clk);
  failed_get:
+	clk_put(clk);
  failed_clock:
 	return err;
 }

Re: [PATCH 4/10] drivers/net/can/flexcan.c: add missing clk_put

[David Miller]

From: Julia Lawall <julia@diku.dk>
Date: Thu, 2 Jun 2011 07:48:50 +0200 (CEST)

> From: Julia Lawall <julia@diku.dk>
> 
> The failed_get label is used after the call to clk_get has succeeded, so it
> should be moved up above the call to clk_put.
> 
> The failed_req labels doesn't do anything different than failed_get, so
> delete it.
> 
> A simplified version of the semantic match that finds this problem is as
> follows: (http://coccinelle.lip6.fr/)
 ...
> Signed-off-by: Julia Lawall <julia@diku.dk>

Applied.

Re: [PATCH 4/10] drivers/net/can/flexcan.c: add missing clk_put

[walter harms]

Am 01.06.2011 19:10, schrieb Julia Lawall:
> From: Julia Lawall <julia@diku.dk>
> 
> The failed_get label is used after the call to clk_get has succeeded, so it
> should be moved up above the call to clk_put.
> 
> A simplified version of the semantic match that finds this problem is as
> follows: (http://coccinelle.lip6.fr/)
> 
> // <smpl>
> @r exists@
> expression e1,e2;
> statement S;
> @@
> 
> e1 = clk_get@p1(...);
> ... when != e1 = e2
>     when != clk_put(e1)
>     when any
> if (...) { ... when != clk_put(e1)
>                when != if (...) { ... clk_put(e1) ... }
> * return@p3 ...;
>  } else S
> // </smpl>
> 
> Signed-off-by: Julia Lawall <julia@diku.dk>
> 
> ---
>  drivers/net/can/flexcan.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> index d499056..121739c 100644
> --- a/drivers/net/can/flexcan.c
> +++ b/drivers/net/can/flexcan.c
> @@ -978,8 +978,8 @@ static int __devinit flexcan_probe(struct platform_device *pdev)
>   failed_map:
>  	release_mem_region(mem->start, mem_size);
>   failed_req:
> -	clk_put(clk);
>   failed_get:
> +	clk_put(clk);
>   failed_clock:
>  	return err;
>  }
> 

So failed_req == failed_get, is that intended ?

re,
 wh

Re: [PATCH 4/10] drivers/net/can/flexcan.c: add missing clk_put

[Julia Lawall]

On Wed, 1 Jun 2011, walter harms wrote:

> 
> 
> Am 01.06.2011 19:10, schrieb Julia Lawall:
> > From: Julia Lawall <julia@diku.dk>
> > 
> > The failed_get label is used after the call to clk_get has succeeded, so it
> > should be moved up above the call to clk_put.
> > 
> > A simplified version of the semantic match that finds this problem is as
> > follows: (http://coccinelle.lip6.fr/)
> > 
> > // <smpl>
> > @r exists@
> > expression e1,e2;
> > statement S;
> > @@
> > 
> > e1 = clk_get@p1(...);
> > ... when != e1 = e2
> >     when != clk_put(e1)
> >     when any
> > if (...) { ... when != clk_put(e1)
> >                when != if (...) { ... clk_put(e1) ... }
> > * return@p3 ...;
> >  } else S
> > // </smpl>
> > 
> > Signed-off-by: Julia Lawall <julia@diku.dk>
> > 
> > ---
> >  drivers/net/can/flexcan.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> > index d499056..121739c 100644
> > --- a/drivers/net/can/flexcan.c
> > +++ b/drivers/net/can/flexcan.c
> > @@ -978,8 +978,8 @@ static int __devinit flexcan_probe(struct platform_device *pdev)
> >   failed_map:
> >  	release_mem_region(mem->start, mem_size);
> >   failed_req:
> > -	clk_put(clk);
> >   failed_get:
> > +	clk_put(clk);
> >   failed_clock:
> >  	return err;
> >  }
> > 
> 
> So failed_req == failed_get, is that intended ?

I have the impression that it is.  failed_req comes after successful 
calls to platform_get_resource and platform_get_irq, which don't allocate 
anything.

julia