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

[Carl-Daniel Hailfinger]

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
>
> Thanks for the detailed review. New patch follows.
>
> Clean up erase function checking.
>
> Signed-off-by: Carl-Daniel Hailfinger

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