[L] Change in flashrom[main]: libflashrom: Add probing v2 which can find all matching chips

[Anastasia Klimchuk (Code Review)]

Anastasia Klimchuk has submitted this change. ( https://review.coreboot.org/c/flashrom/+/87341?usp=email )

Change subject: libflashrom: Add probing v2 which can find all matching chips
......................................................................

libflashrom: Add probing v2 which can find all matching chips

Probing v2 can (if requested) go through all flashchips and find
all the matching chip definitions. This is the way cli behaves,
so cli becomes a client of probing v2.

Previously cli and libflashrom had different probing logic, and
different code in different source files.

This patch also adds tests for probing v2.

Testing from the cli:
./flashrom -p dummy:emulate=W25Q128FV -r dump.rom
./flashrom -p dummy:emulate=MX25L6436 -r dump.rom
./flashrom -p dummy:emulate=MX25L6436 -c "MX25L6473E" -r dump.rom
./flashrom -p dummy:emulate=SST25VF032B -E
./flashrom -p dummy:emulate=S25FL128L -r dump.rom
./flashrom -p dummy:emulate=INVALID -r dump.rom
./flashrom -p dummy:emulate=MX25L6436 -c "NONEXISTENT" -r dump.rom

Change-Id: Idfcf377a8071e22028ba98515f08495ed2a6e9f0
Signed-off-by: Anastasia Klimchuk <[e-mail]>
Reviewed-on: https://review.coreboot.org/c/flashrom/+/87341
Tested-by: build bot (Jenkins) <[e-mail]>
Reviewed-by: Peter Marheine <[e-mail]>
---
M cli_classic.c
M include/libflashrom.h
M libflashrom.c
M libflashrom.map
M tests/dummyflasher.c
M tests/lifecycle.c
M tests/lifecycle.h
M tests/tests.c
M tests/tests.h
9 files changed, 235 insertions(+), 24 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Peter Marheine: Looks good to me, approved




diff --git a/cli_classic.c b/cli_classic.c
index 3e5dab9..f465785 100644
--- a/cli_classic.c
+++ b/cli_classic.c
@@ -1050,9 +1050,10 @@
 	struct flashctx flashes[8] = {{0}};
 	struct flashctx *fill_flash;
 	char *tempstr = NULL;
-	int startchip = -1, chipcount = 0;
 	int i, j;
 	int ret = 0;
+	int all_matched_count = 0;
+	const char **all_matched_names = NULL;
 
 	struct cli_options options = { 0 };
 	static const char optstring[] = "r::Rw::v::nNVEfc:l:i:p:Lzho:x";
@@ -1209,26 +1210,26 @@
 	msg_pdbg("The following protocols are supported: %s.\n", tempstr ? tempstr : "?");
 	free(tempstr);
 
-	for (j = 0; j < registered_master_count; j++) {
-		startchip = 0;
-		while (chipcount < (int)ARRAY_SIZE(flashes)) {
-			startchip = probe_flash(®istered_masters[j], startchip, &flashes[chipcount], 0, options.chip_to_probe);
-			if (startchip == -1)
-				break;
-			chipcount++;
-			startchip++;
-		}
+	all_matched_count = flashrom_flash_probe_v2(&flashes[0], &all_matched_names,
+                                NULL, options.chip_to_probe);
+	if (all_matched_count == -1) {
+		/* -1 is the ret code which means "something went wrong".
+		 * Multiple match and no match are different ret codes.
+		 * More details about the error were printed during actual probing. */
+		msg_cerr("Error: probing failed.\n");
+		ret = 1;
+		goto out_shutdown;
 	}
 
-	if (chipcount > 1) {
+	if (all_matched_count > 1) {
 		msg_cinfo("Multiple flash chip definitions match the detected chip(s): \"%s\"",
 			  flashes[0].chip->name);
-		for (i = 1; i < chipcount; i++)
-			msg_cinfo(", \"%s\"", flashes[i].chip->name);
+		for (int ind = 1; ind < all_matched_count; ind++)
+			msg_cinfo(", \"%s\"", all_matched_names[ind]);
 		msg_cinfo("\nPlease specify which chip definition to use with the -c <chipname> option.\n");
 		ret = 1;
 		goto out_shutdown;
-	} else if (!chipcount) {
+	} else if (!all_matched_count) {
 		msg_cinfo("No EEPROM/flash device found.\n");
 		if (!options.force || !options.chip_to_probe) {
 			msg_cinfo("Note: flashrom can never write if the flash chip isn't found "
@@ -1253,6 +1254,8 @@
 			if (compatible_masters > 1)
 				msg_cinfo("More than one compatible controller found for the requested flash "
 					  "chip, using the first one.\n");
+
+			int startchip = -1;
 			for (j = 0; j < registered_master_count; j++) {
 				mst = ®istered_masters[j];
 				startchip = probe_flash(mst, 0, &flashes[0], 1, options.chip_to_probe);
@@ -1535,10 +1538,11 @@
 out_shutdown:
 	flashrom_programmer_shutdown(NULL);
 out:
-	for (i = 0; i < chipcount; i++) {
-		flashrom_layout_release(flashes[i].default_layout);
-		free(flashes[i].chip);
+	for (int ind = 0; ind < all_matched_count; ind++) {
+		flashrom_layout_release(flashes[ind].default_layout);
+		free(flashes[ind].chip);
 	}
+	flashrom_data_free(all_matched_names);
 
 	free_options(&options);
 	ret |= close_logfile();
diff --git a/include/libflashrom.h b/include/libflashrom.h
index f30ad6b..ca9a5e3 100644
--- a/include/libflashrom.h
+++ b/include/libflashrom.h
@@ -279,6 +279,45 @@
  *         or 1 on any other error.
  */
 int flashrom_flash_probe(struct flashrom_flashctx **flashctx, const struct flashrom_programmer *flashprog, const char *chip_name);
+
+/**
+ * @brief Probe for a flash chip, v2
+ *
+ * Probes for a flash chip and returns a flash context, that can be used
+ * later with flash chip and @ref flashrom-ops "image operations", if
+ * exactly one matching chip is found.
+ *
+ * Returns the list of names for all chips that matched, and the count of
+ * how many chips matched.
+ *
+ * Memory for the list of chips is dynamically allocated according to the
+ * number of chips found, and always needs to be freed with flashrom_data_free
+ * afterwards (including when no matches found or error happened).
+ *
+ * Note that if chip_name param is set, then probing happens only once, only
+ * for this one requested chip name. So the number of matches that can be
+ * returned in this case will be either 1 or 0 (and -1 for error).
+ *
+ * @param[out] flashctx Points to a struct flashrom_flashctx
+ *                      that will be set if exactly one chip is found. *flashctx
+ *                      has to be freed by the caller with @ref flashrom_flash_release.
+ * @param[out] all_matched_names pointer to an array containing the names of all chips
+ *				 that were successfully probed, terminated with a NULL pointer.
+ *				 If no chips are found, the returned array contains
+ *				 a single NULL element. Callers must free the array once unused
+ *				 by calling `flashrom_data_free`.
+ * @param[in] flashprog The flash programmer used to access the chip,
+ *			currently unused.
+ * @param[in] chip_name Name of a chip to probe for, or NULL to probe for
+ *                      all known chips.
+ * @return the number of matched chips (which can be 0) on success,
+ *         -1 if error happened during probing.
+ */
+int flashrom_flash_probe_v2(struct flashrom_flashctx *flashctx,
+				const char *** const all_matched_names,
+				const struct flashrom_programmer *flashprog,
+				const char *chip_name);
+
 /**
  * @brief Returns the size of the specified flash chip in bytes.
  *
diff --git a/libflashrom.c b/libflashrom.c
index d6bd098..a45e0d1 100644
--- a/libflashrom.c
+++ b/libflashrom.c
@@ -329,8 +329,6 @@
 	return programmer_shutdown();
 }
 
-/* TODO: flashrom_programmer_capabilities()? */
-
 int flashrom_flash_probe(struct flashrom_flashctx **const flashctx,
 			 const struct flashrom_programmer *const flashprog,
 			 const char *const chip_name)
@@ -363,6 +361,56 @@
 	return ret;
 }
 
+int flashrom_flash_probe_v2(struct flashrom_flashctx *flashctx,
+				const char *** const all_matched_names,
+				const struct flashrom_programmer *flashprog,
+				const char *chip_name)
+{
+	int startchip;
+	unsigned int all_matched_count = 0; // start with no match found
+	const char **matched_names = calloc(flashchips_size + 1, sizeof(char*));
+
+	for (int i = 0; i < registered_master_count; i++) {
+		startchip = 0;
+		while (all_matched_count < flashchips_size) {
+			struct flashrom_flashctx second_flashctx = { 0, }; // used for second and more matches
+			struct flashctx *context_for_probing = (all_matched_count > 0) ? &second_flashctx : flashctx;
+			startchip = probe_flash(®istered_masters[i], startchip, context_for_probing, 0, chip_name);
+
+			if (startchip == -1)
+				break;
+
+			matched_names[all_matched_count] = context_for_probing->chip->name;
+			all_matched_count++;
+			startchip++;
+
+			if (all_matched_count > 1) {
+				/* It's used for the second and subsequent probing. */
+				flashrom_layout_release(second_flashctx.default_layout);
+				free(second_flashctx.chip);
+			}
+		}
+	}
+
+	matched_names[all_matched_count] = NULL;
+	matched_names = realloc(matched_names, (all_matched_count + 1) * sizeof(char*));
+	*all_matched_names = matched_names;
+
+	/* The return value should be the number of matched chips, or -1 on error.
+	 * However, currently the error is never returned, because probe_flash return code
+	 * is -1 for both cases of error and no match found, so there is no way
+	 * to distinguish between probing error (e.g. error talking to hw, or out of memory)
+	 * and no match.
+	 * (no match can happen if chip is not in our database, even if hw work perfectly).
+	 *
+	 * TODO improve probe_flash return code to distinguish between
+	 * probing error and no match found. */
+	int ret = (int) all_matched_count;
+
+	return ret;
+}
+
+
 size_t flashrom_flash_getsize(const struct flashrom_flashctx *const flashctx)
 {
 	return flashctx->chip->total_size * 1024;
diff --git a/libflashrom.map b/libflashrom.map
index fc6724a..2b757e5 100644
--- a/libflashrom.map
+++ b/libflashrom.map
@@ -6,6 +6,7 @@
     flashrom_flash_erase;
     flashrom_flash_getsize;
     flashrom_flash_probe;
+    flashrom_flash_probe_v2;
     flashrom_flash_release;
     flashrom_image_read;
     flashrom_image_verify;
diff --git a/tests/dummyflasher.c b/tests/dummyflasher.c
index 9878bef..0faecb9 100644
--- a/tests/dummyflasher.c
+++ b/tests/dummyflasher.c
@@ -42,6 +42,57 @@
 	run_probe_lifecycle(state, &dummy_io, &programmer_dummy, "bus=spi,emulate=W25Q128FV", "W25Q128.V");
 }
 
+void dummy_probe_v2_one_match_for_W25Q128FV(void **state)
+{
+	struct io_mock_fallback_open_state dummy_fallback_open_state = {
+		.noc = 0,
+		.paths = { NULL },
+	};
+	const struct io_mock dummy_io = {
+		.fallback_open_state = &dummy_fallback_open_state,
+	};
+
+	const char *expected_matched_names[1] = {"W25Q128.V"};
+	run_probe_v2_lifecycle(state, &dummy_io, &programmer_dummy, "bus=spi,emulate=W25Q128FV",
+				NULL, /* any chip name */
+				expected_matched_names, 1);
+}
+
+void dummy_probe_v2_six_matches_for_MX25L6436(void **state)
+{
+	struct io_mock_fallback_open_state dummy_fallback_open_state = {
+		.noc = 0,
+		.paths = { NULL },
+	};
+	const struct io_mock dummy_io = {
+		.fallback_open_state = &dummy_fallback_open_state,
+	};
+
+	const char *expected_matched_names[6] = {"MX25L6405",
+						 "MX25L6405D",
+						 "MX25L6406E/MX25L6408E",
+						 "MX25L6436E/MX25L6445E/MX25L6465E",
+						 "MX25L6473E",
+						 "MX25L6473F"};
+	run_probe_v2_lifecycle(state, &dummy_io, &programmer_dummy, "bus=spi,emulate=MX25L6436",
+				NULL, /* any chip name */
+				expected_matched_names, 6);
+}
+
+void dummy_probe_v2_no_matches_found(void **state)
+{
+	struct io_mock_fallback_open_state dummy_fallback_open_state = {
+		.noc = 0,
+		.paths = { NULL },
+	};
+	const struct io_mock dummy_io = {
+		.fallback_open_state = &dummy_fallback_open_state,
+	};
+
+	run_probe_v2_lifecycle(state, &dummy_io, &programmer_dummy, "bus=spi,emulate=MX25L6436",
+				"NONEXISTENT", NULL /* no matched names */, 0);
+}
+
 void dummy_probe_variable_size_test_success(void **state)
 {
 	struct io_mock_fallback_open_state dummy_fallback_open_state = {
@@ -163,6 +214,9 @@
 #else
 	SKIP_TEST(dummy_basic_lifecycle_test_success)
 	SKIP_TEST(dummy_probe_lifecycle_test_success)
+	SKIP_TEST(dummy_probe_v2_one_match_for_W25Q128FV)
+	SKIP_TEST(dummy_probe_v2_six_matches_for_MX25L6436)
+	SKIP_TEST(dummy_probe_v2_no_matches_found)
 	SKIP_TEST(dummy_probe_variable_size_test_success)
 	SKIP_TEST(dummy_init_fails_unhandled_param_test_success)
 	SKIP_TEST(dummy_init_success_invalid_param_test_success)
diff --git a/tests/lifecycle.c b/tests/lifecycle.c
index 95c4249..a92e93e 100644
--- a/tests/lifecycle.c
+++ b/tests/lifecycle.c
@@ -17,22 +17,61 @@
 
 static void probe_chip(const struct programmer_entry *prog,
 			struct flashrom_programmer *flashprog,
-			const char *const chip_name)
+			const char *const chip_name,
+			const char **expected_matched_names, /* unused in probe v1 */
+			unsigned int expected_matched_count /* unused in probe v1 */)
 {
 	struct flashrom_flashctx *flashctx;
 
 	printf("Testing flashrom_flash_probe for programmer=%s, chip=%s ... \n", prog->name, chip_name);
+
 	assert_int_equal(0, flashrom_flash_probe(&flashctx, flashprog, chip_name));
+	if (chip_name)
+		assert_int_equal(0, strcmp(chip_name, flashctx->chip->name));
+
 	printf("... flashrom_flash_probe for programmer=%s successful\n", prog->name);
 
 	flashrom_flash_release(flashctx); /* cleanup */
 }
 
+static void probe_chip_v2(const struct programmer_entry *prog,
+			struct flashrom_programmer *flashprog,
+			const char *const chip_name,
+			const char **expected_matched_names,
+			unsigned int expected_matched_count)
+{
+	struct flashrom_flashctx flashctx = { 0 };
+	const char **all_matched_names = NULL;
+
+	printf("Testing flashrom_flash_probe_v2 for programmer=%s, chip=%s ... \n", prog->name, chip_name);
+	assert_int_equal(expected_matched_count, flashrom_flash_probe_v2(&flashctx, &all_matched_names,
+						 flashprog, chip_name));
+
+	for (unsigned int i = 0; i < expected_matched_count; i++)
+		assert_int_equal(0, strcmp(expected_matched_names[i], all_matched_names[i]));
+
+	assert_null(all_matched_names[expected_matched_count]);
+
+	if (chip_name && expected_matched_count > 0)
+		assert_int_equal(0, strcmp(chip_name, flashctx.chip->name));
+
+	printf("... flashrom_flash_probe_v2 for programmer=%s successful\n", prog->name);
+
+	/* cleanup */
+	flashrom_data_free(all_matched_names);
+	flashrom_layout_release(flashctx.default_layout);
+	free(flashctx.chip);
+}
+
 static void run_lifecycle(void **state, const struct io_mock *io, const struct programmer_entry *prog,
 				const char *param, const char *const chip_name,
+				const char **expected_matched_names,
+				unsigned int expected_matched_count,
 				void (*action)(const struct programmer_entry *prog,
 						struct flashrom_programmer *flashprog,
-						const char *const chip_name))
+						const char *const chip_name,
+						const char **expected_matched_names,
+						unsigned int expected_matched_count))
 {
 	(void) state; /* unused */
 
@@ -45,7 +84,7 @@
 	printf("... flashrom_programmer_init for programmer=%s successful\n", prog->name);
 
 	if (action)
-		action(prog, flashprog, chip_name);
+		action(prog, flashprog, chip_name, expected_matched_names, expected_matched_count);
 
 	printf("Testing flashrom_programmer_shutdown for programmer=%s ...\n", prog->name);
 	assert_int_equal(0, flashrom_programmer_shutdown(flashprog));
@@ -59,7 +98,9 @@
 {
 	/* Basic lifecycle only does init and shutdown,
 	 * so neither chip name nor action is needed. */
-	run_lifecycle(state, io, prog, param, NULL /* chip_name */, NULL /* action */);
+	run_lifecycle(state, io, prog, param, NULL /* chip_name */,
+			NULL /* expected_matched_names, */, 0 /* expected_matched_count, */,
+			NULL /* action */);
 }
 
 void run_probe_lifecycle(void **state, const struct io_mock *io,
@@ -67,7 +108,20 @@
 {
 	/* Each probe lifecycle should run independently, without cache. */
 	clear_spi_id_cache();
-	run_lifecycle(state, io, prog, param, chip_name, &probe_chip);
+	run_lifecycle(state, io, prog, param, chip_name,
+			NULL /* expected_matched_names, */, 0 /* expected_matched_count, */,
+			&probe_chip);
+}
+
+void run_probe_v2_lifecycle(void **state, const struct io_mock *io,
+		const struct programmer_entry *prog, const char *param,
+		const char *const chip_name,
+		const char **expected_matched_names, unsigned int expected_matched_count)
+{
+	/* Each probe lifecycle should run independently, without cache. */
+	clear_spi_id_cache();
+	run_lifecycle(state, io, prog, param, chip_name,
+			expected_matched_names, expected_matched_count, &probe_chip_v2);
 }
 
 void run_init_error_path(void **state, const struct io_mock *io, const struct programmer_entry *prog,
diff --git a/tests/lifecycle.h b/tests/lifecycle.h
index b4c2fbe..1565a69 100644
--- a/tests/lifecycle.h
+++ b/tests/lifecycle.h
@@ -34,6 +34,11 @@
 void run_probe_lifecycle(void **state, const struct io_mock *io,
 		const struct programmer_entry *prog, const char *param, const char *const chip_name);
 
+void run_probe_v2_lifecycle(void **state, const struct io_mock *io,
+		const struct programmer_entry *prog, const char *param,
+		const char *const chip_name,
+		const char **expected_matched_names, unsigned int expected_matched_count);
+
 void run_init_error_path(void **state, const struct io_mock *io,
 		const struct programmer_entry *prog, const char *param, const int error_code);
 #endif /* __LIFECYCLE_H__ */
diff --git a/tests/tests.c b/tests/tests.c
index 2a79d32..c43af37 100644
--- a/tests/tests.c
+++ b/tests/tests.c
@@ -504,6 +504,9 @@
 	const struct CMUnitTest lifecycle_tests[] = {
 		cmocka_unit_test(dummy_basic_lifecycle_test_success),
 		cmocka_unit_test(dummy_probe_lifecycle_test_success),
+		cmocka_unit_test(dummy_probe_v2_one_match_for_W25Q128FV),
+		cmocka_unit_test(dummy_probe_v2_six_matches_for_MX25L6436),
+		cmocka_unit_test(dummy_probe_v2_no_matches_found),
 		cmocka_unit_test(dummy_probe_variable_size_test_success),
 		cmocka_unit_test(dummy_init_fails_unhandled_param_test_success),
 		cmocka_unit_test(dummy_init_success_invalid_param_test_success),
diff --git a/tests/tests.h b/tests/tests.h
index e87c00c..6ec4056 100644
--- a/tests/tests.h
+++ b/tests/tests.h
@@ -53,6 +53,9 @@
 /* lifecycle.c */
 void dummy_basic_lifecycle_test_success(void **state);
 void dummy_probe_lifecycle_test_success(void **state);
+void dummy_probe_v2_one_match_for_W25Q128FV(void **state);
+void dummy_probe_v2_six_matches_for_MX25L6436(void **state);
+void dummy_probe_v2_no_matches_found(void **state);
 void dummy_probe_variable_size_test_success(void **state);
 void dummy_init_fails_unhandled_param_test_success(void **state);
 void dummy_init_success_invalid_param_test_success(void **state);

-- 
To view, visit https://review.coreboot.org/c/flashrom/+/87341?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email

Gerrit-MessageType: merged
Gerrit-Project: flashrom
Gerrit-Branch: main
Gerrit-Change-Id: Idfcf377a8071e22028ba98515f08495ed2a6e9f0
Gerrit-Change-Number: 87341
Gerrit-PatchSet: 9
Gerrit-Owner: Anastasia Klimchuk <[e-mail]>
Gerrit-Reviewer: Anastasia Klimchuk <[e-mail]>
Gerrit-Reviewer: Dmitry Zhadinets <[e-mail]>
Gerrit-Reviewer: Peter Marheine <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>