Re: [flashrom] [PATCH] Handle erase failure in partial write, clean up

[Michael Karcher]

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

Regards,
  Michael Karcher