[U-Boot] [PATCH] [v2] powerpc/85xx: fix compatible property for the L2 cache node

[U-Boot] [PATCH] [v2] powerpc/85xx: fix compatible property for the L2 cache node

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

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);
 		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 */
 }
-- 
1.7.3.4

[U-Boot] [PATCH] [v2] powerpc/85xx: fix compatible property for the L2 cache node

[Kumar Gala]

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

[U-Boot] [PATCH] [v2] powerpc/85xx: fix compatible property for the L2 cache node

[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

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Ernest asks Frank how long he has been working for the company.
        "Ever since they threatened to fire me."

[U-Boot] [PATCH] [v2] powerpc/85xx: fix compatible property for the L2 cache node

[Kumar Gala]

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

[U-Boot] [PATCH] [v2] powerpc/85xx: fix compatible property for the L2 cache node

[Wolfgang Denk]

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

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
An age is called Dark not because  the  light  fails  to  shine,  but
because people refuse to see it.           -- James Michener, "Space"

[U-Boot] [PATCH] [v2] powerpc/85xx: fix compatible property for the L2 cache node

[Wolfgang Denk]

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

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
...though his invention worked superbly -- his theory was a crock  of
sewage from beginning to end.         - Vernor Vinge, "The Peace War"

[U-Boot] [PATCH] [v2] powerpc/85xx: fix compatible property for the L2 cache node

[Scott Wood]

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

[U-Boot] [PATCH] [v2] powerpc/85xx: fix compatible property for the L2 cache node

[Wolfgang Denk]

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

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
In general, if you think something isn't in Perl, try it out, because
it usually is :-) - Larry Wall in <1991Jul31.174523.9447@netlabs.com>

[U-Boot] [PATCH] [v2] powerpc/85xx: fix compatible property for the L2 cache node

[Timur Tabi]

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.

-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] [PATCH] [v2] powerpc/85xx: fix compatible property for the L2 cache node

[Wolfgang Denk]

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

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
egrep patterns are full regular expressions; it uses a fast  determi-
nistic algorithm that sometimes needs exponential space.
- unix manuals