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 stefan.tauner@student.tuwien.ac.at
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