* [PATCH 11/14] ARM: brcmstb: delete unneeded test before of_node_put @ 2014-08-08 10:07 Julia Lawall 2014-08-13 22:22 ` Brian Norris 0 siblings, 1 reply; 5+ messages in thread From: Julia Lawall @ 2014-08-08 10:07 UTC (permalink / raw) To: linux-arm-kernel From: Julia Lawall <Julia.Lawall@lip6.fr> Simplify the error path to avoid calling of_node_put when it is not needed. The semantic patch that finds this problem is as follows: (http://coccinelle.lip6.fr/) // <smpl> @@ expression e; @@ -if (e) of_node_put(e); // </smpl> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr> --- arch/arm/mach-bcm/platsmp-brcmstb.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/arch/arm/mach-bcm/platsmp-brcmstb.c b/arch/arm/mach-bcm/platsmp-brcmstb.c index af780e9..c515ea1 100644 --- a/arch/arm/mach-bcm/platsmp-brcmstb.c +++ b/arch/arm/mach-bcm/platsmp-brcmstb.c @@ -227,7 +227,7 @@ static int __init setup_hifcpubiuctrl_regs(struct device_node *np) if (!syscon_np) { pr_err("can't find phandle %s\n", name); rc = -EINVAL; - goto cleanup; + goto out; } cpubiuctrl_block = of_iomap(syscon_np, 0); @@ -256,9 +256,8 @@ static int __init setup_hifcpubiuctrl_regs(struct device_node *np) } cleanup: - if (syscon_np) - of_node_put(syscon_np); - + of_node_put(syscon_np); +out: return rc; } @@ -274,7 +273,7 @@ static int __init setup_hifcont_regs(struct device_node *np) if (!syscon_np) { pr_err("can't find phandle %s\n", name); rc = -EINVAL; - goto cleanup; + goto out; } hif_cont_block = of_iomap(syscon_np, 0); @@ -288,9 +287,8 @@ static int __init setup_hifcont_regs(struct device_node *np) hif_cont_reg = 0; cleanup: - if (syscon_np) - of_node_put(syscon_np); - + of_node_put(syscon_np); +out: return rc; } ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 11/14] ARM: brcmstb: delete unneeded test before of_node_put 2014-08-08 10:07 [PATCH 11/14] ARM: brcmstb: delete unneeded test before of_node_put Julia Lawall @ 2014-08-13 22:22 ` Brian Norris 2014-08-14 5:37 ` Julia Lawall 0 siblings, 1 reply; 5+ messages in thread From: Brian Norris @ 2014-08-13 22:22 UTC (permalink / raw) To: linux-arm-kernel Hi Julia, On Fri, Aug 08, 2014 at 12:07:52PM +0200, Julia Lawall wrote: > From: Julia Lawall <Julia.Lawall@lip6.fr> > > Simplify the error path to avoid calling of_node_put when it is not needed. > > The semantic patch that finds this problem is as follows: > (http://coccinelle.lip6.fr/) > > // <smpl> > @@ > expression e; > @@ > > -if (e) > of_node_put(e); > // </smpl> > > Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr> > > --- > arch/arm/mach-bcm/platsmp-brcmstb.c | 14 ++++++-------- This file is being dropped temporarily, for rework/resubmission at a later time: https://lkml.org/lkml/2014/8/13/617 But thanks for the patch. I'll take it into account in the future. A few comments below. > 1 file changed, 6 insertions(+), 8 deletions(-) > > diff --git a/arch/arm/mach-bcm/platsmp-brcmstb.c b/arch/arm/mach-bcm/platsmp-brcmstb.c > index af780e9..c515ea1 100644 > --- a/arch/arm/mach-bcm/platsmp-brcmstb.c > +++ b/arch/arm/mach-bcm/platsmp-brcmstb.c > @@ -227,7 +227,7 @@ static int __init setup_hifcpubiuctrl_regs(struct device_node *np) > if (!syscon_np) { > pr_err("can't find phandle %s\n", name); > rc = -EINVAL; > - goto cleanup; > + goto out; > } > > cpubiuctrl_block = of_iomap(syscon_np, 0); > @@ -256,9 +256,8 @@ static int __init setup_hifcpubiuctrl_regs(struct device_node *np) > } > > cleanup: > - if (syscon_np) > - of_node_put(syscon_np); > - > + of_node_put(syscon_np); > +out: Is there a good reason for this new label? I thought part of the point of this semantic patch is that the previous line (of_node_put()) is a no-op for NULL arguments. > return rc; > } > > @@ -274,7 +273,7 @@ static int __init setup_hifcont_regs(struct device_node *np) > if (!syscon_np) { > pr_err("can't find phandle %s\n", name); > rc = -EINVAL; > - goto cleanup; > + goto out; > } > > hif_cont_block = of_iomap(syscon_np, 0); > @@ -288,9 +287,8 @@ static int __init setup_hifcont_regs(struct device_node *np) > hif_cont_reg = 0; > > cleanup: > - if (syscon_np) > - of_node_put(syscon_np); > - > + of_node_put(syscon_np); > +out: Ditto. > return rc; > } > > Brian ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 11/14] ARM: brcmstb: delete unneeded test before of_node_put 2014-08-13 22:22 ` Brian Norris @ 2014-08-14 5:37 ` Julia Lawall 2014-08-14 6:53 ` Brian Norris 0 siblings, 1 reply; 5+ messages in thread From: Julia Lawall @ 2014-08-14 5:37 UTC (permalink / raw) To: linux-arm-kernel On Wed, 13 Aug 2014, Brian Norris wrote: > Hi Julia, > > On Fri, Aug 08, 2014 at 12:07:52PM +0200, Julia Lawall wrote: > > From: Julia Lawall <Julia.Lawall@lip6.fr> > > > > Simplify the error path to avoid calling of_node_put when it is not needed. > > > > The semantic patch that finds this problem is as follows: > > (http://coccinelle.lip6.fr/) > > > > // <smpl> > > @@ > > expression e; > > @@ > > > > -if (e) > > of_node_put(e); > > // </smpl> > > > > Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr> > > > > --- > > arch/arm/mach-bcm/platsmp-brcmstb.c | 14 ++++++-------- > > This file is being dropped temporarily, for rework/resubmission at a > later time: > > https://lkml.org/lkml/2014/8/13/617 > > But thanks for the patch. I'll take it into account in the future. A few > comments below. > > > 1 file changed, 6 insertions(+), 8 deletions(-) > > > > diff --git a/arch/arm/mach-bcm/platsmp-brcmstb.c b/arch/arm/mach-bcm/platsmp-brcmstb.c > > index af780e9..c515ea1 100644 > > --- a/arch/arm/mach-bcm/platsmp-brcmstb.c > > +++ b/arch/arm/mach-bcm/platsmp-brcmstb.c > > @@ -227,7 +227,7 @@ static int __init setup_hifcpubiuctrl_regs(struct device_node *np) > > if (!syscon_np) { > > pr_err("can't find phandle %s\n", name); > > rc = -EINVAL; > > - goto cleanup; > > + goto out; > > } > > > > cpubiuctrl_block = of_iomap(syscon_np, 0); > > @@ -256,9 +256,8 @@ static int __init setup_hifcpubiuctrl_regs(struct device_node *np) > > } > > > > cleanup: > > - if (syscon_np) > > - of_node_put(syscon_np); > > - > > + of_node_put(syscon_np); > > +out: > > Is there a good reason for this new label? I thought part of the point > of this semantic patch is that the previous line (of_node_put()) is a > no-op for NULL arguments. Personally, I prefer code to only be executed if it needs to be. It is helpful from a program analysis point of view, and I think it helps someone trying to understand the code. That is, when I am trying to understand some unknown code, I may look at the cleanup code and try to figure out why each piece of it is executed. If some of it is statically known to be irrelevant, it is confusing. But I you think the other way around, and would rather have just one label that contains anything that might ever be useful, then I guess that is a reasonable point of view as well. julia > > return rc; > > } > > > > @@ -274,7 +273,7 @@ static int __init setup_hifcont_regs(struct device_node *np) > > if (!syscon_np) { > > pr_err("can't find phandle %s\n", name); > > rc = -EINVAL; > > - goto cleanup; > > + goto out; > > } > > > > hif_cont_block = of_iomap(syscon_np, 0); > > @@ -288,9 +287,8 @@ static int __init setup_hifcont_regs(struct device_node *np) > > hif_cont_reg = 0; > > > > cleanup: > > - if (syscon_np) > > - of_node_put(syscon_np); > > - > > + of_node_put(syscon_np); > > +out: > > Ditto. > > > return rc; > > } > > > > > > Brian > ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 11/14] ARM: brcmstb: delete unneeded test before of_node_put 2014-08-14 5:37 ` Julia Lawall @ 2014-08-14 6:53 ` Brian Norris 2014-08-27 10:13 ` Dan Carpenter 0 siblings, 1 reply; 5+ messages in thread From: Brian Norris @ 2014-08-14 6:53 UTC (permalink / raw) To: linux-arm-kernel On Thu, Aug 14, 2014 at 07:37:28AM +0200, Julia Lawall wrote: > On Wed, 13 Aug 2014, Brian Norris wrote: > > On Fri, Aug 08, 2014 at 12:07:52PM +0200, Julia Lawall wrote: > > > diff --git a/arch/arm/mach-bcm/platsmp-brcmstb.c b/arch/arm/mach-bcm/platsmp-brcmstb.c > > > index af780e9..c515ea1 100644 > > > --- a/arch/arm/mach-bcm/platsmp-brcmstb.c > > > +++ b/arch/arm/mach-bcm/platsmp-brcmstb.c > > > @@ -227,7 +227,7 @@ static int __init setup_hifcpubiuctrl_regs(struct device_node *np) > > > if (!syscon_np) { > > > pr_err("can't find phandle %s\n", name); > > > rc = -EINVAL; > > > - goto cleanup; > > > + goto out; > > > } > > > > > > cpubiuctrl_block = of_iomap(syscon_np, 0); > > > @@ -256,9 +256,8 @@ static int __init setup_hifcpubiuctrl_regs(struct device_node *np) > > > } > > > > > > cleanup: > > > - if (syscon_np) > > > - of_node_put(syscon_np); > > > - > > > + of_node_put(syscon_np); > > > +out: > > > > Is there a good reason for this new label? I thought part of the point > > of this semantic patch is that the previous line (of_node_put()) is a > > no-op for NULL arguments. > > Personally, I prefer code to only be executed if it needs to be. It is > helpful from a program analysis point of view, and I think it helps > someone trying to understand the code. > > That is, when I am trying to understand some unknown code, I may look at > the cleanup code and try to figure out why each piece of it is executed. > If some of it is statically known to be irrelevant, it is confusing. > > But I you think the other way around, and would rather have just one label > that contains anything that might ever be useful, then I guess that is a > reasonable point of view as well. Yeah, I personally just look to avoid unnecessary labels. Thanks for explaining your thought process. Brian ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 11/14] ARM: brcmstb: delete unneeded test before of_node_put 2014-08-14 6:53 ` Brian Norris @ 2014-08-27 10:13 ` Dan Carpenter 0 siblings, 0 replies; 5+ messages in thread From: Dan Carpenter @ 2014-08-27 10:13 UTC (permalink / raw) To: linux-arm-kernel On Wed, Aug 13, 2014 at 11:53:10PM -0700, Brian Norris wrote: > > > > cleanup: > > > > - if (syscon_np) > > > > - of_node_put(syscon_np); > > > > - > > > > + of_node_put(syscon_np); > > > > +out: > > > > > > Is there a good reason for this new label? I thought part of the point > > > of this semantic patch is that the previous line (of_node_put()) is a > > > no-op for NULL arguments. > > > > Personally, I prefer code to only be executed if it needs to be. It is > > helpful from a program analysis point of view, and I think it helps > > someone trying to understand the code. > > > > That is, when I am trying to understand some unknown code, I may look at > > the cleanup code and try to figure out why each piece of it is executed. > > If some of it is statically known to be irrelevant, it is confusing. > > > > But I you think the other way around, and would rather have just one label > > that contains anything that might ever be useful, then I guess that is a > > reasonable point of view as well. > > Yeah, I personally just look to avoid unnecessary labels. > Having more than one label is better because it helps you avoid "One Err Bugs". This is a common kind of bug which is cause when functions have only one "err:" label which does all the error handling. Some examples of this type of bug are: 234ad18249a4 ('staging: gdm7240: fix error handling of probe()') 85a258b70d48 ('ocfs2: fix error handling in ocfs2_ioctl_move_extents()') 920c4f4c3651 ('drivers/leds/leds-tca6507.c: cleanup error handling in tca6507_probe()') If you unwind in the exact reversed order of how things were allocated then it makes the code a lot easier to understand so it avoids bugs. regards, dan carpenter ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2014-08-27 10:13 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-08-08 10:07 [PATCH 11/14] ARM: brcmstb: delete unneeded test before of_node_put Julia Lawall 2014-08-13 22:22 ` Brian Norris 2014-08-14 5:37 ` Julia Lawall 2014-08-14 6:53 ` Brian Norris 2014-08-27 10:13 ` Dan Carpenter
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).