Proposed prom parse fix + moving.

Proposed prom parse fix + moving.

[Michal Simek]

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

Re: Proposed prom parse fix + moving.

[Grant Likely]

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.

Re: Proposed prom parse fix + moving.

[Benjamin Herrenschmidt]

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.

Re: Proposed prom parse fix + moving.

[Michal Simek]

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

Re: Proposed prom parse fix + moving.

[Ilpo Järvinen]

[-- 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.

Re: Proposed prom parse fix + moving.

[Ilpo Järvinen]

[-- 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

Re: Proposed prom parse fix + moving.

[Grant Likely]

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.

Re: Proposed prom parse fix + moving.

[Grant Likely]

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.