| Message ID | Pine.LNX.4.64.0904180057290.2522@wrl-59.cs.helsinki.fi (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Grant Likely |
| Headers | show |
On Fri, Apr 17, 2009 at 4:07 PM, Ilpo Järvinen <ilpo.jarvinen@helsinki.fi> wrote: > On Fri, 17 Apr 2009, Ilpo Järvinen wrote: > >> On Fri, 17 Apr 2009, Michal Simek wrote: >> >> > Grant Likely wrote: >> > > On Fri, Apr 17, 2009 at 12:08 AM, Michal Simek <monstr@monstr.eu> wrote: >> > >> Hi All, >> > >> >> > >> I have got email from Ilpo about prom_parse file. >> > >> I take this file from powerpc. Who did write prom_parse file and take care about? >> > > >> > > Posting to the linuxppc-dev list is sufficient to start. There are >> > > several people who may be interested. >> > > >> > >> BTW: What about to move prom_parse file to any generic location as we discussed in past? >> > >> Any volunteer? >> > > >> > > I'm kind of working on it. More specifically, I'm looking at >> > > factoring out fdt stuff into common code (drivers/of/of_fdt.c). But I >> > > haven't made a whole lot of progress yet. >> > > >> > >> -------- Original Message -------- >> > >> Subject: [RFC!] [PATCH] microblaze: fix bug in error handling >> > >> Date: Thu, 16 Apr 2009 23:05:53 +0300 (EEST) >> > >> From: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi> >> > >> To: monstr@monstr.eu >> > >> CC: microblaze-uclinux@itee.uq.edu.au >> > >> >> > >> While some version of the patches were on the lkml I read >> > >> some part of the code briefly through but my feedback got >> > >> stuck into postponed emails, so here's one correctness >> > >> related issue I might have found (the rest were just >> > >> cosmetic things). >> > >> >> > >> I'm not sure if the latter return needs the of_node_put or not >> > >> but it seems more likely than not. >> > > >> > > Yes, it does. This change is applicable to >> > > arch/powerpc/kernel/prom_parse.c too. >> > >> > ok. >> > Ilpo: Can you create patch for both architectures? >> >> Sure, but tomorrow as today is a deadline day :-). > > Ok, here's combined patch for both. I came up with slightly > shorter and (IMHO) nicer variant for -EINVAL assignment than > in the first version. > > -- > [PATCH] powerpc & microblaze: add missing of_node_put to error handling > > While reviewing some microblaze patches a while ago, I noticed > a suspicious error handling in of_irq_map_one(), which turned > out to be a copy from arch/powerpc. Grant Likely > <grant.likely@secretlab.ca> confirmed that this is a real bug. > > Merge error handling paths using goto with the normal return > path. > > Powerppc compile tested. > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi> Looks right to me (but I haven't tested). Acked-by: Grant Likely <grant.likely@secretlab.ca> > --- > arch/microblaze/kernel/prom_parse.c | 11 +++++------ > arch/powerpc/kernel/prom_parse.c | 11 +++++------ > 2 files changed, 10 insertions(+), 12 deletions(-) > > diff --git a/arch/microblaze/kernel/prom_parse.c b/arch/microblaze/kernel/prom_parse.c > index ae0352e..d16c32f 100644 > --- a/arch/microblaze/kernel/prom_parse.c > +++ b/arch/microblaze/kernel/prom_parse.c > @@ -903,7 +903,7 @@ int of_irq_map_one(struct device_node *device, > struct device_node *p; > const u32 *intspec, *tmp, *addr; > u32 intsize, intlen; > - int res; > + int res = -EINVAL; > > pr_debug("of_irq_map_one: dev=%s, index=%d\n", > device->full_name, index); > @@ -926,21 +926,20 @@ int of_irq_map_one(struct device_node *device, > > /* Get size of interrupt specifier */ > tmp = of_get_property(p, "#interrupt-cells", NULL); > - if (tmp == NULL) { > - of_node_put(p); > - return -EINVAL; > - } > + if (tmp == NULL) > + goto out; > intsize = *tmp; > > pr_debug(" intsize=%d intlen=%d\n", intsize, intlen); > > /* Check index */ > if ((index + 1) * intsize > intlen) > - return -EINVAL; > + goto out; > > /* Get new specifier and map it */ > res = of_irq_map_raw(p, intspec + index * intsize, intsize, > addr, out_irq); > +out: > of_node_put(p); > return res; > } > diff --git a/arch/powerpc/kernel/prom_parse.c b/arch/powerpc/kernel/prom_parse.c > index 8f0856f..8362620 100644 > --- a/arch/powerpc/kernel/prom_parse.c > +++ b/arch/powerpc/kernel/prom_parse.c > @@ -971,7 +971,7 @@ int of_irq_map_one(struct device_node *device, int index, struct of_irq *out_irq > struct device_node *p; > const u32 *intspec, *tmp, *addr; > u32 intsize, intlen; > - int res; > + int res = -EINVAL; > > DBG("of_irq_map_one: dev=%s, index=%d\n", device->full_name, index); > > @@ -995,21 +995,20 @@ int of_irq_map_one(struct device_node *device, int index, struct of_irq *out_irq > > /* Get size of interrupt specifier */ > tmp = of_get_property(p, "#interrupt-cells", NULL); > - if (tmp == NULL) { > - of_node_put(p); > - return -EINVAL; > - } > + if (tmp == NULL) > + goto out; > intsize = *tmp; > > DBG(" intsize=%d intlen=%d\n", intsize, intlen); > > /* Check index */ > if ((index + 1) * intsize > intlen) > - return -EINVAL; > + goto out; > > /* Get new specifier and map it */ > res = of_irq_map_raw(p, intspec + index * intsize, intsize, > addr, out_irq); > +out: > of_node_put(p); > return res; > } > -- > 1.5.6.5 >
On Fri, Apr 17, 2009 at 11:35 PM, Grant Likely <grant.likely@secretlab.ca> wrote: > On Fri, Apr 17, 2009 at 4:07 PM, Ilpo Järvinen >> [PATCH] powerpc & microblaze: add missing of_node_put to error handling >> >> While reviewing some microblaze patches a while ago, I noticed >> a suspicious error handling in of_irq_map_one(), which turned >> out to be a copy from arch/powerpc. Grant Likely >> <grant.likely@secretlab.ca> confirmed that this is a real bug. >> >> Merge error handling paths using goto with the normal return >> path. >> >> Powerppc compile tested. >> >> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi> > > Looks right to me (but I haven't tested). > > Acked-by: Grant Likely <grant.likely@secretlab.ca> I've picked up the powerpc half of this patch, tested it, and pushed it out to my -next tree and asked Ben or Paul to pull. Michal, I leave it to you to pick up the Microblaze half. Cheers, g. > >> --- >> arch/microblaze/kernel/prom_parse.c | 11 +++++------ >> arch/powerpc/kernel/prom_parse.c | 11 +++++------ >> 2 files changed, 10 insertions(+), 12 deletions(-) >> >> diff --git a/arch/microblaze/kernel/prom_parse.c b/arch/microblaze/kernel/prom_parse.c >> index ae0352e..d16c32f 100644 >> --- a/arch/microblaze/kernel/prom_parse.c >> +++ b/arch/microblaze/kernel/prom_parse.c >> @@ -903,7 +903,7 @@ int of_irq_map_one(struct device_node *device, >> struct device_node *p; >> const u32 *intspec, *tmp, *addr; >> u32 intsize, intlen; >> - int res; >> + int res = -EINVAL; >> >> pr_debug("of_irq_map_one: dev=%s, index=%d\n", >> device->full_name, index); >> @@ -926,21 +926,20 @@ int of_irq_map_one(struct device_node *device, >> >> /* Get size of interrupt specifier */ >> tmp = of_get_property(p, "#interrupt-cells", NULL); >> - if (tmp == NULL) { >> - of_node_put(p); >> - return -EINVAL; >> - } >> + if (tmp == NULL) >> + goto out; >> intsize = *tmp; >> >> pr_debug(" intsize=%d intlen=%d\n", intsize, intlen); >> >> /* Check index */ >> if ((index + 1) * intsize > intlen) >> - return -EINVAL; >> + goto out; >> >> /* Get new specifier and map it */ >> res = of_irq_map_raw(p, intspec + index * intsize, intsize, >> addr, out_irq); >> +out: >> of_node_put(p); >> return res; >> } >> diff --git a/arch/powerpc/kernel/prom_parse.c b/arch/powerpc/kernel/prom_parse.c >> index 8f0856f..8362620 100644 >> --- a/arch/powerpc/kernel/prom_parse.c >> +++ b/arch/powerpc/kernel/prom_parse.c >> @@ -971,7 +971,7 @@ int of_irq_map_one(struct device_node *device, int index, struct of_irq *out_irq >> struct device_node *p; >> const u32 *intspec, *tmp, *addr; >> u32 intsize, intlen; >> - int res; >> + int res = -EINVAL; >> >> DBG("of_irq_map_one: dev=%s, index=%d\n", device->full_name, index); >> >> @@ -995,21 +995,20 @@ int of_irq_map_one(struct device_node *device, int index, struct of_irq *out_irq >> >> /* Get size of interrupt specifier */ >> tmp = of_get_property(p, "#interrupt-cells", NULL); >> - if (tmp == NULL) { >> - of_node_put(p); >> - return -EINVAL; >> - } >> + if (tmp == NULL) >> + goto out; >> intsize = *tmp; >> >> DBG(" intsize=%d intlen=%d\n", intsize, intlen); >> >> /* Check index */ >> if ((index + 1) * intsize > intlen) >> - return -EINVAL; >> + goto out; >> >> /* Get new specifier and map it */ >> res = of_irq_map_raw(p, intspec + index * intsize, intsize, >> addr, out_irq); >> +out: >> of_node_put(p); >> return res; >> } >> -- >> 1.5.6.5 >> > > > > -- > Grant Likely, B.Sc., P.Eng. > Secret Lab Technologies Ltd. >
diff --git a/arch/microblaze/kernel/prom_parse.c b/arch/microblaze/kernel/prom_parse.c index ae0352e..d16c32f 100644 --- a/arch/microblaze/kernel/prom_parse.c +++ b/arch/microblaze/kernel/prom_parse.c @@ -903,7 +903,7 @@ int of_irq_map_one(struct device_node *device, struct device_node *p; const u32 *intspec, *tmp, *addr; u32 intsize, intlen; - int res; + int res = -EINVAL; pr_debug("of_irq_map_one: dev=%s, index=%d\n", device->full_name, index); @@ -926,21 +926,20 @@ int of_irq_map_one(struct device_node *device, /* Get size of interrupt specifier */ tmp = of_get_property(p, "#interrupt-cells", NULL); - if (tmp == NULL) { - of_node_put(p); - return -EINVAL; - } + if (tmp == NULL) + goto out; intsize = *tmp; pr_debug(" intsize=%d intlen=%d\n", intsize, intlen); /* Check index */ if ((index + 1) * intsize > intlen) - return -EINVAL; + goto out; /* Get new specifier and map it */ res = of_irq_map_raw(p, intspec + index * intsize, intsize, addr, out_irq); +out: of_node_put(p); return res; } diff --git a/arch/powerpc/kernel/prom_parse.c b/arch/powerpc/kernel/prom_parse.c index 8f0856f..8362620 100644 --- a/arch/powerpc/kernel/prom_parse.c +++ b/arch/powerpc/kernel/prom_parse.c @@ -971,7 +971,7 @@ int of_irq_map_one(struct device_node *device, int index, struct of_irq *out_irq struct device_node *p; const u32 *intspec, *tmp, *addr; u32 intsize, intlen; - int res; + int res = -EINVAL; DBG("of_irq_map_one: dev=%s, index=%d\n", device->full_name, index); @@ -995,21 +995,20 @@ int of_irq_map_one(struct device_node *device, int index, struct of_irq *out_irq /* Get size of interrupt specifier */ tmp = of_get_property(p, "#interrupt-cells", NULL); - if (tmp == NULL) { - of_node_put(p); - return -EINVAL; - } + if (tmp == NULL) + goto out; intsize = *tmp; DBG(" intsize=%d intlen=%d\n", intsize, intlen); /* Check index */ if ((index + 1) * intsize > intlen) - return -EINVAL; + goto out; /* Get new specifier and map it */ res = of_irq_map_raw(p, intspec + index * intsize, intsize, addr, out_irq); +out: of_node_put(p); return res; }