[U-Boot] Inconsistencies in commands regarding load_addr

[U-Boot] Inconsistencies in commands regarding load_addr

[Benoît Thébaudeau]

Hi all,

I've just noticed that before the commit
045fa1e1142552799ad3203e9e0bc22a11e866ea, ext2load and ext4load were setting the
load_addr global variable, but not fatload. Since then, none of these commands
set load_addr (initially derived from the loadaddr environment variable).

ubifsload also does not set load_addr, but a quick grep shows that some other
filesystem commands set it, e.g. for zfs, jffs2, reiser or cramfs.

Also, some commands set it only on success, while some other commands set it
from the command line arguments unconditionally.

What's the expected correct behavior here?

Best regards,
Beno?t

[U-Boot] Inconsistencies in commands regarding load_addr

[Stephen Warren]

On 10/06/2015 09:00 AM, Beno?t Th?baudeau wrote:
> Hi all,
>
> I've just noticed that before the commit
> 045fa1e1142552799ad3203e9e0bc22a11e866ea, ext2load and ext4load were setting the
> load_addr global variable, but not fatload. Since then, none of these commands
> set load_addr (initially derived from the loadaddr environment variable).

Oh dear; I see that has indeed changed. Still, it's been 3 years so I 
imagine nobody was using the feature?

> ubifsload also does not set load_addr, but a quick grep shows that some other
> filesystem commands set it, e.g. for zfs, jffs2, reiser or cramfs.
>
> Also, some commands set it only on success, while some other commands set it
> from the command line arguments unconditionally.
>
> What's the expected correct behavior here?

I'm not quite sure how useful the behaviour is; I'd tend towards not 
setting $load_addr. If some script wants it set, it can easily do it itself.

Did you just notice this while reading code, or does this break some 
existing use-case? If the latter, it seems reasonable to add the 
previously-working feature back.

[U-Boot] Inconsistencies in commands regarding load_addr

[Benoît Thébaudeau]

On Tue, Oct 6, 2015 at 8:09 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 10/06/2015 09:00 AM, Beno?t Th?baudeau wrote:
>>
>> Hi all,
>>
>> I've just noticed that before the commit
>> 045fa1e1142552799ad3203e9e0bc22a11e866ea, ext2load and ext4load were
>> setting the
>> load_addr global variable, but not fatload. Since then, none of these
>> commands
>> set load_addr (initially derived from the loadaddr environment variable).
>
>
> Oh dear; I see that has indeed changed. Still, it's been 3 years so I
> imagine nobody was using the feature?

Probably.

>> ubifsload also does not set load_addr, but a quick grep shows that some
>> other
>> filesystem commands set it, e.g. for zfs, jffs2, reiser or cramfs.
>>
>> Also, some commands set it only on success, while some other commands set
>> it
>> from the command line arguments unconditionally.
>>
>> What's the expected correct behavior here?
>
>
> I'm not quite sure how useful the behaviour is; I'd tend towards not setting
> $load_addr. If some script wants it set, it can easily do it itself.
>
> Did you just notice this while reading code, or does this break some
> existing use-case? If the latter, it seems reasonable to add the
> previously-working feature back.

Actually, I was working on an older version of U-Boot based on
2012.07, and I got an issue using bootm without any arguments because
ext2load was unexpectedly setting load_addr (this was undocumented).
So I checked how things evolved in mainline in the meantime, and I
noticed the change. I prefer the new behavior, but still, not all
current commands do the same.

Best regards,
Beno?t

[U-Boot] Inconsistencies in commands regarding load_addr

[Wolfgang Denk]

Dear Beno?t,

In message <5613E20F.8060306@wsystem.com> you wrote:
> 
> I've just noticed that before the commit
> 045fa1e1142552799ad3203e9e0bc22a11e866ea, ext2load and ext4load were setting the
> load_addr global variable, but not fatload. Since then, none of these commands
> set load_addr (initially derived from the loadaddr environment variable).

That's bad.

> ubifsload also does not set load_addr, but a quick grep shows that some other
> filesystem commands set it, e.g. for zfs, jffs2, reiser or cramfs.
> 
> Also, some commands set it only on success, while some other commands set it
> from the command line arguments unconditionally.
> 
> What's the expected correct behavior here?

After successful loading the data to memory, load_addr should be set
correctly, for all commands.  In the error case, the value of
load_addr is undefined.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
A verbal contract isn't worth the paper it's written on.
                                                    -- Samuel Goldwyn

[U-Boot] Inconsistencies in commands regarding load_addr

[Stephen Warren]

