| Message ID | 1306948213-20767-4-git-send-email-julia@diku.dk |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; }