| Message ID | 1444480254-14399-4-git-send-email-Julia.Lawall@lip6.fr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Oct 10, 2015 at 02:30:52PM +0200, Julia Lawall wrote: > for_each_child_of_node performs an of_node_get on each iteration, so > a break out of the loop requires an of_node_put. > > The semantic patch that fixes this problem is as follows > (http://coccinelle.lip6.fr): > > // <smpl> > @@ > expression root,e; > local idexpression child; > @@ > > for_each_child_of_node(root, child) { > ... when != of_node_put(child) > when != e = child > ( > return child; > | > + of_node_put(child); > ? return ...; > ) > ... > } > // </smpl> > > Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr> Thanks, I have queued this up as a cleanup for v4.4.
Hi Julia, On Sat, Oct 10, 2015 at 2:30 PM, Julia Lawall <Julia.Lawall@lip6.fr> wrote: > --- a/arch/arm/mach-shmobile/pm-rmobile.c > +++ b/arch/arm/mach-shmobile/pm-rmobile.c > @@ -313,8 +313,10 @@ static int __init rmobile_add_pm_domains(void __iomem *base, > } > > pd = kzalloc(sizeof(*pd), GFP_KERNEL); > - if (!pd) > + if (!pd) { > + of_node_put(np); > return -ENOMEM; > + } While technically this patch is correct, the system will be dead anyway if it ever goes OOM at core_initcall() time. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Mon, 12 Oct 2015, Geert Uytterhoeven wrote: > Hi Julia, > > On Sat, Oct 10, 2015 at 2:30 PM, Julia Lawall <Julia.Lawall@lip6.fr> wrote: > > --- a/arch/arm/mach-shmobile/pm-rmobile.c > > +++ b/arch/arm/mach-shmobile/pm-rmobile.c > > @@ -313,8 +313,10 @@ static int __init rmobile_add_pm_domains(void __iomem *base, > > } > > > > pd = kzalloc(sizeof(*pd), GFP_KERNEL); > > - if (!pd) > > + if (!pd) { > > + of_node_put(np); > > return -ENOMEM; > > + } > > While technically this patch is correct, the system will be dead anyway if it > ever goes OOM at core_initcall() time. Maybe it would be better for the code to be correct to serve as an example (or to avoid serving as a bad example) for others? julia
Hi Julia, On Mon, Oct 12, 2015 at 9:24 AM, Julia Lawall <julia.lawall@lip6.fr> wrote: > On Mon, 12 Oct 2015, Geert Uytterhoeven wrote: >> On Sat, Oct 10, 2015 at 2:30 PM, Julia Lawall <Julia.Lawall@lip6.fr> wrote: >> > --- a/arch/arm/mach-shmobile/pm-rmobile.c >> > +++ b/arch/arm/mach-shmobile/pm-rmobile.c >> > @@ -313,8 +313,10 @@ static int __init rmobile_add_pm_domains(void __iomem *base, >> > } >> > >> > pd = kzalloc(sizeof(*pd), GFP_KERNEL); >> > - if (!pd) >> > + if (!pd) { >> > + of_node_put(np); >> > return -ENOMEM; >> > + } >> >> While technically this patch is correct, the system will be dead anyway if it >> ever goes OOM at core_initcall() time. > > Maybe it would be better for the code to be correct to serve as an example > (or to avoid serving as a bad example) for others? Sure, as it's only a single call, that's fine for me. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Dear Geert Uytterhoeven, On Mon, 12 Oct 2015 09:18:52 +0200, Geert Uytterhoeven wrote: > > pd = kzalloc(sizeof(*pd), GFP_KERNEL); > > - if (!pd) > > + if (!pd) { > > + of_node_put(np); > > return -ENOMEM; > > + } > > While technically this patch is correct, the system will be dead anyway if it > ever goes OOM at core_initcall() time. Then BUG_ON(!pd); ? Thomas
On Mon, Oct 12, 2015 at 9:29 AM, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote: > On Mon, 12 Oct 2015 09:18:52 +0200, Geert Uytterhoeven wrote: > >> > pd = kzalloc(sizeof(*pd), GFP_KERNEL); >> > - if (!pd) >> > + if (!pd) { >> > + of_node_put(np); >> > return -ENOMEM; >> > + } >> >> While technically this patch is correct, the system will be dead anyway if it >> ever goes OOM at core_initcall() time. > > Then BUG_ON(!pd); ? kzalloc() will scream anyway. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
diff --git a/arch/arm/mach-shmobile/pm-rmobile.c b/arch/arm/mach-shmobile/pm-rmobile.c index 89068c8..46d0a1d 100644 --- a/arch/arm/mach-shmobile/pm-rmobile.c +++ b/arch/arm/mach-shmobile/pm-rmobile.c @@ -313,8 +313,10 @@ static int __init rmobile_add_pm_domains(void __iomem *base, } pd = kzalloc(sizeof(*pd), GFP_KERNEL); - if (!pd) + if (!pd) { + of_node_put(np); return -ENOMEM; + } pd->genpd.name = np->name; pd->base = base;
for_each_child_of_node performs an of_node_get on each iteration, so a break out of the loop requires an of_node_put. The semantic patch that fixes this problem is as follows (http://coccinelle.lip6.fr): // <smpl> @@ expression root,e; local idexpression child; @@ for_each_child_of_node(root, child) { ... when != of_node_put(child) when != e = child ( return child; | + of_node_put(child); ? return ...; ) ... } // </smpl> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr> --- arch/arm/mach-shmobile/pm-rmobile.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)