| Message ID | 1304089126-11945-1-git-send-email-timur@freescale.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Kumar Gala |
| Headers | show |
On Apr 29, 2011, at 9:58 AM, Timur Tabi wrote: > The compatible property for the L2 cache node (on 85xx systems that don't > have a CPC) was using a value for the property length that did not match > the actual length of the property. > > Signed-off-by: Timur Tabi <timur@freescale.com> > --- > arch/powerpc/cpu/mpc85xx/fdt.c | 13 +++++++------ > 1 files changed, 7 insertions(+), 6 deletions(-) applied to 85xx - k
Dear Timur Tabi, In message <1304089126-11945-1-git-send-email-timur@freescale.com> you wrote: > The compatible property for the L2 cache node (on 85xx systems that don't > have a CPC) was using a value for the property length that did not match > the actual length of the property. > > Signed-off-by: Timur Tabi <timur@freescale.com> > --- > arch/powerpc/cpu/mpc85xx/fdt.c | 13 +++++++------ > 1 files changed, 7 insertions(+), 6 deletions(-) > > diff --git a/arch/powerpc/cpu/mpc85xx/fdt.c b/arch/powerpc/cpu/mpc85xx/fdt.c > index 642f6c5..a3a4b65 100644 > --- a/arch/powerpc/cpu/mpc85xx/fdt.c > +++ b/arch/powerpc/cpu/mpc85xx/fdt.c > @@ -165,7 +165,6 @@ static inline void ft_fixup_l2cache(void *blob) > int len, off; > u32 *ph; > struct cpu_type *cpu = identify_cpu(SVR_SOC_VER(get_svr())); > - char compat_buf[38]; > > const u32 line_size = 32; > const u32 num_ways = 8; > @@ -192,22 +191,24 @@ static inline void ft_fixup_l2cache(void *blob) > } > > if (cpu) { > + char compat_buf[40]; > + > if (isdigit(cpu->name[0])) > len = sprintf(compat_buf, > - "fsl,mpc%s-l2-cache-controller", cpu->name); > + "fsl,mpc%s-l2-cache-controller" "%c" "cache", > + cpu->name, 0); This is a somewhat funny and complicated way of writing "fsl,mpc%s-l2-cache-controller\0cache" which, when written in plain text, reveals what sort of trickery you are doing here. This code is a dirty hack, and I will not accept it. Best regards, Wolfgang Denk
Dear Kumar Gala, In message <8188C4AC-9553-4F2D-9370-9416D63AF6A7@freescale.com> you wrote: > > On Apr 29, 2011, at 9:58 AM, Timur Tabi wrote: > > > The compatible property for the L2 cache node (on 85xx systems that don't > > have a CPC) was using a value for the property length that did not match > > the actual length of the property. > > > > Signed-off-by: Timur Tabi <timur@freescale.com> > > --- > > arch/powerpc/cpu/mpc85xx/fdt.c | 13 +++++++------ > > 1 files changed, 7 insertions(+), 6 deletions(-) > > applied to 85xx Kumar, don't you think that 32 minutes of review time is way too long for such a patch?? Maybe we should stop doing code reviews, and just accept all crap that gets posted here. Hey, we could automate this and have lots of free time instead. NAK!! Best regards, Wolfgang Denk
On Apr 29, 2011, at 3:33 PM, Wolfgang Denk wrote: > Dear Kumar Gala, > > In message <8188C4AC-9553-4F2D-9370-9416D63AF6A7@freescale.com> you wrote: >> >> On Apr 29, 2011, at 9:58 AM, Timur Tabi wrote: >> >>> The compatible property for the L2 cache node (on 85xx systems that don't >>> have a CPC) was using a value for the property length that did not match >>> the actual length of the property. >>> >>> Signed-off-by: Timur Tabi <timur@freescale.com> >>> --- >>> arch/powerpc/cpu/mpc85xx/fdt.c | 13 +++++++------ >>> 1 files changed, 7 insertions(+), 6 deletions(-) >> >> applied to 85xx > > Kumar, don't you think that 32 minutes of review time is way too long > for such a patch?? Maybe we should stop doing code reviews, and just > accept all crap that gets posted here. > > Hey, we could automate this and have lots of free time instead. > > NAK!! I was thinking the patch wasn't going to have any additional commentary. - k
Dear Kumar Gala, In message <DE97ACBA-6364-4B0C-BBDB-518C45F879EC@freescale.com> you wrote: > > I was thinking the patch wasn't going to have any additional commentary. The rule is that we allow _a_few_working_days_ of review time normally - not just a few minutes. Best regards, Wolfgang Denk
On Fri, 29 Apr 2011 22:30:58 +0200 Wolfgang Denk <wd@denx.de> wrote: > Dear Timur Tabi, > > In message <1304089126-11945-1-git-send-email-timur@freescale.com> you wrote: > > The compatible property for the L2 cache node (on 85xx systems that don't > > have a CPC) was using a value for the property length that did not match > > the actual length of the property. > > > > Signed-off-by: Timur Tabi <timur@freescale.com> > > --- > > arch/powerpc/cpu/mpc85xx/fdt.c | 13 +++++++------ > > 1 files changed, 7 insertions(+), 6 deletions(-) > > > > diff --git a/arch/powerpc/cpu/mpc85xx/fdt.c b/arch/powerpc/cpu/mpc85xx/fdt.c > > index 642f6c5..a3a4b65 100644 > > --- a/arch/powerpc/cpu/mpc85xx/fdt.c > > +++ b/arch/powerpc/cpu/mpc85xx/fdt.c > > @@ -165,7 +165,6 @@ static inline void ft_fixup_l2cache(void *blob) > > int len, off; > > u32 *ph; > > struct cpu_type *cpu = identify_cpu(SVR_SOC_VER(get_svr())); > > - char compat_buf[38]; > > > > const u32 line_size = 32; > > const u32 num_ways = 8; > > @@ -192,22 +191,24 @@ static inline void ft_fixup_l2cache(void *blob) > > } > > > > if (cpu) { > > + char compat_buf[40]; > > + > > if (isdigit(cpu->name[0])) > > len = sprintf(compat_buf, > > - "fsl,mpc%s-l2-cache-controller", cpu->name); > > + "fsl,mpc%s-l2-cache-controller" "%c" "cache", > > + cpu->name, 0); > > This is a somewhat funny and complicated way of writing > > "fsl,mpc%s-l2-cache-controller\0cache" Except that his version works and your version doesn't. With your version sprintf will stop reading the format string after "controller". > which, when written in plain text, reveals what sort of trickery you > are doing here. > > This code is a dirty hack, and I will not accept it. The alternative is two separate sprintfs, manually advancing the pointer in the calling code, but that's a bit more complicated and error-prone (the previous code did it that way, and had an error, thus this patch) and IMHO not more readable. -Scott
Dear Scott Wood, In message <20110429154457.0fee37c6@schlenkerla.am.freescale.net> you wrote: > > > > len = sprintf(compat_buf, > > > - "fsl,mpc%s-l2-cache-controller", cpu->name); > > > + "fsl,mpc%s-l2-cache-controller" "%c" "cache", > > > + cpu->name, 0); > > > > This is a somewhat funny and complicated way of writing > > > > "fsl,mpc%s-l2-cache-controller\0cache" > > Except that his version works and your version doesn't. With your version > sprintf will stop reading the format string after "controller". Yes, and this is why I call this a dirty hack, as it's obfuscating what's going on and what the intended result is. > The alternative is two separate sprintfs, manually advancing the pointer in > the calling code, but that's a bit more complicated and error-prone (the > previous code did it that way, and had an error, thus this patch) and IMHO > not more readable. At least people who read the code in a year will then be able to understand what you are doing. Best regards, Wolfgang Denk
Wolfgang Denk wrote: >> > Except that his version works and your version doesn't. With your version >> > sprintf will stop reading the format string after "controller". > Yes, and this is why I call this a dirty hack, as it's obfuscating > what's going on and what the intended result is. I disagree. It's quite clear what I'm trying to do. I'm trying to insert a NULL character into a string. Since device tree properties use a NULL to delimit multiple strings, it's clear that this is what the "0" is for. Look at the original code: len = sprintf(compat_buf, "fsl,%c%s-l2-cache-controller", tolower(cpu->name[0]), cpu->name + 1); sprintf(&compat_buf[len + 1], "cache"); I think my patch is clearer than this. In fact, because the original code was so obscure, there was a bug in it. I could have done this: len = sprintf(compat_buf, "fsl,%c%s-l2-cache-controller", tolower(cpu->name[0]), cpu->name + 1); len += sprintf(&compat_buf[len + 1], "cache") + 2; Where the "+ 2" is for each NULL in the string. I just don't see how this is better than my version.
Dear Timur Tabi, In message <4DBB2723.4050408@freescale.com> you wrote: > > I disagree. It's quite clear what I'm trying to do. I'm trying to insert a This is your opinion. I disagree. > NULL character into a string. Since device tree properties use a NULL to > delimit multiple strings, it's clear that this is what the "0" is for. Wrong data type. In C strings are _terminated_ by '\0' characters, so using functions that are designed to deal with C strings are obviously not the right tool to deal with data structures that have _embedded_ NUL characters. If you try, it quickly gets ugly like the code I rejected. For example, who gives you any guarantee that sprintf() will continue to append characters after it inserted the first NUL character? A clever implementation could optimize this and return immediately after seeing a NUL... > Look at the original code: > > len = sprintf(compat_buf, > "fsl,%c%s-l2-cache-controller", > tolower(cpu->name[0]), cpu->name + 1); > > sprintf(&compat_buf[len + 1], "cache"); > > I think my patch is clearer than this. In fact, because the original code was > so obscure, there was a bug in it. I could have done this: Why exactly do you think you have to use sprintf() to append a constant string like "cache"? If you want to make clean what's intended, then use something like this: len = sprintf(print_buf, "fsl,%c%s-l2-cache-controller", tolower(cpu->name[0]), cpu->name + 1); /* Include NUL characters */ memcpy(compat_buf, print_buf, len + 1); memcpy(compat_buf + len + 1, "cache", sizeof("cache")); If you want to optimize (I'm a fan of small memory footprint, but I'm also a fan of readable code), use len = sprintf(compat_buf, "fsl,%c%s-l2-cache-controller", tolower(cpu->name[0]), cpu->name + 1); /* Include NUL characters */ memcpy(compat_buf + len + 1, "cache", sizeof("cache")); etc. Best regards, Wolfgang Denk
diff --git a/arch/powerpc/cpu/mpc85xx/fdt.c b/arch/powerpc/cpu/mpc85xx/fdt.c index 642f6c5..a3a4b65 100644 --- a/arch/powerpc/cpu/mpc85xx/fdt.c +++ b/arch/powerpc/cpu/mpc85xx/fdt.c @@ -165,7 +165,6 @@ static inline void ft_fixup_l2cache(void *blob) int len, off; u32 *ph; struct cpu_type *cpu = identify_cpu(SVR_SOC_VER(get_svr())); - char compat_buf[38]; const u32 line_size = 32; const u32 num_ways = 8; @@ -192,22 +191,24 @@ static inline void ft_fixup_l2cache(void *blob) } if (cpu) { + char compat_buf[40]; + if (isdigit(cpu->name[0])) len = sprintf(compat_buf, - "fsl,mpc%s-l2-cache-controller", cpu->name); + "fsl,mpc%s-l2-cache-controller" "%c" "cache", + cpu->name, 0); else len = sprintf(compat_buf, - "fsl,%c%s-l2-cache-controller", - tolower(cpu->name[0]), cpu->name + 1); + "fsl,%c%s-l2-cache-controller" "%c" "cache", + tolower(cpu->name[0]), cpu->name + 1, 0); - sprintf(&compat_buf[len + 1], "cache"); + fdt_setprop(blob, off, "compatible", compat_buf, len + 1); } fdt_setprop(blob, off, "cache-unified", NULL, 0); fdt_setprop_cell(blob, off, "cache-block-size", line_size); fdt_setprop_cell(blob, off, "cache-size", size); fdt_setprop_cell(blob, off, "cache-sets", num_sets); fdt_setprop_cell(blob, off, "cache-level", 2); - fdt_setprop(blob, off, "compatible", compat_buf, sizeof(compat_buf)); /* we dont bother w/L3 since no platform of this type has one */ }
The compatible property for the L2 cache node (on 85xx systems that don't have a CPC) was using a value for the property length that did not match the actual length of the property. Signed-off-by: Timur Tabi <timur@freescale.com> --- arch/powerpc/cpu/mpc85xx/fdt.c | 13 +++++++------ 1 files changed, 7 insertions(+), 6 deletions(-)