[flashrom] [PATCH] Handle erase failure in partial write, clean up
Carl-Daniel Hailfinger
c-d.hailfinger.devel.2006 at gmx.net
Sun Dec 5 16:16:48 CET 2010
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 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>
>
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
--
http://www.hailfinger.org/
More information about the flashrom
mailing list