[L] Change in flashrom[main]: Complete and fix progress feature implementation for all operations
Anastasia Klimchuk has submitted this change. ( https://review.coreboot.org/c/flashrom/+/84102?usp=email )
Change subject: Complete and fix progress feature implementation for all operations ......................................................................
Complete and fix progress feature implementation for all operations
Original progress reporting implemented in CB:49643 and it has some issues, for example:
size_t start_address = start; size_t end_address = len - start;
End address is anything but length minus start address.
update_progress(flash, FLASHROM_PROGRESS_READ, /*current*/ start - start_address + to_read, /*total*/ end_address);
Total should just be length if that's how current value is computed.
---
libflashrom needs to know total size ahead of time. That's init_progress() and changed update_progress().
It also needs to store the last current value to be able to update it. That's stage_progress in flashrom_flashctx.
Measuring accurately amount of data which will be read/erased/written isn't easy because things can be skipped as optimizations. The next patch in the chain aims to address this, there are TODO/FIXME comments there.
---
CLI shares terminal with the rest of the code and has to maintain more state to handle that reasonably well.
Similar to CB:64668, an effort is made to keep the progress on a single line. Non-progress output is kept track of to know when moving to a new line cannot be avoided.
---
A script to test the CLI:
#!/bin/bash t=${1:-rewW} shift
if [[ $t =~ r ]]; then echo ">>> READ" ./flashrom -p dummy:emulate=W25Q128FV,freq=64mhz -r dump.rom --progress "$@" echo fi
if [[ $t =~ e ]]; then echo ">>> ERASE" ./flashrom -p dummy:emulate=W25Q128FV,freq=64mhz -E --progress "$@" echo fi
if [[ $t =~ w ]]; then echo ">>> WRITE (without erase)" dd if=/dev/zero of=zero.rom bs=1M count=16 2> /dev/null ./flashrom -p dummy:emulate=W25Q128FV,freq=64mhz -w zero.rom --progress "$@" echo fi
if [[ $t =~ W ]]; then echo ">>> WRITE (with erase)" dd if=/dev/zero of=zero.rom bs=1M count=16 2> /dev/null dd if=/dev/random of=random.rom bs=1M count=16 2> /dev/null ./flashrom -p dummy:emulate=W25Q128FV,freq=64mhz,image=random.rom -w zero.rom --progress "$@" echo fi
Co-developed-by: Anastasia Klimchuk aklm@flashrom.org Co-developed-by: Sergii Dmytruk sergii.dmytruk@3mdeb.com Change-Id: If1e40fc97f443c4f0c0501cef11cff1f3f84c051 Signed-off-by: Sergii Dmytruk sergii.dmytruk@3mdeb.com Signed-off-by: Anastasia Klimchuk aklm@flashrom.org Reviewed-on: https://review.coreboot.org/c/flashrom/+/84102 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Anastasia Klimchuk aklm@chromium.org --- M 82802ab.c M at45db.c M cli_classic.c M cli_output.c M dediprog.c M doc/release_notes/devel.rst M en29lv640b.c M erasure_layout.c M flashrom.c M include/flash.h M it87spi.c M jedec.c M libflashrom.c M linux_mtd.c M parade_lspcon.c M realtek_mst_i2c_spi.c M spi.c M spi25.c M sst28sf040.c M tests/chip.c M tests/spi25.c M tests/tests.c M tests/tests.h 23 files changed, 333 insertions(+), 51 deletions(-)
Approvals: Anastasia Klimchuk: Looks good to me, approved Sergii Dmytruk: Looks good to me, approved build bot (Jenkins): Verified
diff --git a/82802ab.c b/82802ab.c index a440bd4..148042c 100644 --- a/82802ab.c +++ b/82802ab.c @@ -135,7 +135,7 @@ chip_writeb(flash, 0x40, dst); chip_writeb(flash, *src++, dst++); wait_82802ab(flash); - update_progress(flash, FLASHROM_PROGRESS_WRITE, i + 1, len); + update_progress(flash, FLASHROM_PROGRESS_WRITE, 1); }
/* FIXME: Ignore errors for now. */ diff --git a/at45db.c b/at45db.c index 6d66a6e..c0c6eaf 100644 --- a/at45db.c +++ b/at45db.c @@ -551,7 +551,7 @@ msg_cerr("Writing page %u failed!\n", i); return 1; } - update_progress(flash, FLASHROM_PROGRESS_WRITE, i + page_size, len); + update_progress(flash, FLASHROM_PROGRESS_WRITE, page_size); } return 0; } diff --git a/cli_classic.c b/cli_classic.c index 3343438..3e9db50 100644 --- a/cli_classic.c +++ b/cli_classic.c @@ -1060,9 +1060,9 @@
fill_flash = &flashes[0];
- unsigned int progress_user_data[FLASHROM_PROGRESS_NR]; + struct cli_progress cli_progress = {0}; struct flashrom_progress progress_state = { - .user_data = progress_user_data + .user_data = &cli_progress }; if (options.show_progress) flashrom_set_progress_callback(fill_flash, &flashrom_progress_cb, &progress_state); diff --git a/cli_output.c b/cli_output.c index e5b829a..20295b8 100644 --- a/cli_output.c +++ b/cli_output.c @@ -24,6 +24,14 @@ enum flashrom_log_level verbose_screen = FLASHROM_MSG_INFO; enum flashrom_log_level verbose_logfile = FLASHROM_MSG_DEBUG2;
+/* Enum to indicate what was the latest printed char prior to a progress indicator. */ +enum line_state { + NEWLINE, + MIDLINE, + PROGRESS +}; +static enum line_state line_state = NEWLINE; + static FILE *logfile = NULL;
int close_logfile(void) @@ -75,18 +83,75 @@ return "UNKNOWN"; }
+static void print_progress(const struct cli_progress *cli_progress, enum flashrom_progress_stage stage) +{ + if (!(cli_progress->visible_stages & (1 << stage))) + return; + + msg_ginfo("[%s: %2u%%]", flashrom_progress_stage_to_string(stage), cli_progress->stage_pc[stage]); +} + void flashrom_progress_cb(struct flashrom_flashctx *flashctx) { struct flashrom_progress *progress_state = flashctx->progress_state; unsigned int pc = 0; - unsigned int *percentages = progress_state->user_data; - if (progress_state->current > 0 && progress_state->total > 0) - pc = ((unsigned long long) progress_state->current * 10000llu) / - ((unsigned long long) progress_state->total * 100llu); - if (percentages[progress_state->stage] != pc) { - percentages[progress_state->stage] = pc; - msg_ginfo("[%s] %u%% complete... ", flashrom_progress_stage_to_string(progress_state->stage), pc); + struct cli_progress *cli_progress = progress_state->user_data; + + /* The expectation is that initial progress of zero is reported before doing anything. */ + if (progress_state->current == 0) { + if (!cli_progress->stage_setup) { + cli_progress->stage_setup = true; + + /* Initialization of some stage doesn't imply that it will make any progress, + * only show stages which have progressed. */ + cli_progress->visible_stages = 0; + + if (line_state != NEWLINE) { + /* We're going to clear and replace ongoing progress output, so make a new line. */ + msg_ginfo("\n"); + } + } + + cli_progress->stage_pc[progress_state->stage] = 0; + } else { + cli_progress->stage_setup = false; + cli_progress->visible_stages |= 1 << progress_state->stage; } + + if (progress_state->current > 0 && progress_state->total > 0) + pc = ((unsigned long long) progress_state->current * 100llu) / + ((unsigned long long) progress_state->total); + if (cli_progress->stage_pc[progress_state->stage] != pc) { + cli_progress->stage_pc[progress_state->stage] = pc; + + if (line_state == PROGRESS) { + /* Erase previous output, because it was previous progress step. */ + int i; + for (i = 0; i < 16 * FLASHROM_PROGRESS_NR; ++i) + msg_ginfo("\b \b"); + } else if (line_state == MIDLINE) { + /* Start with new line, to preserve some other previous message */ + msg_ginfo("\n"); + } // Remaining option is NEWLINE, which means nothing to do: newline has been printed already. + + /* The order is deliberate, the operations typically follow this sequence. */ + print_progress(cli_progress, FLASHROM_PROGRESS_READ); + print_progress(cli_progress, FLASHROM_PROGRESS_ERASE); + print_progress(cli_progress, FLASHROM_PROGRESS_WRITE); + + /* There can be output right after the progress, this acts as a separator. */ + msg_ginfo("..."); + + /* Reset the flag, because now the latest message is a progress one. */ + line_state = PROGRESS; + } +} + +static void update_line_state(const char *fmt) +{ + size_t len = strlen(fmt); + if (len > 0) + line_state = (fmt[len - 1] == '\n' ? NEWLINE : MIDLINE); }
/* Please note that level is the verbosity, not the importance of the message. */ @@ -103,6 +168,7 @@
if (level <= verbose_screen) { ret = vfprintf(output_type, fmt, ap); + update_line_state(fmt); /* msg_*spew often happens inside chip accessors in possibly * time-critical operations. Don't slow them down by flushing. */ if (level != FLASHROM_MSG_SPEW) @@ -111,6 +177,7 @@
if ((level <= verbose_logfile) && logfile) { ret = vfprintf(logfile, fmt, logfile_args); + update_line_state(fmt); if (level != FLASHROM_MSG_SPEW) fflush(logfile); } diff --git a/dediprog.c b/dediprog.c index aa3a1cf..ba1f9d6 100644 --- a/dediprog.c +++ b/dediprog.c @@ -658,7 +658,7 @@ msg_perr("SPI bulk write failed, expected %i, got %s!\n", 512, libusb_error_name(ret)); return 1; } - update_progress(flash, FLASHROM_PROGRESS_WRITE, i + 1, count); + update_progress(flash, FLASHROM_PROGRESS_WRITE, chunksize); }
return 0; diff --git a/doc/release_notes/devel.rst b/doc/release_notes/devel.rst index 919d364..c542bea 100644 --- a/doc/release_notes/devel.rst +++ b/doc/release_notes/devel.rst @@ -32,6 +32,11 @@ it. For those platforms don't support ECAM, libpci will terminate the process by exit.
+Progress display +================ + +Progress display feature is now working for all operations: read, erase, write. + Chipset support =============== Added Raptor Point PCH support. diff --git a/en29lv640b.c b/en29lv640b.c index 4d93844..b647392 100644 --- a/en29lv640b.c +++ b/en29lv640b.c @@ -48,7 +48,7 @@ #endif dst += 2; src += 2; - update_progress(flash, FLASHROM_PROGRESS_WRITE, i + 2, len); + update_progress(flash, FLASHROM_PROGRESS_WRITE, 2); }
/* FIXME: Ignore errors for now. */ diff --git a/erasure_layout.c b/erasure_layout.c index 7f3bb46..8b1a26f 100644 --- a/erasure_layout.c +++ b/erasure_layout.c @@ -304,6 +304,8 @@ msg_cerr("ERASE FAILED!\n"); }
+ update_progress(flashctx, FLASHROM_PROGRESS_ERASE, block_len); + // adjust curcontents memset(curcontents+start_addr, erased_value, block_len); // after erase make it unselected again diff --git a/flashrom.c b/flashrom.c index e2c60f4..88fa5d9 100644 --- a/flashrom.c +++ b/flashrom.c @@ -1225,6 +1225,24 @@ return chip - flashchips; }
+static void setup_progress_from_layout(struct flashctx *flashctx, + enum flashrom_progress_stage stage) +{ + if (!flashctx->progress_callback) + return; + + const struct flashrom_layout *const flash_layout = get_layout(flashctx); + + size_t total = 0; + const struct romentry *entry = NULL; + while ((entry = layout_next_included(flash_layout, entry))) { + const struct flash_region *region = &entry->region; + total += region->end - region->start + 1; + } + + init_progress(flashctx, stage, total); +} + /** * @brief Reads the included layout regions into a buffer. * @@ -1241,6 +1259,8 @@ const struct flashrom_layout *const layout = get_layout(flashctx); const struct romentry *entry = NULL;
+ setup_progress_from_layout(flashctx, FLASHROM_PROGRESS_READ); + while ((entry = layout_next_included(layout, entry))) { const struct flash_region *region = &entry->region; const chipoff_t region_start = region->start; @@ -1353,6 +1373,9 @@ memset(curcontents, ~ERASED_VALUE(flashctx), flash_size); memset(newcontents, ERASED_VALUE(flashctx), flash_size);
+ setup_progress_from_layout(flashctx, FLASHROM_PROGRESS_READ); + setup_progress_from_layout(flashctx, FLASHROM_PROGRESS_ERASE); + const struct flashrom_layout *const flash_layout = get_layout(flashctx); const struct romentry *entry = NULL; while ((entry = layout_next_included(flash_layout, entry))) { @@ -1389,6 +1412,10 @@ goto _ret; }
+ setup_progress_from_layout(flashctx, FLASHROM_PROGRESS_READ); + setup_progress_from_layout(flashctx, FLASHROM_PROGRESS_WRITE); + setup_progress_from_layout(flashctx, FLASHROM_PROGRESS_ERASE); + const struct romentry *entry = NULL; while ((entry = layout_next_included(flash_layout, entry))) { ret = erase_write(flashctx, entry->region.start, @@ -1427,6 +1454,8 @@ { const struct romentry *entry = NULL;
+ setup_progress_from_layout(flashctx, FLASHROM_PROGRESS_READ); + while ((entry = layout_next_included(layout, entry))) { const struct flash_region *region = &entry->region; const chipoff_t region_start = region->start; @@ -1924,6 +1953,7 @@ */ msg_cinfo("Reading old flash chip contents... "); if (verify_all) { + init_progress(flashctx, FLASHROM_PROGRESS_READ, flash_size); if (read_flash(flashctx, oldcontents, 0, flash_size)) { msg_cinfo("FAILED.\n"); goto _finalize_ret; @@ -1940,12 +1970,14 @@
bool all_skipped = true;
+ msg_cinfo("Updating flash chip contents... "); if (write_by_layout(flashctx, curcontents, newcontents, &all_skipped)) { msg_cerr("Uh oh. Erase/write failed. "); ret = 2; if (verify_all) { msg_cerr("Checking if anything has changed.\n"); msg_cinfo("Reading current flash chip contents... "); + init_progress(flashctx, FLASHROM_PROGRESS_READ, flash_size); if (!read_flash(flashctx, curcontents, 0, flash_size)) { msg_cinfo("done.\n"); if (!memcmp(oldcontents, curcontents, flash_size)) { diff --git a/include/flash.h b/include/flash.h index d0e55af..60c099c 100644 --- a/include/flash.h +++ b/include/flash.h @@ -549,6 +549,11 @@ typedef int (blockprotect_func_t)(struct flashctx *flash); blockprotect_func_t *lookup_blockprotect_func_ptr(const struct flashchip *const chip);
+struct stage_progress { + size_t current; + size_t total; +}; + struct flashrom_flashctx { struct flashchip *chip; /* FIXME: The memory mappings should be saved in a more structured way. */ @@ -587,6 +592,7 @@ /* Progress reporting */ flashrom_progress_callback *progress_callback; struct flashrom_progress *progress_state; + struct stage_progress stage_progress[FLASHROM_PROGRESS_NR]; };
/* Timing used in probe routines. ZERO is -2 to differentiate between an unset @@ -677,6 +683,12 @@ */ #define ERROR_FLASHROM_LIMIT -201
+struct cli_progress { + unsigned int stage_pc[FLASHROM_PROGRESS_NR]; + unsigned int visible_stages; /* Bitmask of stages with non-zero progress. */ + bool stage_setup; /* Flag to know when to reset progress data. */ +}; + /* cli_common.c */ void print_chip_support_status(const struct flashchip *chip);
@@ -716,7 +728,8 @@ #define msg_gspew(...) print(FLASHROM_MSG_SPEW, __VA_ARGS__) /* general debug spew */ #define msg_pspew(...) print(FLASHROM_MSG_SPEW, __VA_ARGS__) /* programmer debug spew */ #define msg_cspew(...) print(FLASHROM_MSG_SPEW, __VA_ARGS__) /* chip debug spew */ -void update_progress(struct flashctx *flash, enum flashrom_progress_stage stage, size_t current, size_t total); +void init_progress(struct flashctx *flash, enum flashrom_progress_stage stage, size_t total); +void update_progress(struct flashctx *flash, enum flashrom_progress_stage stage, size_t increment);
/* spi.c */ struct spi_command { @@ -730,4 +743,5 @@ int spi_send_multicommand(const struct flashctx *flash, struct spi_command *cmds);
enum chipbustype get_buses_supported(void); + #endif /* !__FLASH_H__ */ diff --git a/it87spi.c b/it87spi.c index 9c64659..fddae3b 100644 --- a/it87spi.c +++ b/it87spi.c @@ -292,7 +292,7 @@ int ret = it8716f_spi_page_program(flash, buf, start); if (ret) return ret; - update_progress(flash, FLASHROM_PROGRESS_WRITE, chip->page_size - len, chip->page_size); + update_progress(flash, FLASHROM_PROGRESS_WRITE, chip->page_size); start += chip->page_size; len -= chip->page_size; buf += chip->page_size; diff --git a/jedec.c b/jedec.c index 2d18de7..df0d93a 100644 --- a/jedec.c +++ b/jedec.c @@ -382,7 +382,7 @@ if (write_byte_program_jedec_common(flash, src, dst)) failed = 1; dst++, src++; - update_progress(flash, FLASHROM_PROGRESS_WRITE, i + 1, len); + update_progress(flash, FLASHROM_PROGRESS_WRITE, 1); } if (failed) msg_cerr(" writing sector at 0x%" PRIxPTR " failed!\n", olddst); @@ -467,7 +467,7 @@
if (jedec_write_page(flash, buf + starthere - start, starthere, lenhere)) return 1; - update_progress(flash, FLASHROM_PROGRESS_WRITE, i + 1, nwrites + 1); + update_progress(flash, FLASHROM_PROGRESS_WRITE, lenhere); }
return 0; diff --git a/libflashrom.c b/libflashrom.c index 7c885cf..dfc86e6 100644 --- a/libflashrom.c +++ b/libflashrom.c @@ -70,16 +70,36 @@ flashctx->progress_state = progress_state; } /** @private */ -void update_progress(struct flashrom_flashctx *flashctx, enum flashrom_progress_stage stage, size_t current, size_t total) +void init_progress(struct flashrom_flashctx *flashctx, enum flashrom_progress_stage stage, size_t total) { if (flashctx->progress_callback == NULL) return; - if (current > total) - current = total; + + struct stage_progress *stage_progress = &flashctx->stage_progress[stage]; + stage_progress->current = 0; + stage_progress->total = total; + + /* This is used to trigger callback invocation, with 0 current state and 0 increment: as an init call. */ + update_progress(flashctx, stage, 0); +} +/** @private */ +void update_progress(struct flashrom_flashctx *flashctx, enum flashrom_progress_stage stage, size_t increment) +{ + if (flashctx->progress_callback == NULL) + return; + + struct stage_progress *stage_progress = &flashctx->stage_progress[stage]; + + stage_progress->current += increment; + if (stage_progress->current > stage_progress->total) { + msg_gwarn("Fixing total value of stage %d progress on the fly.", stage); + stage_progress->total = stage_progress->current; + }
flashctx->progress_state->stage = stage; - flashctx->progress_state->current = current; - flashctx->progress_state->total = total; + flashctx->progress_state->current = stage_progress->current; + flashctx->progress_state->total = stage_progress->total; + flashctx->progress_callback(flashctx); }
diff --git a/linux_mtd.c b/linux_mtd.c index eea0cf2..d5fc509 100644 --- a/linux_mtd.c +++ b/linux_mtd.c @@ -215,7 +215,7 @@ }
i += step; - update_progress(flash, FLASHROM_PROGRESS_READ, i, len); + update_progress(flash, FLASHROM_PROGRESS_READ, step); }
return 0; @@ -258,7 +258,7 @@ }
i += step; - update_progress(flash, FLASHROM_PROGRESS_WRITE, i, len); + update_progress(flash, FLASHROM_PROGRESS_WRITE, step); }
return 0; @@ -295,7 +295,6 @@ __func__, ret, strerror(errno)); return 1; } - update_progress(flash, FLASHROM_PROGRESS_ERASE, u + data->erasesize, len); }
return 0; diff --git a/parade_lspcon.c b/parade_lspcon.c index dfcf659..16283e7 100644 --- a/parade_lspcon.c +++ b/parade_lspcon.c @@ -354,7 +354,7 @@ for (i = 0; i < len; i += TUNNEL_PAGE_SIZE) { ret |= parade_lspcon_map_page(fd, start + i); ret |= parade_lspcon_read_data(fd, PAGE_ADDRESS, buf + i, min(len - i, TUNNEL_PAGE_SIZE)); - update_progress(flash, FLASHROM_PROGRESS_READ, i + TUNNEL_PAGE_SIZE, len); + update_progress(flash, FLASHROM_PROGRESS_READ, TUNNEL_PAGE_SIZE); }
return ret; @@ -395,7 +395,7 @@ for (unsigned int i = 0; i < len; i += TUNNEL_PAGE_SIZE) { ret |= parade_lspcon_map_page(fd, start + i); ret |= parade_lspcon_write_page(fd, buf + i, min(len - i, TUNNEL_PAGE_SIZE)); - update_progress(flash, FLASHROM_PROGRESS_WRITE, i + TUNNEL_PAGE_SIZE, len); + update_progress(flash, FLASHROM_PROGRESS_WRITE, TUNNEL_PAGE_SIZE); }
ret |= parade_lspcon_enable_write_protection(fd); diff --git a/realtek_mst_i2c_spi.c b/realtek_mst_i2c_spi.c index 57677ec..3a89eab 100644 --- a/realtek_mst_i2c_spi.c +++ b/realtek_mst_i2c_spi.c @@ -397,7 +397,7 @@ ret |= realtek_mst_i2c_execute_write(fd); if (ret) break; - update_progress(flash, FLASHROM_PROGRESS_WRITE, i + RTK_PAGE_SIZE, len); + update_progress(flash, FLASHROM_PROGRESS_WRITE, page_len); }
return ret; diff --git a/spi.c b/spi.c index 3e81da2..b2ffc33 100644 --- a/spi.c +++ b/spi.c @@ -104,8 +104,6 @@ { int ret; size_t to_read; - size_t start_address = start; - size_t end_address = len - start; for (; len; len -= to_read, buf += to_read, start += to_read) { /* Do not cross 16MiB boundaries in a single transfer. This helps with @@ -115,7 +113,6 @@ ret = flash->mst->spi.read(flash, buf, start, to_read); if (ret) return ret; - update_progress(flash, FLASHROM_PROGRESS_READ, start - start_address + to_read, end_address); } return 0; } diff --git a/spi25.c b/spi25.c index 6a6ee75..ab479e5 100644 --- a/spi25.c +++ b/spi25.c @@ -681,14 +681,12 @@ { int ret; size_t to_read; - size_t start_address = start; - size_t end_address = len - start; for (; len; len -= to_read, buf += to_read, start += to_read) { to_read = min(chunksize, len); ret = spi_nbyte_read(flash, start, buf, to_read); if (ret) return ret; - update_progress(flash, FLASHROM_PROGRESS_READ, start - start_address + to_read, end_address); + update_progress(flash, FLASHROM_PROGRESS_READ, to_read); } return 0; } @@ -708,8 +706,6 @@ * we're OK for now. */ unsigned int page_size = flash->chip->page_size; - size_t start_address = start; - size_t end_address = len - start;
/* Warning: This loop has a very unusual condition and body. * The loop needs to go through each page with at least one affected @@ -733,8 +729,9 @@ rc = spi_nbyte_program(flash, starthere + j, buf + starthere - start + j, towrite); if (rc) return rc; + + update_progress(flash, FLASHROM_PROGRESS_WRITE, towrite); } - update_progress(flash, FLASHROM_PROGRESS_WRITE, start - start_address + lenhere, end_address); }
return 0; @@ -754,7 +751,7 @@ for (i = start; i < start + len; i++) { if (spi_nbyte_program(flash, i, buf + i - start, 1)) return 1; - update_progress(flash, FLASHROM_PROGRESS_WRITE, i - start, len - start); + update_progress(flash, FLASHROM_PROGRESS_WRITE, 1); } return 0; } diff --git a/sst28sf040.c b/sst28sf040.c index 9080684..e6a4b36 100644 --- a/sst28sf040.c +++ b/sst28sf040.c @@ -92,7 +92,7 @@
/* wait for Toggle bit ready */ toggle_ready_jedec(flash, bios); - update_progress(flash, FLASHROM_PROGRESS_WRITE, i + 1, len); + update_progress(flash, FLASHROM_PROGRESS_WRITE, 1); }
return 0; diff --git a/tests/chip.c b/tests/chip.c index 604d79e..ac60663 100644 --- a/tests/chip.c +++ b/tests/chip.c @@ -46,6 +46,10 @@ .buf = { 0 }, };
+struct progress_user_data { + size_t last_seen; /* % of progress last reported, to be asserted in the progress callback. */ +}; + static int read_chip(struct flashctx *flash, uint8_t *buf, unsigned int start, unsigned int len) { printf("Read chip called with start=0x%x, len=0x%x\n", start, len); @@ -76,6 +80,21 @@ return 0; }
+static void progress_callback(struct flashctx *flash) { + struct progress_user_data *progress_user_data = flash->progress_state->user_data; + if (flash->progress_state->current == 0) { + printf("Progress started for stage %d, initial callback call\n", flash->progress_state->stage); + } else { + /* Progress cannot go backwards. */ + assert_true(flash->progress_state->current >= progress_user_data->last_seen); + } + + if (flash->progress_state->current >= flash->progress_state->total) + printf("Progress complete for stage %d, final callback call\n", flash->progress_state->stage); + + progress_user_data->last_seen = flash->progress_state->current; +} + static void setup_chip(struct flashrom_flashctx *flashctx, struct flashrom_layout **layout, struct flashchip *chip, const char *programmer_param, const struct io_mock *io) { @@ -210,6 +229,41 @@ teardown(&layout); }
+void erase_chip_with_progress(void **state) +{ + (void) state; /* unused */ + + static struct io_mock_fallback_open_state data = { + .noc = 0, + .paths = { NULL }, + }; + const struct io_mock chip_io = { + .fallback_open_state = &data, + }; + + g_test_write_injector = write_chip; + g_test_read_injector = read_chip; + g_test_erase_injector[0] = block_erase_chip; + struct flashrom_flashctx flashctx = { 0 }; + struct flashrom_layout *layout; + struct flashchip mock_chip = chip_8MiB; + const char *param = ""; /* Default values for all params. */ + + setup_chip(&flashctx, &layout, &mock_chip, param, &chip_io); + + struct progress_user_data progress_user_data = {0}; + struct flashrom_progress progress_state = { + .user_data = &progress_user_data + }; + flashrom_set_progress_callback(&flashctx, progress_callback, &progress_state); + + printf("Erase chip operation started.\n"); + assert_int_equal(0, flashrom_flash_erase(&flashctx)); + printf("Erase chip operation done.\n"); + + teardown(&layout); +} + void erase_chip_with_dummyflasher_test_success(void **state) { (void) state; /* unused */ @@ -277,6 +331,49 @@ free(buf); }
+void read_chip_with_progress(void **state) +{ + (void) state; /* unused */ + + static struct io_mock_fallback_open_state data = { + .noc = 0, + .paths = { NULL }, + }; + const struct io_mock chip_io = { + .fallback_open_state = &data, + }; + + g_test_write_injector = write_chip; + g_test_read_injector = read_chip; + g_test_erase_injector[0] = block_erase_chip; + struct flashrom_flashctx flashctx = { 0 }; + struct flashrom_layout *layout; + struct flashchip mock_chip = chip_8MiB; + const char *param = ""; /* Default values for all params. */ + + setup_chip(&flashctx, &layout, &mock_chip, param, &chip_io); + + struct progress_user_data progress_user_data = {0}; + struct flashrom_progress progress_state = { + .user_data = &progress_user_data + }; + flashrom_set_progress_callback(&flashctx, progress_callback, &progress_state); + + const char *const filename = "read_chip.test"; + unsigned long size = mock_chip.total_size * 1024; + unsigned char *buf = calloc(size, sizeof(unsigned char)); + assert_non_null(buf); + + printf("Read chip operation started.\n"); + assert_int_equal(0, flashrom_image_read(&flashctx, buf, size)); + assert_int_equal(0, write_buf_to_file(buf, size, filename)); + printf("Read chip operation done.\n"); + + teardown(&layout); + + free(buf); +} + void read_chip_with_dummyflasher_test_success(void **state) { (void) state; /* unused */ @@ -365,6 +462,49 @@ free(newcontents); }
+void write_chip_with_progress(void **state) +{ + (void) state; /* unused */ + + static struct io_mock_fallback_open_state data = { + .noc = 0, + .paths = { NULL }, + }; + const struct io_mock chip_io = { + .fallback_open_state = &data, + }; + + g_test_write_injector = write_chip; + g_test_read_injector = read_chip; + g_test_erase_injector[0] = block_erase_chip; + struct flashrom_flashctx flashctx = { 0 }; + struct flashrom_layout *layout; + struct flashchip mock_chip = chip_8MiB; + const char *param = ""; /* Default values for all params. */ + + setup_chip(&flashctx, &layout, &mock_chip, param, &chip_io); + + struct progress_user_data progress_user_data = {0}; + struct flashrom_progress progress_state = { + .user_data = &progress_user_data + }; + flashrom_set_progress_callback(&flashctx, progress_callback, &progress_state); + + const char *const filename = "-"; + unsigned long size = mock_chip.total_size * 1024; + uint8_t *const newcontents = malloc(size); + assert_non_null(newcontents); + + printf("Write chip operation started.\n"); + assert_int_equal(0, read_buf_from_file(newcontents, size, filename)); + assert_int_equal(0, flashrom_image_write(&flashctx, newcontents, size, NULL)); + printf("Write chip operation done.\n"); + + teardown(&layout); + + free(newcontents); +} + void write_chip_with_dummyflasher_test_success(void **state) { (void) state; /* unused */ diff --git a/tests/spi25.c b/tests/spi25.c index 8729593..a442c7b 100644 --- a/tests/spi25.c +++ b/tests/spi25.c @@ -55,7 +55,9 @@ */ return __real_spi_send_command(flash, writecnt, readcnt, writearr, readarr);
- check_expected_ptr(flash); + if (!flash->progress_callback) + check_expected_ptr(flash); + assert_int_equal(writecnt, mock_type(int)); assert_int_equal(writearr[0], mock_type(int));
@@ -71,22 +73,22 @@ { struct flashrom_progress *progress_state = flashctx->progress_state; uint32_t *cnt = (uint32_t *) progress_state->user_data; - assert_int_equal(0x300, progress_state->total); + assert_int_equal(0x400, progress_state->total); switch (*cnt) { case 0: - assert_int_equal(0x100, progress_state->current); + assert_int_equal(0x0, progress_state->current); break; case 1: - assert_int_equal(0x200, progress_state->current); + assert_int_equal(0x100, progress_state->current); break; case 2: - assert_int_equal(0x300, progress_state->current); + assert_int_equal(0x200, progress_state->current); break; case 3: assert_int_equal(0x300, progress_state->current); break; case 4: - assert_int_equal(0x300, progress_state->current); + assert_int_equal(0x400, progress_state->current); break; default: fail(); @@ -94,7 +96,7 @@ (*cnt)++; }
-void spi_read_chunked_test_success(void **state) +void default_spi_read_test_success(void **state) { (void) state; /* unused */ uint8_t buf[0x400] = { 0x0 }; @@ -115,15 +117,16 @@ .user_data = (void *) &cnt, }; flashrom_set_progress_callback(&flashctx, spi_read_progress_cb, &progress_state); + init_progress(&flashctx, FLASHROM_PROGRESS_READ, 0x400); for (int i = 0; i < 4; i++) { - expect_memory(__wrap_spi_send_command, flash, - &flashctx, sizeof(flashctx)); will_return(__wrap_spi_send_command, JEDEC_WRDI); will_return(__wrap_spi_send_command, JEDEC_READ); will_return(__wrap_spi_send_command, max_data_read); } - assert_int_equal(0, spi_chip_read(&flashctx, buf, offset, sizeof(buf))); + assert_int_equal(0, default_spi_read(&flashctx, buf, offset, sizeof(buf))); assert_int_equal(5, cnt); + + flashrom_set_progress_callback(&flashctx, NULL, NULL); }
void spi_write_enable_test_success(void **state) diff --git a/tests/tests.c b/tests/tests.c index 3fee056..00b4665 100644 --- a/tests/tests.c +++ b/tests/tests.c @@ -441,7 +441,7 @@ const struct CMUnitTest spi25_tests[] = { cmocka_unit_test(spi_write_enable_test_success), cmocka_unit_test(spi_write_disable_test_success), - cmocka_unit_test(spi_read_chunked_test_success), + cmocka_unit_test(default_spi_read_test_success), cmocka_unit_test(probe_spi_rdid_test_success), cmocka_unit_test(probe_spi_rdid4_test_success), cmocka_unit_test(probe_spi_rems_test_success), @@ -495,10 +495,13 @@
const struct CMUnitTest chip_tests[] = { cmocka_unit_test(erase_chip_test_success), + cmocka_unit_test(erase_chip_with_progress), cmocka_unit_test(erase_chip_with_dummyflasher_test_success), cmocka_unit_test(read_chip_test_success), + cmocka_unit_test(read_chip_with_progress), cmocka_unit_test(read_chip_with_dummyflasher_test_success), cmocka_unit_test(write_chip_test_success), + cmocka_unit_test(write_chip_with_progress), cmocka_unit_test(write_chip_with_dummyflasher_test_success), cmocka_unit_test(write_chip_feature_no_erase), cmocka_unit_test(write_nonaligned_region_with_dummyflasher_test_success), diff --git a/tests/tests.h b/tests/tests.h index 100bda1..dba33fb 100644 --- a/tests/tests.h +++ b/tests/tests.h @@ -34,7 +34,7 @@ /* spi25.c */ void spi_write_enable_test_success(void **state); void spi_write_disable_test_success(void **state); -void spi_read_chunked_test_success(void **state); +void default_spi_read_test_success(void **state); void probe_spi_rdid_test_success(void **state); void probe_spi_rdid4_test_success(void **state); void probe_spi_rems_test_success(void **state); @@ -82,10 +82,13 @@
/* chip.c */ void erase_chip_test_success(void **state); +void erase_chip_with_progress(void **state); void erase_chip_with_dummyflasher_test_success(void **state); void read_chip_test_success(void **state); +void read_chip_with_progress(void **state); void read_chip_with_dummyflasher_test_success(void **state); void write_chip_test_success(void **state); +void write_chip_with_progress(void **state); void write_chip_with_dummyflasher_test_success(void **state); void write_chip_feature_no_erase(void **state); void write_nonaligned_region_with_dummyflasher_test_success(void **state);
-
Anastasia Klimchuk (Code Review)