[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

-- 
2.18.0

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

-- 
2.18.0

Dear Sam,

In message [off-list ref] 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... OK

instead 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

-- 
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
Fools ignore complexity. Pragmatists suffer it. Some  can  avoid  it.
Geniuses remove it.
     - Perlis's Programming Proverb #58, SIGPLAN Notices, Sept.  1982

Dear Sam,

In message [off-list ref] 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.

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

-- 
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
There are no data that cannot be plotted on a straight  line  if  the
axis are chosen correctly.

On Wed, Jul 18, 2018 at 9:19 AM, Wolfgang Denk [off-list ref] wrote:

Dear Sam,

In message [off-list ref] you wrote:

quoted

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

Why 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 at 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 [off-list ref] you wrote:

quoted

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

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.

Agreed, I don't think we want to go down the path of just suppressing
some of these error messages, that's not helpful.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180718/f87fd88e/attachment.sig>

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180718/8c5f8bda/attachment.sig>

On Wed, Jul 18, 2018 at 3:53 PM, Tom Rini [off-list ref] wrote:

On Wed, Jul 18, 2018 at 01:09:12AM +0300, Sam Protsenko wrote:

quoted

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:

quoted

    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

-- 
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
"Wish not to seem, but to be, the best."                  - Aeschylus

On Wed, Jul 18, 2018 at 04:04:49PM +0300, Sam Protsenko wrote:

On Wed, Jul 18, 2018 at 3:53 PM, Tom Rini [off-list ref] wrote:

quoted

On Wed, Jul 18, 2018 at 01:09:12AM +0300, Sam Protsenko wrote:

quoted

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" ?

quoted

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180718/1770e7be/attachment.sig>

On Wed, Jul 18, 2018 at 04:09:33PM +0200, Wolfgang Denk wrote:

Dear Tom,

In message <20180718125351.GE4609@bill-the-cat> you wrote:

quoted

quoted

    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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180719/5878a7ad/attachment.sig>

On Thu, Jul 19, 2018 at 3:52 PM, Tom Rini [off-list ref] wrote:

On Wed, Jul 18, 2018 at 04:09:33PM +0200, Wolfgang Denk wrote:

quoted

Dear Tom,

In message <20180718125351.GE4609@bill-the-cat> you wrote:

quoted

quoted

    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:

quoted

quoted

quoted

    Loading Environment from FAT... MMC: no card present
    ** Bad device mmc 0 **
    Failed (-5)
    Loading Environment from MMC... OK

...

quoted

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

-- 
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
Die meisten Menschen pflegen im Kindesalter vom Zeigen auf Gegenstän-
de (Mausbewegung) und "ga" sagen  (Mausklick)  abzukommen,  zugunsten
eines  mächtigeren  und langwierig zu erlernenden Tools (Sprache).
                             -- Achim Linder in de.comp.os.linux.misc

Dear Sam,

In message [off-list ref] 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

-- 
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
"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_

On Thu, Jul 19, 2018 at 10:52 PM, Wolfgang Denk [off-list ref] wrote:

Dear Sam,

In message [off-list ref] you wrote:

quoted

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 at 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_