| Message ID | 20180717220912.11358-2-semen.protsenko@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC,1/2] env: Drop error messages when loading environment | expand |
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... > --> OK > > instead 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 -- Tom
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... >> --> OK >> >> instead 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?): 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
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... > > --> OK > > > > instead 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 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: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... > >> --> OK > >> > >> instead 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?): > > 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: > Succeeded > > just 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. -- Tom
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... > > > --> OK > > > > > > instead 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 > > 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. -- Tom
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... >> > > --> OK >> > > >> > > instead 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 >> >> 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 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 <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_
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
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(-)