[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 <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/