[U-Boot] Inconsistencies in commands regarding load_addr
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
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.
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
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
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).
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
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
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
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
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
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:-(
participants (4)
-
Benoît Thébaudeau -
Benoît Thébaudeau -
Stephen Warren -
Wolfgang Denk