| Message ID | 20180720151856.23458-1-semen.protsenko@linaro.org |
|---|---|
| Headers | show |
| Series | env: Make environment loading log more clear | expand |
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... 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!
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
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 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... 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
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
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(-)