[PATCH v2 0/2] env: Make environment loading log more clear

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH v2 0/2] env: Make environment loading log more clear

Sam Protsenko
This patch series intended to make boot log better. Basically here we
just remove unwanted error messages, relying on the message from most
deep API to be printed (like mmc subsystem). At the moment this looks
like most clean solution to cluttered log problem, as any other solution
will be hackish.

With this patch set applied we will see something like this:

    Loading Environment from FAT... MMC: no card present
    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

Sam Protsenko (2):
  env: Don't print "Failed" error message
  disk: part: Don't show redundant error message

 disk/part.c |  2 +-
 env/env.c   | 12 +++++++-----
 2 files changed, 8 insertions(+), 6 deletions(-)

--
2.18.0

_______________________________________________
U-Boot mailing list
[hidden email]
https://lists.denx.de/listinfo/u-boot
Reply | Threaded
Open this post in threaded view
|

[PATCH v2 1/2] env: Don't print "Failed" error message

Sam Protsenko
"Failed" error message from env_load() only clutters the log with
unnecessary details, as we already have all needed warnings by that
time. Example:

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

Remove this "Failed" message to keep log short and clear.

Signed-off-by: Sam Protsenko <[hidden email]>
---
Changes in v2:
  - Join two consecutive "if (!ret)" conditions
  - Add the comment with requirement for underlying API to print error
    message (as we don't print "Failed" message anymore)

 env/env.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/env/env.c b/env/env.c
index 5c0842ac07..c830001eee 100644
--- a/env/env.c
+++ b/env/env.c
@@ -195,14 +195,16 @@ int env_load(void)
  continue;
 
  printf("Loading Environment from %s... ", drv->name);
+ /*
+ * In error case, the error message must be printed during
+ * drv->load() in some underlying API, and it must be exactly
+ * one message.
+ */
  ret = drv->load();
- if (ret)
- printf("Failed (%d)\n", ret);
- else
+ if (!ret) {
  printf("OK\n");
-
- if (!ret)
  return 0;
+ }
  }
 
  return -ENODEV;
--
2.18.0

_______________________________________________
U-Boot mailing list
[hidden email]
https://lists.denx.de/listinfo/u-boot
Reply | Threaded
Open this post in threaded view
|

[PATCH v2 2/2] disk: part: Don't show redundant error message

Sam Protsenko
In reply to this post by Sam Protsenko
Underlying API should already print some meaningful error message, so
this one is just brings more noise. E.g. we can see log like this:

    MMC: no card present
    ** Bad device mmc 0 **

Obviously, second error message is unwanted. Let's only print it in case
when DEBUG is defined to keep log short and clear.

Signed-off-by: Sam Protsenko <[hidden email]>
---
Changes in v2:
  - Instead of removing error message, print it with debug()

 disk/part.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/disk/part.c b/disk/part.c
index 9266a09ec3..9e457a6e72 100644
--- a/disk/part.c
+++ b/disk/part.c
@@ -400,7 +400,7 @@ int blk_get_device_by_str(const char *ifname, const char *dev_hwpart_str,
 
  *dev_desc = get_dev_hwpart(ifname, dev, hwpart);
  if (!(*dev_desc) || ((*dev_desc)->type == DEV_TYPE_UNKNOWN)) {
- printf("** Bad device %s %s **\n", ifname, dev_hwpart_str);
+ debug("** Bad device %s %s **\n", ifname, dev_hwpart_str);
  dev = -ENOENT;
  goto cleanup;
  }
--
2.18.0

_______________________________________________
U-Boot mailing list
[hidden email]
https://lists.denx.de/listinfo/u-boot
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 2/2] disk: part: Don't show redundant error message

Simon Glass-3
On 20 July 2018 at 09:18, Sam Protsenko <[hidden email]> wrote:

> Underlying API should already print some meaningful error message, so
> this one is just brings more noise. E.g. we can see log like this:
>
>     MMC: no card present
>     ** Bad device mmc 0 **
>
> Obviously, second error message is unwanted. Let's only print it in case
> when DEBUG is defined to keep log short and clear.
>
> Signed-off-by: Sam Protsenko <[hidden email]>
> ---
> Changes in v2:
>   - Instead of removing error message, print it with debug()
>
>  disk/part.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Simon Glass <[hidden email]>
_______________________________________________
U-Boot mailing list
[hidden email]
https://lists.denx.de/listinfo/u-boot
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 0/2] env: Make environment loading log more clear

