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

[Stefan Tauner]

On Fri, 01 Jul 2011 00:19:30 +0200
Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net> wrote:

> Am 30.06.2011 21:34 schrieb Stefan Tauner:
> > On Thu, 30 Jun 2011 20:38:33 +0200
> > Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net> wrote:
> >
> >   
> >> Am 30.06.2011 20:07 schrieb Carl-Daniel Hailfinger:
> >>     
> >>> Am 30.06.2011 19:54 schrieb Carl-Daniel Hailfinger:  
> >>> 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.
> >>     
> > i dont understand what you are talking about. the erasers are allocated
> > no matter if they "exist" or not in a fix-sized array. a segfault
> > could only happen if we try to dereference a block eraser function and
> > jump there which we don't do here. what access(es) should segfault
> > exactly?
> >
> > i have tried my current, reworked patch with:
> > "./flashrom -p dummy:bus=spi,emulate="M25P10.RES" -V -c "M25P10.RES"
> > -E" after deleting the block erasers for that chip i.e. .block_erasers = {};
> > i get the expected output of "Erase is not working on this chip.
> > Aborting."
> >   
> 
> Did you also try specifying --force as well? That should get further
> along until it tries to actually write/erase. We have precautions there
> against empty erasers, so the segfault would not happen unless you axe
> the now redundant check for usable_erasefunctions!=0 in
> erase_and_write_flash(). Mh. There are even more safety checks in that
> code, so we might be fine. I need to recheck this.

jup:
Erase is not working on this chip. Continuing anyway.
Erasing and writing flash chip... ERROR: flashrom has no erase function for this flash chip.
Your flash chip is in an unknown state.

that is the "redundant" check you are talking about in action afaiks.
removing that results in:
Erase is not working on this chip. Continuing anyway.
Erasing and writing flash chip... Looking at blockwise erase function 0... not defined. trying... 

Done.
that is because the body of the inner loop in walk_eraseregions is
never executed because eraser.eraseblocks[i].count is always 0 hence
the NULL pointer is never dereferenced.

the check for usable_erasefunctions!=0 is only "redundant" if one can
live with the printing of the superfluous output of "Looking at
blockwise erase function 0... not defined." etc.

if i add an eraser with a block size but no erase function i get:
Erasing and writing flash chip... Looking at blockwise erase function 0... eraseblock layout is known, but matching block erase function is not implemented. trying... 0x000000-0x007fff:ESegmentation fault

so we may want to add a erasefn != NULL check to
erase_and_write_block_helper? or even in the startup self checker (to
look for erasers with blocks spreading > 0 bytes, but without a erase function)?

anyway this does not seem to be in the scope of this patch at all? i
was under the assumption all the time that you thought it was.

-- 
Kind regards/Mit freundlichen Grüßen, Stefan Tauner