[PATCH] powerpc/mpc5xxx: Use of_get_next_parent to simplify code

[PATCH] powerpc/mpc5xxx: Use of_get_next_parent to simplify code

[Christophe JAILLET]

of_get_next_parent can be used to simplify the while() loop and
avoid the need of a temp variable.

Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
 arch/powerpc/sysdev/mpc5xxx_clocks.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/arch/powerpc/sysdev/mpc5xxx_clocks.c b/arch/powerpc/sysdev/mpc5xxx_clocks.c
index f4f0301..5732926 100644
--- a/arch/powerpc/sysdev/mpc5xxx_clocks.c
+++ b/arch/powerpc/sysdev/mpc5xxx_clocks.c
@@ -13,7 +13,6 @@
 
 unsigned long mpc5xxx_get_bus_frequency(struct device_node *node)
 {
-	struct device_node *np;
 	const unsigned int *p_bus_freq = NULL;
 
 	of_node_get(node);
@@ -22,9 +21,7 @@ unsigned long mpc5xxx_get_bus_frequency(struct device_node *node)
 		if (p_bus_freq)
 			break;
 
-		np = of_get_parent(node);
-		of_node_put(node);
-		node = np;
+		node = of_get_next_parent(node);
 	}
 	of_node_put(node);
 
-- 
2.1.4

Re: [PATCH] powerpc/mpc5xxx: Use of_get_next_parent to simplify code

[Julia Lawall]

On Sun, 11 Oct 2015, Christophe JAILLET wrote:

> of_get_next_parent can be used to simplify the while() loop and
> avoid the need of a temp variable.

Can you do something with the loop in __of_translate_address, in 
drivers/of/address.c?  Is there not an iterator for this?

julia


> 
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
>  arch/powerpc/sysdev/mpc5xxx_clocks.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/arch/powerpc/sysdev/mpc5xxx_clocks.c b/arch/powerpc/sysdev/mpc5xxx_clocks.c
> index f4f0301..5732926 100644
> --- a/arch/powerpc/sysdev/mpc5xxx_clocks.c
> +++ b/arch/powerpc/sysdev/mpc5xxx_clocks.c
> @@ -13,7 +13,6 @@
>  
>  unsigned long mpc5xxx_get_bus_frequency(struct device_node *node)
>  {
> -	struct device_node *np;
>  	const unsigned int *p_bus_freq = NULL;
>  
>  	of_node_get(node);
> @@ -22,9 +21,7 @@ unsigned long mpc5xxx_get_bus_frequency(struct device_node *node)
>  		if (p_bus_freq)
>  			break;
>  
> -		np = of_get_parent(node);
> -		of_node_put(node);
> -		node = np;
> +		node = of_get_next_parent(node);
>  	}
>  	of_node_put(node);
>  
> -- 
> 2.1.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] powerpc/mpc5xxx: Use of_get_next_parent to simplify code

[Christophe JAILLET]

Le 11/10/2015 22:44, Julia Lawall a écrit :
>
>> of_get_next_parent can be used to simplify the while() loop and
>> avoid the need of a temp variable.
> Can you do something with the loop in __of_translate_address, in
> drivers/of/address.c?  Is there not an iterator for this?
>
> julia
>

Hi Julia,

There does not seem to be any 'for_each_parent_of_node' or equivalent.

Best regards,
CJ

Re: powerpc/mpc5xxx: Use of_get_next_parent to simplify code

[Michael Ellerman]

On Sun, 2015-11-10 at 20:27:40 UTC, Christophe Jaillet wrote:
> of_get_next_parent can be used to simplify the while() loop and
> avoid the need of a temp variable.
> 
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
>  arch/powerpc/sysdev/mpc5xxx_clocks.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/arch/powerpc/sysdev/mpc5xxx_clocks.c b/arch/powerpc/sysdev/mpc5xxx_clocks.c
> index f4f0301..5732926 100644
> --- a/arch/powerpc/sysdev/mpc5xxx_clocks.c
> +++ b/arch/powerpc/sysdev/mpc5xxx_clocks.c
> @@ -13,7 +13,6 @@
>  
>  unsigned long mpc5xxx_get_bus_frequency(struct device_node *node)
>  {
> -	struct device_node *np;
>  	const unsigned int *p_bus_freq = NULL;
>  
>  	of_node_get(node);
> @@ -22,9 +21,7 @@ unsigned long mpc5xxx_get_bus_frequency(struct device_node *node)
>  		if (p_bus_freq)
>  			break;
>  
> -		np = of_get_parent(node);
> -		of_node_put(node);
> -		node = np;
> +		node = of_get_next_parent(node);
>  	}
>  	of_node_put(node);


This conversion is OK, but the logic in the function is still wrong.

It uses of_get_property() inside the loop, but then drops the reference to the
node before dereferencing the p_bus_freq pointer, which could by then point to
junk if the node has been freed.

Instead it should use of_property_read_u32() to actually read the property
value before dropping the reference.

