[U-Boot] [PATCH] cmd/fdt: Make fdt get value endian-safe for single-cell properties
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(-)
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. */
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
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. */
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
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
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
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
participants (2)
-
Andreas Färber -
Simon Glass