Re: [flashrom] [PATCH 1/3] Do not read the flash chip twice in verification mode.

[Carl-Daniel Hailfinger]

Am 23.07.2012 15:33 schrieb Stefan Tauner:
> Kyösti Mälkki noticed that we unnecessarily read the flash chip twice when
> called with --verify. The first one is the mandatory read before everything
> (to be able to detect the seriousness of errors), but the second one is not
> necessary because we can just use the former for the comparison.

Indeed.
> This introduces a small output change: previously we printed ERASE or
> VERIFY depending on the callee. This special case has been dropped
> because it is unnecessary to print it (and wrong for the verification
> function to need to know why it is verifying exactly).
> If an erase fails we mention that fact explicitly already, similar for verify.

I'm not convinced yet. I'll try to compare the failure output with and
without this patch before I ack.
> Signed-off-by: Stefan Tauner
>
> flash.h    |    2 +-
>  flashrom.c |   86 +++++++++++++++++++++++++++---------------------------------
>  jedec.c    |    2 +-
>  3 files changed, 41 insertions(+), 49 deletions(-)
>
> diff --git a/flash.h b/flash.h
> index d669512..5bb1211 100644
> --- a/flash.h
> +++ b/flash.h
> @@ -241,7 +241,7 @@ int min(int a, int b);
>  int max(int a, int b);
>  void tolower_string(char *str);
>  char *extract_param(char **haystack, const char *needle, const char *delim);
> -int verify_range(struct flashctx *flash, uint8_t *cmpbuf, unsigned int start, unsigned int len, const char *message);
> +int verify_range(struct flashctx *flash, uint8_t *cmpbuf, unsigned int start, unsigned int len);
>  int need_erase(uint8_t *have, uint8_t *want, unsigned int len, enum write_granularity gran);
>  char *strcat_realloc(char *dest, const char *src);
>  void print_version(void);
> diff --git a/flashrom.c b/flashrom.c
> index fc52c4a..70687ff 100644
> --- a/flashrom.c
> +++ b/flashrom.c
> @@ -548,6 +548,26 @@ static unsigned int count_usable_erasers(const struct flashctx *flash)
>    return usable_erasefunctions;
>  }
>
> +int compare_range(uint8_t *wantbuf, uint8_t *havebuf, unsigned int start, unsigned int len)

