| Message ID | 1444888580-12966-1-git-send-email-christophe.jaillet@wanadoo.fr (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Anatolij Gustschin |
| Headers | show |
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
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
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
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
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
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
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);
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(-)