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>

82802ab.c

@@ -135,7 +135,7 @@ int write_82802ab(struct flashctx *flash, const uint8_t *src, unsigned int start
 		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. */

at45db.c

@@ -551,7 +551,7 @@ int spi_write_at45db(struct flashctx *flash, const uint8_t *buf, unsigned int st
 			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;
 }

cli_classic.c

@@ -1060,9 +1060,9 @@ int main(int argc, char *argv[])
 
 	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);

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 @@ static const char *flashrom_progress_stage_to_string(enum flashrom_progress_stag
 	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;
+	struct cli_progress *cli_progress = progress_state->user_data;
-	if (progress_state->current > 0 && progress_state->total > 0)
+
-		pc = ((unsigned long long) progress_state->current * 10000llu) /
+	/* The expectation is that initial progress of zero is reported before doing anything. */
-		     ((unsigned long long) progress_state->total * 100llu);
+	if (progress_state->current == 0) {
-	if (percentages[progress_state->stage] != pc) {
+		if (!cli_progress->stage_setup) {
-		percentages[progress_state->stage] = pc;
+			cli_progress->stage_setup = true;
-		msg_ginfo("[%s] %u%% complete... ", flashrom_progress_stage_to_string(progress_state->stage), pc);
+
+			/* 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 @@ int flashrom_print_cb(enum flashrom_log_level level, const char *fmt, va_list ap
 
 	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 @@ int flashrom_print_cb(enum flashrom_log_level level, const char *fmt, va_list ap
 
 	if ((level <= verbose_logfile) && logfile) {
 		ret = vfprintf(logfile, fmt, logfile_args);
+		update_line_state(fmt);
 		if (level != FLASHROM_MSG_SPEW)
 			fflush(logfile);
 	}

dediprog.c

@@ -658,7 +658,7 @@ static int dediprog_spi_bulk_write(struct flashctx *flash, const uint8_t *buf, u
 			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;

doc/release_notes/devel.rst

@@ -32,6 +32,11 @@ The ECAM has been supported for a very long time, most platforms should support
 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.

en29lv640b.c

@@ -48,7 +48,7 @@ int write_en29lv640b(struct flashctx *flash, const uint8_t *src, unsigned int st
 #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. */

erasure_layout.c

@@ -304,6 +304,8 @@ static int erase_write_helper(struct flashctx *const flashctx, chipoff_t region_
 				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

flashrom.c

@@ -1225,6 +1225,24 @@ notfound:
 	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 @@ static int read_by_layout(struct flashctx *const flashctx, uint8_t *const buffer
 	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 @@ static int erase_by_layout(struct flashctx *const flashctx)
 	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 @@ static int write_by_layout(struct flashctx *const flashctx,
 		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 @@ static int verify_by_layout(
 {
 	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 @@ int flashrom_image_write(struct flashctx *const flashctx, void *const buffer, co
 		 */
 		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 @@ int flashrom_image_write(struct flashctx *const flashctx, void *const buffer, co
 
 	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)) {

include/flash.h

@@ -549,6 +549,11 @@ typedef int (*chip_restore_fn_cb_t)(struct flashctx *flash, void *data);
 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 @@ struct flashrom_flashctx {
 	/* 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 @@ int write_flash(struct flashctx *flash, const uint8_t *buf, unsigned int start,
  */
 #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 @@ __attribute__((format(printf, 2, 3)));
 #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_command(const struct flashctx *flash, unsigned int writecnt, unsign
 int spi_send_multicommand(const struct flashctx *flash, struct spi_command *cmds);
 
 enum chipbustype get_buses_supported(void);
+
 #endif				/* !__FLASH_H__ */

it87spi.c

@@ -292,7 +292,7 @@ static int it8716f_spi_chip_write_256(struct flashctx *flash, const uint8_t *buf
 			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;

jedec.c

@@ -382,7 +382,7 @@ int write_jedec_1(struct flashctx *flash, const uint8_t *src, unsigned int start
 		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 @@ int write_jedec(struct flashctx *flash, const uint8_t *buf, unsigned int start,
 
 		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;

libflashrom.c

@@ -70,16 +70,36 @@ void flashrom_set_progress_callback(struct flashrom_flashctx *flashctx, flashrom
 	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->current = stage_progress->current;
-	flashctx->progress_state->total = total;
+	flashctx->progress_state->total = stage_progress->total;
+
 	flashctx->progress_callback(flashctx);
 }
 

linux_mtd.c

@@ -215,7 +215,7 @@ static int linux_mtd_read(struct flashctx *flash, uint8_t *buf,
 		}
 
 		i += step;
-		update_progress(flash, FLASHROM_PROGRESS_READ, i, len);
+		update_progress(flash, FLASHROM_PROGRESS_READ, step);
 	}
 
 	return 0;
@@ -258,7 +258,7 @@ static int linux_mtd_write(struct flashctx *flash, const uint8_t *buf,
 		}
 
 		i += step;
-		update_progress(flash, FLASHROM_PROGRESS_WRITE, i, len);
+		update_progress(flash, FLASHROM_PROGRESS_WRITE, step);
 	}
 
 	return 0;
@@ -295,7 +295,6 @@ static int linux_mtd_erase(struct flashctx *flash,
 		                 __func__, ret, strerror(errno));
 		        return 1;
 		}
-		update_progress(flash, FLASHROM_PROGRESS_ERASE, u + data->erasesize, len);
 	}
 
 	return 0;

parade_lspcon.c

@@ -354,7 +354,7 @@ static int parade_lspcon_read(struct flashctx *flash, uint8_t *buf,
 	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 @@ static int parade_lspcon_write_256(struct flashctx *flash, const uint8_t *buf,
 	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);

realtek_mst_i2c_spi.c

@@ -397,7 +397,7 @@ static int realtek_mst_i2c_spi_write_256(struct flashctx *flash, const uint8_t *
 		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;

spi.c

@@ -104,8 +104,6 @@ int spi_chip_read(struct flashctx *flash, uint8_t *buf, unsigned int start,
 {
 	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 @@ int spi_chip_read(struct flashctx *flash, uint8_t *buf, unsigned int start,
 		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;
 }

spi25.c

@@ -681,14 +681,12 @@ int spi_read_chunked(struct flashctx *flash, uint8_t *buf, unsigned int start,
 {
 	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 @@ int spi_write_chunked(struct flashctx *flash, const uint8_t *buf, unsigned int s
 	 * 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 @@ int spi_write_chunked(struct flashctx *flash, const uint8_t *buf, unsigned int s
 			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 @@ int spi_chip_write_1(struct flashctx *flash, const uint8_t *buf, unsigned int st
 	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;
 }

sst28sf040.c

@@ -92,7 +92,7 @@ int write_28sf040(struct flashctx *flash, const uint8_t *src, unsigned int start
 
 		/* 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;

tests/chip.c

@@ -46,6 +46,10 @@ static struct {
 	.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 @@ static int block_erase_chip(struct flashctx *flash, unsigned int blockaddr, unsi
 	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 @@ void erase_chip_test_success(void **state)
 	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 @@ void read_chip_test_success(void **state)
 	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 @@ void write_chip_test_success(void **state)
 	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 */

tests/spi25.c

@@ -55,7 +55,9 @@ int __wrap_spi_send_command(const struct flashctx *flash,
 		*/
 		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 @@ static void spi_read_progress_cb(struct flashrom_flashctx *flashctx)
 {
 	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 @@ static void spi_read_progress_cb(struct flashrom_flashctx *flashctx)
 	(*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 @@ void spi_read_chunked_test_success(void **state)
 		.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)

tests/tests.c

@@ -441,7 +441,7 @@ int main(int argc, char *argv[])
 	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 @@ int main(int argc, char *argv[])
 
 	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),

tests/tests.h

@@ -34,7 +34,7 @@ void flashbuses_to_text_test_success(void **state);
 /* 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 @@ void layout_region_invalid_range_test_success(void **state);
 
 /* 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);