Handle erase failure in partial write. Clean up erase function checking. Update a few comments and messages to improve readability.
The erase failure handling is a genuine bugfix which is needed on locked down chipsets and in the unlikely case that write fails halfway through or an incorrect chip definition is used.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Index: flashrom-cleanup_erase_and_write/flashrom.c =================================================================== --- flashrom-cleanup_erase_and_write/flashrom.c (Revision 1236) +++ flashrom-cleanup_erase_and_write/flashrom.c (Arbeitskopie) @@ -1429,57 +1429,84 @@ return 0; }
+static int check_block_eraser(struct flashchip *flash, int k, int log) +{ + struct block_eraser eraser = flash->block_erasers[k]; + + if (log) + msg_cdbg("Looking at blockwise erase function %i... ", k); + if (!eraser.block_erase && !eraser.eraseblocks[0].count) { + if (log) + msg_cdbg("not defined. "); + return 1; + } + if (!eraser.block_erase && eraser.eraseblocks[0].count) { + if (log) + msg_cdbg("eraseblock layout is known, but no " + "matching block erase function found. "); + return 1; + } + if (eraser.block_erase && !eraser.eraseblocks[0].count) { + if (log) + msg_cdbg("block erase function found, but " + "eraseblock layout is unknown. "); + return 1; + } + return 0; +} + int erase_and_write_flash(struct flashchip *flash, uint8_t *oldcontents, uint8_t *newcontents) { - int k, ret = 0, found = 0; + int k, ret = 0; uint8_t *curcontents; unsigned long size = flash->total_size * 1024; + int usable_erasefunctions = 0;
+ for (k = 0; k < NUM_ERASEFUNCTIONS; k++) + if (!check_block_eraser(flash, k, 0)) + usable_erasefunctions++; + msg_cinfo("Erasing and writing flash chip... "); + if (!usable_erasefunctions) { + msg_cerr("ERROR: flashrom has no erase function for this flash " + "chip.\n"); + return 1; + } + curcontents = (uint8_t *) malloc(size); /* Copy oldcontents to curcontents to avoid clobbering oldcontents. */ memcpy(curcontents, oldcontents, size);
- msg_cinfo("Erasing and writing flash chip... "); for (k = 0; k < NUM_ERASEFUNCTIONS; k++) { - struct block_eraser eraser = flash->block_erasers[k]; - - msg_cdbg("Looking at blockwise erase function %i... ", k); - if (!eraser.block_erase && !eraser.eraseblocks[0].count) { - msg_cdbg("not defined. " - "Looking for another erase function.\n"); + if (check_block_eraser(flash, k, 1) && usable_erasefunctions) { + msg_cdbg("Looking for another erase function.\n"); continue; } - if (!eraser.block_erase && eraser.eraseblocks[0].count) { - msg_cdbg("eraseblock layout is known, but no " - "matching block erase function found. " - "Looking for another erase function.\n"); - continue; - } - if (eraser.block_erase && !eraser.eraseblocks[0].count) { - msg_cdbg("block erase function found, but " - "eraseblock layout is unknown. " - "Looking for another erase function.\n"); - continue; - } - found = 1; + usable_erasefunctions--; msg_cdbg("trying... "); ret = walk_eraseregions(flash, k, &erase_and_write_block_helper, curcontents, newcontents); msg_cdbg("\n"); /* If everything is OK, don't try another erase function. */ if (!ret) break; - /* FIXME: Reread the whole chip here so we know the current - * chip contents? curcontents might be up to date, but this - * code is only reached if something failed, and then we don't - * know exactly what failed, and how. + /* Write/erase failed, so try to find out what the current chip + * contents are. If no usable erase functions remain, we could + * abort the loop instead of continuing, the effect is the same. + * The only difference is whether the reason for other unusable + * functions is printed or not. If in doubt, verbosity wins. */ + if (!usable_erasefunctions) + continue; + if (flash->read(flash, curcontents, 0, size)) { + /* Now we are truly screwed. Read failed as well. */ + msg_cerr("Can't read anymore!\n"); + /* We have no idea about the flash chip contents, so + * retrying with another erase function is pointless. + */ + break; + } } /* Free the scratchpad. */ free(curcontents); - if (!found) { - msg_cerr("ERROR: flashrom has no erase function for this flash chip.\n"); - return 1; - }
if (ret) { msg_cerr("FAILED!\n"); @@ -1827,18 +1854,15 @@ * preserved, but in that case we might perform unneeded erase which * takes time as well. */ - msg_cdbg("Reading old flash chip contents...\n"); + msg_cdbg("Reading current flash chip contents...\n"); if (flash->read(flash, oldcontents, 0, size)) { ret = 1; goto out; }
- // This should be moved into each flash part's code to do it - // cleanly. This does the job. + /* Build a new image from the given layout. */ handle_romentries(flash, oldcontents, newcontents);
- // //////////////////////////////////////////////////////////// - if (write_it) { if (erase_and_write_flash(flash, oldcontents, newcontents)) { msg_cerr("Uh oh. Erase/write failed. Checking if "
Am Dienstag, den 30.11.2010, 01:42 +0100 schrieb Carl-Daniel Hailfinger:
Handle erase failure in partial write. Clean up erase function checking. Update a few comments and messages to improve readability.
Reviewing would be easier of course if the first one would be a separate patch from the second and third one, but as it seems they don't overlap, it's OK, too.
+static int check_block_eraser(struct flashchip *flash, int k, int log) +{
- struct block_eraser eraser = flash->block_erasers[k];
- if (log)
msg_cdbg("Looking at blockwise erase function %i... ", k);
Why don't you leave this message output in erase_and_write_flash and kill the "log" parameter?
- if (!eraser.block_erase && !eraser.eraseblocks[0].count) {
if (log)msg_cdbg("not defined. ");return 1;- }
- if (!eraser.block_erase && eraser.eraseblocks[0].count) {
if (log)msg_cdbg("eraseblock layout is known, but no ""matching block erase function found. ");
"not found" sounds like some active searching was done. "missing" or "not implemented" or "not defined" seems to get better to the point.
return 1;- }
- if (eraser.block_erase && !eraser.eraseblocks[0].count) {
if (log)msg_cdbg("block erase function found, but "
dito here, "exists"/"implemented"/"defined"
"eraseblock layout is unknown. ");return 1;- }
- return 0;
+}
But a general question is: Who needs this level of detail? The user doesn't really care about what erase function is used and why, as long as flashing works. And the developer can easily look up in the flashrom source, which of these three possibilities applies, if the index is given. The old code already prints the number. [...]
/* Write/erase failed, so try to find out what the current chip* contents are. If no usable erase functions remain, we could* abort the loop instead of continuing, the effect is the same.* The only difference is whether the reason for other unusable* functions is printed or not. If in doubt, verbosity wins.if (!usable_erasefunctions)continue;if (flash->read(flash, curcontents, 0, size)) {/* Now we are truly screwed. Read failed as well. */msg_cerr("Can't read anymore!\n");/* We have no idea about the flash chip contents, so* retrying with another erase function is pointless.*/break;}
This change looks fine.
As my only nitpicks were about coding style and log verbosity, and I don't see any technical problems:
Acked-by: Michael Karcher flashrom@mkarcher.dialup.fu-berlin.de
Regards, Michael Karcher
On 01.12.2010 11:12, Michael Karcher wrote:
Am Dienstag, den 30.11.2010, 01:42 +0100 schrieb Carl-Daniel Hailfinger:
Handle erase failure in partial write. Clean up erase function checking. Update a few comments and messages to improve readability.
Reviewing would be easier of course if the first one would be a separate patch from the second and third one, but as it seems they don't overlap, it's OK, too.
Noted.
Acked-by: Michael Karcher flashrom@mkarcher.dialup.fu-berlin.de
Thanks for your review! I split the patch as suggested.
Handle erase failure in partial write.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net Acked-by: Michael Karcher flashrom@mkarcher.dialup.fu-berlin.de
Index: flashrom-handle_failed_erase_or_write/flashrom.c =================================================================== --- flashrom-handle_failed_erase_or_write/flashrom.c (Revision 1236) +++ flashrom-handle_failed_erase_or_write/flashrom.c (Arbeitskopie) @@ -1468,11 +1468,20 @@ /* If everything is OK, don't try another erase function. */ if (!ret) break; - /* FIXME: Reread the whole chip here so we know the current - * chip contents? curcontents might be up to date, but this - * code is only reached if something failed, and then we don't - * know exactly what failed, and how. + /* Write/erase failed, so try to find out what the current chip + * contents are. If no usable erase functions remain, we could + * abort the loop instead of continuing, the effect is the same. + * The only difference is whether the reason for other unusable + * functions is printed or not. If in doubt, verbosity wins. */ + if (flash->read(flash, curcontents, 0, size)) { + /* Now we are truly screwed. Read failed as well. */ + msg_cerr("Can't read anymore!\n"); + /* We have no idea about the flash chip contents, so + * retrying with another erase function is pointless. + */ + break; + } } /* Free the scratchpad. */ free(curcontents);
On 02.12.2010 03:40, Carl-Daniel Hailfinger wrote:
Handle erase failure in partial write.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net Acked-by: Michael Karcher flashrom@mkarcher.dialup.fu-berlin.de
and committed in r1238. Thanks for the review!
Regards, Carl-Daniel
On 01.12.2010 11:12, Michael Karcher wrote:
Am Dienstag, den 30.11.2010, 01:42 +0100 schrieb Carl-Daniel Hailfinger:
Handle erase failure in partial write. Clean up erase function checking. Update a few comments and messages to improve readability.
Reviewing would be easier of course if the first one would be a separate patch from the second and third one, but as it seems they don't overlap, it's OK, too.
Sorry for that.
+static int check_block_eraser(struct flashchip *flash, int k, int log) +{
- struct block_eraser eraser = flash->block_erasers[k];
- if (log)
msg_cdbg("Looking at blockwise erase function %i... ", k);Why don't you leave this message output in erase_and_write_flash and kill the "log" parameter?
If I kill the "log" parameter, I can't selectively print the messages below.
- if (!eraser.block_erase && !eraser.eraseblocks[0].count) {
if (log)msg_cdbg("not defined. ");return 1;- }
- if (!eraser.block_erase && eraser.eraseblocks[0].count) {
if (log)msg_cdbg("eraseblock layout is known, but no ""matching block erase function found. ");"not found" sounds like some active searching was done. "missing" or "not implemented" or "not defined" seems to get better to the point.
"not implemented".
return 1;- }
- if (eraser.block_erase && !eraser.eraseblocks[0].count) {
if (log)msg_cdbg("block erase function found, but "dito here, "exists"/"implemented"/"defined"
"not defined".
"eraseblock layout is unknown. ");return 1;- }
- return 0;
+}
But a general question is: Who needs this level of detail? The user doesn't really care about what erase function is used and why, as long as flashing works. And the developer can easily look up in the flashrom source, which of these three possibilities applies, if the index is given. The old code already prints the number.
Hm... sometimes eraseblock definitions and erase functions change between revisions. For example, a few chips support certain erase methods only for AAI or only for LPC. I plan to annotate erase functions with the supported buses sometime in the future. With this level of detail, we always know about the reason why something was skipped, which may include the bus in the future.
As my only nitpicks were about coding style and log verbosity, and I don't see any technical problems:
Acked-by: Michael Karcher flashrom@mkarcher.dialup.fu-berlin.de
Thanks for the detailed review. New patch follows.
Clean up erase function checking.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Index: flashrom-cleanup_erase_and_write/flashrom.c =================================================================== --- flashrom-cleanup_erase_and_write/flashrom.c (Revision 1238) +++ flashrom-cleanup_erase_and_write/flashrom.c (Arbeitskopie) @@ -1429,39 +1429,58 @@ return 0; }
+static int check_block_eraser(struct flashchip *flash, int k, int log) +{ + struct block_eraser eraser = flash->block_erasers[k]; + + if (!eraser.block_erase && !eraser.eraseblocks[0].count) { + if (log) + msg_cdbg("not defined. "); + return 1; + } + if (!eraser.block_erase && eraser.eraseblocks[0].count) { + if (log) + msg_cdbg("eraseblock layout is known, but matching " + "block erase function is not implemented. "); + return 1; + } + if (eraser.block_erase && !eraser.eraseblocks[0].count) { + if (log) + msg_cdbg("block erase function found, but " + "eraseblock layout is not defined. "); + return 1; + } + return 0; +} + int erase_and_write_flash(struct flashchip *flash, uint8_t *oldcontents, uint8_t *newcontents) { - int k, ret = 0, found = 0; + int k, ret = 0; uint8_t *curcontents; unsigned long size = flash->total_size * 1024; + int usable_erasefunctions = 0;
+ for (k = 0; k < NUM_ERASEFUNCTIONS; k++) + if (!check_block_eraser(flash, k, 0)) + usable_erasefunctions++; + msg_cinfo("Erasing and writing flash chip... "); + if (!usable_erasefunctions) { + msg_cerr("ERROR: flashrom has no erase function for this flash " + "chip.\n"); + return 1; + } + curcontents = (uint8_t *) malloc(size); /* Copy oldcontents to curcontents to avoid clobbering oldcontents. */ memcpy(curcontents, oldcontents, size);
- msg_cinfo("Erasing and writing flash chip... "); for (k = 0; k < NUM_ERASEFUNCTIONS; k++) { - struct block_eraser eraser = flash->block_erasers[k]; - msg_cdbg("Looking at blockwise erase function %i... ", k); - if (!eraser.block_erase && !eraser.eraseblocks[0].count) { - msg_cdbg("not defined. " - "Looking for another erase function.\n"); + if (check_block_eraser(flash, k, 1) && usable_erasefunctions) { + msg_cdbg("Looking for another erase function.\n"); continue; } - if (!eraser.block_erase && eraser.eraseblocks[0].count) { - msg_cdbg("eraseblock layout is known, but no " - "matching block erase function found. " - "Looking for another erase function.\n"); - continue; - } - if (eraser.block_erase && !eraser.eraseblocks[0].count) { - msg_cdbg("block erase function found, but " - "eraseblock layout is unknown. " - "Looking for another erase function.\n"); - continue; - } - found = 1; + usable_erasefunctions--; msg_cdbg("trying... "); ret = walk_eraseregions(flash, k, &erase_and_write_block_helper, curcontents, newcontents); msg_cdbg("\n"); @@ -1474,6 +1493,8 @@ * The only difference is whether the reason for other unusable * functions is printed or not. If in doubt, verbosity wins. */ + if (!usable_erasefunctions) + continue; if (flash->read(flash, curcontents, 0, size)) { /* Now we are truly screwed. Read failed as well. */ msg_cerr("Can't read anymore!\n"); @@ -1485,10 +1506,6 @@ } /* Free the scratchpad. */ free(curcontents); - if (!found) { - msg_cerr("ERROR: flashrom has no erase function for this flash chip.\n"); - return 1; - }
if (ret) { msg_cerr("FAILED!\n");
On 02.12.2010 13:36, Carl-Daniel Hailfinger wrote:
On 01.12.2010 11:12, Michael Karcher wrote:
Am Dienstag, den 30.11.2010, 01:42 +0100 schrieb Carl-Daniel Hailfinger:
Handle erase failure in partial write. Clean up erase function checking. Update a few comments and messages to improve readability.
Reviewing would be easier of course if the first one would be a separate patch from the second and third one, but as it seems they don't overlap, it's OK, too.
Sorry for that.
+static int check_block_eraser(struct flashchip *flash, int k, int log) +{
- struct block_eraser eraser = flash->block_erasers[k];
- if (log)
msg_cdbg("Looking at blockwise erase function %i... ", k);Why don't you leave this message output in erase_and_write_flash and kill the "log" parameter?
If I kill the "log" parameter, I can't selectively print the messages below.
- if (!eraser.block_erase && !eraser.eraseblocks[0].count) {
if (log)msg_cdbg("not defined. ");return 1;- }
- if (!eraser.block_erase && eraser.eraseblocks[0].count) {
if (log)msg_cdbg("eraseblock layout is known, but no ""matching block erase function found. ");"not found" sounds like some active searching was done. "missing" or "not implemented" or "not defined" seems to get better to the point.
"not implemented".
return 1;- }
- if (eraser.block_erase && !eraser.eraseblocks[0].count) {
if (log)msg_cdbg("block erase function found, but "dito here, "exists"/"implemented"/"defined"
"not defined".
"eraseblock layout is unknown. ");return 1;- }
- return 0;
+}
But a general question is: Who needs this level of detail? The user doesn't really care about what erase function is used and why, as long as flashing works. And the developer can easily look up in the flashrom source, which of these three possibilities applies, if the index is given. The old code already prints the number.
Hm... sometimes eraseblock definitions and erase functions change between revisions. For example, a few chips support certain erase methods only for AAI or only for LPC. I plan to annotate erase functions with the supported buses sometime in the future. With this level of detail, we always know about the reason why something was skipped, which may include the bus in the future.
As my only nitpicks were about coding style and log verbosity, and I don't see any technical problems:
Acked-by: Michael Karcher flashrom@mkarcher.dialup.fu-berlin.de
Thanks for the detailed review. New patch follows.
Clean up erase function checking.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
I used your ack although I couldn't make all the changes you requested. If you insist on removing the log parameter and the verbose output, I can send a separate patch for that.
Committed in r1244.
Regards, Carl-Daniel
Am Sonntag, den 05.12.2010, 16:16 +0100 schrieb Carl-Daniel Hailfinger:
Acked-by: Michael Karcher flashrom@mkarcher.dialup.fu-berlin.de
Thanks for the detailed review. New patch follows.
Clean up erase function checking.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
I used your ack although I couldn't make all the changes you requested. If you insist on removing the log parameter and the verbose output, I can send a separate patch for that.
No, its fine the way you did it.
Regards, Michael Karcher
-
Carl-Daniel Hailfinger -
Michael Karcher