diff mbox

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

Message ID 1477497756-13040-1-git-send-email-afaerber@suse.de
State Changes Requested
Delegated to: Simon Glass
Headers show

Commit Message

Andreas Färber Oct. 26, 2016, 4:02 p.m. UTC
On a Raspberry Pi 2 disagreements on cell endianness can be observed:

  U-Boot> fdt print /soc/gpio@7e200000 phandle
  phandle = <0x0000000d>
  U-Boot> fdt get value myvar /soc/gpio@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(-)

Comments

Simon Glass Oct. 26, 2016, 7:19 p.m. UTC | #1
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@7e200000 phandle
>   phandle = <0x0000000d>
>   U-Boot> fdt get value myvar /soc/gpio@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
Andreas Färber Oct. 27, 2016, 12:44 p.m. UTC | #2
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@7e200000 phandle
>>   phandle = <0x0000000d>
>>   U-Boot> fdt get value myvar /soc/gpio@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. */
Simon Glass Oct. 28, 2016, 1:52 a.m. UTC | #3
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@7e200000 phandle
>>>   phandle = <0x0000000d>
>>>   U-Boot> fdt get value myvar /soc/gpio@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
Andreas Färber Nov. 15, 2016, 5:07 p.m. UTC | #4
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@7e200000 phandle
>>>>   phandle = <0x0000000d>
>>>>   U-Boot> fdt get value myvar /soc/gpio@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
Simon Glass Nov. 16, 2016, 12:18 a.m. UTC | #5
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@7e200000 phandle
>>>>>   phandle = <0x0000000d>
>>>>>   U-Boot> fdt get value myvar /soc/gpio@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
Simon Glass Nov. 25, 2016, 7:37 p.m. UTC | #6
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@7e200000 phandle
>>>>>>   phandle = <0x0000000d>
>>>>>>   U-Boot> fdt get value myvar /soc/gpio@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
diff mbox

Patch

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