Sam Protsenko
In reply to this post by Sam Protsenko
On Fri, Jul 20, 2018 at 6:18 PM, Sam Protsenko
<[hidden email]> wrote:

> This patch series intended to make boot log better. Basically here we
> just remove unwanted error messages, relying on the message from most
> deep API to be printed (like mmc subsystem). At the moment this looks
> like most clean solution to cluttered log problem, as any other solution
> will be hackish.
>
> With this patch set applied we will see something like this:
>
>     Loading Environment from FAT... MMC: no card present
>     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
>
> Sam Protsenko (2):
>   env: Don't print "Failed" error message
>   disk: part: Don't show redundant error message
>
>  disk/part.c |  2 +-
>  env/env.c   | 12 +++++++-----
>  2 files changed, 8 insertions(+), 6 deletions(-)
>
> --
> 2.18.0
>

Hi Wolfgang,

Does this series look ok to you? Can you please review?

Thanks!
_______________________________________________
U-Boot mailing list
[hidden email]
https://lists.denx.de/listinfo/u-boot
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 0/2] env: Make environment loading log more clear

Yaniv Levinsky
In reply to this post by Sam Protsenko
On 07/20/2018 06:18 PM, Sam Protsenko wrote:

> This patch series intended to make boot log better. Basically here we
> just remove unwanted error messages, relying on the message from most
> deep API to be printed (like mmc subsystem). At the moment this looks
> like most clean solution to cluttered log problem, as any other solution
> will be hackish.
>
> With this patch set applied we will see something like this:
>
>     Loading Environment from FAT... MMC: no card present
>     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
>
> Sam Protsenko (2):
>   env: Don't print "Failed" error message
>   disk: part: Don't show redundant error message
>
>  disk/part.c |  2 +-
>  env/env.c   | 12 +++++++-----
>  2 files changed, 8 insertions(+), 6 deletions(-)
>

Hi Sam,

After following the discussion from "Make U-Boot log great again" to
here, it made me wonder: Does the user really need to be exposed to all
the failed attempts to load the environment if it succeeded eventually?

Maybe the maintainers are willing the consider a more drastic solution
for clearing the console clutter when the environment loads.

What if the only thing the user would see on a successful load is this:
        ENV:   Loaded from MMC
And the rest of the usual clutter would be visible only if DEBUG is set.

It shouldn't be too hard to implement (Rising GD_FLG_SILENT if DEBUG not
defined) and it is very consistent with the rest of the printed messages
on boot. The problem is how and what to print on a failed load.

I think it would be best if we could keep the above pattern like so:
        ENV:   Failed to load from FAT - MMC: No card present (-5)
        ENV:   Failed to load from MMC - No MMC card found (-5)
        ENV:   Using default environment
The last line would print only if (gd->flags & GD_FLG_ENV_DEFAULT)

This might be harder to implement, but do you think it could work?

Yaniv
_______________________________________________
U-Boot mailing list
[hidden email]
https://lists.denx.de/listinfo/u-boot
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 0/2] env: Make environment loading log more clear

Wolfgang Denk
In reply to this post by Sam Protsenko
Dear Sam,

In message <CAKaJLVs26p_Q8pDSn8=[hidden email]> you wrote:
>
> Does this series look ok to you? Can you please review?

Sorry for the delay.

Hm... I wonder if we should handle patch 1/2 in the same way as done
in 2/2, i. e. not remove the printf() in the error case, but turn it
into a debug() ?

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: [hidden email]
God made the integers; all else is the work of Man.       - Kronecker
_______________________________________________
U-Boot mailing list
[hidden email]
https://lists.denx.de/listinfo/u-boot
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 0/2] env: Make environment loading log more clear

Sam Protsenko
In reply to this post by Yaniv Levinsky
Hi Yaniv,

On Wed, Jul 25, 2018 at 2:05 PM, Yaniv Levinsky
<[hidden email]> wrote:

> On 07/20/2018 06:18 PM, Sam Protsenko wrote:
>> This patch series intended to make boot log better. Basically here we
>> just remove unwanted error messages, relying on the message from most
>> deep API to be printed (like mmc subsystem). At the moment this looks
>> like most clean solution to cluttered log problem, as any other solution
>> will be hackish.
>>
>> With this patch set applied we will see something like this:
>>
>>     Loading Environment from FAT... MMC: no card present
>>     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
>>
>> Sam Protsenko (2):
>>   env: Don't print "Failed" error message
>>   disk: part: Don't show redundant error message
>>
>>  disk/part.c |  2 +-
>>  env/env.c   | 12 +++++++-----
>>  2 files changed, 8 insertions(+), 6 deletions(-)
>>
>
> Hi Sam,
>
> After following the discussion from "Make U-Boot log great again" to
> here, it made me wonder: Does the user really need to be exposed to all
> the failed attempts to load the environment if it succeeded eventually?
>
> Maybe the maintainers are willing the consider a more drastic solution
> for clearing the console clutter when the environment loads.
>
> What if the only thing the user would see on a successful load is this:
>         ENV:   Loaded from MMC
> And the rest of the usual clutter would be visible only if DEBUG is set.
>

I believe we discussed something similar, and came to conclusion than
it's not good to hide useful warnings, user should see it. You may
want to go through discussion on my RFC patches I sent, I guess
Wolfgang mentioned that and we agreed.

> It shouldn't be too hard to implement (Rising GD_FLG_SILENT if DEBUG not
> defined) and it is very consistent with the rest of the printed messages
> on boot. The problem is how and what to print on a failed load.
>

On one of my RFC patches I considered using GD_FLG_SILED, but it won't
work if CONFIG_SILENT_CONSOLE is not defined (look at
common/console.c, in puts() implementation). And a lot of platforms
don't define that config option, so we can't rely on it.

Moreover, as Wolfgang mentioned in my RFC patches, we shouldn't do
post-beautifying the log, but we should instead disable all not
desired messages, and only leave most relevant one (that's what I'm
trying to do in patch series).

> I think it would be best if we could keep the above pattern like so:
>         ENV:   Failed to load from FAT - MMC: No card present (-5)
>         ENV:   Failed to load from MMC - No MMC card found (-5)
>         ENV:   Using default environment
> The last line would print only if (gd->flags & GD_FLG_ENV_DEFAULT)
>

The same problem here. We know return code only when all errors were
already printed. So it would require doing some post-cleanup, which is
not possible for all platforms. Also Wolfgang is obviously against
that approach (and I can see his point, it's better and easier to just
cleanup errors reporting). This way we will have pretty much the same
as you suggested. With my patches you'll see something like this:

    Loading Environment from FAT... MMC: no card present
    Loading Environment from MMC... OK


> This might be harder to implement, but do you think it could work?
>

Overall: please check my RFC patches I sent recently. I believe they
implement quite similar feature you suggest, but maintainers don't
think it's the best approach to the problem.

> Yaniv
_______________________________________________
U-Boot mailing list
[hidden email]
https://lists.denx.de/listinfo/u-boot
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 0/2] env: Make environment loading log more clear

Sam Protsenko
In reply to this post by Wolfgang Denk
Hi Wolfgang,

On Thu, Jul 26, 2018 at 4:28 PM, Wolfgang Denk <[hidden email]> wrote:

> Dear Sam,
>
> In message <CAKaJLVs26p_Q8pDSn8=[hidden email]> you wrote:
>>
>> Does this series look ok to you? Can you please review?
>
> Sorry for the delay.
>
> Hm... I wonder if we should handle patch 1/2 in the same way as done
> in 2/2, i. e. not remove the printf() in the error case, but turn it
> into a debug() ?
>

I thought about it, and it didn't seem like very useful message to me
at that time. But ok, I think it won't hurt to have it anyway. Will
send next patch series with the fix soon.

Thank!

> 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: [hidden email]
> God made the integers; all else is the work of Man.       - Kronecker
_______________________________________________
U-Boot mailing list
[hidden email]
https://lists.denx.de/listinfo/u-boot