[flashrom] [PATCH 1/3] Do not read the flash chip twice in verification mode.
Carl-Daniel Hailfinger
c-d.hailfinger.devel.2006 at gmx.net
Wed Aug 1 01:20:01 CEST 2012
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 at 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
--
http://www.hailfinger.org/
More information about the flashrom
mailing list