[flashrom] [PATCH] Handle erase failure in partial write, clean up
Carl-Daniel Hailfinger
c-d.hailfinger.devel.2006 at gmx.net
Thu Dec 2 13:36:18 CET 2010
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 at 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 at 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");
--
http://www.hailfinger.org/
More information about the flashrom
mailing list