cheers

[PATCH v2] powerpc/mpc5xxx: Avoid dereferencing potentially freed memory

[Christophe JAILLET]

Use 'of_property_read_u32()' instead of 'of_get_property()'+pointer
dereference in order to avoid access to potentially freed memory.

Use 'of_get_next_parent()' to simplify the while() loop and avoid the
need of a temp variable.

Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
v2: Use of_property_read_u32 instead of of_get_property+pointer dereference
*** Untested ***
---
 arch/powerpc/sysdev/mpc5xxx_clocks.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/arch/powerpc/sysdev/mpc5xxx_clocks.c b/arch/powerpc/sysdev/mpc5xxx_clocks.c
index f4f0301..92fbcf8 100644
--- a/arch/powerpc/sysdev/mpc5xxx_clocks.c
+++ b/arch/powerpc/sysdev/mpc5xxx_clocks.c
@@ -13,21 +13,17 @@
 
 unsigned long mpc5xxx_get_bus_frequency(struct device_node *node)
 {
-	struct device_node *np;
-	const unsigned int *p_bus_freq = NULL;
+	u32 bus_freq = 0;
 
 	of_node_get(node);
 	while (node) {
-		p_bus_freq = of_get_property(node, "bus-frequency", NULL);
-		if (p_bus_freq)
+		if (!of_property_read_u32(node, "bus-frequency", &bus_freq))
 			break;
 
-		np = of_get_parent(node);
-		of_node_put(node);
-		node = np;
+		node = of_get_next_parent(node);
 	}
 	of_node_put(node);
 
-	return p_bus_freq ? *p_bus_freq : 0;
+	return bus_freq;
 }
 EXPORT_SYMBOL(mpc5xxx_get_bus_frequency);
-- 
2.1.4

Re: [PATCH v2] powerpc/mpc5xxx: Avoid dereferencing potentially freed memory

[Michael Ellerman]

On Thu, 2015-10-15 at 07:56 +0200, Christophe JAILLET wrote:
> Use 'of_property_read_u32()' instead of 'of_get_property()'+pointer
> dereference in order to avoid access to potentially freed memory.
> 
> Use 'of_get_next_parent()' to simplify the while() loop and avoid the
> need of a temp variable.
> 
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> v2: Use of_property_read_u32 instead of of_get_property+pointer dereference
> *** Untested ***

Thanks.

Can someone with an mpc5xxx test this?

cheers

Re: [PATCH v2] powerpc/mpc5xxx: Avoid dereferencing potentially freed memory

[Christophe JAILLET]

Le 15/10/2015 08:36, Michael Ellerman a écrit :
> On Thu, 2015-10-15 at 07:56 +0200, Christophe JAILLET wrote:
>> Use 'of_property_read_u32()' instead of 'of_get_property()'+pointer
>> dereference in order to avoid access to potentially freed memory.
>>
>> Use 'of_get_next_parent()' to simplify the while() loop and avoid the
>> need of a temp variable.
>>
>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>> ---
>> v2: Use of_property_read_u32 instead of of_get_property+pointer dereference
>> *** Untested ***
> Thanks.
>
> Can someone with an mpc5xxx test this?
>
> cheers
>

Hi,
I don't think it is an issue, but while looking at another similar 
patch, I noticed that the proposed patch adds a call to be32_to_cpup() 
(within of_property_read_u32).
Apparently, powerPC is a BE architecture, so this call should be a no-op.

Just wanted to point it out, in case of.

Best regards,
CJ

Re: [PATCH v2] powerpc/mpc5xxx: Avoid dereferencing potentially freed memory

[Gabriel Paubert]

On Fri, Oct 16, 2015 at 08:20:13AM +0200, Christophe JAILLET wrote:
> Le 15/10/2015 08:36, Michael Ellerman a écrit :
> >On Thu, 2015-10-15 at 07:56 +0200, Christophe JAILLET wrote:
> >>Use 'of_property_read_u32()' instead of 'of_get_property()'+pointer
> >>dereference in order to avoid access to potentially freed memory.
> >>
> >>Use 'of_get_next_parent()' to simplify the while() loop and avoid the
> >>need of a temp variable.
> >>
> >>Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> >>---
> >>v2: Use of_property_read_u32 instead of of_get_property+pointer dereference
> >>*** Untested ***
> >Thanks.
> >
> >Can someone with an mpc5xxx test this?
> >
> >cheers
> >
> 
> Hi,
> I don't think it is an issue, but while looking at another similar
> patch, I noticed that the proposed patch adds a call to
> be32_to_cpup() (within of_property_read_u32).
> Apparently, powerPC is a BE architecture, so this call should be a no-op.

Sadly no more. 32 bit is BE only, but 64 bit can be either BEtter or
LEsser.

    Gabriel

Re: [PATCH v2] powerpc/mpc5xxx: Avoid dereferencing potentially freed memory

[Michael Ellerman]

