* [PATCH] powerpc/mpc5xxx: Use of_get_next_parent to simplify code @ 2015-10-11 20:27 ` Christophe JAILLET 0 siblings, 0 replies; 27+ messages in thread From: Christophe JAILLET @ 2015-10-11 20:27 UTC (permalink / raw) To: benh, paulus, mpe Cc: linuxppc-dev, linux-kernel, kernel-janitors, 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 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH] powerpc/mpc5xxx: Use of_get_next_parent to simplify code @ 2015-10-11 20:27 ` Christophe JAILLET 0 siblings, 0 replies; 27+ messages in thread From: Christophe JAILLET @ 2015-10-11 20:27 UTC (permalink / raw) To: benh, paulus, mpe Cc: linuxppc-dev, linux-kernel, kernel-janitors, 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 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH] powerpc/mpc5xxx: Use of_get_next_parent to simplify code 2015-10-11 20:27 ` Christophe JAILLET @ 2015-10-11 20:44 ` Julia Lawall -1 siblings, 0 replies; 27+ messages in thread From: Julia Lawall @ 2015-10-11 20:44 UTC (permalink / raw) To: Christophe JAILLET Cc: benh, paulus, mpe, linuxppc-dev, linux-kernel, kernel-janitors 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 > ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] powerpc/mpc5xxx: Use of_get_next_parent to simplify code @ 2015-10-11 20:44 ` Julia Lawall 0 siblings, 0 replies; 27+ messages in thread From: Julia Lawall @ 2015-10-11 20:44 UTC (permalink / raw) To: Christophe JAILLET Cc: benh, paulus, mpe, linuxppc-dev, linux-kernel, kernel-janitors 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 > ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] powerpc/mpc5xxx: Use of_get_next_parent to simplify code 2015-10-11 20:44 ` Julia Lawall @ 2015-10-12 5:05 ` Christophe JAILLET -1 siblings, 0 replies; 27+ messages in thread From: Christophe JAILLET @ 2015-10-12 5:05 UTC (permalink / raw) To: linux-kernel; +Cc: linuxppc-dev, kernel-janitors 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] powerpc/mpc5xxx: Use of_get_next_parent to simplify code @ 2015-10-12 5:05 ` Christophe JAILLET 0 siblings, 0 replies; 27+ messages in thread From: Christophe JAILLET @ 2015-10-12 5:05 UTC (permalink / raw) To: linuxppc-dev; +Cc: linux-kernel, kernel-janitors 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: powerpc/mpc5xxx: Use of_get_next_parent to simplify code 2015-10-11 20:27 ` Christophe JAILLET @ 2015-10-14 4:00 ` Michael Ellerman -1 siblings, 0 replies; 27+ messages in thread From: Michael Ellerman @ 2015-10-14 4:00 UTC (permalink / raw) To: Christophe Jaillet, benh, paulus Cc: kernel-janitors, Christophe JAILLET, linuxppc-dev, linux-kernel 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: powerpc/mpc5xxx: Use of_get_next_parent to simplify code @ 2015-10-14 4:00 ` Michael Ellerman 0 siblings, 0 replies; 27+ messages in thread From: Michael Ellerman @ 2015-10-14 4:00 UTC (permalink / raw) To: Christophe Jaillet, benh, paulus Cc: kernel-janitors, linuxppc-dev, linux-kernel 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v2] powerpc/mpc5xxx: Avoid dereferencing potentially freed memory 2015-10-14 4:00 ` Michael Ellerman @ 2015-10-15 5:56 ` Christophe JAILLET -1 siblings, 0 replies; 27+ messages in thread From: Christophe JAILLET @ 2015-10-15 5:56 UTC (permalink / raw) To: benh, paulus, mpe Cc: linuxppc-dev, linux-kernel, kernel-janitors, 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 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2] powerpc/mpc5xxx: Avoid dereferencing potentially freed memory @ 2015-10-15 5:56 ` Christophe JAILLET 0 siblings, 0 replies; 27+ messages in thread From: Christophe JAILLET @ 2015-10-15 5:56 UTC (permalink / raw) To: benh, paulus, mpe Cc: linuxppc-dev, linux-kernel, kernel-janitors, 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 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH v2] powerpc/mpc5xxx: Avoid dereferencing potentially freed memory 2015-10-15 5:56 ` Christophe JAILLET (?) @ 2015-10-15 6:36 ` Michael Ellerman -1 siblings, 0 replies; 27+ messages in thread From: Michael Ellerman @ 2015-10-15 6:36 UTC (permalink / raw) To: Christophe JAILLET Cc: benh, paulus, linuxppc-dev, linux-kernel, kernel-janitors 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] powerpc/mpc5xxx: Avoid dereferencing potentially freed memory @ 2015-10-15 6:36 ` Michael Ellerman 0 siblings, 0 replies; 27+ messages in thread From: Michael Ellerman @ 2015-10-15 6:36 UTC (permalink / raw) To: Christophe JAILLET Cc: benh, paulus, linuxppc-dev, linux-kernel, kernel-janitors 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] powerpc/mpc5xxx: Avoid dereferencing potentially freed memory @ 2015-10-15 6:36 ` Michael Ellerman 0 siblings, 0 replies; 27+ messages in thread From: Michael Ellerman @ 2015-10-15 6:36 UTC (permalink / raw) To: Christophe JAILLET Cc: benh, paulus, linuxppc-dev, linux-kernel, kernel-janitors 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] powerpc/mpc5xxx: Avoid dereferencing potentially freed memory 2015-10-15 6:36 ` Michael Ellerman @ 2015-10-16 6:20 ` Christophe JAILLET -1 siblings, 0 replies; 27+ messages in thread From: Christophe JAILLET @ 2015-10-16 6:20 UTC (permalink / raw) To: Michael Ellerman Cc: benh, paulus, linuxppc-dev, linux-kernel, kernel-janitors 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] powerpc/mpc5xxx: Avoid dereferencing potentially freed memory @ 2015-10-16 6:20 ` Christophe JAILLET 0 siblings, 0 replies; 27+ messages in thread From: Christophe JAILLET @ 2015-10-16 6:20 UTC (permalink / raw) To: Michael Ellerman Cc: benh, paulus, linuxppc-dev, linux-kernel, kernel-janitors 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 -- 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] powerpc/mpc5xxx: Avoid dereferencing potentially freed memory 2015-10-16 6:20 ` Christophe JAILLET @ 2015-10-16 7:15 ` Gabriel Paubert -1 siblings, 0 replies; 27+ messages in thread From: Gabriel Paubert @ 2015-10-16 7:15 UTC (permalink / raw) To: Christophe JAILLET Cc: Michael Ellerman, kernel-janitors, paulus, linuxppc-dev, linux-kernel 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] powerpc/mpc5xxx: Avoid dereferencing potentially freed memory @ 2015-10-16 7:15 ` Gabriel Paubert 0 siblings, 0 replies; 27+ messages in thread From: Gabriel Paubert @ 2015-10-16 7:15 UTC (permalink / raw) To: Christophe JAILLET Cc: Michael Ellerman, kernel-janitors, paulus, linuxppc-dev, linux-kernel 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 -- 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] powerpc/mpc5xxx: Avoid dereferencing potentially freed memory 2015-10-16 6:20 ` Christophe JAILLET (?) @ 2015-10-16 9:49 ` Michael Ellerman -1 siblings, 0 replies; 27+ messages in thread From: Michael Ellerman @ 2015-10-16 9:49 UTC (permalink / raw) To: Christophe JAILLET Cc: benh, paulus, linuxppc-dev, linux-kernel, kernel-janitors 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] powerpc/mpc5xxx: Avoid dereferencing potentially freed memory @ 2015-10-16 9:49 ` Michael Ellerman 0 siblings, 0 replies; 27+ messages in thread From: Michael Ellerman @ 2015-10-16 9:49 UTC (permalink / raw) To: Christophe JAILLET Cc: benh, paulus, linuxppc-dev, linux-kernel, kernel-janitors 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] powerpc/mpc5xxx: Avoid dereferencing potentially freed memory @ 2015-10-16 9:49 ` Michael Ellerman 0 siblings, 0 replies; 27+ messages in thread From: Michael Ellerman @ 2015-10-16 9:49 UTC (permalink / raw) To: Christophe JAILLET Cc: benh, paulus, linuxppc-dev, linux-kernel, kernel-janitors 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 -- 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] powerpc/mpc5xxx: Avoid dereferencing potentially freed memory 2015-10-16 9:49 ` Michael Ellerman @ 2015-10-16 20:05 ` Christophe JAILLET -1 siblings, 0 replies; 27+ messages in thread From: Christophe JAILLET @ 2015-10-16 20:05 UTC (permalink / raw) To: Michael Ellerman Cc: benh, paulus, linuxppc-dev, linux-kernel, kernel-janitors 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] powerpc/mpc5xxx: Avoid dereferencing potentially freed memory @ 2015-10-16 20:05 ` Christophe JAILLET 0 siblings, 0 replies; 27+ messages in thread From: Christophe JAILLET @ 2015-10-16 20:05 UTC (permalink / raw) To: Michael Ellerman Cc: benh, paulus, linuxppc-dev, linux-kernel, kernel-janitors 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] powerpc/mpc5xxx: Avoid dereferencing potentially freed memory 2015-10-16 20:05 ` Christophe JAILLET (?) @ 2015-10-19 4:38 ` Michael Ellerman -1 siblings, 0 replies; 27+ messages in thread From: Michael Ellerman @ 2015-10-19 4:38 UTC (permalink / raw) To: Christophe JAILLET Cc: benh, paulus, linuxppc-dev, linux-kernel, kernel-janitors 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] powerpc/mpc5xxx: Avoid dereferencing potentially freed memory @ 2015-10-19 4:38 ` Michael Ellerman 0 siblings, 0 replies; 27+ messages in thread From: Michael Ellerman @ 2015-10-19 4:38 UTC (permalink / raw) To: Christophe JAILLET Cc: benh, paulus, linuxppc-dev, linux-kernel, kernel-janitors 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] powerpc/mpc5xxx: Avoid dereferencing potentially freed memory @ 2015-10-19 4:38 ` Michael Ellerman 0 siblings, 0 replies; 27+ messages in thread From: Michael Ellerman @ 2015-10-19 4:38 UTC (permalink / raw) To: Christophe JAILLET Cc: benh, paulus, linuxppc-dev, linux-kernel, kernel-janitors 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: powerpc/mpc5xxx: Use of_get_next_parent to simplify code 2015-10-11 20:27 ` Christophe JAILLET @ 2015-10-15 11:11 ` Michael Ellerman -1 siblings, 0 replies; 27+ messages in thread From: Michael Ellerman @ 2015-10-15 11:11 UTC (permalink / raw) To: Christophe Jaillet, benh, paulus Cc: kernel-janitors, Christophe JAILLET, linuxppc-dev, linux-kernel 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: powerpc/mpc5xxx: Use of_get_next_parent to simplify code @ 2015-10-15 11:11 ` Michael Ellerman 0 siblings, 0 replies; 27+ messages in thread From: Michael Ellerman @ 2015-10-15 11:11 UTC (permalink / raw) To: Christophe Jaillet, benh, paulus Cc: kernel-janitors, linuxppc-dev, linux-kernel 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2015-10-19 4:38 UTC | newest] Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-10-11 20:27 [PATCH] powerpc/mpc5xxx: Use of_get_next_parent to simplify code Christophe JAILLET 2015-10-11 20:27 ` Christophe JAILLET 2015-10-11 20:44 ` Julia Lawall 2015-10-11 20:44 ` Julia Lawall 2015-10-12 5:05 ` Christophe JAILLET 2015-10-12 5:05 ` Christophe JAILLET 2015-10-14 4:00 ` Michael Ellerman 2015-10-14 4:00 ` Michael Ellerman 2015-10-15 5:56 ` [PATCH v2] powerpc/mpc5xxx: Avoid dereferencing potentially freed memory Christophe JAILLET 2015-10-15 5:56 ` Christophe JAILLET 2015-10-15 6:36 ` Michael Ellerman 2015-10-15 6:36 ` Michael Ellerman 2015-10-15 6:36 ` Michael Ellerman 2015-10-16 6:20 ` Christophe JAILLET 2015-10-16 6:20 ` Christophe JAILLET 2015-10-16 7:15 ` Gabriel Paubert 2015-10-16 7:15 ` Gabriel Paubert 2015-10-16 9:49 ` Michael Ellerman 2015-10-16 9:49 ` Michael Ellerman 2015-10-16 9:49 ` Michael Ellerman 2015-10-16 20:05 ` Christophe JAILLET 2015-10-16 20:05 ` Christophe JAILLET 2015-10-19 4:38 ` Michael Ellerman 2015-10-19 4:38 ` Michael Ellerman 2015-10-19 4:38 ` Michael Ellerman 2015-10-15 11:11 ` powerpc/mpc5xxx: Use of_get_next_parent to simplify code Michael Ellerman 2015-10-15 11:11 ` Michael Ellerman
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.