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

login
register
mail settings
Submitter Phil Sutter
Date Feb. 21, 2013, 5:21 p.m.
Message ID <1361467316-29044-4-git-send-email-phil.sutter@viprinet.com>
Download mbox | patch
Permalink /patch/222378/
State Accepted
Delegated to: Scott Wood
Headers show

Comments

Phil Sutter - Feb. 21, 2013, 5:21 p.m.
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(-)
Scott Wood - Feb. 23, 2013, 1:59 a.m.
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
Phil Sutter - Feb. 25, 2013, 9:39 a.m.
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
Scott Wood - Feb. 25, 2013, 10:40 p.m.
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

Patch

diff --git a/common/env_nand.c b/common/env_nand.c
index c0c985c..60a87ec 100644
--- a/common/env_nand.c
+++ b/common/env_nand.c
@@ -316,6 +316,7 @@  int get_nand_env_oob(nand_info_t *nand, unsigned long *result)
 void env_relocate_spec(void)
 {
 #if !defined(ENV_IS_EMBEDDED)
+	int read1_fail = 0, read2_fail = 0;
 	int crc1_ok = 0, crc2_ok = 0;
 	env_t *ep, *tmp_env1, *tmp_env2;
 
@@ -327,11 +328,14 @@  void env_relocate_spec(void)
 		goto done;
 	}
 
-	if (readenv(CONFIG_ENV_OFFSET, (u_char *) tmp_env1))
-		puts("No Valid Environment Area found\n");
+	read1_fail = readenv(CONFIG_ENV_OFFSET, (u_char *) tmp_env1);
+	read2_fail = readenv(CONFIG_ENV_OFFSET_REDUND, (u_char *) tmp_env2);
 
-	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;