[flashrom] [PATCH 2/3] add check_block_erasers which returns the number of well-defined erasers for a chip

[Carl-Daniel Hailfinger]

Am 30.06.2011 20:07 schrieb Carl-Daniel Hailfinger:
> Am 30.06.2011 19:54 schrieb Carl-Daniel Hailfinger:
>   
>> Am 24.06.2011 16:53 schrieb Stefan Tauner:
>>   
>>     
>>> This can be used in various situations (including one in the upcoming SFDP patch) and
>>> removes one FIXME in current HEAD. Needed to move check_block_eraser (which checks a
>>> single eraser) up to avoid (upcoming) forward declaration(s).
>>>   
>>>     
>>>       
>> Since nobody objected to the "forward declarations" RFC, I think we can
>> safely say that moving code around inside a file is a bad idea. Please
>> kill that part.
>>
>>
>>   
>>     
>>> Signed-off-by: Stefan Tauner <stefan.tauner at student.tuwien.ac.at>
>>> ---
>>>  flashrom.c |   70 ++++++++++++++++++++++++++++++++++-------------------------
>>>  1 files changed, 40 insertions(+), 30 deletions(-)
>>>
>>> diff --git a/flashrom.c b/flashrom.c
>>> index 6979d84..aed10aa 100644
>>> --- a/flashrom.c
>>> +++ b/flashrom.c
>>> @@ @@
>>> +/* Returns the number of well-defined erasers for a chip.
>>> + * The log parameter controls output. */
>>> +static int check_block_erasers(const struct flashchip *flash, int log)
>>>   
>>>     
>>>       
>> Hm. Can you call it count_usable_erasers or count_usable_block_erasers
>> instead?
>>
>>
>>   
>>     
>>> +{
>>> +	int usable_erasefunctions = 0;
>>> +	int k;
>>> +	for (k = 0; k < NUM_ERASEFUNCTIONS; k++) {
>>> +		if (!check_block_eraser(flash, k, 0))
>>> +			usable_erasefunctions++;
>>> +	}
>>> +	return usable_erasefunctions;
>>> +}
>>> +
>>>   
>>>     
>>>       
>> Rest looks good to me.
>>   
>>     
> With the changes outlined above:
> Acked-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net>
>   

Ahem. I should have quoted the part where a new bug appears, and should
have requested a fix for that.

>>> @@ -1814,13 +1824,13 @@ int chip_safety_check(struct flashchip
>>> *flash, int force, int read_it, int write } if (erase_it ||
>>> write_it) { /* Write needs erase. */ - if (flash->tested &
>>> TEST_BAD_ERASE) { + if (flash->tested & TEST_BAD_ERASE && 

As discussed on IRC, this should be ||.


>>> + !check_block_erasers(flash, 0)) { msg_cerr("Erase is not working
>>> on this chip. "); if (!force) 

And once you switch to || above, you may get a segfault if no block
erasers exist. Zero usable erase functions is a non-recoverable
condition, it can not be overridden with --force.

We do perform that check for more than zero usable erase functions
elsewhere already. Either move that check here (with a separate if
statement, and add a comment in the original place that checking has
moved), or leave it where it is and keep the code here as is.


>>> return 1; msg_cerr("Continuing anyway.\n"); } - /* FIXME: Check if
>>> at least one erase function exists. */ } if (write_it) { if
>>> (flash->tested & TEST_BAD_WRITE) { 

Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/