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 <[email protected]> > --- > 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 -- http://www.hailfinger.org/ _______________________________________________ flashrom mailing list [email protected] http://www.flashrom.org/mailman/listinfo/flashrom