On Fri, 2015-10-16 at 08:20 +0200, Christophe JAILLET wrote:
> Le 15/10/2015 08:36, Michael Ellerman a écrit :
> > On Thu, 2015-10-15 at 07:56 +0200, Christophe JAILLET wrote:
> > > Use 'of_property_read_u32()' instead of
> > > 'of_get_property()'+pointer
> > > dereference in order to avoid access to potentially freed memory.
> > > 
> > > Use 'of_get_next_parent()' to simplify the while() loop and avoid
> > > the
> > > need of a temp variable.
> > > 
> > > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> > > ---
> > > v2: Use of_property_read_u32 instead of of_get_property+pointer
> > > dereference
> > > *** Untested ***
> > Thanks.
> > 
> > Can someone with an mpc5xxx test this?
> 
> Hi,
> I don't think it is an issue, but while looking at another similar 
> patch, I noticed that the proposed patch adds a call to
> be32_to_cpup() 
> (within of_property_read_u32).
> Apparently, powerPC is a BE architecture, so this call should be a no
> -op.
> 
> Just wanted to point it out, in case of.

Hi Christoph,

I'm not sure I follow.

The device tree is always big endian, but of_property_read_u32() does
the
conversion to CPU endian for you already. That is one of the advantages
of
using it.

cheers

Re: [PATCH v2] powerpc/mpc5xxx: Avoid dereferencing potentially freed memory

[Christophe JAILLET]

Le 16/10/2015 11:49, Michael Ellerman a écrit :
> On Fri, 2015-10-16 at 08:20 +0200, Christophe JAILLET wrote:
>> Le 15/10/2015 08:36, Michael Ellerman a écrit :
>>> On Thu, 2015-10-15 at 07:56 +0200, Christophe JAILLET wrote:
>>>> Use 'of_property_read_u32()' instead of
>>>> 'of_get_property()'+pointer
>>>> dereference in order to avoid access to potentially freed memory.
>>>>
>>>> Use 'of_get_next_parent()' to simplify the while() loop and avoid
>>>> the
>>>> need of a temp variable.
>>>>
>>>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>>>> ---
>>>> v2: Use of_property_read_u32 instead of of_get_property+pointer
>>>> dereference
>>>> *** Untested ***
>>> Thanks.
>>>
>>> Can someone with an mpc5xxx test this?
>> Hi,
>> I don't think it is an issue, but while looking at another similar
>> patch, I noticed that the proposed patch adds a call to
>> be32_to_cpup()
>> (within of_property_read_u32).
>> Apparently, powerPC is a BE architecture, so this call should be a no
>> -op.
>>
>> Just wanted to point it out, in case of.
> Hi Christoph,
>
> I'm not sure I follow.
>
> The device tree is always big endian, but of_property_read_u32() does
> the
> conversion to CPU endian for you already. That is one of the advantages
> of
> using it.
>
> cheers
>

Hi,
sorry if un-clear.

What I mean is that in the patch related 
'powerpc/sysdev/mpc5xxx_clocks.c', there was no call to 'be32_to_cpup'.
So in the proposed patch, 'of_property_read_u32' adds it.

While in the patch against 'powerpc/kernel/prom.c', 'be32_to_cpup' was 
called explicitly.
So using 'of_property_read_u32' keep the same logic.


Basically the code from 'mpc5xxx_clocks.c' and from 'prom.c' were 
written the same way. I found spurious that a call to 'be32_to_cpup' was 
done in only one case.
Maybe, it was a missing in 'mpc5xxx_clocks.c'.


I don't know if it can be an issue or not. I just find it 'strange'.


CJ

Re: [PATCH v2] powerpc/mpc5xxx: Avoid dereferencing potentially freed memory

[Michael Ellerman]

On Fri, 2015-10-16 at 22:05 +0200, Christophe JAILLET wrote:
> Hi,
> sorry if un-clear.
> 
> What I mean is that in the patch related 
> 'powerpc/sysdev/mpc5xxx_clocks.c', there was no call to 'be32_to_cpup'.
> So in the proposed patch, 'of_property_read_u32' adds it.
> 
> While in the patch against 'powerpc/kernel/prom.c', 'be32_to_cpup' was 
> called explicitly.
> So using 'of_property_read_u32' keep the same logic.

Ah right, I understand now.

> Basically the code from 'mpc5xxx_clocks.c' and from 'prom.c' were 
> written the same way. I found spurious that a call to 'be32_to_cpup' was 
> done in only one case.
> Maybe, it was a missing in 'mpc5xxx_clocks.c'.

Yes it was missing in that code.

But that's not a real bug because that code only ever runs on BE systems.

cheers

Re: powerpc/mpc5xxx: Use of_get_next_parent to simplify code

[Michael Ellerman]

On Sun, 2015-11-10 at 20:27:40 UTC, Christophe Jaillet wrote:
> of_get_next_parent can be used to simplify the while() loop and
> avoid the need of a temp variable.
> 
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/b340587e68b479e52039f800

cheers