[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 at 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 at ni.com>
>> Cc: Gerald Van Baren <gvb at unssw.com>
>> Signed-off-by: Andreas Färber <afaerber at 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)