verify_range() and check_erased_range() check each page separately. While that may have seemed like a good idea back when the code was introduced, it has no benefits for any of the chips where we support write because all of them handle cross-page reads nicely. The only class of chips where this change could be a problem is chips with non power of two sector sizes which have gaps in the address space. We could simply require their read functions to provide gap-free results and leave it at that.
This patch should speed up verify on Dediprog by a factor of ~100.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Index: flashrom-verify_not_chunked/flashrom.c =================================================================== --- flashrom-verify_not_chunked/flashrom.c (Revision 1232) +++ flashrom-verify_not_chunked/flashrom.c (Arbeitskopie) @@ -726,9 +726,8 @@ */ int verify_range(struct flashchip *flash, uint8_t *cmpbuf, int start, int len, char *message) { - int i, j, starthere, lenhere, ret = 0; - int page_size = flash->page_size; - uint8_t *readbuf = malloc(page_size); + int i, ret = 0; + uint8_t *readbuf = malloc(len); int failcount = 0;
if (!len) @@ -753,37 +752,21 @@ if (!message) message = "VERIFY"; - /* Warning: This loop has a very unusual condition and body. - * The loop needs to go through each page with at least one affected - * byte. The lowest page number is (start / page_size) since that - * division rounds down. The highest page number we want is the page - * where the last byte of the range lives. That last byte has the - * address (start + len - 1), thus the highest page number is - * (start + len - 1) / page_size. Since we want to include that last - * page as well, the loop condition uses <=. - */ - for (i = start / page_size; i <= (start + len - 1) / page_size; i++) { - /* Byte position of the first byte in the range in this page. */ - starthere = max(start, i * page_size); - /* Length of bytes in the range in this page. */ - lenhere = min(start + len, (i + 1) * page_size) - starthere; - ret = flash->read(flash, readbuf, starthere, lenhere); - if (ret) { - msg_gerr("Verification impossible because read failed " - "at 0x%x (len 0x%x)\n", starthere, lenhere); - break; + ret = flash->read(flash, readbuf, start, len); + if (ret) { + msg_gerr("Verification impossible because read failed " + "at 0x%x (len 0x%x)\n", start, len); + } + + 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]); } - for (j = 0; j < lenhere; j++) { - if (cmpbuf[starthere - start + j] != readbuf[j]) { - /* Only print the first failure. */ - if (!failcount++) - msg_cerr("%s FAILED at 0x%08x! " - "Expected=0x%02x, Read=0x%02x,", - message, starthere + j, - cmpbuf[starthere - start + j], - readbuf[j]); - } - } } if (failcount) { msg_cerr(" failed byte count from 0x%08x-0x%08x: 0x%x\n",
Thanks to Michael Karcher for reviewing the patch and pointing out a missing abort on read failure.
verify_range() and check_erased_range() check each page separately. While that may have seemed like a good idea back when the code was introduced, it has no benefits for any of the chips where we support write because all of them handle cross-page reads nicely. The only class of chips where this change could be a problem is chips with non power of two sector sizes which have gaps in the address space. We could simply require their read functions to provide gap-free results and leave it at that.
This patch should speed up verify on Dediprog by a factor of ~100.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Index: flashrom-verify_not_chunked/flashrom.c =================================================================== --- flashrom-verify_not_chunked/flashrom.c (Revision 1232) +++ flashrom-verify_not_chunked/flashrom.c (Arbeitskopie) @@ -726,9 +726,8 @@ */ int verify_range(struct flashchip *flash, uint8_t *cmpbuf, int start, int len, char *message) { - int i, j, starthere, lenhere, ret = 0; - int page_size = flash->page_size; - uint8_t *readbuf = malloc(page_size); + int i, ret = 0; + uint8_t *readbuf = malloc(len); int failcount = 0;
if (!len) @@ -753,37 +752,22 @@ if (!message) message = "VERIFY"; - /* Warning: This loop has a very unusual condition and body. - * The loop needs to go through each page with at least one affected - * byte. The lowest page number is (start / page_size) since that - * division rounds down. The highest page number we want is the page - * where the last byte of the range lives. That last byte has the - * address (start + len - 1), thus the highest page number is - * (start + len - 1) / page_size. Since we want to include that last - * page as well, the loop condition uses <=. - */ - for (i = start / page_size; i <= (start + len - 1) / page_size; i++) { - /* Byte position of the first byte in the range in this page. */ - starthere = max(start, i * page_size); - /* Length of bytes in the range in this page. */ - lenhere = min(start + len, (i + 1) * page_size) - starthere; - ret = flash->read(flash, readbuf, starthere, lenhere); - if (ret) { - msg_gerr("Verification impossible because read failed " - "at 0x%x (len 0x%x)\n", starthere, lenhere); - break; + ret = flash->read(flash, readbuf, start, len); + if (ret) { + msg_gerr("Verification impossible because read failed " + "at 0x%x (len 0x%x)\n", start, len); + return 1; + } + + 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]); } - for (j = 0; j < lenhere; j++) { - if (cmpbuf[starthere - start + j] != readbuf[j]) { - /* Only print the first failure. */ - if (!failcount++) - msg_cerr("%s FAILED at 0x%08x! " - "Expected=0x%02x, Read=0x%02x,", - message, starthere + j, - cmpbuf[starthere - start + j], - readbuf[j]); - } - } } if (failcount) { msg_cerr(" failed byte count from 0x%08x-0x%08x: 0x%x\n",
Am Dienstag, den 16.11.2010, 17:27 +0100 schrieb Carl-Daniel Hailfinger:
Thanks to Michael Karcher for reviewing the patch and pointing out a missing abort on read failure.
- ret = flash->read(flash, readbuf, start, len);
- if (ret) {
msg_gerr("Verification impossible because read failed ""at 0x%x (len 0x%x)\n", start, len);return 1;
make this "return ret" to not lose the error code.
Otherwise, the patch looks good to me.
Acked-By: Michael Karcher flashrom@mkarcher.dialup.fu-berlin.de
Regards, Michael Karcher
On 16.11.2010 17:53, Michael Karcher wrote:
Am Dienstag, den 16.11.2010, 17:27 +0100 schrieb Carl-Daniel Hailfinger:
Thanks to Michael Karcher for reviewing the patch and pointing out a missing abort on read failure.
- ret = flash->read(flash, readbuf, start, len);
- if (ret) {
msg_gerr("Verification impossible because read failed ""at 0x%x (len 0x%x)\n", start, len);return 1;make this "return ret" to not lose the error code.
Changed.
Otherwise, the patch looks good to me.
Acked-By: Michael Karcher flashrom@mkarcher.dialup.fu-berlin.de
Thanks for the review! Committed in r1233.
Regards, Carl-Daniel
Otherwise, the patch looks good to me.
Acked-By: Michael Karcher flashrom@mkarcher.dialup.fu-berlin.de
Thanks for the review! Committed in r1233.
With this change the verify speeds on the dediprog drop from 1 minute to 4.8 seconds with my W25x10 (128k) part.
-
Carl-Daniel Hailfinger -
Michael Karcher -
Richard Smith