[U-Boot] [PATCH] cmd_nvedit.c: setenv_hex must prefix hex with '0x'

Stephen Warren
2013-10-04T21:47:24Z

On 10/04/2013 03:35 PM, Wolfgang Denk wrote:
> Dear Tom Rini,
>
> In message <1380901758-30360-1-git-send-email-trini@ti.com> you wrote:
>> setenv_hex is only called with hex values, but does not prefix the
>> strings with '0x', as in general U-Boot assumes hex values not decimal
>> values, and this saves space in the environment. However, some
>> functions such as 'load' take some values that are most easily described
>> in hex (load address) and decimal (size, offset within a file).
>>
>> This can lead to the situation where, for example, spl export is run,
>> which leads to a call of setenv_hex of the fdtaddr, which will be re-set
>> in the environment. Then 'saveenv' may be run (after updating other
>> parts of the environment for falcon mode), causing an invalid for 'load'
>> fdtaddr to be saved to the environment and leading to future boots to
>> fail if using 'load' to read the fdt file.
>
> I think this is not a correct way to fix the issues at hand.
>
> All U-Boot (with the single unfortunate exception of the "sleep"
> command) defaults to hex input - that's what's documented, and what
> people have always been using for many, many years. Applying your
> patch means that we modify just a single use case, while setting the
> same environment variables from the command line (without the leading
> 0x) would still trigger a problem.
>
> I understand that the actual cause of these issues is commit 3f83c87
> "fs: fix number base behaviour change in fatload/ext*load". Reviewing
> this patch I think that it should have never been applied. The fact
> that "load" and "fatload" / "ext*load" behave differently are reason
> enough to reject this patch. "load" should just behave like every
> other command, and default to hex input.
>
> I think we should NAK your patch, and suggest to fix the problem by
> reverting commit 3f83c87 and making "load" default to hex input mode.

Reverting 3f83c87 would do the opposite of what you want; it'd make
extload/fatload require 0x prefixes instead of assuming hex. Perhaps
what you want is a tweak to that patch so that the generic load/ls
commands always expect a hex value, rather than requiring the 0x prefix?