Felix Singer submitted this change.

View Change

Revert "libflashrom: Return progress state to the library user"

This reverts commit 40892b0c08fbc8029921e91511dd3f91fc956f90.

The feature of returning progress for libflashrom users was
introduced in original commit, however later a bug was found and
reported as https://ticket.coreboot.org/issues/390.

Reverting in a release branch to unblock release candidate, since
it is unknown how much time needed to fix the bug. Meanwhile the
feature remains in a master branch and will be fixed under
ticket 390.

TEST=scenarios below run successfully
1) flashrom -h does not show --progress
2) flashrom -p dummy:emulate=W25Q128FV -r /tmp/dump.bin
3) flashrom -p dummy:emulate=W25Q128FV -v /tmp/dump.bin
4) flashrom -p dummy:emulate=W25Q128FV -E
5) head -c 16777216 </dev/urandom >/tmp/image.bin
flashrom -p dummy:image=/tmp/image.bin,emulate=W25Q128FV \
	 -w /tmp/dump.bin

Change-Id: Id3d7ffcaf266a60a44eb453fd09b7c63c05349c2
Signed-off-by: Edward O'Callaghan <quasisec@google.com>
Signed-off-by: Anastasia Klimchuk <aklm@chromium.org>
Reviewed-on: https://review.coreboot.org/c/flashrom/+/69283
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Felix Singer <felixsinger@posteo.net>
---
M 82802ab.c
M at45db.c
M cli_classic.c
M cli_output.c
M dediprog.c
M en29lv640b.c
M flashrom.8.tmpl
M include/flash.h
M include/libflashrom.h
M it87spi.c
M jedec.c
M libflashrom.c
M libflashrom.map
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/spi25.c
M tests/tests.c
M tests/tests.h
22 files changed, 37 insertions(+), 184 deletions(-)

diff --git a/82802ab.c b/82802ab.c
index 90c9cf0..f2f2707 100644
--- a/82802ab.c
+++ b/82802ab.c
@@ -135,7 +135,6 @@
 		chip_writeb(flash, 0x40, dst);
 		chip_writeb(flash, *src++, dst++);
 		wait_82802ab(flash);
-		update_progress(flash, FLASHROM_PROGRESS_WRITE, i + 1, len);
 	}
 
 	/* FIXME: Ignore errors for now. */
diff --git a/at45db.c b/at45db.c
index 992f3a4..074d924 100644
--- a/at45db.c
+++ b/at45db.c
@@ -550,7 +550,6 @@
 			msg_cerr("Writing page %u failed!\n", i);
 			return 1;
 		}
-		update_progress(flash, FLASHROM_PROGRESS_WRITE, i + page_size, len);
 	}
 	return 0;
 }
diff --git a/cli_classic.c b/cli_classic.c
index b66094c..a77422e 100644
--- a/cli_classic.c
+++ b/cli_classic.c
@@ -79,7 +79,6 @@
 #if CONFIG_PRINT_WIKI == 1
 	       " -z | --list-supported-wiki         print supported devices in wiki syntax\n"
 #endif
-	       "      --progress                    show progress percentage on the standard output\n"
 	       " -p | --programmer <name>[:<param>] specify the programmer device. One of\n");
 	list_programmers_linebreak(4, 80, 0);
 	printf(".\n\nYou can specify one of -h, -R, -L, "
@@ -580,7 +579,6 @@
 	bool read_it = false, extract_it = false, write_it = false, erase_it = false, verify_it = false;
 	bool dont_verify_it = false, dont_verify_all = false;
 	bool list_supported = false;
-	bool show_progress = false;
 	struct flashrom_layout *layout = NULL;
 	static const struct programmer_entry *prog = NULL;
 	enum {
@@ -596,7 +594,6 @@
 		OPTION_WP_ENABLE,
 		OPTION_WP_DISABLE,
 		OPTION_WP_LIST,
-		OPTION_PROGRESS,
 	};
 	int ret = 0;
 
@@ -633,7 +630,6 @@
 		{"help",		0, NULL, 'h'},
 		{"version",		0, NULL, 'R'},
 		{"output",		1, NULL, 'o'},
-		{"progress",		0, NULL, OPTION_PROGRESS},
 		{NULL,			0, NULL, 0},
 	};
 
@@ -874,9 +870,6 @@
 				cli_classic_abort_usage("No log filename specified.\n");
 			}
 			break;
-		case OPTION_PROGRESS:
-			show_progress = true;
-			break;
 		default:
 			cli_classic_abort_usage(NULL);
 			break;
@@ -1048,13 +1041,6 @@
 
 	fill_flash = &flashes[0];
 
