Am 23.07.2012 15:34 schrieb Stefan Tauner:
> blacklisting and ignorelisting opcodes is just not enough.
> this patch does not introduce the most outstand failure injection ever, but
> it certainly helped me testing the other patch. maybe we want to make
> similar failures user configurable?
>
> Signed-off-by: Stefan Tauner <stefan.tauner(a)student.tuwien.ac.at>
I really like simulating chip failure. What about doing this with a
programmer parameter, maybe
erasestuckbyte=val@addr+count:val@addr+count etc. (essentially an AND
mask for the desired erase result)
writestuckbyte=val@addr+count:val@addr+count etc. (essentially an OR
mask for the desired write result)
val would be the hex representation of a single byte, addr would be the
in-chip address where the stuck byte is placed, and count would be how
often you have to write/erase to get the right value (with 0 as special
count meaning the byte is stuck forever).
For erasestuckbyte=0xfe@0xbabe+2 we'd get the following result:
1. Address 0xbabe has value 0xfe&oldval_at_0xbabe
2. Run erase
3. Address 0xbabe has value 0xfe
4. Run erase again (same or different block size)
5. Address 0xbabe has value 0xff
6. Write anything to address 0xbabe
7. Address 0xbabe has desired value
8. If you erase again, address 0xbabe has value 0xfe
Write resets the counter for erasestuckbyte.
For writstuckbyte=0x01@0xbabe+2 we'd get the following result:
1. Address 0xbabe has value 0x01|oldval_at_0xbabe
2. Run erase
3. Address 0xbabe has value 0xff
4. Write newval to address 0xbabe
5. Address 0xbabe has value 0x01|newval_at_0xbabe
6. Write newval to address 0xbabe
7. Address 0xbabe has desired value
Erase resets the counter for writestuckbyte
What do you think? Is this completely overdesigning a solution, or is it
a useful unit test?
Regards,
Carl-Daniel
--
http://www.hailfinger.org/
blacklisting and ignorelisting opcodes is just not enough.
this patch does not introduce the most outstand failure injection ever, but
it certainly helped me testing the other patch. maybe we want to make
similar failures user configurable?
Signed-off-by: Stefan Tauner <stefan.tauner(a)student.tuwien.ac.at>
---
dummyflasher.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/dummyflasher.c b/dummyflasher.c
index 66d0df0..5a51d1c 100644
--- a/dummyflasher.c
+++ b/dummyflasher.c
@@ -493,6 +493,7 @@ static int emulate_spi_chip_response(unsigned int writecnt,
{
unsigned int offs, i, toread;
static int unsigned aai_offs;
+ static int num_read = 0;
if (writecnt == 0) {
msg_perr("No command sent to the chip!\n");
@@ -587,6 +588,13 @@ static int emulate_spi_chip_response(unsigned int writecnt,
offs = writearr[1] << 16 | writearr[2] << 8 | writearr[3];
/* Truncate to emu_chip_size. */
offs %= emu_chip_size;
+ if(offs==(emu_chip_size/8)){
+ num_read++;
+ if(num_read >= 4){ // verify will fail iff there has been at least one erase retry
+ memset(readarr, 0x13, readcnt);
+ break;
+ }
+ }
if (readcnt > 0)
memcpy(readarr, flashchip_contents + offs, readcnt);
break;
@@ -602,6 +610,10 @@ static int emulate_spi_chip_response(unsigned int writecnt,
msg_perr("Max BYTE PROGRAM size exceeded!\n");
return 1;
}
+ if(offs==(emu_chip_size/4)){ // will be detected at verification
+ memset(flashchip_contents + offs, 0x14, writecnt - 4);
+ break;
+ }
memcpy(flashchip_contents + offs, writearr + 4, writecnt - 4);
break;
case JEDEC_AAI_WORD_PROGRAM:
@@ -659,6 +671,9 @@ static int emulate_spi_chip_response(unsigned int writecnt,
if (offs & (emu_jedec_se_size - 1))
msg_pdbg("Unaligned SECTOR ERASE 0x20: 0x%x\n", offs);
offs &= ~(emu_jedec_se_size - 1);
+ if(offs==(emu_chip_size/2)){ // this will be detected early (iff block needs an erase)
+ break;
+ }
memset(flashchip_contents + offs, 0xff, emu_jedec_se_size);
break;
case JEDEC_BE_52:
--
Kind regards, Stefan Tauner
Quick hack to print a warning if we skip all blocks because they are already
equal to the requested image. We want something like this to make users
aware... and some developers that regularly fall for this too *coughcough*.
Signed-off-by: Stefan Tauner <stefan.tauner(a)student.tuwien.ac.at>
---
flashrom.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/flashrom.c b/flashrom.c
index 70687ff..ebc3118 100644
--- a/flashrom.c
+++ b/flashrom.c
@@ -1218,6 +1218,7 @@ static int selfcheck_eraseblocks(const struct flashchip *flash)
return ret;
}
+static int all_skipped = 1;
static int erase_and_write_block_helper(struct flashctx *flash,
unsigned int start, unsigned int len,
uint8_t *curcontents,
@@ -1266,6 +1267,8 @@ static int erase_and_write_block_helper(struct flashctx *flash,
}
if (skip)
msg_cdbg("S");
+ else
+ all_skipped = 0;
return ret;
}
@@ -1391,6 +1394,8 @@ int erase_and_write_flash(struct flashctx *flash, uint8_t *oldcontents,
if (ret) {
msg_cerr("FAILED!\n");
} else {
+ if(all_skipped)
+ msg_cinfo("\nWarning: Chip contents were already identical to the requested image.\n");
msg_cinfo("Erase/write done.\n");
}
return ret;
--
Kind regards, Stefan Tauner
Kyösti Mälkki noticed that we unnecessarily read the flash chip twice when
called with --verify. The first one is the mandatory read before everything
(to be able to detect the seriousness of errors), but the second one is not
necessary because we can just use the former for the comparison.
This introduces a small output change: previously we printed ERASE or
VERIFY depending on the callee. This special case has been dropped
because it is unnecessary to print it (and wrong for the verification
function to need to know why it is verifying exactly).
If an erase fails we mention that fact explicitly already, similar for verify.
Signed-off-by: Stefan Tauner <stefan.tauner(a)student.tuwien.ac.at>
---
flash.h | 2 +-
flashrom.c | 86 +++++++++++++++++++++++++++---------------------------------
jedec.c | 2 +-
3 files changed, 41 insertions(+), 49 deletions(-)
diff --git a/flash.h b/flash.h
index d669512..5bb1211 100644
--- a/flash.h
+++ b/flash.h
@@ -241,7 +241,7 @@ int min(int a, int b);
int max(int a, int b);
void tolower_string(char *str);
char *extract_param(char **haystack, const char *needle, const char *delim);
-int verify_range(struct flashctx *flash, uint8_t *cmpbuf, unsigned int start, unsigned int len, const char *message);
+int verify_range(struct flashctx *flash, uint8_t *cmpbuf, unsigned int start, unsigned int len);
int need_erase(uint8_t *have, uint8_t *want, unsigned int len, enum write_granularity gran);
char *strcat_realloc(char *dest, const char *src);
void print_version(void);
diff --git a/flashrom.c b/flashrom.c
index fc52c4a..70687ff 100644
--- a/flashrom.c
+++ b/flashrom.c
@@ -548,6 +548,26 @@ static unsigned int count_usable_erasers(const struct flashctx *flash)
return usable_erasefunctions;
}
+int compare_range(uint8_t *wantbuf, uint8_t *havebuf, unsigned int start, unsigned int len)
+{
+ int ret = 0, failcount = 0;
+ unsigned int i;
+ for (i = 0; i < len; i++) {
+ if (wantbuf[i] != havebuf[i]) {
+ /* Only print the first failure. */
+ if (!failcount++)
+ msg_cerr("FAILED at 0x%08x! Expected=0x%02x, Found=0x%02x,",
+ start + i, wantbuf[i], havebuf[i]);
+ }
+ }
+ if (failcount) {
+ msg_cerr(" failed byte count from 0x%08x-0x%08x: 0x%x\n",
+ start, start + len - 1, failcount);
+ ret = -1;
+ }
+ return ret;
+}
+
/* start is an offset to the base address of the flash chip */
int check_erased_range(struct flashctx *flash, unsigned int start,
unsigned int len)
@@ -560,7 +580,7 @@ int check_erased_range(struct flashctx *flash, unsigned int start,
exit(1);
}
memset(cmpbuf, 0xff, len);
- ret = verify_range(flash, cmpbuf, start, len, "ERASE");
+ ret = verify_range(flash, cmpbuf, start, len);
free(cmpbuf);
return ret;
}
@@ -570,15 +590,12 @@ int check_erased_range(struct flashctx *flash, unsigned int start,
* flash content at location start
* @start offset to the base address of the flash chip
* @len length of the verified area
- * @message string to print in the "FAILED" message
* @return 0 for success, -1 for failure
*/
-int verify_range(struct flashctx *flash, uint8_t *cmpbuf, unsigned int start,
- unsigned int len, const char *message)
+int verify_range(struct flashctx *flash, uint8_t *cmpbuf, unsigned int start, unsigned int len)
{
- unsigned int i;
uint8_t *readbuf = malloc(len);
- int ret = 0, failcount = 0;
+ int ret = 0;
if (!len)
goto out_free;
@@ -599,8 +616,6 @@ int verify_range(struct flashctx *flash, uint8_t *cmpbuf, unsigned int start,
ret = -1;
goto out_free;
}
- if (!message)
- message = "VERIFY";
ret = flash->read(flash, readbuf, start, len);
if (ret) {
@@ -609,22 +624,7 @@ int verify_range(struct flashctx *flash, uint8_t *cmpbuf, unsigned int start,
return ret;
}
- for (i = 0; i < len; i++) {
- if (cmpbuf[i] != readbuf[i]) {
- /* Only print the first failure. */
- if (!failcount++)
- msg_cerr("%s FAILED at 0x%08x! "
- "Expected=0x%02x, Read=0x%02x,",
- message, start + i, cmpbuf[i],
- readbuf[i]);
- }
- }
- if (failcount) {
- msg_cerr(" failed byte count from 0x%08x-0x%08x: 0x%x\n",
- start, start + len - 1, failcount);
- ret = -1;
- }
-
+ ret = compare_range(cmpbuf, readbuf, start, len);
out_free:
free(readbuf);
return ret;
@@ -1060,21 +1060,6 @@ notfound:
return flash - flashchips;
}
-int verify_flash(struct flashctx *flash, uint8_t *buf)
-{
- int ret;
- unsigned int total_size = flash->total_size * 1024;
-
- msg_cinfo("Verifying flash... ");
-
- ret = verify_range(flash, buf, 0, total_size, NULL);
-
- if (!ret)
- msg_cinfo("VERIFIED. \n");
-
- return ret;
-}
-
int read_buf_from_file(unsigned char *buf, unsigned long size,
const char *filename)
{
@@ -1838,15 +1823,22 @@ int doit(struct flashctx *flash, int force, const char *filename, int read_it,
}
if (verify_it) {
- /* Work around chips which need some time to calm down. */
- if (write_it)
+ msg_cinfo("Verifying flash... ");
+
+ if (write_it) {
+ /* Work around chips which need some time to calm down. */
programmer_delay(1000*1000);
- ret = verify_flash(flash, newcontents);
- /* If we tried to write, and verification now fails, we
- * might have an emergency situation.
- */
- if (ret && write_it)
- emergency_help_message();
+ ret = verify_range(flash, newcontents, 0, size);
+ /* If we tried to write, and verification now fails, we
+ * might have an emergency situation.
+ */
+ if (ret)
+ emergency_help_message();
+ } else {
+ ret = compare_range(newcontents, oldcontents, 0, size);
+ }
+ if (!ret)
+ msg_cinfo("VERIFIED.\n");
}
out:
diff --git a/jedec.c b/jedec.c
index 69c0c0c..1fa1a10 100644
--- a/jedec.c
+++ b/jedec.c
@@ -409,7 +409,7 @@ retry:
dst = d;
src = s;
- failed = verify_range(flash, src, start, page_size, NULL);
+ failed = verify_range(flash, src, start, page_size);
if (failed && tried++ < MAX_REFLASH_TRIES) {
msg_cerr("retrying.\n");
--
Kind regards, Stefan Tauner
Thanks to Kyösti for bringing up this emberrassing issue.
Let's fix this quickly before anyone notices and please let us forget
that it ever happened. :)
The good thing is, that i had a few ideas while fixing the problem that
may improve the overall situation, looking forward to comments.
One idea i did not follow up yet is getting rid of read_flash_to_file(…).
It is as useless as verify_flash was before my patch and just complicates
things (e.g. for layout integration).
Stefan Tauner (3):
Do not read the flash chip twice in verification mode.
Not for merge: Warn if we skipped all blocks
Not for merge: make the dummy programmer FAIL
dummyflasher.c | 15 ++++++++++
flash.h | 2 +-
flashrom.c | 91 +++++++++++++++++++++++++++-----------------------------
jedec.c | 2 +-
4 files changed, 61 insertions(+), 49 deletions(-)
--
Kind regards, Stefan Tauner
Hi Jernej,
I saw that you sent a pciutils/libpci patch to support your Direct IO
driver on Windows.
flashrom is a GPL'ed utility to read/write flash chips, e.g. the one
storing BIOS/EFI on a mainboard.
Right now, flashrom is quite limited on Windows: It doesn't support
anything which needs MMIO/IOPort access. While it would be possible to
use WinIo, the WinIo license seems to be a bit odd and may be
incompatible with the GPL. Your Direct IO driver has the advantage of
being GPL, but it is missing a key feature needed by flashrom: Mapping
physical memory to the application (similar to MapPhysToLin in WinIo).
flashrom needs uncached mappings of device memory regions, and cached
mappings of RAM regions storing DMI info. I'm not sure whether
NtMapViewOfSection would already be enough for this purpose, or if your
Direct IO driver would have to be extended to offer such functionality.
Regards,
Carl-Daniel
If only one programmer driver is compiled in, make that driver the
default. If more than one driver is compiled in, require --programmer
specification at the command line.
3 results from a default flashrom configuration:
compiler@p35:/sources/tmptrees/ready/flashrom-programmer_no_default>
./flashrom
flashrom v0.9.5.2-r1547 on Linux 2.6.34.10-0.6-default (i686)
Please select a programmer with --programmer . Valid choices are:
internal, dummy, nic3com, nicrealtek, gfxnvidia, drkaiser, satasii,
ft2232_spi,
serprog, buspirate_spi, rayer_spi, pony_spi, nicintel, nicintel_spi,
ogp_spi,
satamv, linux_spi
compiler@p35:/sources/tmptrees/ready/flashrom-programmer_no_default>
./flashrom -p foo
flashrom v0.9.5.2-r1547 on Linux 2.6.34.10-0.6-default (i686)
Error: Unknown programmer foo. Valid choices are:
internal, dummy, nic3com, nicrealtek, gfxnvidia, drkaiser, satasii,
ft2232_spi,
serprog, buspirate_spi, rayer_spi, pony_spi, nicintel, nicintel_spi,
ogp_spi,
satamv, linux_spi
Please run "flashrom --help" for usage info.
compiler@p35:/sources/tmptrees/ready/flashrom-programmer_no_default>
./flashrom -p internal -p internal
flashrom v0.9.5.2-r1547 on Linux 2.6.34.10-0.6-default (i686)
Error: --programmer specified more than once. You can separate multiple
parameters for a programmer with ",". Please see the man page for details.
Please run "flashrom --help" for usage info.
1 result from a flashrom configuration with only dummy compiled in:
compiler@p35:/sources/tmptrees/ready/flashrom-programmer_no_default>
./flashrom
flashrom v0.9.5.2-r1547 on Linux 2.6.34.10-0.6-default (i686)
Calibrating delay loop... OK.
No EEPROM/flash device found.
Note: flashrom can never write if the flash chip isn't found automatically.
This patch represents rough consensus from IRC. I would like to require
--programmer in all cases to make sure nobody gets bitten by two
different single-programmer builds (e.g. dediprog and internal), but
this patch is already a step in the right direction.
Please check that the printed error messages make sense. I took the
liberty of removing "flashrom is free software..." from the output to
keep this mail readable.
Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006(a)gmx.net>
Index: flashrom-programmer_no_default/cli_classic.c
===================================================================
--- flashrom-programmer_no_default/cli_classic.c (Revision 1547)
+++ flashrom-programmer_no_default/cli_classic.c (Arbeitskopie)
@@ -31,20 +31,19 @@
#include "flashchips.h"
#include "programmer.h"
-#if CONFIG_INTERNAL == 1
-static enum programmer default_programmer = PROGRAMMER_INTERNAL;
-#elif CONFIG_DUMMY == 1
-static enum programmer default_programmer = PROGRAMMER_DUMMY;
-#else
-/* If neither internal nor dummy are selected, we must pick a sensible default.
- * Since there is no reason to prefer a particular external programmer, we fail
- * if more than one of them is selected. If only one is selected, it is clear
- * that the user wants that one to become the default.
+/* If only one programmer is compiled in, it is the default.
+ * In all other cases there is no default and the user has to specify the programmer with -p .
*/
+static enum programmer default_programmer =
#if CONFIG_NIC3COM+CONFIG_NICREALTEK+CONFIG_NICNATSEMI+CONFIG_GFXNVIDIA+CONFIG_DRKAISER+CONFIG_SATASII+CONFIG_ATAHPT+CONFIG_FT2232_SPI+CONFIG_SERPROG+CONFIG_BUSPIRATE_SPI+CONFIG_DEDIPROG+CONFIG_RAYER_SPI+CONFIG_NICINTEL+CONFIG_NICINTEL_SPI+CONFIG_OGP_SPI+CONFIG_SATAMV > 1
-#error Please enable either CONFIG_DUMMY or CONFIG_INTERNAL or disable support for all programmers except one.
+ PROGRAMMER_INVALID
+#else
+#if CONFIG_INTERNAL == 1
+ PROGRAMMER_INTERNAL
#endif
-static enum programmer default_programmer =
+#if CONFIG_DUMMY == 1
+ PROGRAMMER_DUMMY
+#endif
#if CONFIG_NIC3COM == 1
PROGRAMMER_NIC3COM
#endif
@@ -96,8 +95,8 @@
#if CONFIG_LINUX_SPI == 1
PROGRAMMER_LINUX_SPI
#endif
+#endif
;
-#endif
static void cli_classic_usage(const char *name)
{
@@ -107,11 +106,11 @@
#endif
"-E|-r <file>|-w <file>|-v <file>]\n"
" [-c <chipname>] [-l <file>] [-o <file>]\n"
- " [-i <image>] [-p <programmername>[:<parameters>]]\n\n");
+ " [-i <image>] -p <programmername>[:<parameters>]\n\n");
printf("Please note that the command line interface for flashrom has "
"changed between\n"
- "0.9.1 and 0.9.2 and will change again before flashrom 1.0.\n"
+ "0.9.5 and 0.9.6 and will change again before flashrom 1.0.\n"
"Do not use flashrom in scripts or other automated tools "
"without checking\n"
"that your flashrom version won't interpret options in a "
@@ -360,8 +359,8 @@
}
}
if (prog == PROGRAMMER_INVALID) {
- fprintf(stderr, "Error: Unknown programmer "
- "%s.\n", optarg);
+ fprintf(stderr, "Error: Unknown programmer %s. Valid choices are:\n", optarg);
+ list_programmers_linebreak(0, 80, 0);
cli_classic_abort_usage();
}
break;
@@ -469,7 +468,15 @@
}
if (prog == PROGRAMMER_INVALID)
- prog = default_programmer;
+ if (default_programmer == PROGRAMMER_INVALID) {
+ /* More than one programmer compiled in, there is no default choice. */
+ msg_perr("Please select a programmer with --programmer . Valid choices are:\n");
+ list_programmers_linebreak(0, 80, 0);
+ ret = 1;
+ goto out;
+ } else {
+ prog = default_programmer;
+ }
/* FIXME: Delay calibration should happen in programmer code. */
myusec_calibrate_delay();
--
http://www.hailfinger.org/