Hello Mike Banon, I'd like you to do a code review. Please visit https://review.coreboot.org/c/flashrom/+/31830 to review the following change. Change subject: Add "--dangerous-bruteforce" mode for the unreliable ISP environments ...................................................................... Add "--dangerous-bruteforce" mode for the unreliable ISP environments Some in-system-programming environments are very unreliable, e.g. when you connect a SOIC8 test clip to the flash chip of WR841ND v9 wireless router: because the low power SOC of this router becomes powered and is constantly trying to boot, the router's flash chip is correctly detected only 30% of times, and when it is detected just 30% of the read operations are successful - the rest result in zeroes or garbage data being read from a flash chip. While in this new "--dangerous-bruteforce" mode, flashrom will be constantly probing for a flash chip until it is correctly detected - and then, depending on the operation selected by a user, at the moments of chip availability it will be reading or writing by the small chunks of 256 bytes maximum size, verifying each small chunk and retrying the operation until the success or timeout (~ 60 sec). Constant probing between the small chunk operations is also being done - to avoid a possible situation where we have got two identical blocks of zeroes while reading a chunk and erroneously accepted it. Currently this mode is active only for those SPI25 chips which are ".write = spi_chip_write_256" and ".read = spi_chip_read", but later could be expanded to more functions to become useful for more chips. NOTE: this mode is experimental! Also, some targets like WR841ND v9 could block the ISP chip access completely if there is no bootable firmware inside (e.g. after you've tried to erase the chip contents). In such a situation it could be hard to recover the firmware without desoldering a flash chip and flashing it separately. Please do not erase or write to a flash chip using this mode if you are not sure. Signed-off-by: Mike Banon <mikebdp2@gmail.com> Change-Id: If47476793721a0061fd65bc6d1f98d48b03dc6c1 --- M cli_classic.c M flash.h M flashrom.c M libflashrom.c M libflashrom.h M spi25.c 6 files changed, 148 insertions(+), 15 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/30/31830/1 diff --git a/cli_classic.c b/cli_classic.c index ced08c6..454df2b 100644 --- a/cli_classic.c +++ b/cli_classic.c @@ -51,6 +51,7 @@ " -V | --verbose more verbose output\n" " -c | --chip <chipname> probe only for specified flash chip\n" " -f | --force force specific operations (see man page)\n" + " --dangerous-bruteforce verify each small chunk and probe constantly\n" " -n | --noverify don't auto-verify\n" " -N | --noverify-all verify included regions only (cf. -i)\n" " -l | --layout <layoutfile> read ROM layout from <layoutfile>\n" @@ -104,7 +105,7 @@ #if CONFIG_PRINT_WIKI == 1 int list_supported_wiki = 0; #endif - int read_it = 0, write_it = 0, erase_it = 0, verify_it = 0; + int read_it = 0, write_it = 0, erase_it = 0, verify_it = 0, bruteforce = 0, timeout = 30000; int dont_verify_it = 0, dont_verify_all = 0, list_supported = 0, operation_specified = 0; struct flashrom_layout *layout = NULL; enum programmer prog = PROGRAMMER_INVALID; @@ -113,6 +114,7 @@ OPTION_FMAP, OPTION_FMAP_FILE, OPTION_FLASH_CONTENTS, + OPTION_BRUTEFORCE, }; int ret = 0; @@ -127,6 +129,7 @@ {"chip", 1, NULL, 'c'}, {"verbose", 0, NULL, 'V'}, {"force", 0, NULL, 'f'}, + {"dangerous-bruteforce", 0, NULL, OPTION_BRUTEFORCE}, {"layout", 1, NULL, 'l'}, {"ifd", 0, NULL, OPTION_IFD}, {"fmap", 0, NULL, OPTION_FMAP}, @@ -228,6 +231,9 @@ case 'f': force = 1; break; + case OPTION_BRUTEFORCE: + bruteforce = 1; + break; case 'l': if (layoutfile) { fprintf(stderr, "Error: --layout specified " @@ -503,16 +509,27 @@ msg_pdbg("The following protocols are supported: %s.\n", tempstr); free(tempstr); - for (j = 0; j < registered_master_count; j++) { - startchip = 0; - while (chipcount < ARRAY_SIZE(flashes)) { - startchip = probe_flash(®istered_masters[j], startchip, &flashes[chipcount], 0); - if (startchip == -1) - break; - chipcount++; - startchip++; + do { + chipcount = 0; + for (j = 0; j < registered_master_count; j++) { + startchip = 0; + while (chipcount < ARRAY_SIZE(flashes)) { + startchip = probe_flash(®istered_masters[j], startchip, &flashes[chipcount], 0); + if (startchip == -1) + break; + chipcount++; + startchip++; + } } - } + timeout--; + if (!timeout) { + msg_cerr("Error: Timed out trying to detect a chip.\n"); + ret = 1; + goto out_shutdown; + } + } while (bruteforce && ( !chipcount || // wait for the correct chip detection + ((flashes[0].chip->manufacture_id == GENERIC_MANUF_ID) || + (flashes[0].chip->model_id == GENERIC_DEVICE_ID)) ) ); if (chipcount > 1) { msg_cinfo("Multiple flash chip definitions match the detected chip(s): \"%s\"", @@ -650,6 +667,7 @@ #if CONFIG_INTERNAL == 1 flashrom_flag_set(fill_flash, FLASHROM_FLAG_FORCE_BOARDMISMATCH, !!force_boardmismatch); #endif + flashrom_flag_set(fill_flash, FLASHROM_FLAG_BRUTEFORCE, !!bruteforce); flashrom_flag_set(fill_flash, FLASHROM_FLAG_VERIFY_AFTER_WRITE, !dont_verify_it); flashrom_flag_set(fill_flash, FLASHROM_FLAG_VERIFY_WHOLE_CHIP, !dont_verify_all); diff --git a/flash.h b/flash.h index 911214c..6ba116e 100644 --- a/flash.h +++ b/flash.h @@ -256,6 +256,7 @@ struct { bool force; bool force_boardmismatch; + bool bruteforce; bool verify_after_write; bool verify_whole_chip; } flags; diff --git a/flashrom.c b/flashrom.c index e82c32e..8f2c859 100644 --- a/flashrom.c +++ b/flashrom.c @@ -1641,6 +1641,8 @@ size_t i, j; bool first = true; struct block_eraser *const eraser = &flashctx->chip->block_erasers[erasefunction]; + unsigned int bruteforce = flashrom_flag_get(flashctx, FLASHROM_FLAG_BRUTEFORCE); + int timeout; info->erase_start = 0; for (i = 0; i < NUM_ERASEREGIONS; ++i) { @@ -1662,9 +1664,34 @@ msg_cdbg(", "); msg_cdbg("0x%06x-0x%06x:", info->erase_start, info->erase_end); - ret = per_blockfn(flashctx, info, eraser->block_erase); - if (ret) - return ret; + if (!bruteforce) { + ret = per_blockfn(flashctx, info, eraser->block_erase); + if (ret) + return ret; + } else { + timeout = 30000; + while(1) { + timeout--; + if (!timeout) { + msg_cerr("%s timeout during command execution\n", __func__); + return 2; + } + + /* First probe. */ + if (flashctx->chip->probe(flashctx) != 1) + continue; + + ret = per_blockfn(flashctx, info, eraser->block_erase); + if (ret) + continue; + + /* Second probe. */ + if (flashctx->chip->probe(flashctx) != 1) + continue; + + break; + } + } } if (info->region_end < info->erase_start) break; diff --git a/libflashrom.c b/libflashrom.c index f90a22c..6dd0167 100644 --- a/libflashrom.c +++ b/libflashrom.c @@ -250,6 +250,7 @@ switch (flag) { case FLASHROM_FLAG_FORCE: flashctx->flags.force = value; break; case FLASHROM_FLAG_FORCE_BOARDMISMATCH: flashctx->flags.force_boardmismatch = value; break; + case FLASHROM_FLAG_BRUTEFORCE: flashctx->flags.bruteforce = value; break; case FLASHROM_FLAG_VERIFY_AFTER_WRITE: flashctx->flags.verify_after_write = value; break; case FLASHROM_FLAG_VERIFY_WHOLE_CHIP: flashctx->flags.verify_whole_chip = value; break; } @@ -267,6 +268,7 @@ switch (flag) { case FLASHROM_FLAG_FORCE: return flashctx->flags.force; case FLASHROM_FLAG_FORCE_BOARDMISMATCH: return flashctx->flags.force_boardmismatch; + case FLASHROM_FLAG_BRUTEFORCE: return flashctx->flags.bruteforce; case FLASHROM_FLAG_VERIFY_AFTER_WRITE: return flashctx->flags.verify_after_write; case FLASHROM_FLAG_VERIFY_WHOLE_CHIP: return flashctx->flags.verify_whole_chip; default: return false; diff --git a/libflashrom.h b/libflashrom.h index 38c95d2..0f2ead8 100644 --- a/libflashrom.h +++ b/libflashrom.h @@ -51,6 +51,7 @@ enum flashrom_flag { FLASHROM_FLAG_FORCE, FLASHROM_FLAG_FORCE_BOARDMISMATCH, + FLASHROM_FLAG_BRUTEFORCE, FLASHROM_FLAG_VERIFY_AFTER_WRITE, FLASHROM_FLAG_VERIFY_WHOLE_CHIP, }; diff --git a/spi25.c b/spi25.c index 8b6c462..832c836 100644 --- a/spi25.c +++ b/spi25.c @@ -21,6 +21,7 @@ #include <stddef.h> #include <string.h> #include <stdbool.h> +#include <stdlib.h> #include "flash.h" #include "flashchips.h" #include "chipdrivers.h" @@ -655,6 +656,48 @@ return spi_send_command(flash, 1 + addr_len, len, cmd, bytes); } +static int spi_rw_bruteforce(struct flashctx *flash, uint8_t *buf1, uint8_t *buf2, + unsigned int startrw_here, unsigned int torw_len, bool rw_flag) { + int rc = 0; + int timeout = 30000; + while (1) { + timeout--; + if (!timeout) { + msg_cerr("%s timeout during command execution\n", __func__); + return -1; + } + + /* First probe. */ + if (flash->chip->probe(flash) != 1) + continue; + + /* First operation. */ + if (rw_flag == 0) + rc = spi_nbyte_read(flash, startrw_here, buf1, torw_len); + else + rc = spi_nbyte_program(flash, startrw_here, buf1, torw_len); + + if (rc != 0) + break; + + /* Second probe. */ + if (flash->chip->probe(flash) != 1) + continue; + + /* Second operation. */ + rc = spi_nbyte_read(flash, startrw_here, buf2, torw_len); + if (rc != 0) + break; + + /* Compare two buffers. */ + if (memcmp(buf1, buf2, torw_len)) + continue; + + break; + } + return rc; +} + /* * Read a part of the flash chip. * FIXME: Use the chunk code from Michael Karcher instead. @@ -668,6 +711,19 @@ /* Limit for multi-die 4-byte-addressing chips. */ unsigned int area_size = min(flash->chip->total_size * 1024, 16 * 1024 * 1024); + unsigned int bruteforce = flashrom_flag_get(flash, FLASHROM_FLAG_BRUTEFORCE); + uint8_t *const buf1 = bruteforce ? malloc(chunksize) : NULL; + uint8_t *const buf2 = bruteforce ? malloc(chunksize) : NULL; + + if (bruteforce) { + if (!buf1 || !buf2) { + msg_gerr("Out of memory!\n"); + goto _free_ret; + } + msg_cdbg("spi_read_chunked: chunksize %i ---> %i\n", chunksize, min(chunksize, 256)); + chunksize = min(chunksize, 256); + } + /* Warning: This loop has a very unusual condition and body. * The loop needs to go through each area with at least one affected * byte. The lowest area number is (start / area_size) since that @@ -685,7 +741,11 @@ lenhere = min(start + len, (i + 1) * area_size) - starthere; for (j = 0; j < lenhere; j += chunksize) { toread = min(chunksize, lenhere - j); - rc = spi_nbyte_read(flash, starthere + j, buf + starthere - start + j, toread); + if (bruteforce) { + rc = spi_rw_bruteforce(flash, buf1, buf2, starthere + j, toread, 0 /* read */); + memcpy(buf + starthere - start + j, buf1, toread); + } else + rc = spi_nbyte_read(flash, starthere + j, buf + starthere - start + j, toread); if (rc) break; } @@ -693,6 +753,9 @@ break; } +_free_ret: + free(buf1); + free(buf2); return rc; } @@ -712,6 +775,19 @@ */ unsigned int page_size = flash->chip->page_size; + unsigned int bruteforce = flashrom_flag_get(flash, FLASHROM_FLAG_BRUTEFORCE); + uint8_t *const buf1 = bruteforce ? malloc(chunksize) : NULL; + uint8_t *const buf2 = bruteforce ? malloc(chunksize) : NULL; + + if (bruteforce) { + if (!buf1 || !buf2) { + msg_gerr("Out of memory!\n"); + goto _free_ret; + } + msg_cdbg("spi_write_chunked: chunksize %i ---> %i\n", chunksize, min(chunksize, 256)); + chunksize = min(chunksize, 256); + } + /* Warning: This loop has a very unusual condition and body. * The loop needs to go through each page with at least one affected * byte. The lowest page number is (start / page_size) since that @@ -731,12 +807,20 @@ int rc; towrite = min(chunksize, lenhere - j); - rc = spi_nbyte_program(flash, starthere + j, buf + starthere - start + j, towrite); + if (bruteforce) { + memcpy(buf1, buf + starthere - start + j, towrite); + rc = spi_rw_bruteforce(flash, buf1, buf2, starthere + j, towrite, 1 /* write */); + } else + rc = spi_nbyte_program(flash, starthere + j, buf + starthere - start + j, towrite); + if (rc) return rc; } } +_free_ret: + free(buf1); + free(buf2); return 0; } -- To view, visit https://review.coreboot.org/c/flashrom/+/31830 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: If47476793721a0061fd65bc6d1f98d48b03dc6c1 Gerrit-Change-Number: 31830 Gerrit-PatchSet: 1 Gerrit-Owner: mikeb mikeb <mikebdp2@gmail.com> Gerrit-Reviewer: Mike Banon <mikebdp2@gmail.com> Gerrit-MessageType: newchange