[U-Boot] [PATCH] cmd/fdt: Make fdt get value endian-safe for single-cell properties

[U-Boot] [PATCH] cmd/fdt: Make fdt get value endian-safe for single-cell properties

[Andreas Färber]

On a Raspberry Pi 2 disagreements on cell endianness can be observed:

  U-Boot> fdt print /soc/gpio at 7e200000 phandle
  phandle = <0x0000000d>
  U-Boot> fdt get value myvar /soc/gpio at 7e200000 phandle; printenv myvar
  myvar=0x0D000000

Fix this by always treating the pointer as __be32 and converting it in
fdt_value_setenv(), like its counterpart fdt_parse_prop() already does.

Fixes: bc80295 ("fdt: Add get commands to fdt")
Cc: Joe Hershberger <joe.hershberger@ni.com>
Cc: Gerald Van Baren <gvb@unssw.com>
Signed-off-by: Andreas F?rber <afaerber@suse.de>
---
 cmd/fdt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/cmd/fdt.c b/cmd/fdt.c
index b503357..a131f17 100644
--- a/cmd/fdt.c
+++ b/cmd/fdt.c
@@ -58,7 +58,7 @@ static int fdt_value_setenv(const void *nodep, int len, const char *var)
 	else if (len == 4) {
 		char buf[11];
 
-		sprintf(buf, "0x%08X", *(uint32_t *)nodep);
+		sprintf(buf, "0x%08X", __be32_to_cpu(*(__be32 *)nodep));
 		setenv(var, buf);
 	} else if (len%4 == 0 && len <= 20) {
 		/* Needed to print things like sha1 hashes. */
-- 
2.6.6

[U-Boot] [PATCH] cmd/fdt: Make fdt get value endian-safe for single-cell properties

[Simon Glass]

Hi Andreas,

On 26 October 2016 at 09:02, Andreas F?rber <afaerber@suse.de> wrote:
> On a Raspberry Pi 2 disagreements on cell endianness can be observed:
>
>   U-Boot> fdt print /soc/gpio at 7e200000 phandle
>   phandle = <0x0000000d>
>   U-Boot> fdt get value myvar /soc/gpio at 7e200000 phandle; printenv myvar
>   myvar=0x0D000000
>
> Fix this by always treating the pointer as __be32 and converting it in
> fdt_value_setenv(), like its counterpart fdt_parse_prop() already does.
>
> Fixes: bc80295 ("fdt: Add get commands to fdt")
> Cc: Joe Hershberger <joe.hershberger@ni.com>
> Cc: Gerald Van Baren <gvb@unssw.com>
> Signed-off-by: Andreas F?rber <afaerber@suse.de>
> ---
>  cmd/fdt.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

The patch looks good, but please can you add a function comment to
fdt_value_setenv() ? You might even consider changing nodep to a
__be32.

>
> diff --git a/cmd/fdt.c b/cmd/fdt.c
> index b503357..a131f17 100644
> --- a/cmd/fdt.c
> +++ b/cmd/fdt.c
> @@ -58,7 +58,7 @@ static int fdt_value_setenv(const void *nodep, int len, const char *var)
>         else if (len == 4) {
>                 char buf[11];
>
> -               sprintf(buf, "0x%08X", *(uint32_t *)nodep);
> +               sprintf(buf, "0x%08X", __be32_to_cpu(*(__be32 *)nodep));
>                 setenv(var, buf);
>         } else if (len%4 == 0 && len <= 20) {
>                 /* Needed to print things like sha1 hashes. */
> --
> 2.6.6
>

Regards,
Simon

[U-Boot] [PATCH] cmd/fdt: Make fdt get value endian-safe for single-cell properties

[Andreas Färber]

Hi Simon,

Am 26.10.2016 um 21:19 schrieb Simon Glass:
> On 26 October 2016 at 09:02, Andreas F?rber <afaerber@suse.de> wrote:
>> On a Raspberry Pi 2 disagreements on cell endianness can be observed:
>>
>>   U-Boot> fdt print /soc/gpio at 7e200000 phandle
>>   phandle = <0x0000000d>
>>   U-Boot> fdt get value myvar /soc/gpio at 7e200000 phandle; printenv myvar
>>   myvar=0x0D000000
>>
>> Fix this by always treating the pointer as __be32 and converting it in
>> fdt_value_setenv(), like its counterpart fdt_parse_prop() already does.
>>
>> Fixes: bc80295 ("fdt: Add get commands to fdt")
>> Cc: Joe Hershberger <joe.hershberger@ni.com>
>> Cc: Gerald Van Baren <gvb@unssw.com>
>> Signed-off-by: Andreas F?rber <afaerber@suse.de>
>> ---
>>  cmd/fdt.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> The patch looks good, but please can you add a function comment to
> fdt_value_setenv() ?

Where and what should it say? There is a general comment above the
function already, and by choosing __be32* instead of uint32_t* I thought
my code was fairly self-documenting.

> You might even consider changing nodep to a
> __be32.

Hm, it's const void* so we're casting it everywhere, but __be32* doesn't
feel correct when we're also using the variable to access strings or arrays.

I am not aware of having a test case for any sha1 hashes, so not sure if
further endianness fixes may be needed in this function? I do assume it
breaks extending multi-cell pinctrl-0 properties and the like: If we
store them as one hex sequence then we might use them as [] array but
can't extend them <> style with a native hex number read from a phandle.


BTW why is the overlay support (fdt apply) not enabled by default? Seems
to build okay, but because it's defaulting to disabled for all boards
it's unavailable in our distro and led me to play with these low-level
operations. Considering that the Raspberry Pi boards use a FAT
filesystem rather than some memory-constrained partition and that they
have a number of Hats available, shouldn't we enable it at least in the
rpi* defconfigs? That would give the feature some more build- and
possibly functional testing.

Regards,
Andreas

>> diff --git a/cmd/fdt.c b/cmd/fdt.c
>> index b503357..a131f17 100644
>> --- a/cmd/fdt.c
>> +++ b/cmd/fdt.c
>> @@ -58,7 +58,7 @@ static int fdt_value_setenv(const void *nodep, int len, const char *var)
>>         else if (len == 4) {
>>                 char buf[11];
>>
>> -               sprintf(buf, "0x%08X", *(uint32_t *)nodep);
>> +               sprintf(buf, "0x%08X", __be32_to_cpu(*(__be32 *)nodep));
>>                 setenv(var, buf);
>>         } else if (len%4 == 0 && len <= 20) {
>>                 /* Needed to print things like sha1 hashes. */

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 N?rnberg, Germany
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)

[U-Boot] [PATCH] cmd/fdt: Make fdt get value endian-safe for single-cell properties

[Simon Glass]

Hi Andreas,

On 27 October 2016 at 05:44, Andreas F?rber <afaerber@suse.de> wrote:
> Hi Simon,
>
> Am 26.10.2016 um 21:19 schrieb Simon Glass:
>> On 26 October 2016 at 09:02, Andreas F?rber <afaerber@suse.de> wrote:
>>> On a Raspberry Pi 2 disagreements on cell endianness can be observed:
>>>
>>>   U-Boot> fdt print /soc/gpio at 7e200000 phandle
>>>   phandle = <0x0000000d>
>>>   U-Boot> fdt get value myvar /soc/gpio at 7e200000 phandle; printenv myvar
>>>   myvar=0x0D000000
>>>
>>> Fix this by always treating the pointer as __be32 and converting it in
>>> fdt_value_setenv(), like its counterpart fdt_parse_prop() already does.
>>>
>>> Fixes: bc80295 ("fdt: Add get commands to fdt")
>>> Cc: Joe Hershberger <joe.hershberger@ni.com>
>>> Cc: Gerald Van Baren <gvb@unssw.com>
>>> Signed-off-by: Andreas F?rber <afaerber@suse.de>
>>> ---
>>>  cmd/fdt.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> The patch looks good, but please can you add a function comment to
>> fdt_value_setenv() ?
>
> Where and what should it say? There is a general comment above the
> function already, and by choosing __be32* instead of uint32_t* I thought
> my code was fairly self-documenting.

It should explain what the function does and, importantly, what the
arguments and return value are.

>
>> You might even consider changing nodep to a
>> __be32.
>
> Hm, it's const void* so we're casting it everywhere, but __be32* doesn't
> feel correct when we're also using the variable to access strings or arrays.

OK fair enough, let's leave it as it is.

>
> I am not aware of having a test case for any sha1 hashes, so not sure if
> further endianness fixes may be needed in this function? I do assume it
> breaks extending multi-cell pinctrl-0 properties and the like: If we
> store them as one hex sequence then we might use them as [] array but
> can't extend them <> style with a native hex number read from a phandle.
>
>
> BTW why is the overlay support (fdt apply) not enabled by default? Seems
> to build okay, but because it's defaulting to disabled for all boards
> it's unavailable in our distro and led me to play with these low-level
> operations. Considering that the Raspberry Pi boards use a FAT
> filesystem rather than some memory-constrained partition and that they
> have a number of Hats available, shouldn't we enable it at least in the
> rpi* defconfigs? That would give the feature some more build- and
> possibly functional testing.

It is probably just for code-size reasons.

Regards,
Simon

[U-Boot] [PATCH] cmd/fdt: Make fdt get value endian-safe for single-cell properties

[Andreas Färber]

Hi Simon,

Am 28.10.2016 um 03:52 schrieb Simon Glass:
> On 27 October 2016 at 05:44, Andreas F?rber <afaerber@suse.de> wrote:
>> Am 26.10.2016 um 21:19 schrieb Simon Glass:
>>> On 26 October 2016 at 09:02, Andreas F?rber <afaerber@suse.de> wrote:
>>>> On a Raspberry Pi 2 disagreements on cell endianness can be observed:
>>>>
>>>>   U-Boot> fdt print /soc/gpio at 7e200000 phandle
>>>>   phandle = <0x0000000d>
>>>>   U-Boot> fdt get value myvar /soc/gpio at 7e200000 phandle; printenv myvar
>>>>   myvar=0x0D000000
>>>>
>>>> Fix this by always treating the pointer as __be32 and converting it in
>>>> fdt_value_setenv(), like its counterpart fdt_parse_prop() already does.
>>>>
>>>> Fixes: bc80295 ("fdt: Add get commands to fdt")
>>>> Cc: Joe Hershberger <joe.hershberger@ni.com>
>>>> Cc: Gerald Van Baren <gvb@unssw.com>
>>>> Signed-off-by: Andreas F?rber <afaerber@suse.de>
>>>> ---
>>>>  cmd/fdt.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> The patch looks good, but please can you add a function comment to
>>> fdt_value_setenv() ?
>>
>> Where and what should it say? There is a general comment above the
>> function already, and by choosing __be32* instead of uint32_t* I thought
>> my code was fairly self-documenting.
> 
> It should explain what the function does and, importantly, what the
> arguments and return value are.

Can you please do general documentation updates in a separate patch on
top of this bug fix, so that we can get this merged soonish?

Thanks,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 N?rnberg, Germany
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)

[U-Boot] [PATCH] cmd/fdt: Make fdt get value endian-safe for single-cell properties

[Simon Glass]

Hi Andreas,

On 15 November 2016 at 10:07, Andreas F?rber <afaerber@suse.de> wrote:
> Hi Simon,
>
> Am 28.10.2016 um 03:52 schrieb Simon Glass:
>> On 27 October 2016 at 05:44, Andreas F?rber <afaerber@suse.de> wrote:
>>> Am 26.10.2016 um 21:19 schrieb Simon Glass:
>>>> On 26 October 2016 at 09:02, Andreas F?rber <afaerber@suse.de> wrote:
>>>>> On a Raspberry Pi 2 disagreements on cell endianness can be observed:
>>>>>
>>>>>   U-Boot> fdt print /soc/gpio at 7e200000 phandle
>>>>>   phandle = <0x0000000d>
>>>>>   U-Boot> fdt get value myvar /soc/gpio at 7e200000 phandle; printenv myvar
>>>>>   myvar=0x0D000000
>>>>>
>>>>> Fix this by always treating the pointer as __be32 and converting it in
>>>>> fdt_value_setenv(), like its counterpart fdt_parse_prop() already does.
>>>>>
>>>>> Fixes: bc80295 ("fdt: Add get commands to fdt")
>>>>> Cc: Joe Hershberger <joe.hershberger@ni.com>
>>>>> Cc: Gerald Van Baren <gvb@unssw.com>
>>>>> Signed-off-by: Andreas F?rber <afaerber@suse.de>
>>>>> ---
>>>>>  cmd/fdt.c | 2 +-
>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> The patch looks good, but please can you add a function comment to
>>>> fdt_value_setenv() ?
>>>
>>> Where and what should it say? There is a general comment above the
>>> function already, and by choosing __be32* instead of uint32_t* I thought
>>> my code was fairly self-documenting.
>>
>> It should explain what the function does and, importantly, what the
>> arguments and return value are.
>
> Can you please do general documentation updates in a separate patch on
> top of this bug fix, so that we can get this merged soonish?

Actually that is what I was asking you to do. A separate patch is fine :-)

