[U-Boot] [RFC 1/2] env: Drop error messages when loading environment
This is just a draft to discuss ideas related to "Make U-Boot log great again" thread.
With this patch we will have something like this:
Loading Environment from FAT... Failed (-5) Loading Environment from MMC... OK
instead of this:
Loading Environment from FAT... MMC: no card present ** Bad device mmc 0 ** Failed (-5) Loading Environment from MMC... OK
Signed-off-by: Sam Protsenko semen.protsenko@linaro.org --- env/env.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/env/env.c b/env/env.c index 5c0842ac07..85598fa5d4 100644 --- a/env/env.c +++ b/env/env.c @@ -187,6 +187,7 @@ int env_load(void)
for (prio = 0; (drv = env_driver_lookup(ENVOP_LOAD, prio)); prio++) { int ret; + unsigned long have_console = gd->have_console;
if (!drv->load) continue; @@ -195,7 +196,11 @@ int env_load(void) continue;
printf("Loading Environment from %s... ", drv->name); + + /* Suppress console output for drv->load() */ + gd->have_console = 0; ret = drv->load(); + gd->have_console = have_console; if (ret) printf("Failed (%d)\n", ret); else
This is just a draft to discuss ideas related to "Make U-Boot log great again" thread.
With this patch we will see something like:
Loading Environment from FAT... --> MMC: no card present --> ** Bad device mmc 0 ** --> Failed (-5) Loading Environment from MMC... --> OK
instead of:
Loading Environment from FAT... MMC: no card present ** Bad device mmc 0 ** Failed (-5) Loading Environment from MMC... OK
Signed-off-by: Sam Protsenko semen.protsenko@linaro.org --- common/console.c | 8 ++++++++ env/env.c | 4 +++- include/asm-generic/global_data.h | 1 + 3 files changed, 12 insertions(+), 1 deletion(-)
diff --git a/common/console.c b/common/console.c index 2ba33dc574..1bbafa33a1 100644 --- a/common/console.c +++ b/common/console.c @@ -533,6 +533,14 @@ void putc(const char c)
void puts(const char *s) { + if (gd->pr_prefix) { + const char *prefix = gd->pr_prefix; + + gd->pr_prefix = NULL; + puts(prefix); + gd->pr_prefix = prefix; + } + #ifdef CONFIG_DEBUG_UART if (!gd || !(gd->flags & GD_FLG_SERIAL_READY)) { while (*s) { diff --git a/env/env.c b/env/env.c index 5c0842ac07..56a105ea55 100644 --- a/env/env.c +++ b/env/env.c @@ -194,12 +194,14 @@ int env_load(void) if (!env_has_inited(drv->location)) continue;
- printf("Loading Environment from %s... ", drv->name); + printf("Loading Environment from %s...\n", drv->name); + gd->pr_prefix = " --> "; ret = drv->load(); if (ret) printf("Failed (%d)\n", ret); else printf("OK\n"); + gd->pr_prefix = NULL;
if (!ret) return 0; diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h index 0fd4900392..32b80db96b 100644 --- a/include/asm-generic/global_data.h +++ b/include/asm-generic/global_data.h @@ -44,6 +44,7 @@ typedef struct global_data { unsigned long board_type; #endif unsigned long have_console; /* serial_init() was called */ + const char *pr_prefix; /* print prefix before message */ #if CONFIG_IS_ENABLED(PRE_CONSOLE_BUFFER) unsigned long precon_buf_idx; /* Pre-Console buffer index */ #endif
Dear Sam,
In message 20180717220912.11358-2-semen.protsenko@linaro.org you wrote:
This is just a draft to discuss ideas related to "Make U-Boot log great again" thread.
With this patch we will see something like:
Loading Environment from FAT... --> MMC: no card present --> ** Bad device mmc 0 ** --> Failed (-5)
This may be ok in the error case, but...
Loading Environment from MMC... --> OK
it is definitely really ugly in the normal, non-error case.
NAK.
void puts(const char *s) {
- if (gd->pr_prefix) {
const char *prefix = gd->pr_prefix;gd->pr_prefix = NULL;puts(prefix);gd->pr_prefix = prefix;- }
Besides - global data is a precious resource on may systems, sometimes limited to very few kB of memory. It should be only used to the extend where it really cannot be avoided, but not for any such "beautifying" stuff.
Best regards,
Wolfgang Denk
On Wed, Jul 18, 2018 at 01:09:12AM +0300, Sam Protsenko wrote:
This is just a draft to discuss ideas related to "Make U-Boot log great again" thread.
With this patch we will see something like:
Loading Environment from FAT... --> MMC: no card present --> ** Bad device mmc 0 ** --> Failed (-5) Loading Environment from MMC... --> OKinstead of:
Loading Environment from FAT... MMC: no card present ** Bad device mmc 0 ** Failed (-5) Loading Environment from MMC... OK
So, I think maybe (and given Wolfgang's comments) we should think about how the output might want to look, and how to get there without GD changes. Perhaps: Attempting to load Environment from FAT (do we have more easily available info at this point?): MMC: no card present ** Bad device mmc 0 ** Failed (-5) Loading Environment from MMC... Attempting to load Environment from MMC: Succeeded
On Wed, Jul 18, 2018 at 3:53 PM, Tom Rini trini@konsulko.com wrote:
On Wed, Jul 18, 2018 at 01:09:12AM +0300, Sam Protsenko wrote:
This is just a draft to discuss ideas related to "Make U-Boot log great again" thread.
With this patch we will see something like:
Loading Environment from FAT... --> MMC: no card present --> ** Bad device mmc 0 ** --> Failed (-5) Loading Environment from MMC... --> OKinstead of:
Loading Environment from FAT... MMC: no card present ** Bad device mmc 0 ** Failed (-5) Loading Environment from MMC... OKSo, I think maybe (and given Wolfgang's comments) we should think about how the output might want to look, and how to get there without GD changes. Perhaps: Attempting to load Environment from FAT (do we have more easily available info at this point?):
Which exactly info do you mean?
MMC: no card present ** Bad device mmc 0 ** Failed (-5) Loading Environment from MMC... Attempting to load Environment from MMC: Succeeded
What do you think if we add some prefix to first message, like:
---> Attempting to load Environment from FAT: MMC: no card present ** Bad device mmc 0 ** Failed (-5) Loading Environment from MMC... ---> Attempting to load Environment from MMC: Succeeded
just to emphasize that possible errors are belong to prefixed line? Does it seem better or more ugly to you?
Overall, I agree that this is probably the only sane thing we can do right now, without meddling too much with drivers code.
-- Tom
On Wed, Jul 18, 2018 at 04:04:49PM +0300, Sam Protsenko wrote:
On Wed, Jul 18, 2018 at 3:53 PM, Tom Rini trini@konsulko.com wrote:
On Wed, Jul 18, 2018 at 01:09:12AM +0300, Sam Protsenko wrote:
This is just a draft to discuss ideas related to "Make U-Boot log great again" thread.
With this patch we will see something like:
Loading Environment from FAT... --> MMC: no card present --> ** Bad device mmc 0 ** --> Failed (-5) Loading Environment from MMC... --> OKinstead of:
Loading Environment from FAT... MMC: no card present ** Bad device mmc 0 ** Failed (-5) Loading Environment from MMC... OKSo, I think maybe (and given Wolfgang's comments) we should think about how the output might want to look, and how to get there without GD changes. Perhaps: Attempting to load Environment from FAT (do we have more easily available info at this point?):
Which exactly info do you mean?
Do we easily know things like what device / partition we're trying? Or only "env type is $X" ?
MMC: no card present ** Bad device mmc 0 ** Failed (-5) Loading Environment from MMC... Attempting to load Environment from MMC: Succeeded
What do you think if we add some prefix to first message, like:
---> Attempting to load Environment from FAT: MMC: no card present ** Bad device mmc 0 ** Failed (-5) Loading Environment from MMC... ---> Attempting to load Environment from MMC: Succeededjust to emphasize that possible errors are belong to prefixed line? Does it seem better or more ugly to you?
I don't know. I'm not a fan, but I don't always have the best taste.
Dear Tom,
In message 20180718125351.GE4609@bill-the-cat you wrote:
Loading Environment from FAT... --> MMC: no card present --> ** Bad device mmc 0 ** --> Failed (-5) Loading Environment from MMC... --> OKinstead of:
Loading Environment from FAT... MMC: no card present ** Bad device mmc 0 ** Failed (-5) Loading Environment from MMC... OKSo, I think maybe (and given Wolfgang's comments) we should think about how the output might want to look, and how to get there without GD changes. Perhaps: Attempting to load Environment from FAT (do we have more easily available info at this point?): MMC: no card present ** Bad device mmc 0 ** Failed (-5) Loading Environment from MMC... Attempting to load Environment from MMC: Succeeded
Just my 0.02€:
In the non-error case, the output should be a single (ideally short) line.
Rationale: to many lines of ourput clutter your screen and make you miss context faster; to many/long lines take time to print so they make booting slower.
In the error case, the user should be able to understand what the problem was and decide if it was critical or can be ignored (like here when intentionally booting without SDCard).
Best regards,
Wolfgang Denk
On Wed, Jul 18, 2018 at 04:09:33PM +0200, Wolfgang Denk wrote:
Dear Tom,
In message 20180718125351.GE4609@bill-the-cat you wrote:
Loading Environment from FAT... --> MMC: no card present --> ** Bad device mmc 0 ** --> Failed (-5) Loading Environment from MMC... --> OKinstead of:
Loading Environment from FAT... MMC: no card present ** Bad device mmc 0 ** Failed (-5) Loading Environment from MMC... OKSo, I think maybe (and given Wolfgang's comments) we should think about how the output might want to look, and how to get there without GD changes. Perhaps: Attempting to load Environment from FAT (do we have more easily available info at this point?): MMC: no card present ** Bad device mmc 0 ** Failed (-5) Loading Environment from MMC... Attempting to load Environment from MMC: Succeeded
Just my 0.02€:
In the non-error case, the output should be a single (ideally short) line.
Rationale: to many lines of ourput clutter your screen and make you miss context faster; to many/long lines take time to print so they make booting slower.
In the error case, the user should be able to understand what the problem was and decide if it was critical or can be ignored (like here when intentionally booting without SDCard).
I understand, but I don't know if we can get there still. The problem is we don't know if we've succeeded until we've done the relevant probing and that in turn is what's breaking the single line, and got us to where we are now.
On Thu, Jul 19, 2018 at 3:52 PM, Tom Rini trini@konsulko.com wrote:
On Wed, Jul 18, 2018 at 04:09:33PM +0200, Wolfgang Denk wrote:
Dear Tom,
In message 20180718125351.GE4609@bill-the-cat you wrote:
Loading Environment from FAT... --> MMC: no card present --> ** Bad device mmc 0 ** --> Failed (-5) Loading Environment from MMC... --> OKinstead of:
Loading Environment from FAT... MMC: no card present ** Bad device mmc 0 ** Failed (-5) Loading Environment from MMC... OKSo, I think maybe (and given Wolfgang's comments) we should think about how the output might want to look, and how to get there without GD changes. Perhaps: Attempting to load Environment from FAT (do we have more easily available info at this point?): MMC: no card present ** Bad device mmc 0 ** Failed (-5) Loading Environment from MMC... Attempting to load Environment from MMC: Succeeded
Just my 0.02€:
In the non-error case, the output should be a single (ideally short) line.
Rationale: to many lines of ourput clutter your screen and make you miss context faster; to many/long lines take time to print so they make booting slower.
In the error case, the user should be able to understand what the problem was and decide if it was critical or can be ignored (like here when intentionally booting without SDCard).
I understand, but I don't know if we can get there still. The problem is we don't know if we've succeeded until we've done the relevant probing and that in turn is what's breaking the single line, and got us to where we are now.
Actually we can, please see my new RFC patch [1]. It's a bit hacky, but the only other way to do so is to rework drivers (MMC, etc).
Also, I figured how to do prefixing (to display MMC errors as nested w.r.t. "Loading environment), without adding new field to gd. We can just add some new GD_LG_ and print prefix when it's installed. I'm gonna send new RFC soon. Please let me know what you think about [1].
[1] https://lists.denx.de/pipermail/u-boot/2018-July/335223.html
-- Tom
Dear Sam,
In message CAKaJLVuxoS0mz9pDDYqmqomsSfd83kSed=LZ261=wfUUC-Q-Xw@mail.gmail.com you wrote:
Also, I figured how to do prefixing (to display MMC errors as nested w.r.t. "Loading environment), without adding new field to gd. We can just add some new GD_LG_ and print prefix when it's installed. I'm gonna send new RFC soon. Please let me know what you think about [1].
This is IMO a totally wrong approach. We don't want to add more code (and more output) jsut to paper over inconsistent error reporting. This problem should be fixed at the root cause, i. e. we should report errors where they are detected, and only once. If all callers can rely that, in the error path, their callees which return error codes, have already printed appropriate messages, there is no need to print even more lines.
What we need is not fancy code, but consistent (and consequent) error handling.
Best regards,
Wolfgang Denk
On Thu, Jul 19, 2018 at 10:52 PM, Wolfgang Denk wd@denx.de wrote:
Dear Sam,
In message CAKaJLVuxoS0mz9pDDYqmqomsSfd83kSed=LZ261=wfUUC-Q-Xw@mail.gmail.com you wrote:
Also, I figured how to do prefixing (to display MMC errors as nested w.r.t. "Loading environment), without adding new field to gd. We can just add some new GD_LG_ and print prefix when it's installed. I'm gonna send new RFC soon. Please let me know what you think about [1].
This is IMO a totally wrong approach. We don't want to add more code (and more output) jsut to paper over inconsistent error reporting. This problem should be fixed at the root cause, i. e. we should report errors where they are detected, and only once. If all callers can rely that, in the error path, their callees which return error codes, have already printed appropriate messages, there is no need to print even more lines.
What we need is not fancy code, but consistent (and consequent) error handling.
As a matter of fact, that was first thing that came into my mind when I started looking into this (and actually I mentioned that way in my first RFC, I guess). I will try to investigate it further and come back with more meaningful patch.
My concern w.r.t. this approach is next: if we suppress consequent warning messages, there might be some other use-case (other than loading environment), where there is another caller of that API, and we would just lose all error messages at all. I'll try to check if we have such cases as well.
Overall: agree with your review, thanks.
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@denx.de "One day," said a dull voice from down below, "I'm going to be back in form again and you're going to be very sorry you said that. For a very long time. I might even go so far as to make even more Time just for you to be sorry in." - Terry Pratchett, _Small Gods_
Dear Tom,
In message 20180719125230.GJ4609@bill-the-cat you wrote:
Loading Environment from FAT... MMC: no card present ** Bad device mmc 0 ** Failed (-5) Loading Environment from MMC... OK
...
In the non-error case, the output should be a single (ideally short) line.
Rationale: to many lines of ourput clutter your screen and make you miss context faster; to many/long lines take time to print so they make booting slower.
In the error case, the user should be able to understand what the problem was and decide if it was critical or can be ignored (like here when intentionally booting without SDCard).
I understand, but I don't know if we can get there still. The problem is we don't know if we've succeeded until we've done the relevant probing and that in turn is what's breaking the single line, and got us to where we are now.
Well, IMO the approach should be the other way round - think about where we print errror messages, and when.
In the example above the "MMC: no card present" makes most sense.
When we come to printing the "** Bad device mmc 0 **" we should be in an error path, where all possible causes have already printed an appropriate message, so this line can be removed.
Ditto for the "Failed (-5)" which is pretty useless anyway - if error handling was consequent, this message should never be needed, as all error paths ending there would have printed proper messages long before.
So instead of adding prefixes or fancy postformatting we should clean up error handling and use a consistent style to report the errors where they are found and the causes for the errors are known.
Thanks.
Best regards,
Wolfgang Denk
Dear Sam,
In message 20180717220912.11358-1-semen.protsenko@linaro.org you wrote:
This is just a draft to discuss ideas related to "Make U-Boot log great again" thread.
With this patch we will have something like this:
Loading Environment from FAT... Failed (-5) Loading Environment from MMC... OKinstead of this:
Loading Environment from FAT... MMC: no card present ** Bad device mmc 0 ** Failed (-5) Loading Environment from MMC... OK
Why do you think an error message "Failed (-5)" looks great?
From a user's point of view, the "MMC: no card present"
is _much_ better (but of course it could still be improved).
Printing cryptic error codes has never been a good idea and is definitly not a "great" idea.
Best regards,
Wolfgang Denk
On Wed, Jul 18, 2018 at 9:19 AM, Wolfgang Denk wd@denx.de wrote:
Dear Sam,
In message 20180717220912.11358-1-semen.protsenko@linaro.org you wrote:
This is just a draft to discuss ideas related to "Make U-Boot log great again" thread.
With this patch we will have something like this:
Loading Environment from FAT... Failed (-5) Loading Environment from MMC... OKinstead of this:
Loading Environment from FAT... MMC: no card present ** Bad device mmc 0 ** Failed (-5) Loading Environment from MMC... OKWhy do you think an error message "Failed (-5)" looks great?
As I mentioned in commit message, this RFC series is merely for discussing ideas. I don't think it's "great" (for the same reasons you mentioned), but this is one of things we *could* do technically, to make boot log straight. What I mean "technically" doable: for example we would like to see log like this:
Loading Environment from FAT... Failed (-5): -> MMC: no card present -> ** Bad device mmc 0 ** Loading Environment from MMC... OK
But to do so, we would probably need to do one of these: 1. Rework the code for all boot sources (like drivers/mmc/mmc.c), so that that code doesn't print warnings to console, but instead filling some error buffer, so we can print that buffer later from env.c. I'm not sure it's a sane idea 2. Issuing some escape codes for moving cursor up and down, to modify already printed message, also has a lot of implications and I don't thing it's sane as well... 3. Messing with GD_FLG_RECORD and GD_FLG_SILENT also doesn't seem to be right here, as much of platforms don't enable CONFIG_CONSOLE_RECORD and CONFIG_SILENT_CONSOLE
So *technically*, we can only do what I did in two RFC patches I sent. The only other way I see, is to make boot log look like this:
--> Loading Environment from FAT... MMC: no card present ** Bad device mmc 0 ** Failed (-5): --> Loading Environment from MMC... OK
Not sure if I like this way, but it's doable.
If you have any preferences about what I said, or if you have any other ideas on how to approach this, please share. That's the whole reason why I sent this RFC series :)
From a user's point of view, the "MMC: no card present" is _much_ better (but of course it could still be improved).
Printing cryptic error codes has never been a good idea and is definitly not a "great" idea.
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@denx.de Fools ignore complexity. Pragmatists suffer it. Some can avoid it. Geniuses remove it. - Perlis's Programming Proverb #58, SIGPLAN Notices, Sept. 1982
On Wed, Jul 18, 2018 at 08:19:50AM +0200, Wolfgang Denk wrote:
Dear Sam,
In message 20180717220912.11358-1-semen.protsenko@linaro.org you wrote:
This is just a draft to discuss ideas related to "Make U-Boot log great again" thread.
With this patch we will have something like this:
Loading Environment from FAT... Failed (-5) Loading Environment from MMC... OKinstead of this:
Loading Environment from FAT... MMC: no card present ** Bad device mmc 0 ** Failed (-5) Loading Environment from MMC... OKWhy do you think an error message "Failed (-5)" looks great?
From a user's point of view, the "MMC: no card present" is _much_ better (but of course it could still be improved).
Printing cryptic error codes has never been a good idea and is definitly not a "great" idea.
Agreed, I don't think we want to go down the path of just suppressing some of these error messages, that's not helpful.
participants (3)
-
Sam Protsenko -
Tom Rini -
Wolfgang Denk