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

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

Regards,
Carl-Daniel

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

Regards,
Carl-Daniel

[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

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

[Stefan Tauner]

On Thu, 30 Jun 2011 20:38:33 +0200
Carl-Daniel Hailfinger  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
>
> 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."

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

[Stefan Tauner]

On Thu, 30 Jun 2011 21:34:31 +0200
Stefan Tauner  wrote:
> On Thu, 30 Jun 2011 20:38:33 +0200
> Carl-Daniel Hailfinger  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
> >
> > 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."

appended is my current version.

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

[Stefan Tauner]

On Thu, 30 Jun 2011 22:26:25 +0200
Stefan Tauner  wrote:
> appended is my current version.

now.

[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 22:37 schrieb Stefan Tauner:
> add count_usable_erasers which returns the number of well-defined erasers for a chip
>
> This can be used in various situations (including one in the upcoming SFDP patch) and
> removes one FIXME in current HEAD. Needed to add a declaration of check_block_eraser.
>
> Signed-off-by: Stefan Tauner
>
> diff --git a/flashrom.c b/flashrom.c
> index 13d398e..10035c4 100644
> --- a/flashrom.c
> +++ b/flashrom.c
> @@ -1818,13 +1830,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 ||
>     !count_usable_erasers(flash, 0)) {

Please split that if statement. Allowing --force makes sense for
TEST_BAD_ERASE, but it makes no sense if no erase functions are present.
You can reuse the error message, the compiler/linker will only store the
string once.
> 	msg_cerr("Erase is not working on this chip. ");
> 	if (!force)
> 		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) {

With the split if statement, this is
Acked-by: Carl-Daniel Hailfinger

Regards,
Carl-Daniel

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

[Carl-Daniel Hailfinger]

Sorry, another comment...

Am 01.07.2011 00:25 schrieb Carl-Daniel Hailfinger:
> Am 30.06.2011 22:37 schrieb Stefan Tauner:
> > add count_usable_erasers which returns the number of well-defined erasers for a chip
> >
> > This can be used in various situations (including one in the upcoming SFDP patch) and
> > removes one FIXME in current HEAD. Needed to add a declaration of check_block_eraser.
> >
> > Signed-off-by: Stefan Tauner
> >
> > diff --git a/flashrom.c b/flashrom.c
> > index 13d398e..10035c4 100644
> > --- a/flashrom.c
> > +++ b/flashrom.c
> > @@ -1818,13 +1830,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 ||
> >     !count_usable_erasers(flash, 0)) {
>
> Please split that if statement. Allowing --force makes sense for
> TEST_BAD_ERASE, but it makes no sense if no erase functions are present.
> You can reuse the error message, the compiler/linker will only store the
> string once.

Would it make sense to remove the check for usable_erasefunctions!=0 in
erase_and_write_flash() if we already do it here?
> > 	msg_cerr("Erase is not working on this chip. ");
> > 	if (!force)
> > 		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) {
>
> With the split if statement, this is
> Acked-by: Carl-Daniel Hailfinger

Regards,
Carl-Daniel

[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:27:35 +0200
Carl-Daniel Hailfinger  wrote:
> Sorry, another comment...
>
> Am 01.07.2011 00:25 schrieb Carl-Daniel Hailfinger:
> > Am 30.06.2011 22:37 schrieb Stefan Tauner:
> > > add count_usable_erasers which returns the number of well-defined erasers for a chip
> > >
> > > This can be used in various situations (including one in the upcoming SFDP patch) and
> > > removes one FIXME in current HEAD. Needed to add a declaration of check_block_eraser.
> > >
> > > Signed-off-by: Stefan Tauner
> > >
> > > diff --git a/flashrom.c b/flashrom.c
> > > index 13d398e..10035c4 100644
> > > --- a/flashrom.c
> > > +++ b/flashrom.c
> > > @@ -1818,13 +1830,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 ||
> > >     !count_usable_erasers(flash, 0)) {
> >
> > Please split that if statement. Allowing --force makes sense for
> > TEST_BAD_ERASE, but it makes no sense if no erase functions are present.
> > You can reuse the error message, the compiler/linker will only store the
> > string once.
>
> Would it make sense to remove the check for usable_erasefunctions!=0 in
> erase_and_write_flash() if we already do it here?

i think so, yes.
after i split up the if this should no longer be needed because it
should never get that far. testing agrees, so i have removed it and
committed the whole thing in r1358. thanks for the review!

[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 21:34 schrieb Stefan Tauner:
> On Thu, 30 Jun 2011 20:38:33 +0200
> Carl-Daniel Hailfinger  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
> >
> > 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.

Regards,
Carl-Daniel

[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  wrote:
> Am 30.06.2011 21:34 schrieb Stefan Tauner:
> > On Thu, 30 Jun 2011 20:38:33 +0200
> > Carl-Daniel Hailfinger  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
> > >
> > > 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.