powerpc/85xx: fix compatible property for the L2 cache node

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 <[email protected]>

---
arch/powerpc/cpu/mpc85xx/fdt.c | 13 +++++++------

1 files changed, 7 insertions(+), 6 deletions(-)

Quoted 4 lines · Timur Tabi

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.

Quoted 4 lines

Signed-off-by Timur Tabi <[email protected]>

---
arch/powerpc/cpu/mpc85xx/fdt.c | 13 +++++++------

1 files changed, 7 insertions(+), 6 deletions(-)

applied to 85xx

Dear Kumar Gala,

Quoted 7 lines · In message <[email protected]> you wrote:

Quoted 4 lines · Timur Tabi

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.

Quoted 6 lines

Quoted 4 lines

Signed-off-by Timur Tabi <[email protected]>

---
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.

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 <[email protected]>

---
 arch/powerpc/cpu/mpc85xx/fdt.c |   13 +++++++------
 1 files changed, 7 insertions(+), 6 deletions(-)

arch/powerpc/cpu/mpc85xx/fdt.c +0 -1

index 642f6c5..a3a4b65 100644
@@ -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;

Dear Timur Tabi,

Quoted 22 lines · In message <[email protected]> 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 <[email protected]>

---
 arch/powerpc/cpu/mpc85xx/fdt.c |   13 +++++++------
 1 files changed, 7 insertions(+), 6 deletions(-)

arch/powerpc/cpu/mpc85xx/fdt.c +0 -1

index 642f6c5..a3a4b65 100644
@@ -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;

arch/powerpc/cpu/mpc85xx/fdt.c static inline void ft_fixup_l2cache(void *blob) · line 199

Quoted 10 lines

arch/powerpc/cpu/mpc85xx/fdt.c +4 -1

@@ -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.

Quoted 8 lines

arch/powerpc/cpu/mpc85xx/fdt.c static inline void ft_fixup_l2cache(void *blob) · line 199

Quoted 5 lines

arch/powerpc/cpu/mpc85xx/fdt.c +2 -1

 			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

Quoted 1 line

				"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".

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.

Quoted 2 lines

Quoted 2 lines

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: