[U-Boot] [PATCH v2 0/2] env: Make environment loading log more clear
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(-)
"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 semen.protsenko@linaro.org --- 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;
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 semen.protsenko@linaro.org --- 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; }
On 20 July 2018 at 09:18, Sam Protsenko semen.protsenko@linaro.org 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 semen.protsenko@linaro.org
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 sjg@chromium.org
On Fri, Jul 20, 2018 at 6:18 PM, Sam Protsenko semen.protsenko@linaro.org 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... OKinstead of:
Loading Environment from FAT... MMC: no card present ** Bad device mmc 0 ** Failed (-5) Loading Environment from MMC... OKSam 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!
Dear Sam,
In message CAKaJLVs26p_Q8pDSn8=4Q88+Bt-_GG7b54dDGoPeQVTDm7s+LA@mail.gmail.com 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
Hi Wolfgang,
On Thu, Jul 26, 2018 at 4:28 PM, Wolfgang Denk wd@denx.de wrote:
Dear Sam,
In message CAKaJLVs26p_Q8pDSn8=4Q88+Bt-_GG7b54dDGoPeQVTDm7s+LA@mail.gmail.com 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: wd@denx.de God made the integers; all else is the work of Man. - Kronecker
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... OKinstead of:
Loading Environment from FAT... MMC: no card present ** Bad device mmc 0 ** Failed (-5) Loading Environment from MMC... OKSam 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
Hi Yaniv,
On Wed, Jul 25, 2018 at 2:05 PM, Yaniv Levinsky yaniv.levinsky@compulab.co.il 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... OKinstead of:
Loading Environment from FAT... MMC: no card present ** Bad device mmc 0 ** Failed (-5) Loading Environment from MMC... OKSam 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
participants (4)
-
Sam Protsenko -
Simon Glass -
Wolfgang Denk -
Yaniv Levinsky