On Thu, 21 Jul 2011 03:08:36 +0200 Carl-Daniel Hailfinger <[email protected]> wrote:
> Am 21.07.2011 02:41 schrieb Carl-Daniel Hailfinger: > > Here's the fix with no message changes. I think that part is where we > > both agree. Please note that a separate followup patch with improved > > messages (either from you or from me) is also very desirable for 0.9.4, > > and by now I pretty much agree with your reasoning. > > > > And here are the pure message changes on top. > AFAICS they should be mostly what you created yourself and a few small > tweaks by me, so it should probably carry your signoff. In the meantime, > this has my signoff to make sure nobody thinks the patch is restricted. > > Signed-off-by: Carl-Daniel Hailfinger <[email protected]> > > --- flashrom-cosmetics_blockwalker_read_write_error/flashrom.c > 2011-07-21 02:50:59.000000000 +0200 > +++ flashrom-cosmetics_blockwalker_read_write_error/flashrom.c > 2011-07-21 03:00:20.000000000 +0200 > @@ -1526,15 +1526,14 @@ > msg_cdbg("No usable erase functions left.\n"); > break; > } > - msg_cdbg("Looking at blockwise erase function %i... ", k); > + msg_cdbg("Trying erase function %i... ", k); > if (check_block_eraser(flash, k, 1)) { > msg_cdbg("Looking for another erase function.\n"); > continue; > } > usable_erasefunctions--; > - msg_cdbg("trying... "); > - ret = walk_eraseregions(flash, k, > &erase_and_write_block_helper, curcontents, newcontents); > - msg_cdbg("\n"); > + ret = walk_eraseregions(flash, k, &erase_and_write_block_helper, > + curcontents, newcontents); > /* If everything is OK, don't try another erase function. */ > if (!ret) > break; > @@ -1544,14 +1543,19 @@ > */ > if (!usable_erasefunctions) > continue; this case now is an exception: it does not print a "Looking for another erase function" message. a possible fix is to add this at the top of the loop: if (k != 0) msg_cdbg("Looking for another erase function.\n"); > + /* Reading the whole chip may take a while, inform the user even > + * in non-verbose mode. > + */ > + msg_cinfo("Reading current flash chip contents... "); > if (flash->read(flash, curcontents, 0, size)) { > /* Now we are truly screwed. Read failed as well. */ > - msg_cerr("Can't read anymore!\n"); > + msg_cerr("Can't read anymore! Aborting.\n"); > /* We have no idea about the flash chip contents, so > * retrying with another erase function is pointless. > */ > break; > } > + msg_cinfo("done. Trying next erase function.\n"); i am quoting you: > msg_cdbg("Looking for another erase function.\n"); > That would make the messages more consistent. and you are mixing dbg with info in your version. the done should be info, the trying thingy dbg. i almost missed that too... this loop is a hard nut :) > } > /* Free the scratchpad. */ > free(curcontents); > @@ -1938,13 +1942,13 @@ > * preserved, but in that case we might perform unneeded erase which > * takes time as well. > */ > - msg_cdbg("Reading old flash chip contents... "); > + msg_cinfo("Reading old flash chip contents... "); > if (flash->read(flash, oldcontents, 0, size)) { > ret = 1; > - msg_cdbg("FAILED.\n"); > + msg_cinfo("FAILED.\n"); > goto out; > } > - msg_cdbg("done.\n"); > + msg_cinfo("done.\n"); > > // This should be moved into each flash part's code to do it > // cleanly. This does the job. > i have fixed those issues in the attached patch and below are the dummy-simulated outputs of writes and erases. verbose output with one error: Reading old flash chip contents... done. Erasing and writing flash chip... Trying erase function 0... 0x000000-0x007fff:EBLOCK ERASE 0xd8 fails because i said so! Invalid command sent to flash chip! spi_block_erase_d8 failed during command execution at address 0x0 Reading current flash chip contents... done. Looking for another erase function. Trying erase function 1... 0x000000-0x01ffff:EW Done. Verifying flash... VERIFIED. non-verbose output with one error: Reading old flash chip contents... done. Erasing and writing flash chip... Invalid command sent to flash chip! spi_block_erase_d8 failed during command execution at address 0x0 Reading current flash chip contents... done. Done. Verifying flash... VERIFIED. verbose output without errors: Reading old flash chip contents... done. Erasing and writing flash chip... Trying erase function 0... 0x000000-0x007fff:EW, 0x008000-0x00ffff:EW, 0x010000-0x017fff:EW, 0x018000-0x01ffff:EW Done. Verifying flash... VERIFIED. new non-verbose output without errors: Reading old flash chip contents... done. Erasing and writing flash chip... Done. Verifying flash... VERIFIED. non-verbose erase with one error: Erasing and writing flash chip... Invalid command sent to flash chip! spi_block_erase_d8 failed during command execution at address 0x0 Reading current flash chip contents... done. Done. ^ that one is a bit odd, because we dont tell the use why we do it and it is not obvious. OTOH one could argue that this is caused by sharing the erase_and_write_flash function as is. any ideas how this could be fixed? another thing that is obvious in the case above but applies to all: the D in Done should be d (or vice-versa). non-verbose erase with no errors: Erasing and writing flash chip... Done. verbose erase with one error: Erasing and writing flash chip... Trying erase function 0... 0x000000-0x007fff:EBLOCK ERASE 0xd8 fails because i said so! Invalid command sent to flash chip! spi_block_erase_d8 failed during command execution at address 0x0 Reading current flash chip contents... done. Looking for another erase function. Trying erase function 1... 0x000000-0x01ffff:E Done. verbose erase with no errors: Erasing and writing flash chip... Trying erase function 0... 0x000000-0x007fff:E, 0x008000-0x00ffff:E, 0x010000-0x017fff:E, 0x018000-0x01ffff:E Done. we should also get rid of that extra \n in the case of errors. i think it is the one in walk_eraseregions in the if that checks do_something. this breaks consistency in that function, but because some/most/all(?) error messages in erasers have a line break at the end this is the right thing to do. i have commented it out in the attached patch (the outputs above are with it still included), so you can spot it easily. in general i think this is a step forward and i am glad that all those "invested hours" show some return ;) regarding the sign-off... i dont really care, but the best solution is probably, that we both sign it... we actually have done that already for "our" parts respectively. i dont see a problem with that, but i am fine with any other solution too. todo: - \n - Done -- Kind regards/Mit freundlichen Grüßen, Stefan Tauner
>From bc459a0305f3796431ec7a6142674d092b2be68f Mon Sep 17 00:00:00 2001 From: Stefan Tauner <[email protected]> Date: Thu, 21 Jul 2011 18:33:20 +0200 Subject: [PATCH] fix output of erase_and_write_flash and surroundings see http://www.flashrom.org/pipermail/flashrom/2011-July/007220.html for a discussion about the details. Signed-off-by: Carl-Daniel Hailfinger <[email protected]> Signed-off-by: Stefan Tauner <[email protected]> --- flashrom.c | 28 ++++++++++++++++------------ 1 files changed, 16 insertions(+), 12 deletions(-) diff --git a/flashrom.c b/flashrom.c index 5ddcd41..015bac0 100644 --- a/flashrom.c +++ b/flashrom.c @@ -1471,7 +1471,7 @@ static int walk_eraseregions(struct flashchip *flash, int erasefunction, start + len - 1); if (do_something(flash, start, len, param1, param2, eraser.block_erase)) { - msg_cdbg("\n"); + //msg_cdbg("\n"); return 1; } start += len; @@ -1522,19 +1522,18 @@ int erase_and_write_flash(struct flashchip *flash, uint8_t *oldcontents, uint8_t memcpy(curcontents, oldcontents, size); for (k = 0; k < NUM_ERASEFUNCTIONS; k++) { + if (k != 0) + msg_cdbg("Looking for another erase function.\n"); if (!usable_erasefunctions) { msg_cdbg("No usable erase functions left.\n"); break; } - msg_cdbg("Looking at blockwise erase function %i... ", k); - if (check_block_eraser(flash, k, 1)) { - msg_cdbg("Looking for another erase function.\n"); + msg_cdbg("Trying erase function %i... ", k); + if (check_block_eraser(flash, k, 1)) continue; - } usable_erasefunctions--; - msg_cdbg("trying... "); - ret = walk_eraseregions(flash, k, &erase_and_write_block_helper, curcontents, newcontents); - msg_cdbg("\n"); + ret = walk_eraseregions(flash, k, &erase_and_write_block_helper, + curcontents, newcontents); /* If everything is OK, don't try another erase function. */ if (!ret) break; @@ -1544,14 +1543,19 @@ int erase_and_write_flash(struct flashchip *flash, uint8_t *oldcontents, uint8_t */ if (!usable_erasefunctions) continue; + /* Reading the whole chip may take a while, inform the user even + * in non-verbose mode. + */ + msg_cinfo("Reading current flash chip contents... "); if (flash->read(flash, curcontents, 0, size)) { /* Now we are truly screwed. Read failed as well. */ - msg_cerr("Can't read anymore!\n"); + msg_cerr("Can't read anymore! Aborting.\n"); /* We have no idea about the flash chip contents, so * retrying with another erase function is pointless. */ break; } + msg_cinfo("done. "); } /* Free the scratchpad. */ free(curcontents); @@ -1938,13 +1942,13 @@ int doit(struct flashchip *flash, int force, const char *filename, int read_it, * preserved, but in that case we might perform unneeded erase which * takes time as well. */ - msg_cdbg("Reading old flash chip contents... "); + msg_cinfo("Reading old flash chip contents... "); if (flash->read(flash, oldcontents, 0, size)) { ret = 1; - msg_cdbg("FAILED.\n"); + msg_cinfo("FAILED.\n"); goto out; } - msg_cdbg("done.\n"); + msg_cinfo("done.\n"); // This should be moved into each flash part's code to do it // cleanly. This does the job. -- 1.7.1
_______________________________________________ flashrom mailing list [email protected] http://www.flashrom.org/mailman/listinfo/flashrom