-	unsigned int progress_user_data[FLASHROM_PROGRESS_NR];
-	struct flashrom_progress progress_state = {
-		 .user_data = progress_user_data
-	};
-	if (show_progress)
-		flashrom_set_progress_callback(fill_flash, &flashrom_progress_cb, &progress_state);
-
 	print_chip_support_status(fill_flash->chip);
 
 	unsigned int limitexceeded = count_max_decode_exceedings(fill_flash);
diff --git a/cli_output.c b/cli_output.c
index e5b829a..629db67 100644
--- a/cli_output.c
+++ b/cli_output.c
@@ -64,31 +64,6 @@
 	verbose_screen = oldverbose_screen;
 }
 
-static const char *flashrom_progress_stage_to_string(enum flashrom_progress_stage stage)
-{
-	if (stage == FLASHROM_PROGRESS_READ)
-		return "READ";
-	if (stage == FLASHROM_PROGRESS_WRITE)
-		return "WRITE";
-	if (stage == FLASHROM_PROGRESS_ERASE)
-		return "ERASE";
-	return "UNKNOWN";
-}
-
-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);
-	}
-}
-
 /* Please note that level is the verbosity, not the importance of the message. */
 int flashrom_print_cb(enum flashrom_log_level level, const char *fmt, va_list ap)
 {
diff --git a/dediprog.c b/dediprog.c
index b4279d8..2117451 100644
--- a/dediprog.c
+++ b/dediprog.c
@@ -658,7 +658,6 @@
 			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);
 	}
 
 	return 0;
diff --git a/en29lv640b.c b/en29lv640b.c
index 8a8d641..07d8624 100644
--- a/en29lv640b.c
+++ b/en29lv640b.c
@@ -48,7 +48,6 @@
 #endif
 		dst += 2;
 		src += 2;
-		update_progress(flash, FLASHROM_PROGRESS_WRITE, i + 2, len);
 	}
 
 	/* FIXME: Ignore errors for now. */
diff --git a/flashrom.8.tmpl b/flashrom.8.tmpl
index e672869..b977908 100644
--- a/flashrom.8.tmpl
+++ b/flashrom.8.tmpl
@@ -53,8 +53,7 @@
              [\fB\-\-wp\-status\fR] [\fB\-\-wp\-list\fR] [\fB\-\-wp\-enable\fR|\fB\-\-wp\-disable\fR]
              [\fB\-\-wp\-range\fR <start>,<length>|\fB\-\-wp\-region\fR <region>]
              [\fB\-n\fR] [\fB\-N\fR] [\fB\-f\fR])]
-         [\fB\-V\fR[\fBV\fR[\fBV\fR]]] [\fB-o\fR <logfile>] [\fB\-\-progress\fR]
-
+         [\fB\-V\fR[\fBV\fR[\fBV\fR]]] [\fB-o\fR <logfile>]
 .SH DESCRIPTION
 .B flashrom
 is a utility for detecting, reading, writing, verifying and erasing flash
@@ -450,9 +449,6 @@
 way to gather logs from flashrom because they will be verbose even if the
 on-screen messages are not verbose and don't require output redirection.
 .TP
-.B "\-\-progress"
-Show progress percentage of operations on the standard output.
-.TP
 .B "\-R, \-\-version"
 Show version information and exit.
 .SH PROGRAMMER-SPECIFIC INFORMATION
diff --git a/include/flash.h b/include/flash.h
index 2ba235d..7738ee3 100644
--- a/include/flash.h
+++ b/include/flash.h
@@ -420,9 +420,6 @@
 		chip_restore_fn_cb_t func;
 		uint8_t status;
 	} chip_restore_fn[MAX_CHIP_RESTORE_FUNCTIONS];
-	/* Progress reporting */
-	flashrom_progress_callback *progress_callback;
-	struct flashrom_progress *progress_state;
 };
 
 /* Timing used in probe routines. ZERO is -2 to differentiate between an unset
@@ -518,7 +515,6 @@
 int close_logfile(void);
 void start_logging(void);
 int flashrom_print_cb(enum flashrom_log_level level, const char *fmt, va_list ap);
-void flashrom_progress_cb(struct flashrom_flashctx *flashctx);
 /* Let gcc and clang check for correct printf-style format strings. */
 int print(enum flashrom_log_level level, const char *fmt, ...)
 #ifdef __MINGW32__