On 10/07/2015 10:40 PM, Wolfgang Denk wrote:
> Dear Beno?t,
>
> In message <5613E20F.8060306@wsystem.com> you wrote:
>>
>> I've just noticed that before the commit
>> 045fa1e1142552799ad3203e9e0bc22a11e866ea, ext2load and ext4load were setting the
>> load_addr global variable, but not fatload. Since then, none of these commands
>> set load_addr (initially derived from the loadaddr environment variable).
>
> That's bad.
>
>> ubifsload also does not set load_addr, but a quick grep shows that some other
>> filesystem commands set it, e.g. for zfs, jffs2, reiser or cramfs.
>>
>> Also, some commands set it only on success, while some other commands set it
>> from the command line arguments unconditionally.
>>
>> What's the expected correct behavior here?
>
> After successful loading the data to memory, load_addr should be set
> correctly, for all commands.  In the error case, the value of
> load_addr is undefined.

Is this documented anywhere? If not, I'm not convinced that there's a 
contract to be followed; it "just happens" that some filesystem-related 
commands work(ed) that way (and as Beno?t pointed out, apparently some 
don't irrespective of the mentioned patch).

[U-Boot] Inconsistencies in commands regarding load_addr

[Wolfgang Denk]

Dear Stephen,

In message <56167DB6.3000508@wwwdotorg.org> you wrote:
>
> >> What's the expected correct behavior here?
> >
> > After successful loading the data to memory, load_addr should be set
> > correctly, for all commands.  In the error case, the value of
> > load_addr is undefined.
> 
> Is this documented anywhere? If not, I'm not convinced that there's a 
> contract to be followed; it "just happens" that some filesystem-related 
> commands work(ed) that way (and as Beno?t pointed out, apparently some 
> don't irrespective of the mentioned patch).

I'm afraid it's not documented, but it is what I would consider a sane
and consistent behaviour.  If we intend to implement POLA [1] (and I
very much think we should), this is how U-Boot should behave.


[1] https://en.wikipedia.org/wiki/Principle_of_least_astonishment


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
What we hope ever to do with ease, we must learn  first  to  do  with
diligence.                                           - Samuel Johnson

[U-Boot] Inconsistencies in commands regarding load_addr

[Benoît Thébaudeau]

Dear Wolfgang,

On 08/10/2015 23:29, Wolfgang Denk wrote:
> Dear Stephen,
> 
> In message <56167DB6.3000508@wwwdotorg.org> you wrote:
>>
>>>> What's the expected correct behavior here?
>>>
>>> After successful loading the data to memory, load_addr should be set
>>> correctly, for all commands.  In the error case, the value of
>>> load_addr is undefined.
>>
>> Is this documented anywhere? If not, I'm not convinced that there's a 
>> contract to be followed; it "just happens" that some filesystem-related 
>> commands work(ed) that way (and as Beno?t pointed out, apparently some 
>> don't irrespective of the mentioned patch).
> 
> I'm afraid it's not documented, but it is what I would consider a sane
> and consistent behaviour.  If we intend to implement POLA [1] (and I
> very much think we should), this is how U-Boot should behave.
> 
> 
> [1] https://en.wikipedia.org/wiki/Principle_of_least_astonishment

I'm not certain that this would be the least astonishing behavior. When I read
the documentation, I rather expect the loadaddr environment variable to be used
whenever the address is omitted in a command invocation. Moreover, one may have
to read/load several data pieces before booting, and the last loaded piece would
not necessarily be the one containing the kernel to be booted. This should at
least be documented.

Another approach would be to compel users to pass an address for all commands.
Implicit behaviors are always dangerous, all the more if they are undocumented.
But of course, this would break some existing configurations.

Best regards,
Beno?t

[U-Boot] Inconsistencies in commands regarding load_addr

[Wolfgang Denk]

Dear Beno?t Th?baudeau,

In message <56177AC8.1020707@wsystem.com> you wrote:
> 
> I'm not certain that this would be the least astonishing behavior. When I read
> the documentation, I rather expect the loadaddr environment variable to be used
> whenever the address is omitted in a command invocation. Moreover, one may have
> to read/load several data pieces before booting, and the last loaded piece would
> not necessarily be the one containing the kernel to be booted. This should at
> least be documented.

I agree about the need for documentation part.

Regarding the "load address" topic, be careful, as there has always
been a lot of confusion (due to unfortunate historic choice of names).
There is the "load address" as part of the image formates (uImage, FIT
image), which means the address where the image (OS code) gets loaded
(or even uncompressed) _to_.  This is recorded in the image itself,
and has nothing to do woth the "loadaddr" variable, which states where
the image is located in system memory.

A command, that _loads_ an image to memory, should either use the
current setting of "loadaddr" (if no argument is given), of, if the
argument is given, set "loadaddr" to that value, so that further
commands can refer to that address by default.

> Another approach would be to compel users to pass an address for all commands.

That would break a ton of existing scripts, and is just cumbersome.
It is so easy to type for example just

	tftp 400000 filename
	imi
	bootm

without having to care about the "loadaddr" setting.

> Implicit behaviors are always dangerous, all the more if they are undocumented.

I agree that documentation could be a lot better.  But then, while many
people tend to critizise the exising documentation, very few actually
contribute to improving it.

> But of course, this would break some existing configurations.

True.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Great teachers have small audiences while they are still alive.

[U-Boot] Inconsistencies in commands regarding load_addr

[Benoît Thébaudeau]

Dear Wolfgang,

On 09/10/2015 15:18, Wolfgang Denk wrote:
> Regarding the "load address" topic, be careful, as there has always
> been a lot of confusion (due to unfortunate historic choice of names).
> There is the "load address" as part of the image formates (uImage, FIT
> image), which means the address where the image (OS code) gets loaded
> (or even uncompressed) _to_.  This is recorded in the image itself,
> and has nothing to do woth the "loadaddr" variable, which states where
> the image is located in system memory.

Indeed, but I was only referring to the load address below.

> A command, that _loads_ an image to memory, should either use the
> current setting of "loadaddr" (if no argument is given), of, if the
> argument is given, set "loadaddr" to that value, so that further
> commands can refer to that address by default.

Makes sense.

Currently, it's all mixed up between CONFIG_SYS_LOAD_ADDR, the loadaddr
environment variable and the load_addr global C variable.

The 1st issue is that loadaddr and load_addr currently diverge if the user
changes loadaddr or if commands change load_addr.

The 2nd issue is that some commands use the value of loadaddr as a default,
whereas others use load_addr. And if that fails, CONFIG_SYS_LOAD_ADDR is
sometimes used as a fallback value.

The 3rd issue is that some read/load commands set load_addr, but not all (e.g.:
mmc read, ext2load), which breaks the whole feature, but fixing this could break
existing configurations relying on the current behavior.

Best regards,
Beno?t

[U-Boot] Inconsistencies in commands regarding load_addr

[Wolfgang Denk]

Dear Beno?t,

In message <5617C8B9.30204@wsystem.com> you wrote:
> 
> Currently, it's all mixed up between CONFIG_SYS_LOAD_ADDR, the loadaddr
> environment variable and the load_addr global C variable.
> 
> The 1st issue is that loadaddr and load_addr currently diverge if the user
> changes loadaddr or if commands change load_addr.
> 
> The 2nd issue is that some commands use the value of loadaddr as a default,
> whereas others use load_addr. And if that fails, CONFIG_SYS_LOAD_ADDR is
> sometimes used as a fallback value.
> 
> The 3rd issue is that some read/load commands set load_addr, but not all (e.g.:
> mmc read, ext2load), which breaks the whole feature, but fixing this could break
> existing configurations relying on the current behavior.

Thanks for the analysis.  As always, patches are welcome :-)

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Time is an illusion perpetrated by the manufacturers of space.

[U-Boot] Inconsistencies in commands regarding load_addr

[Stephen Warren]

On 10/09/2015 02:28 AM, Beno?t Th?baudeau wrote:
> Dear Wolfgang,
>
> On 08/10/2015 23:29, Wolfgang Denk wrote:
>> Dear Stephen,
>>
>> In message <56167DB6.3000508@wwwdotorg.org> you wrote:
>>>
>>>>> What's the expected correct behavior here?
>>>>
>>>> After successful loading the data to memory, load_addr should be set
>>>> correctly, for all commands.  In the error case, the value of
>>>> load_addr is undefined.
>>>
>>> Is this documented anywhere? If not, I'm not convinced that there's a
>>> contract to be followed; it "just happens" that some filesystem-related
>>> commands work(ed) that way (and as Beno?t pointed out, apparently some
>>> don't irrespective of the mentioned patch).
>>
>> I'm afraid it's not documented, but it is what I would consider a sane
>> and consistent behaviour.  If we intend to implement POLA [1] (and I
>> very much think we should), this is how U-Boot should behave.
>>
>>
>> [1] https://en.wikipedia.org/wiki/Principle_of_least_astonishment
>
> I'm not certain that this would be the least astonishing behavior. When I read
> the documentation, I rather expect the loadaddr environment variable to be used
> whenever the address is omitted in a command invocation. Moreover, one may have
> to read/load several data pieces before booting, and the last loaded piece would
> not necessarily be the one containing the kernel to be booted. This should at
> least be documented.
>
> Another approach would be to compel users to pass an address for all commands.
> Implicit behaviors are always dangerous, all the more if they are undocumented.
> But of course, this would break some existing configurations.

I tend to agree with all of the above; U-Boot's 
implicit/automatic/hidden/undocumented usage of variables that I didn't 
specify on the command-line, and setting of variables as a side-effect 
of executing commands, has always been quite astonishing (rather than 
the opposite of astonishing) to me:-(