* Proposed prom parse fix + moving. @ 2009-04-17 6:08 Michal Simek 2009-04-17 6:44 ` Grant Likely 2009-04-17 7:43 ` Benjamin Herrenschmidt 0 siblings, 2 replies; 8+ messages in thread From: Michal Simek @ 2009-04-17 6:08 UTC (permalink / raw) To: linuxppc-dev; +Cc: ilpo.jarvinen, Arnd Bergmann 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? BTW: What about to move prom_parse file to any generic location as we discussed in past? Any volunteer? Thanks, Michal -------- 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. Not even compile tested. Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi> --- arch/microblaze/kernel/prom_parse.c | 11 +++++++---- 1 files changed, 7 insertions(+), 4 deletions(-) diff --git a/arch/microblaze/kernel/prom_parse.c b/arch/microblaze/kernel/prom_parse.c index ae0352e..d1174bc 100644 --- a/arch/microblaze/kernel/prom_parse.c +++ b/arch/microblaze/kernel/prom_parse.c @@ -927,20 +927,23 @@ 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; + res = -EINVAL; + goto out; } intsize = *tmp; pr_debug(" intsize=%d intlen=%d\n", intsize, intlen); /* Check index */ - if ((index + 1) * intsize > intlen) - return -EINVAL; + if ((index + 1) * intsize > intlen) { + res = -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 -- Michal Simek, Ing. (M.Eng) w: www.monstr.eu p: +42-0-721842854 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: Proposed prom parse fix + moving. 2009-04-17 6:08 Proposed prom parse fix + moving Michal Simek @ 2009-04-17 6:44 ` Grant Likely 2009-04-17 12:59 ` Michal Simek 2009-04-17 7:43 ` Benjamin Herrenschmidt 1 sibling, 1 reply; 8+ messages in thread From: Grant Likely @ 2009-04-17 6:44 UTC (permalink / raw) To: monstr; +Cc: ilpo.jarvinen, linuxppc-dev, Arnd Bergmann 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 car= e 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 dis= cussed 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=E4rvinen <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. > > Not even compile tested. > > Signed-off-by: Ilpo J=E4rvinen <ilpo.jarvinen@helsinki.fi> > --- > =A0arch/microblaze/kernel/prom_parse.c | =A0 11 +++++++---- > =A01 files changed, 7 insertions(+), 4 deletions(-) > > diff --git a/arch/microblaze/kernel/prom_parse.c b/arch/microblaze/kernel= /prom_parse.c > index ae0352e..d1174bc 100644 > --- a/arch/microblaze/kernel/prom_parse.c > +++ b/arch/microblaze/kernel/prom_parse.c > @@ -927,20 +927,23 @@ int of_irq_map_one(struct device_node *device, > =A0 =A0 =A0 =A0/* Get size of interrupt specifier */ > =A0 =A0 =A0 =A0tmp =3D of_get_property(p, "#interrupt-cells", NULL); > =A0 =A0 =A0 =A0if (tmp =3D=3D NULL) { > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 of_node_put(p); > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -EINVAL; > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 res =3D -EINVAL; > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto out; > =A0 =A0 =A0 =A0} > =A0 =A0 =A0 =A0intsize =3D *tmp; > > =A0 =A0 =A0 =A0pr_debug(" intsize=3D%d intlen=3D%d\n", intsize, intlen); > > =A0 =A0 =A0 =A0/* Check index */ > - =A0 =A0 =A0 if ((index + 1) * intsize > intlen) > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -EINVAL; > + =A0 =A0 =A0 if ((index + 1) * intsize > intlen) { > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 res =3D -EINVAL; > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto out; > + =A0 =A0 =A0 } > > =A0 =A0 =A0 =A0/* Get new specifier and map it */ > =A0 =A0 =A0 =A0res =3D of_irq_map_raw(p, intspec + index * intsize, intsi= ze, > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0addr, out_= irq); > +out: > =A0 =A0 =A0 =A0of_node_put(p); > =A0 =A0 =A0 =A0return res; > =A0} > -- > 1.5.6.5 > > > -- > Michal Simek, Ing. (M.Eng) > w: www.monstr.eu p: +42-0-721842854 > --=20 Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Proposed prom parse fix + moving. 2009-04-17 6:44 ` Grant Likely @ 2009-04-17 12:59 ` Michal Simek 2009-04-17 13:02 ` Ilpo Järvinen 0 siblings, 1 reply; 8+ messages in thread From: Michal Simek @ 2009-04-17 12:59 UTC (permalink / raw) To: Grant Likely; +Cc: ilpo.jarvinen, linuxppc-dev, Arnd Bergmann 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? Thanks, Michal > >> Not even compile tested. >> >> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi> >> --- >> arch/microblaze/kernel/prom_parse.c | 11 +++++++---- >> 1 files changed, 7 insertions(+), 4 deletions(-) >> >> diff --git a/arch/microblaze/kernel/prom_parse.c b/arch/microblaze/kernel/prom_parse.c >> index ae0352e..d1174bc 100644 >> --- a/arch/microblaze/kernel/prom_parse.c >> +++ b/arch/microblaze/kernel/prom_parse.c >> @@ -927,20 +927,23 @@ 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; >> + res = -EINVAL; >> + goto out; >> } >> intsize = *tmp; >> >> pr_debug(" intsize=%d intlen=%d\n", intsize, intlen); >> >> /* Check index */ >> - if ((index + 1) * intsize > intlen) >> - return -EINVAL; >> + if ((index + 1) * intsize > intlen) { >> + res = -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 >> >> >> -- >> Michal Simek, Ing. (M.Eng) >> w: www.monstr.eu p: +42-0-721842854 >> > > > -- Michal Simek, Ing. (M.Eng) w: www.monstr.eu p: +42-0-721842854 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Proposed prom parse fix + moving. 2009-04-17 12:59 ` Michal Simek @ 2009-04-17 13:02 ` Ilpo Järvinen 2009-04-17 22:07 ` Ilpo Järvinen 0 siblings, 1 reply; 8+ messages in thread From: Ilpo Järvinen @ 2009-04-17 13:02 UTC (permalink / raw) To: Michal Simek; +Cc: linuxppc-dev, Arnd Bergmann [-- Attachment #1: Type: TEXT/PLAIN, Size: 1616 bytes --] 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 :-). -- i. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Proposed prom parse fix + moving. 2009-04-17 13:02 ` Ilpo Järvinen @ 2009-04-17 22:07 ` Ilpo Järvinen 2009-04-18 5:35 ` Grant Likely 0 siblings, 1 reply; 8+ messages in thread From: Ilpo Järvinen @ 2009-04-17 22:07 UTC (permalink / raw) To: benh; +Cc: linuxppc-dev, Michal Simek, Arnd Bergmann [-- Attachment #1: Type: TEXT/PLAIN, Size: 4686 bytes --] 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> --- 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: Proposed prom parse fix + moving. 2009-04-17 22:07 ` Ilpo Järvinen @ 2009-04-18 5:35 ` Grant Likely 2009-04-20 21:15 ` Grant Likely 0 siblings, 1 reply; 8+ messages in thread From: Grant Likely @ 2009-04-18 5:35 UTC (permalink / raw) To: Ilpo Järvinen; +Cc: Michal Simek, Arnd Bergmann, linuxppc-dev On Fri, Apr 17, 2009 at 4:07 PM, Ilpo J=E4rvinen <ilpo.jarvinen@helsinki.fi> wrote: > On Fri, 17 Apr 2009, Ilpo J=E4rvinen 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> wr= ote: >> > >> 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 ta= ke care about? >> > > >> > > Posting to the linuxppc-dev list is sufficient to start. =A0There ar= e >> > > 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. =A0More 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=E4rvinen <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. =A0This 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=E4rvinen <ilpo.jarvinen@helsinki.fi> Looks right to me (but I haven't tested). Acked-by: Grant Likely <grant.likely@secretlab.ca> > --- > =A0arch/microblaze/kernel/prom_parse.c | =A0 11 +++++------ > =A0arch/powerpc/kernel/prom_parse.c =A0 =A0| =A0 11 +++++------ > =A02 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, > =A0 =A0 =A0 =A0struct device_node *p; > =A0 =A0 =A0 =A0const u32 *intspec, *tmp, *addr; > =A0 =A0 =A0 =A0u32 intsize, intlen; > - =A0 =A0 =A0 int res; > + =A0 =A0 =A0 int res =3D -EINVAL; > > =A0 =A0 =A0 =A0pr_debug("of_irq_map_one: dev=3D%s, index=3D%d\n", > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0device->full_name, index); > @@ -926,21 +926,20 @@ int of_irq_map_one(struct device_node *device, > > =A0 =A0 =A0 =A0/* Get size of interrupt specifier */ > =A0 =A0 =A0 =A0tmp =3D of_get_property(p, "#interrupt-cells", NULL); > - =A0 =A0 =A0 if (tmp =3D=3D NULL) { > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 of_node_put(p); > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -EINVAL; > - =A0 =A0 =A0 } > + =A0 =A0 =A0 if (tmp =3D=3D NULL) > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto out; > =A0 =A0 =A0 =A0intsize =3D *tmp; > > =A0 =A0 =A0 =A0pr_debug(" intsize=3D%d intlen=3D%d\n", intsize, intlen); > > =A0 =A0 =A0 =A0/* Check index */ > =A0 =A0 =A0 =A0if ((index + 1) * intsize > intlen) > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -EINVAL; > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto out; > > =A0 =A0 =A0 =A0/* Get new specifier and map it */ > =A0 =A0 =A0 =A0res =3D of_irq_map_raw(p, intspec + index * intsize, intsi= ze, > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0addr, out_= irq); > +out: > =A0 =A0 =A0 =A0of_node_put(p); > =A0 =A0 =A0 =A0return res; > =A0} > 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 in= dex, struct of_irq *out_irq > =A0 =A0 =A0 =A0struct device_node *p; > =A0 =A0 =A0 =A0const u32 *intspec, *tmp, *addr; > =A0 =A0 =A0 =A0u32 intsize, intlen; > - =A0 =A0 =A0 int res; > + =A0 =A0 =A0 int res =3D -EINVAL; > > =A0 =A0 =A0 =A0DBG("of_irq_map_one: dev=3D%s, index=3D%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 > > =A0 =A0 =A0 =A0/* Get size of interrupt specifier */ > =A0 =A0 =A0 =A0tmp =3D of_get_property(p, "#interrupt-cells", NULL); > - =A0 =A0 =A0 if (tmp =3D=3D NULL) { > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 of_node_put(p); > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -EINVAL; > - =A0 =A0 =A0 } > + =A0 =A0 =A0 if (tmp =3D=3D NULL) > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto out; > =A0 =A0 =A0 =A0intsize =3D *tmp; > > =A0 =A0 =A0 =A0DBG(" intsize=3D%d intlen=3D%d\n", intsize, intlen); > > =A0 =A0 =A0 =A0/* Check index */ > =A0 =A0 =A0 =A0if ((index + 1) * intsize > intlen) > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -EINVAL; > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto out; > > =A0 =A0 =A0 =A0/* Get new specifier and map it */ > =A0 =A0 =A0 =A0res =3D of_irq_map_raw(p, intspec + index * intsize, intsi= ze, > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 addr, out_irq); > +out: > =A0 =A0 =A0 =A0of_node_put(p); > =A0 =A0 =A0 =A0return res; > =A0} > -- > 1.5.6.5 > --=20 Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Proposed prom parse fix + moving. 2009-04-18 5:35 ` Grant Likely @ 2009-04-20 21:15 ` Grant Likely 0 siblings, 0 replies; 8+ messages in thread From: Grant Likely @ 2009-04-20 21:15 UTC (permalink / raw) To: Ilpo Järvinen; +Cc: Michal Simek, Arnd Bergmann, linuxppc-dev 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=E4rvinen >> [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=E4rvinen <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. > >> --- >> =A0arch/microblaze/kernel/prom_parse.c | =A0 11 +++++------ >> =A0arch/powerpc/kernel/prom_parse.c =A0 =A0| =A0 11 +++++------ >> =A02 files changed, 10 insertions(+), 12 deletions(-) >> >> diff --git a/arch/microblaze/kernel/prom_parse.c b/arch/microblaze/kerne= l/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, >> =A0 =A0 =A0 =A0struct device_node *p; >> =A0 =A0 =A0 =A0const u32 *intspec, *tmp, *addr; >> =A0 =A0 =A0 =A0u32 intsize, intlen; >> - =A0 =A0 =A0 int res; >> + =A0 =A0 =A0 int res =3D -EINVAL; >> >> =A0 =A0 =A0 =A0pr_debug("of_irq_map_one: dev=3D%s, index=3D%d\n", >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0device->full_name, index)= ; >> @@ -926,21 +926,20 @@ int of_irq_map_one(struct device_node *device, >> >> =A0 =A0 =A0 =A0/* Get size of interrupt specifier */ >> =A0 =A0 =A0 =A0tmp =3D of_get_property(p, "#interrupt-cells", NULL); >> - =A0 =A0 =A0 if (tmp =3D=3D NULL) { >> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 of_node_put(p); >> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -EINVAL; >> - =A0 =A0 =A0 } >> + =A0 =A0 =A0 if (tmp =3D=3D NULL) >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto out; >> =A0 =A0 =A0 =A0intsize =3D *tmp; >> >> =A0 =A0 =A0 =A0pr_debug(" intsize=3D%d intlen=3D%d\n", intsize, intlen); >> >> =A0 =A0 =A0 =A0/* Check index */ >> =A0 =A0 =A0 =A0if ((index + 1) * intsize > intlen) >> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -EINVAL; >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto out; >> >> =A0 =A0 =A0 =A0/* Get new specifier and map it */ >> =A0 =A0 =A0 =A0res =3D of_irq_map_raw(p, intspec + index * intsize, ints= ize, >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0addr, out= _irq); >> +out: >> =A0 =A0 =A0 =A0of_node_put(p); >> =A0 =A0 =A0 =A0return res; >> =A0} >> 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 i= ndex, struct of_irq *out_irq >> =A0 =A0 =A0 =A0struct device_node *p; >> =A0 =A0 =A0 =A0const u32 *intspec, *tmp, *addr; >> =A0 =A0 =A0 =A0u32 intsize, intlen; >> - =A0 =A0 =A0 int res; >> + =A0 =A0 =A0 int res =3D -EINVAL; >> >> =A0 =A0 =A0 =A0DBG("of_irq_map_one: dev=3D%s, index=3D%d\n", device->ful= l_name, index); >> >> @@ -995,21 +995,20 @@ int of_irq_map_one(struct device_node *device, int= index, struct of_irq *out_irq >> >> =A0 =A0 =A0 =A0/* Get size of interrupt specifier */ >> =A0 =A0 =A0 =A0tmp =3D of_get_property(p, "#interrupt-cells", NULL); >> - =A0 =A0 =A0 if (tmp =3D=3D NULL) { >> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 of_node_put(p); >> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -EINVAL; >> - =A0 =A0 =A0 } >> + =A0 =A0 =A0 if (tmp =3D=3D NULL) >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto out; >> =A0 =A0 =A0 =A0intsize =3D *tmp; >> >> =A0 =A0 =A0 =A0DBG(" intsize=3D%d intlen=3D%d\n", intsize, intlen); >> >> =A0 =A0 =A0 =A0/* Check index */ >> =A0 =A0 =A0 =A0if ((index + 1) * intsize > intlen) >> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -EINVAL; >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto out; >> >> =A0 =A0 =A0 =A0/* Get new specifier and map it */ >> =A0 =A0 =A0 =A0res =3D of_irq_map_raw(p, intspec + index * intsize, ints= ize, >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 addr, out_irq); >> +out: >> =A0 =A0 =A0 =A0of_node_put(p); >> =A0 =A0 =A0 =A0return res; >> =A0} >> -- >> 1.5.6.5 >> > > > > -- > Grant Likely, B.Sc., P.Eng. > Secret Lab Technologies Ltd. > --=20 Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Proposed prom parse fix + moving. 2009-04-17 6:08 Proposed prom parse fix + moving Michal Simek 2009-04-17 6:44 ` Grant Likely @ 2009-04-17 7:43 ` Benjamin Herrenschmidt 1 sibling, 0 replies; 8+ messages in thread From: Benjamin Herrenschmidt @ 2009-04-17 7:43 UTC (permalink / raw) To: monstr; +Cc: linuxppc-dev, Arnd Bergmann, ilpo.jarvinen On Fri, 2009-04-17 at 08:08 +0200, Michal Simek 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? I wrote it :-) > BTW: What about to move prom_parse file to any generic location as we discussed in past? > Any volunteer? Well.. it relies on some bits and pieces that are rather powerpc specific afaik such as our irq mapping scheme. I suppose we could split those bits and move them to a separate file though. Off hand the patch looks ok but I'll have to look more closely when I get back from vacation. Cheers, Ben. ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2009-04-20 21:15 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2009-04-17 6:08 Proposed prom parse fix + moving Michal Simek 2009-04-17 6:44 ` Grant Likely 2009-04-17 12:59 ` Michal Simek 2009-04-17 13:02 ` Ilpo Järvinen 2009-04-17 22:07 ` Ilpo Järvinen 2009-04-18 5:35 ` Grant Likely 2009-04-20 21:15 ` Grant Likely 2009-04-17 7:43 ` Benjamin Herrenschmidt
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).