Re: [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
> > >
> > > 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

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