public inbox for u-boot@lists.denx.de
 help / color / mirror / Atom feed
From: Stephen Warren <swarren@wwwdotorg.org>
To: u-boot@lists.denx.de
Subject: [U-Boot] [PATCH] cmd_nvedit.c: setenv_hex must prefix hex with '0x'
Date: Fri, 04 Oct 2013 15:47:24 -0600	[thread overview]
Message-ID: <524F376C.7070703@wwwdotorg.org> (raw)
In-Reply-To: <20131004213519.75C4E38040E@gemini.denx.de>

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?

  reply	other threads:[~2013-10-04 21:47 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-10-04 15:49 [U-Boot] [PATCH] cmd_nvedit.c: setenv_hex must prefix hex with '0x' Tom Rini
2013-10-04 21:35 ` Wolfgang Denk
2013-10-04 21:47   ` Stephen Warren [this message]
2013-10-04 22:12     ` Wolfgang Denk
2013-10-04 22:30       ` Stephen Warren
2013-10-05 19:07 ` [U-Boot] [PATCH] Fix number base handling of "load" command Wolfgang Denk
2013-10-07 16:14   ` Stephen Warren
2013-10-07 19:40     ` Wolfgang Denk
2013-10-07 19:42   ` [U-Boot] [PATCH V2] " Wolfgang Denk
2013-10-07 20:04   ` [U-Boot] [PATCH] " Tom Rini

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=524F376C.7070703@wwwdotorg.org \
    --to=swarren@wwwdotorg.org \
    --cc=u-boot@lists.denx.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox