| Message ID | 1304113875-6749-1-git-send-email-timur@freescale.com |
|---|---|
| State | Accepted |
| Headers | show |
Dear Timur Tabi, In message <1304113875-6749-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 | 20 ++++++++++++-------- > 1 files changed, 12 insertions(+), 8 deletions(-) > > diff --git a/arch/powerpc/cpu/mpc85xx/fdt.c b/arch/powerpc/cpu/mpc85xx/fdt.c > index 642f6c5..121db5f 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,27 @@ static inline void ft_fixup_l2cache(void *blob) > } > > if (cpu) { > + char buf[40]; > + > if (isdigit(cpu->name[0])) > - len = sprintf(compat_buf, > - "fsl,mpc%s-l2-cache-controller", cpu->name); > + /* MPCxxxx, where xxxx == 4-digit number */ > + len = sprintf(buf, "fsl,mpc%s-l2-cache-controller", > + cpu->name) + 1; > else > - len = sprintf(compat_buf, > - "fsl,%c%s-l2-cache-controller", > - tolower(cpu->name[0]), cpu->name + 1); > + /* Pxxxx or Txxxx, where xxxx == 4-digit number */ > + len = sprintf(buf, "fsl,%c%s-l2-cache-controller", > + tolower(cpu->name[0]), cpu->name + 1) + 1; Why do we need this "if" at all? tolower() on a digit is a nop, so you can omit the first branch. > + /* append "cache" to the string */ > + len += sprintf(buf + len, "cache") + 1; This is wrong and misleading. This is not an operation on a C string. You do not "append" (or concatenate) the string cache. You build a specifically structured data set, which is not a C string. So please don't call it a string. Best regards, Wolfgang Denk
Wolfgang Denk wrote: > Why do we need this "if" at all? tolower() on a digit is a nop, so you > can omit the first branch. Because cpu->name looks like one of two ways: 8578 or P4080 In the case of "8578", we want to convert that to "mpc8578". In the case of "P4080", we want to convert that to "p4080". The "if" is need to determine whether to prepend the "mpc". >> > + /* append "cache" to the string */ >> > + len += sprintf(buf + len, "cache") + 1; > This is wrong and misleading. This is not an operation on a C string. > You do not "append" (or concatenate) the string cache. You build a > specifically structured data set, which is not a C string. So please > don't call it a string. Ok.
Dear Tabi Timur-B04825, In message <4DBB3E01.7000704@freescale.com> you wrote: > Wolfgang Denk wrote: > > Why do we need this "if" at all? tolower() on a digit is a nop, so you > > can omit the first branch. > > Because cpu->name looks like one of two ways: > > 8578 > > or > > P4080 > > In the case of "8578", we want to convert that to "mpc8578". In the case o > f "P4080", we want to convert that to "p4080". > > The "if" is need to determine whether to prepend the "mpc". This gets lost in the code. If you keep the if / else, you have to add braces for the multiline statements. Best regards, Wolfgang Denk
Wolfgang Denk wrote: > This gets lost in the code. That's why I added the comments: /* MPCxxxx, where xxxx == 4-digit number */ > If you keep the if / else, you have to add braces for the multiline > statements. Ok.
diff --git a/arch/powerpc/cpu/mpc85xx/fdt.c b/arch/powerpc/cpu/mpc85xx/fdt.c index 642f6c5..121db5f 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,27 @@ static inline void ft_fixup_l2cache(void *blob) } if (cpu) { + char buf[40]; + if (isdigit(cpu->name[0])) - len = sprintf(compat_buf, - "fsl,mpc%s-l2-cache-controller", cpu->name); + /* MPCxxxx, where xxxx == 4-digit number */ + len = sprintf(buf, "fsl,mpc%s-l2-cache-controller", + cpu->name) + 1; else - len = sprintf(compat_buf, - "fsl,%c%s-l2-cache-controller", - tolower(cpu->name[0]), cpu->name + 1); + /* Pxxxx or Txxxx, where xxxx == 4-digit number */ + len = sprintf(buf, "fsl,%c%s-l2-cache-controller", + tolower(cpu->name[0]), cpu->name + 1) + 1; + + /* append "cache" to the string */ + len += sprintf(buf + len, "cache") + 1; - sprintf(&compat_buf[len + 1], "cache"); + fdt_setprop(blob, off, "compatible", buf, len); } 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 | 20 ++++++++++++-------- 1 files changed, 12 insertions(+), 8 deletions(-)