@@ -547,7 +543,6 @@
 #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);
 
 /* spi.c */
 struct spi_command {
diff --git a/include/libflashrom.h b/include/libflashrom.h
index 9bbdcc5..3c99b30 100644
--- a/include/libflashrom.h
+++ b/include/libflashrom.h
@@ -69,34 +69,6 @@
  */
 void flashrom_set_log_callback(flashrom_log_callback *log_callback);
 
-enum flashrom_progress_stage {
-	FLASHROM_PROGRESS_READ,
-	FLASHROM_PROGRESS_WRITE,
-	FLASHROM_PROGRESS_ERASE,
-	FLASHROM_PROGRESS_NR,
-};
-struct flashrom_progress {
-	enum flashrom_progress_stage stage;
-	size_t current;
-	size_t total;
-	void *user_data;
-};
-struct flashrom_flashctx;
-typedef void(flashrom_progress_callback)(struct flashrom_flashctx *flashctx);
-/**
- * @brief Set the progress callback function.
- *
- * Set a callback function which will be invoked whenever libflashrom wants
- * to indicate the progress has changed. This allows frontends to do whatever
- * they see fit with such values, e.g. update a progress bar in a GUI tool.
- *
- * @param progress_callback Pointer to the new progress callback function.
- * @param progress_state Pointer to progress state to include with the progress
- * callback.
- */
-void flashrom_set_progress_callback(struct flashrom_flashctx *const flashctx,
-		flashrom_progress_callback *progress_callback, struct flashrom_progress *progress_state);
-
 /** @} */ /* end flashrom-general */
 
 /**
@@ -203,6 +175,7 @@
  * @{
  */
 
+struct flashrom_flashctx;
 /**
  * @brief Probe for a flash chip.
  *
diff --git a/it87spi.c b/it87spi.c
index a022041..e1edd20 100644
--- a/it87spi.c
+++ b/it87spi.c
@@ -292,7 +292,6 @@
 			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);
 			start += chip->page_size;
 			len -= chip->page_size;
 			buf += chip->page_size;
diff --git a/jedec.c b/jedec.c
index c4ee0c7..1b35c8d 100644
--- a/jedec.c
+++ b/jedec.c
@@ -421,7 +421,6 @@
 		if (write_byte_program_jedec_common(flash, src, dst, mask))
 			failed = 1;
 		dst++, src++;
-		update_progress(flash, FLASHROM_PROGRESS_WRITE, i + 1, len);
 	}
 	if (failed)
 		msg_cerr(" writing sector at 0x%" PRIxPTR " failed!\n", olddst);
@@ -488,7 +487,6 @@
 	 * we're OK for now.
 	 */
 	unsigned int page_size = flash->chip->page_size;
-	unsigned int nwrites = (start + len - 1) / page_size;
 
 	/* Warning: This loop has a very unusual condition and body.
 	 * The loop needs to go through each page with at least one affected
@@ -499,7 +497,7 @@
 	 * (start + len - 1) / page_size. Since we want to include that last
 	 * page as well, the loop condition uses <=.
 	 */
-	for (i = start / page_size; i <= nwrites; i++) {
+	for (i = start / page_size; i <= (start + len - 1) / page_size; i++) {
 		/* Byte position of the first byte in the range in this page. */
 		/* starthere is an offset to the base address of the chip. */
 		starthere = max(start, i * page_size);
@@ -508,7 +506,6 @@
 
 		if (write_page_write_jedec_common(flash, buf + starthere - start, starthere, lenhere))
 			return 1;
-		update_progress(flash, FLASHROM_PROGRESS_WRITE, i + 1, nwrites + 1);
 	}
 
 	return 0;
diff --git a/libflashrom.c b/libflashrom.c
index cbc6243..ce9d351 100644
--- a/libflashrom.c
+++ b/libflashrom.c
@@ -65,25 +65,6 @@
 	return 0;
 }
 
-void flashrom_set_progress_callback(struct flashrom_flashctx *flashctx, flashrom_progress_callback *progress_callback, struct flashrom_progress *progress_state)
-{
-	flashctx->progress_callback = progress_callback;
-	flashctx->progress_state = progress_state;
-}
-/** @private */
-void update_progress(struct flashrom_flashctx *flashctx, enum flashrom_progress_stage stage, size_t current, size_t total)
-{
-	if (flashctx->progress_callback == NULL)
-		return;
-	if (current > total)
-		current = total;
-
-	flashctx->progress_state->stage = stage;
-	flashctx->progress_state->current = current;
-	flashctx->progress_state->total = total;
-	flashctx->progress_callback(flashctx);
-}
-
 const char *flashrom_version_info(void)
 {
 	return flashrom_version;
diff --git a/libflashrom.map b/libflashrom.map
index e3c7a76..7be9e2e 100644
--- a/libflashrom.map
+++ b/libflashrom.map
@@ -26,7 +26,6 @@
     flashrom_programmer_init;
     flashrom_programmer_shutdown;
     flashrom_set_log_callback;
-    flashrom_set_progress_callback;
     flashrom_shutdown;
     flashrom_supported_boards;
     flashrom_supported_chipsets;
diff --git a/linux_mtd.c b/linux_mtd.c
index b8aee06..5a1360e 100644
--- a/linux_mtd.c
+++ b/linux_mtd.c
@@ -215,7 +215,6 @@
 		}
 
 		i += step;
-		update_progress(flash, FLASHROM_PROGRESS_READ, i, len);
 	}
 
 	return 0;
