Fix eraseblock walking and add a few more checks to make sure such bugs get caught in the future. I found this bug during a code review.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Index: flashrom-blockerase_walk_fix/flashrom.c =================================================================== --- flashrom-blockerase_walk_fix/flashrom.c (Revision 769) +++ flashrom-blockerase_walk_fix/flashrom.c (Arbeitskopie) @@ -563,10 +563,11 @@ int erase_flash(struct flashchip *flash) { int i, j, k, ret = 0, found = 0; + unsigned int start, len;
printf("Erasing flash chip... "); for (k = 0; k < NUM_ERASEFUNCTIONS; k++) { - unsigned long done = 0; + unsigned int done = 0; struct block_eraser eraser = flash->block_erasers[k];
printf_debug("Looking at blockwise erase function %i... ", k); @@ -594,13 +595,23 @@ * members so the loop below won't be executed for them. */ for (j = 0; j < eraser.eraseblocks[i].count; j++) { - ret = eraser.block_erase(flash, done + eraser.eraseblocks[i].size * j, eraser.eraseblocks[i].size); + start = done + eraser.eraseblocks[i].size * j; + len = eraser.eraseblocks[i].size; + printf_debug("0x%06x-0x%06x, ", start, len); + ret = eraser.block_erase(flash, start, len); if (ret) break; } if (ret) break; + done += eraser.eraseblocks[i].count * + eraser.eraseblocks[i].size; } + printf_debug("\n"); + if (done != flash->total_size * 1024) + fprintf(stderr, "ERROR: Erase region walking erased " + "0x%06x bytes total, expected 0x%06x bytes.", + done, flash->total_size * 1024); /* If everything is OK, don't try another erase function. */ if (!ret) break;
On 23.11.2009 03:26, Carl-Daniel Hailfinger wrote:
Fix eraseblock walking and add a few more checks to make sure such bugs get caught in the future. I found this bug during a code review.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Ping?
Regards, Carl-Daniel
Looks fine to me! the printf_debug doesn't seem correct, should it be: printf_debug("0x%06x-0x%06x, ", start, start+len) ? Acked-by: Sean Nelson audiohacked@gmail.com
On 04.12.2009 17:37, Sean Nelson wrote:
Looks fine to me! the printf_debug doesn't seem correct, should it be: printf_debug("0x%06x-0x%06x, ", start, start+len) ? Acked-by: Sean Nelson audiohacked@gmail.com
Ouch, yes. Thanks for the review. Actually, it should be start, start+len-1. And the code didn't check for broken eraseblock definitions (more than 0 eraseblocks with size 0). We all hope that our eraseblock definitions are OK, but... Anyway, we need to move some of the checks into a function that runs on startup and verifies data structure correctness. That's post 0.9.2 material, though. New version follows. I'll commit in a few days so interested parties can find new bugs in my code. ;-)
Fix eraseblock walking and add a few more checks to make sure such bugs get caught in the future. I found this bug during a code review.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net Acked-by: Sean Nelson audiohacked@gmail.com
Index: flashrom-blockerase_walk_fix/flashrom.c =================================================================== --- flashrom-blockerase_walk_fix/flashrom.c (Revision 792) +++ flashrom-blockerase_walk_fix/flashrom.c (Arbeitskopie) @@ -773,10 +773,11 @@ int erase_flash(struct flashchip *flash) { int i, j, k, ret = 0, found = 0; + unsigned int start, len;
printf("Erasing flash chip... "); for (k = 0; k < NUM_ERASEFUNCTIONS; k++) { - unsigned long done = 0; + unsigned int done = 0; struct block_eraser eraser = flash->block_erasers[k];
printf_debug("Looking at blockwise erase function %i... ", k); @@ -800,17 +801,39 @@ found = 1; printf_debug("trying... "); for (i = 0; i < NUM_ERASEREGIONS; i++) { + /* Blocks with zero size are bugs in flashchips.c. + * FIXME: This check should be performed on startup. + */ + if (eraser.eraseblocks[i].count && + !eraser.eraseblocks[i].size) { + fprintf(stderr, "ERROR: Erase region with size " + "0 for this chip. Please report a bug " + "at flashrom@flashrom.org\n"); + ret = 1; + break; + } /* count==0 for all automatically initialized array * members so the loop below won't be executed for them. */ for (j = 0; j < eraser.eraseblocks[i].count; j++) { - ret = eraser.block_erase(flash, done + eraser.eraseblocks[i].size * j, eraser.eraseblocks[i].size); + start = done + eraser.eraseblocks[i].size * j; + len = eraser.eraseblocks[i].size; + printf_debug("0x%06x-0x%06x, ", start, + start + len - 1); + ret = eraser.block_erase(flash, start, len); if (ret) break; } if (ret) break; + done += eraser.eraseblocks[i].count * + eraser.eraseblocks[i].size; } + printf_debug("\n"); + if (done != flash->total_size * 1024) + fprintf(stderr, "ERROR: Erase region walking erased " + "0x%06x bytes total, expected 0x%06x bytes.", + done, flash->total_size * 1024); /* If everything is OK, don't try another erase function. */ if (!ret) break;
Looks good to me! Can't think/see any problems.
On 05.12.2009 06:03, Sean Nelson wrote:
Looks good to me! Can't think/see any problems.
Thanks, r800.
Regards, Carl-Daniel
-
Carl-Daniel Hailfinger -
Sean Nelson