Make this function static?
> +{
> int ret = 0, failcount = 0;
> unsigned int i;
> for (i = 0; i < len; i++) {
> if (wantbuf[i] != havebuf[i]) {
> 	/* Only print the first failure. */
> 	if (!failcount++)
> 		msg_cerr("FAILED at 0x%08x! Expected=0x%02x, Found=0x%02x,",
> 			 start + i, wantbuf[i], havebuf[i]);
> }
> }
> if (failcount) {
> msg_cerr(" failed byte count from 0x%08x-0x%08x: 0x%x\n",
> 	 start, start + len - 1, failcount);
> ret = -1;
> }
> return ret;
>
> +}
>
> /* start is an offset to the base address of the flash chip */
>  int check_erased_range(struct flashctx *flash, unsigned int start,
>    	       unsigned int len)
> @@ -560,7 +580,7 @@ int check_erased_range(struct flashctx *flash, unsigned int start,
>    	exit(1);
>    }
>    memset(cmpbuf, 0xff, len);
> ret = verify_range(flash, cmpbuf, start, len, "ERASE");
> ret = verify_range(flash, cmpbuf, start, len);
> free(cmpbuf);
> return ret;
>
> }
> @@ -570,15 +590,12 @@ int check_erased_range(struct flashctx *flash, unsigned int start,
> flash content at location start
> @start	offset to the base address of the flash chip
> @len		length of the verified area
> @message	string to print in the "FAILED" message
> @return	0 for success, -1 for failure
> */
>
> -int verify_range(struct flashctx *flash, uint8_t *cmpbuf, unsigned int start,
>  unsigned int len, const char *message)
>
> +int verify_range(struct flashctx *flash, uint8_t *cmpbuf, unsigned int start, unsigned int len)
>  {
> unsigned int i;
> uint8_t *readbuf = malloc(len);
> int ret = 0, failcount = 0;
>
> int ret = 0;
>
> if (!len)
> goto out_free;
>
> @@ -599,8 +616,6 @@ int verify_range(struct flashctx *flash, uint8_t *cmpbuf, unsigned int start,
>    	ret = -1;
>    	goto out_free;
>    }
>
> if (!message)
> message = "VERIFY";
>
> ret = flash->read(flash, readbuf, start, len);
> if (ret) {
>
> @@ -609,22 +624,7 @@ int verify_range(struct flashctx *flash, uint8_t *cmpbuf, unsigned int start,
>    	return ret;
>    }
> for (i = 0; i < len; i++) {
> if (cmpbuf[i] != readbuf[i]) {
> 	/* Only print the first failure. */
> 	if (!failcount++)
> 		msg_cerr("%s FAILED at 0x%08x! "
> 			 "Expected=0x%02x, Read=0x%02x,",
> 			 message, start + i, cmpbuf[i],
> 			 readbuf[i]);
> }
> }
> if (failcount) {
> msg_cerr(" failed byte count from 0x%08x-0x%08x: 0x%x\n",
> 	 start, start + len - 1, failcount);
> ret = -1;
> }
> ret = compare_range(cmpbuf, readbuf, start, len);

Good idea to refactor this.
> out_free:
>    free(readbuf);
>    return ret;
> @@ -1060,21 +1060,6 @@ notfound:
>    return flash - flashchips;
>  }
>
> -int verify_flash(struct flashctx *flash, uint8_t *buf)
> -{
> int ret;
> unsigned int total_size = flash->total_size * 1024;
> msg_cinfo("Verifying flash... ");
> ret = verify_range(flash, buf, 0, total_size, NULL);
> if (!ret)
> msg_cinfo("VERIFIED.          \n");
> return ret;
>
> -}

I believe that this hunk conflicts with some of your layout patches.
Maybe not a direct patch hunk conflict, but having a standalone function
which verifies the flash chip according to some layout rules is a good
thing. I'd keep verify_flash for now. OTOH, the hunk below makes it
painfully obvious that the current verify_flash abstraction is not the
right abstraction.
Two functions compare_all_ranges() defaulting to a full chip range and
verify_all_ranges() with similar characteristics would IMHO be a good
abstraction, especially in light of the pending improved layout support.
> int read_buf_from_file(unsigned char *buf, unsigned long size,
>    	       const char *filename)
>  {
> @@ -1838,15 +1823,22 @@ int doit(struct flashctx *flash, int force, const char *filename, int read_it,
>    }
>
> if (verify_it) {
> /* Work around chips which need some time to calm down. */
> if (write_it)
> msg_cinfo("Verifying flash... ");
> if (write_it) {
> 	/* Work around chips which need some time to calm down. */
> programmer_delay(1000*1000);
> ret = verify_flash(flash, newcontents);
> /* If we tried to write, and verification now fails, we
>  * might have an emergency situation.
>  */
> if (ret && write_it)
> 	emergency_help_message();
> 	ret = verify_range(flash, newcontents, 0, size);
> 	/* If we tried to write, and verification now fails, we
> 	 * might have an emergency situation.
> 	 */
> 	if (ret)
> 		emergency_help_message();
> } else {
> 	ret = compare_range(newcontents, oldcontents, 0, size);
> }
> if (!ret)
> 	msg_cinfo("VERIFIED.\n");
> }
>
> out:
> diff --git a/jedec.c b/jedec.c
> index 69c0c0c..1fa1a10 100644
> --- a/jedec.c
> +++ b/jedec.c
> @@ -409,7 +409,7 @@ retry:
>
> dst = d;
>    src = s;
> failed = verify_range(flash, src, start, page_size, NULL);
>
> failed = verify_range(flash, src, start, page_size);
>
> if (failed && tried++ < MAX_REFLASH_TRIES) {
> msg_cerr("retrying.\n");

I agree with the direction of the patch, I'm just not yet sure about the
implementation.

Regards,
Carl-Daniel

[PATCH 1/3] Do not read the flash chip twice in verification mode.

[Stefan Tauner]

On Wed, 01 Aug 2012 01:20:01 +0200
Carl-Daniel Hailfinger  wrote:
> > This introduces a small output change: previously we printed ERASE or
> > VERIFY depending on the callee. This special case has been dropped
> > because it is unnecessary to print it (and wrong for the verification
> > function to need to know why it is verifying exactly).
> > If an erase fails we mention that fact explicitly already, similar for verify.
>
> I'm not convinced yet. I'll try to compare the failure output with and
> without this patch before I ack.

maybe this helps :)

new non-verbose behavior:
Erasing and writing flash chip... FAILED at 0x00040000! Expected=0xff, Found=0x91, failed byte count from 0x00040000-0x00040fff: 0xff0
ERASE FAILED!
Reading current flash chip contents... done. Erase/write done.
Verifying flash... FAILED at 0x00010000! Expected=0x6b, Found=0x13, failed byte count from 0x00000000-0x0007ffff: 0x101
Your flash chip is in an unknown state.

old non-verbose behavior:
Erasing and writing flash chip... ERASE FAILED at 0x00040000! Expected=0xff, Read=0x9c, failed byte count from 0x00040000-0x00040fff: 0xfea
ERASE FAILED!
Reading current flash chip contents... done. Erase/write done.
Verifying flash... VERIFY FAILED at 0x00010000! Expected=0x11, Read=0x13, failed byte count from 0x00000000-0x0007ffff: 0xff
Your flash chip is in an unknown state.

new verbose:
Erasing and writing flash chip... Trying erase function 0... 0x000000-0x000fff:EW, 0x001000-0x001fff:EW, 0x002000-0x002fff:EW, 0x003000-0x003fff:EW, 0x004000-0x004fff:EW, 0x005000-0x005fff:EW,
[…]
0x03c000-0x03cfff:EW, 0x03d000-0x03dfff:EW, 0x03e000-0x03efff:EW, 0x03f000-0x03ffff:EW, 0x040000-0x040fff:EFAILED at 0x00040000! Expected=0xff, Found=0x9c, failed byte count from 0x00040000-0x00040fff: 0xfea
ERASE FAILED!
Reading current flash chip contents... done. Looking for another erase function.
Trying erase function 1... 0x000000-0x007fff:S, 0x008000-0x00ffff:S, 0x010000-0x017fff:S, 0x018000-0x01ffff:S, 0x020000-0x027fff:EW, 0x028000-0x02ffff:S, 0x030000-0x037fff:S, 0x038000-0x03ffff:S, 0x040000-0x047fff:EW, 0x048000-0x04ffff:EW, 0x050000-0x057fff:EW, 0x058000-0x05ffff:EW, 0x060000-0x067fff:EW, 0x068000-0x06ffff:EW, 0x070000-0x077fff:EW, 0x078000-0x07ffff:EW
Erase/write done.
Verifying flash... FAILED at 0x00010000! Expected=0x11, Found=0x13, failed byte count from 0x00000000-0x0007ffff: 0xff
Your flash chip is in an unknown state.

old verbose:
Erasing and writing flash chip... Trying erase function 0... 0x000000-0x000fff:EW, 0x001000-0x001fff:EW, 0x002000-0x002fff:EW, 0x003000-0x003fff:EW, 0x004000-0x004fff:EW, 0x005000-0x005fff:EW, 
[…]
0x03c000-0x03cfff:EW, 0x03d000-0x03dfff:EW, 0x03e000-0x03efff:EW, 0x03f000-0x03ffff:EW, 0x040000-0x040fff:EERASE FAILED at 0x00040000! Expected=0xff, Read=0x91, failed byte count from 0x00040000-0x00040fff: 0xff0
ERASE FAILED!
Reading current flash chip contents... done. Looking for another erase function.
Trying erase function 1... 0x000000-0x007fff:S, 0x008000-0x00ffff:S, 0x010000-0x017fff:S, 0x018000-0x01ffff:S, 0x020000-0x027fff:EW, 0x028000-0x02ffff:S, 0x030000-0x037fff:S, 0x038000-0x03ffff:S, 0x040000-0x047fff:EW, 0x048000-0x04ffff:EW, 0x050000-0x057fff:EW, 0x058000-0x05ffff:EW, 0x060000-0x067fff:EW, 0x068000-0x06ffff:EW, 0x070000-0x077fff:EW, 0x078000-0x07ffff:EW
Erase/write done.
Verifying flash... VERIFY FAILED at 0x00010000! Expected=0x6b, Read=0x13, failed byte count from 0x00000000-0x0007ffff: 0x101
Your flash chip is in an unknown state.

so the verify output is pretty clearly better (less redundant) now than
before, the erase output is a little worse than before, but not much.

maybe printing "Verification" instead of "ERASE" and "VERIFY" might be
clearer, but the op-dependent parameter is clearly not that important
to warrant keeping it (and complicating the matter that way).

[PATCH 1/3] Do not read the flash chip twice in verification mode.

[Stefan Tauner]

On Thu, 9 Aug 2012 01:01:30 +0200
Stefan Tauner  wrote:
> > > This introduces a small output change: previously we printed ERASE or
> > > VERIFY depending on the callee. This special case has been dropped
> > > because it is unnecessary to print it (and wrong for the verification
> > > function to need to know why it is verifying exactly).
> > > If an erase fails we mention that fact explicitly already, similar for verify.
> >
> > I'm not convinced yet. I'll try to compare the failure output with and
> > without this patch before I ack.

1/4 year is too long.
committed in r1619.

[PATCH 1/3] Do not read the flash chip twice in verification mode.

[Carl-Daniel Hailfinger]

Am 27.10.2012 17:38 schrieb Stefan Tauner:
> On Thu, 9 Aug 2012 01:01:30 +0200
> Stefan Tauner  wrote:
> > > > This introduces a small output change: previously we printed ERASE or
> > > > VERIFY depending on the callee. This special case has been dropped
> > > > because it is unnecessary to print it (and wrong for the verification
> > > > function to need to know why it is verifying exactly).
> > > > If an erase fails we mention that fact explicitly already, similar for verify.
> > >
> > > I'm not convinced yet. I'll try to compare the failure output with and
> > > without this patch before I ack.
>
> 1/4 year is too long.
> committed in r1619.

Point taken. Thanks for merging this.

Regards,
Carl-Daniel