| Submitter | Carl-Daniel Hailfinger |
|---|---|
| Date | 2010-10-15 15:18:06 |
| Message ID | <4CB870AE.6010502@gmx.net> |
| Download | mbox | patch |
| Permalink | /patch/2116/ |
| State | Bitrotted |
| Headers | show |
Comments
* Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net> [101015 17:18]: > Add a function which checks erase and write operations and makes sure > that neither touch anything outside the desired region. > Runtime is O(n^2). What's n in this case? While in theory it would not matter, in practice n being the number of bytes of flash would have significantly more impact than n being the number of erase blocks of the chip, or blocks to actually write. > A less paranoid version can have O(n) runtime, but I think it is > valuable to have the paranoid option. > > Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net> It's very unclear from this patch what the impact on flashrom would be later on as it just adds dead, untestable code. ;-) What's the use case? Since you mention test patterns in the code below the implication is that this code is only used to verify whether a flash chip driver is consistent, so it would be something that a normal user does not have to suffer from. > Index: flashrom-test_chip_infrastructure/flashrom.c > =================================================================== > --- flashrom-test_chip_infrastructure/flashrom.c (Revision 1212) > +++ flashrom-test_chip_infrastructure/flashrom.c (Arbeitskopie) > @@ -1271,6 +1271,65 @@ > return ret; > } > > +/* This function expects the previous flash contents in oldcontents, and the > + * wanted flash contents in newcontents. Usually both are test patterns, and > + * they should be distinct. The region from start to start+len-1 is expected > + * to be already erased. > + */ Not for this patch, but have you considered using doxygen style comments? > +int autotest_write_eraseregion_paranoid(struct flashchip *flash, > + unsigned int start, unsigned int len, > + uint8_t *oldcontents, > + uint8_t *newcontents) > +{ > + /* Checking integrity of just erased range is not needed, the erase > + * function will do that itself. Do it anyway for nicer diagnostics. > + */ > + if (check_erased_range(flash, start, len)) { > + msg_cerr("Error in just erased range 0x%x-0x%x\n", > + start, start + len - 1); > + return VERIFY_ERROR; > + } Is there a better way to have nicer diagnostics? If this is only used for driver verification I guess it does not matter. Otherwise it would be really ugly. So, in the case that this is driver verification code only: Acked-by: Stefan Reinauer <stepan@coreboot.org>
Patch
Index: flashrom-test_chip_infrastructure/flash.h =================================================================== --- flashrom-test_chip_infrastructure/flash.h (Revision 1212) +++ flashrom-test_chip_infrastructure/flash.h (Arbeitskopie) @@ -37,6 +37,8 @@ /* Error codes */ #define TIMEOUT_ERROR -101 +#define VERIFY_ERROR -201 +#define COMMAND_ERROR -202 typedef unsigned long chipaddr; Index: flashrom-test_chip_infrastructure/flashrom.c =================================================================== --- flashrom-test_chip_infrastructure/flashrom.c (Revision 1212) +++ flashrom-test_chip_infrastructure/flashrom.c (Arbeitskopie) @@ -1271,6 +1271,65 @@ return ret; } +/* This function expects the previous flash contents in oldcontents, and the + * wanted flash contents in newcontents. Usually both are test patterns, and + * they should be distinct. The region from start to start+len-1 is expected + * to be already erased. + */ +int autotest_write_eraseregion_paranoid(struct flashchip *flash, + unsigned int start, unsigned int len, + uint8_t *oldcontents, + uint8_t *newcontents) +{ + /* Checking integrity of just erased range is not needed, the erase + * function will do that itself. Do it anyway for nicer diagnostics. + */ + if (check_erased_range(flash, start, len)) { + msg_cerr("Error in just erased range 0x%x-0x%x\n", + start, start + len - 1); + return VERIFY_ERROR; + } + /* Check integrity of previously written range. */ + if (verify_range(flash, newcontents, 0, start, NULL)) { + msg_cerr("Error in previously written range 0x%x-0x%x\n", + 0, start - 1); + return VERIFY_ERROR; + } + /* Check integrity of not yet written range. */ + if (verify_range(flash, oldcontents + start + len, start + len, + flash->total_size * 1024 - start - len, NULL)) { + msg_cerr("Error in not yet written range 0x%x-0x%x\n", + start + len, flash->total_size * 1024 - 1); + return VERIFY_ERROR; + } + /* Check if the write function fails. */ + if (flash->write(flash, newcontents + start, start, len)) { + msg_cerr("Error during write in range 0x%x-0x%x\n", + start, start + len - 1); + return COMMAND_ERROR; + } + /* Check integrity of just written range. */ + if (verify_range(flash, newcontents + start, start, len, NULL)) { + msg_cerr("Error in just written range 0x%x-0x%x\n", + start, start + len - 1); + return VERIFY_ERROR; + } + /* Recheck integrity of previously written range. */ + if (verify_range(flash, newcontents, 0, start, NULL)) { + msg_cerr("Error in previously written range 0x%x-0x%x\n", + 0, start - 1); + return VERIFY_ERROR; + } + /* Recheck integrity of not yet written range. */ + if (verify_range(flash, oldcontents + start + len, start + len, + flash->total_size * 1024 - start - len, NULL)) { + msg_cerr("Error in not yet written range 0x%x-0x%x\n", + start + len, flash->total_size * 1024 - 1); + return VERIFY_ERROR; + } + return 0; +} + static int walk_eraseregions(struct flashchip *flash, int erasefunction, int (*do_something) (struct flashchip *flash, unsigned int addr,
Add a function which checks erase and write operations and makes sure that neither touch anything outside the desired region. Runtime is O(n^2). A less paranoid version can have O(n) runtime, but I think it is valuable to have the paranoid option. Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net>