@@ -258,7 +257,6 @@
 		}
 
 		i += step;
-		update_progress(flash, FLASHROM_PROGRESS_WRITE, i, len);
 	}
 
 	return 0;
@@ -295,7 +293,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 a50346e..f53b489 100644
--- a/parade_lspcon.c
+++ b/parade_lspcon.c
@@ -354,7 +354,6 @@
 	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);
 	}
 
 	return ret;
@@ -395,7 +394,6 @@
 	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);
 	}
 
 	ret |= parade_lspcon_enable_write_protection(fd);
diff --git a/realtek_mst_i2c_spi.c b/realtek_mst_i2c_spi.c
index 2a5d5ed..fad3ba9 100644
--- a/realtek_mst_i2c_spi.c
+++ b/realtek_mst_i2c_spi.c
@@ -397,7 +397,6 @@
 		ret |= realtek_mst_i2c_execute_write(fd);
 		if (ret)
 			break;
-		update_progress(flash, FLASHROM_PROGRESS_WRITE, i + RTK_PAGE_SIZE, len);
 	}
 
 	return ret;
diff --git a/spi.c b/spi.c
index 38d8d01..13378e2 100644
--- a/spi.c
+++ b/spi.c
@@ -101,8 +101,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
@@ -112,7 +110,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 061a326..c8d13aa 100644
--- a/spi25.c
+++ b/spi25.c
@@ -693,14 +693,11 @@
 {
 	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);
 	}
 	return 0;
 }
@@ -720,8 +717,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
@@ -746,7 +741,6 @@
 			if (rc)
 				return rc;
 		}
-		update_progress(flash, FLASHROM_PROGRESS_WRITE, start - start_address + lenhere, end_address);
 	}
 
 	return 0;
@@ -766,7 +760,6 @@
 	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);
 	}
 	return 0;
 }
diff --git a/sst28sf040.c b/sst28sf040.c
index 9080684..eb9aa58 100644
--- a/sst28sf040.c
+++ b/sst28sf040.c
@@ -92,7 +92,6 @@
 
 		/* wait for Toggle bit ready */
 		toggle_ready_jedec(flash, bios);
-		update_progress(flash, FLASHROM_PROGRESS_WRITE, i + 1, len);
 	}
 
 	return 0;
diff --git a/tests/spi25.c b/tests/spi25.c
index 8729593..8768d09 100644
--- a/tests/spi25.c
+++ b/tests/spi25.c
@@ -67,65 +67,6 @@
 	return 0;
 }
 
-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);
-	switch (*cnt) {
-		case 0:
-			assert_int_equal(0x100, progress_state->current);
-			break;
-		case 1:
-			assert_int_equal(0x200, progress_state->current);
-			break;
-		case 2:
-			assert_int_equal(0x300, progress_state->current);
-			break;
-		case 3:
-			assert_int_equal(0x300, progress_state->current);
-			break;
-		case 4:
-			assert_int_equal(0x300, progress_state->current);
-			break;
-		default:
-			fail();
-	}
-	(*cnt)++;
-}
-
-void spi_read_chunked_test_success(void **state)
-{
-	(void) state; /* unused */
-	uint8_t buf[0x400] = { 0x0 };
-	uint32_t cnt = 0;
-	const unsigned int max_data_read = 0x100;
-	const unsigned int offset = 0x100;
-	struct registered_master mst = {
-		.spi.read = default_spi_read,
-		.spi.max_data_read = max_data_read
-	};
-
-	/* setup initial test state */
-	struct flashctx flashctx = {
-		.chip = &mock_chip,
-		.mst = &mst
-	};
-	struct flashrom_progress progress_state = {
-		.user_data = (void *) &cnt,
-	};
-	flashrom_set_progress_callback(&flashctx, spi_read_progress_cb, &progress_state);
-	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(5, cnt);
-}
-
 void spi_write_enable_test_success(void **state)
 {
 	(void) state; /* unused */
diff --git a/tests/tests.c b/tests/tests.c
index 53bad65..30ff5b5 100644
--- a/tests/tests.c
+++ b/tests/tests.c
@@ -405,7 +405,6 @@
 	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(probe_spi_rdid_test_success),
 		cmocka_unit_test(probe_spi_rdid4_test_success),
 		cmocka_unit_test(probe_spi_rems_test_success),
diff --git a/tests/tests.h b/tests/tests.h
index 7d84101..3ad4f4f 100644
--- a/tests/tests.h
+++ b/tests/tests.h
@@ -31,7 +31,6 @@
 /* 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 probe_spi_rdid_test_success(void **state);
 void probe_spi_rdid4_test_success(void **state);
 void probe_spi_rems_test_success(void **state);

To view, visit change 69283. To unsubscribe, or for help writing mail filters, visit settings.