Regards,
Simon

[U-Boot] [PATCH] cmd/fdt: Make fdt get value endian-safe for single-cell properties

[Simon Glass]

Hi Andreas,

On 15 November 2016 at 17:18, Simon Glass <sjg@chromium.org> wrote:
> Hi Andreas,
>
> On 15 November 2016 at 10:07, Andreas F?rber <afaerber@suse.de> wrote:
>> Hi Simon,
>>
>> Am 28.10.2016 um 03:52 schrieb Simon Glass:
>>> On 27 October 2016 at 05:44, Andreas F?rber <afaerber@suse.de> wrote:
>>>> Am 26.10.2016 um 21:19 schrieb Simon Glass:
>>>>> On 26 October 2016 at 09:02, Andreas F?rber <afaerber@suse.de> wrote:
>>>>>> On a Raspberry Pi 2 disagreements on cell endianness can be observed:
>>>>>>
>>>>>>   U-Boot> fdt print /soc/gpio at 7e200000 phandle
>>>>>>   phandle = <0x0000000d>
>>>>>>   U-Boot> fdt get value myvar /soc/gpio at 7e200000 phandle; printenv myvar
>>>>>>   myvar=0x0D000000
>>>>>>
>>>>>> Fix this by always treating the pointer as __be32 and converting it in
>>>>>> fdt_value_setenv(), like its counterpart fdt_parse_prop() already does.
>>>>>>
>>>>>> Fixes: bc80295 ("fdt: Add get commands to fdt")
>>>>>> Cc: Joe Hershberger <joe.hershberger@ni.com>
>>>>>> Cc: Gerald Van Baren <gvb@unssw.com>
>>>>>> Signed-off-by: Andreas F?rber <afaerber@suse.de>
>>>>>> ---
>>>>>>  cmd/fdt.c | 2 +-
>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> The patch looks good, but please can you add a function comment to
>>>>> fdt_value_setenv() ?
>>>>
>>>> Where and what should it say? There is a general comment above the
>>>> function already, and by choosing __be32* instead of uint32_t* I thought
>>>> my code was fairly self-documenting.
>>>
>>> It should explain what the function does and, importantly, what the
>>> arguments and return value are.
>>
>> Can you please do general documentation updates in a separate patch on
>> top of this bug fix, so that we can get this merged soonish?
>
> Actually that is what I was asking you to do. A separate patch is fine :-)

Are you able to respin this or send another patch? If we are going to
get this applied for he next release, now is the time...

Also I think it is better to use fdt32_to_cpu() instead __be32_to_cpu().

Regards,
Simon