Change in ...flashrom[master]: Add "--dangerous-bruteforce" mode for the unreliable ISP environments
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; }
mikeb mikeb has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/31830 )
Change subject: Add "--dangerous-bruteforce" mode for the unreliable ISP environments ......................................................................
Patch Set 2:
It's the rebased code from CB:23840 with the following improvements: 1) "gingerly" --> "dangerous-bruteforce" 2) timeout implemented. As you could see from a NOTE added to its' commit message, this mode has turned out "more dangerous than expected": there is a chance that after attempting the erase/write operations you will not be able to revert their results by using this "dangerous-bruteforce" mode - or even to read again using this mode - until you will recover your target's bootable firmware by the other methods.
After encountering this roadblock I'm not sure when I'll be able to complete the rest of your suggestions mentioned at https://review.coreboot.org/c/flashrom/+/23840 , but at least I have never encountered any problems with the not-writing operations such as probe and read - these should be safe...
Hello Mike Banon, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/31830
to look at the new patch set (#5).
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, 150 insertions(+), 15 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/30/31830/5
Mike Banon has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/31830 )
Change subject: Add "--dangerous-bruteforce" mode for the unreliable ISP environments ......................................................................
Patch Set 5:
This patch helps to at least read a current firmware of WR841ND v9 and maybe of some other routers, so would be nice to see it merged eventually.
Hello Mike Banon, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/31830
to look at the new patch set (#6).
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, 150 insertions(+), 15 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/30/31830/6
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/31830 )
Change subject: Add "--dangerous-bruteforce" mode for the unreliable ISP environments ......................................................................
Patch Set 6: Code-Review-1
I would not want to see this merged, so as to not encourage in-circuit flashing on boards which are not designed for that due to the potential risks involved.
Breaking hardware is easier than one could think: just powering a mainboard via a clip can violate the power sequencing requirements of the board and damage to other ICs on the board. Even using an external power supply could harm some boards. Overloading data pins is very easy to do. Even using an ohmeter (multimeter in resistance mode) on an unpowered board can kill circuits.
Let's see some examples. Let's suppose we have a chip (e.g. a southbridge) that uses two voltage rails: rail A (3.3V) and rail B (1.8V). Suppose we're only powering one rail at a time, while the other rail is left unpowered (therefore, at zero volts). The voltage difference between both rails (A - B) changes significantly:
Normal: 3.3V - 1.8V = 1.5V Only rail A: 3.3V - 0V = 3.3V Only rail B: 0V - 1.8V = -1.8V
The rail A case means that some parts of the chip would see more than twice of the voltage they expect. Doesn't look good. The rail B case is even worse: some parts of the chip would be reverse-biased (powered backwards). I don't even want to think about it.
If there are protection diodes, you could survive such a disaster. But how do you know they are installed in first place? They could be omitted on designs with a strict power sequence, or on cheap designs. Moreover, if any of these diodes is clamping the voltage down to a safe level, this means more current is drawn from the programmer. Most 3.3V regulators on flashers are tiny and easy to overload, and their failure mode usually results in them passing through their input voltage to the output, which is usually 5V from the USB ports. Then, everything gets overvolted, and things die.
Some people suggest using a beefier external power supply, such as an ATX computer PSU, if the flasher is not giving out enough power. That is an insane recommendation nobody should ever follow unless their goal is to light that board on fire. If any clamp circuits were active and overloading the flasher's (relatively weak) voltage source to protect the board, they will now have to handle currents of up to a few dozen amps, coming from the external PSU. Do you like fireworks?
If you're powering the board using its own power supply, so that the flash chip is powered by the board itself, not the programmer, then you have to deal with the other active masters on the SPI bus (ideally, just one). If you can make all of them stay idle or in reset, the SPI bus should be inactive and you should be able to flash without requiring this mode.
If you can't make the other SPI masters shut up, then your flasher will be fighting with the other masters on the SPI bus. Data corruption aside, if a master drives a pin high while the other one drives the same pin low, then we have problems.
If the circuit board has resistors between the flash chip and the onboard masters, the onboard masters will have a higher impedance than your flasher, so theoretically you could probably flash fine without needing this mode.
However, if the board has no resistors between the masters (usually the case on cheap boards), you have a short-circuit. On most SPI masters, the absolute maximum allowable current flowing through I/O pins is very low. A short-circuit can easily kill those pins, and thus render the SPI interface useless. And killing a CH341A that way will hurt much less than killing a computer mainboard with a soldered CPU.
If somebody wants to flash a mainboard which is not designed to be flashed in-circuit, they should consider removing the flash chip. If flashing needs to be done regularly, then they should consider making the flash chip easily removable by installing a socket, or modifying the board so that it can handle in-circuit flashing: a diode on VCC to prevent powering the rest of the board, and resistors on the data signals to avoid driving outputs. Some boards' design makes the last option impossible to sanely implement, though.
I can see why people want to avoid doing any of the above: it involves using soldering equipment on rather small components. Without soldering, there's no good solution.
Mike Banon has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/31830 )
Change subject: Add "--dangerous-bruteforce" mode for the unreliable ISP environments ......................................................................
Patch Set 6:
Patch Set 6: Code-Review-1
I would not want to see this merged, so as to not encourage in-circuit flashing on boards which are not designed for that due to the potential risks involved.
Angel, thank you very much for your wise detailed reply. Although I still intend to maintain this patch (since it turned out to be useful at least for this TL-WR841ND v9 and haven't damaged its' hardware), now I understand why it's not a good idea to merge it - may be more dangerous than a "dangerous" word implies ;)
-
Angel Pons (Code Review) -
Mike Banon (Code Review)
-
mikeb mikeb (Code Review)