Patchwork [U-Boot,PATCHv2,3/4] env_nand.c: clarify log messages when env reading fails

The single message is misleading, since there is no equivalent success
note when reading the other copy succeeds. Instead, warn if one of the
redundant copies could not be loaded and emphasise on the error when
reading both fails.

Signed-off-by: Phil Sutter <phil.sutter@viprinet.com>
---
 common/env_nand.c |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

On Thu, Feb 21, 2013 at 06:21:55PM +0100, Phil Sutter wrote:
> The single message is misleading, since there is no equivalent success
> note when reading the other copy succeeds. Instead, warn if one of the
> redundant copies could not be loaded and emphasise on the error when
> reading both fails.
> 
> Signed-off-by: Phil Sutter <phil.sutter@viprinet.com>
> ---
>  common/env_nand.c |   12 ++++++++----
>  1 files changed, 8 insertions(+), 4 deletions(-)

Applied to u-boot-nand-flash.

> -	if (readenv(CONFIG_ENV_OFFSET_REDUND, (u_char *) tmp_env2))
> -		puts("No Valid Redundant Environment Area found\n");
> +	if (read1_fail && read2_fail)
> +		puts("*** Error - No Valid Environment Area found\n");
> +	else if (read1_fail || read2_fail)
> +		puts("*** Warning - some problems detected "
> +		     "reading environment; recovered successfully\n");
>  
>  	crc1_ok = crc32(0, tmp_env1->data, ENV_SIZE) == tmp_env1->crc;
>  	crc2_ok = crc32(0, tmp_env2->data, ENV_SIZE) == tmp_env2->crc;

We should also give a message if one of the CRCs is bad, though that's
an existing problem.

-Scott

Scott,

On Fri, Feb 22, 2013 at 07:59:41PM -0600, Scott Wood wrote:
> On Thu, Feb 21, 2013 at 06:21:55PM +0100, Phil Sutter wrote:
> > The single message is misleading, since there is no equivalent success
> > note when reading the other copy succeeds. Instead, warn if one of the
> > redundant copies could not be loaded and emphasise on the error when
> > reading both fails.
> > 
> > Signed-off-by: Phil Sutter <phil.sutter@viprinet.com>
> > ---
> >  common/env_nand.c |   12 ++++++++----
> >  1 files changed, 8 insertions(+), 4 deletions(-)
> 
> Applied to u-boot-nand-flash.
> 
> > -	if (readenv(CONFIG_ENV_OFFSET_REDUND, (u_char *) tmp_env2))
> > -		puts("No Valid Redundant Environment Area found\n");
> > +	if (read1_fail && read2_fail)
> > +		puts("*** Error - No Valid Environment Area found\n");
> > +	else if (read1_fail || read2_fail)
> > +		puts("*** Warning - some problems detected "
> > +		     "reading environment; recovered successfully\n");
> >  
> >  	crc1_ok = crc32(0, tmp_env1->data, ENV_SIZE) == tmp_env1->crc;
> >  	crc2_ok = crc32(0, tmp_env2->data, ENV_SIZE) == tmp_env2->crc;
> 
> We should also give a message if one of the CRCs is bad, though that's
> an existing problem.

Yes, that would be nice. While writing this, I also had the idea of
introducing some macros for unified message output, like so:

| #define __print(level, ...) {
|	printf("*** %s - ", level);
|	printf(__VA_ARGS__);
|	printf("\n");
| }
| #define perror(...) __print("Error", __VA_ARGS__)
| #define pwarn(...) __print("Warning", __VA_ARGS__)
| ...

What do you think? That would require diligently touching a lot of
source files, of course.

Best wishes,

Phil Sutter
Software Engineer

On 02/25/2013 03:39:12 AM, Phil Sutter wrote:
> Scott,
> 
> On Fri, Feb 22, 2013 at 07:59:41PM -0600, Scott Wood wrote:
> > We should also give a message if one of the CRCs is bad, though  
> that's
> > an existing problem.
> 
> Yes, that would be nice. While writing this, I also had the idea of
> introducing some macros for unified message output, like so:
> 
> | #define __print(level, ...) {
> |	printf("*** %s - ", level);
> |	printf(__VA_ARGS__);
> |	printf("\n");
> | }
> | #define perror(...) __print("Error", __VA_ARGS__)
> | #define pwarn(...) __print("Warning", __VA_ARGS__)
> | ...
> 
> What do you think? That would require diligently touching a lot of
> source files, of course.

This suggestion belongs in its own thread with an appropriate subject,  
but if we do anything like this we should use pr_warn(), pr_err(), etc.  
since we share a bunch of code with Linux -- although output from that  
code may be made worse since it's not expecting the prefixes...

-Scott