From e5f377c6629ee5df53d854e61acc56d41d4721a8 Mon Sep 17 00:00:00 2001 From: Anastasia Klimchuk Date: Mon, 9 Jun 2025 23:06:44 +1000 Subject: [PATCH] probe_flash: Introduce an error code for "other" probing errors Previously probe_flash had the same return code for the case when no chips were matched, and when some other error happened during probing. However these are two different scenarios and it is useful for the caller to distinguish between them. In fact, the caller (libflashrom it is) wanted to distinguish between "no chips found" and "some other probing error" from the very beginning. libflashrom probe API documented returning special error code for "other error". However it was not possible to know when "other error" happened because probe_flash never returned that back, it could only say "no matched chips found". This patch introduces -2 as "other error" code from probe_flash, while -1 remains as "no chips found". Both libflashrom probe APIs v1 and v2 are now handling "other error" from probe_flash and return it to the API callers as was promised in the documentation. This also adds a unit test for error code propagation for "no chips found" error. Change-Id: I4a271550bea2b36c657c71ce6cb1927082663c3c Signed-off-by: Anastasia Klimchuk Reviewed-on: https://review.coreboot.org/c/flashrom/+/88008 Tested-by: build bot (Jenkins) Reviewed-by: Stefan Reinauer Reviewed-by: Peter Marheine --- cli_classic.c | 8 +++--- flashrom.c | 15 +++++++++--- include/flash.h | 3 +++ include/libflashrom.h | 2 ++ libflashrom.c | 33 +++++++++++++------------ tests/libflashrom.c | 57 +++++++++++++++++++++++++++++++++++++++++++ tests/tests.c | 1 + tests/tests.h | 1 + 8 files changed, 98 insertions(+), 22 deletions(-) diff --git a/cli_classic.c b/cli_classic.c index 870f8da0c..a9f9dc107 100644 --- a/cli_classic.c +++ b/cli_classic.c @@ -1250,14 +1250,14 @@ int main(int argc, char *argv[]) msg_cinfo("More than one compatible controller found for the requested flash " "chip, using the first one.\n"); - int startchip = -1; + int force_probe_ret = ERROR_FLASHROM_PROBE_NO_CHIPS_FOUND; for (j = 0; j < registered_master_count; j++) { mst = ®istered_masters[j]; - startchip = probe_flash(mst, 0, &context, 1, options.chip_to_probe); - if (startchip != -1) + force_probe_ret = probe_flash(mst, 0, &context, 1, options.chip_to_probe); + if (force_probe_ret >= 0) break; } - if (startchip == -1) { + if (force_probe_ret < 0) { // FIXME: This should never happen! Ask for a bug report? msg_cinfo("Probing for flash chip '%s' failed.\n", options.chip_to_probe); ret = 1; diff --git a/flashrom.c b/flashrom.c index af1555283..457df728b 100644 --- a/flashrom.c +++ b/flashrom.c @@ -1104,6 +1104,15 @@ static probe_func_t *lookup_probe_func_ptr(const struct flashchip *chip) return NULL; } +/* + * Probes the entries in flashchips array one by one, starting from `startchip` index. + * Probing keeps going until first match found or end of array reached. + * + * Returns: + * the position of the matched chip, i.e. index of the entry in flashchips array + * ERROR_FLASHROM_PROBE_NO_CHIPS_FOUND if no matches found + * ERROR_FLASHROM_PROBE_INTERNAL_ERROR if some unexpected error happened during this operation +*/ int probe_flash(struct registered_master *mst, int startchip, struct flashctx *flash, int force, const char *const chip_to_probe) { const struct flashchip *chip; @@ -1130,7 +1139,7 @@ int probe_flash(struct registered_master *mst, int startchip, struct flashctx *f flash->chip = calloc(1, sizeof(*flash->chip)); if (!flash->chip) { msg_gerr("Out of memory!\n"); - return -1; + return ERROR_FLASHROM_PROBE_INTERNAL_ERROR; } *flash->chip = *chip; flash->mst = mst; @@ -1188,10 +1197,10 @@ notfound: } if (!flash->chip) - return -1; + return ERROR_FLASHROM_PROBE_NO_CHIPS_FOUND; if (init_default_layout(flash) < 0) - return -1; + return ERROR_FLASHROM_PROBE_INTERNAL_ERROR; tmp = flashbuses_to_text(flash->chip->bustype); msg_cinfo("%s %s flash chip \"%s\" (%d kB, %s) ", force ? "Assuming" : "Found", diff --git a/include/flash.h b/include/flash.h index b5ea2b5e7..59f9a8ae5 100644 --- a/include/flash.h +++ b/include/flash.h @@ -720,6 +720,9 @@ int write_flash(struct flashctx *flash, const uint8_t *buf, unsigned int start, */ #define ERROR_FLASHROM_LIMIT -201 +#define ERROR_FLASHROM_PROBE_NO_CHIPS_FOUND -1 +#define ERROR_FLASHROM_PROBE_INTERNAL_ERROR -2 + struct cli_progress { unsigned int stage_pc[FLASHROM_PROGRESS_NR]; unsigned int visible_stages; /* Bitmask of stages with non-zero progress. */ diff --git a/include/libflashrom.h b/include/libflashrom.h index 0b3a5b5fc..73261dd24 100644 --- a/include/libflashrom.h +++ b/include/libflashrom.h @@ -4,6 +4,8 @@ * Copyright (C) 2010 Google Inc. * Copyright (C) 2012 secunet Security Networks AG * (Written by Nico Huber for secunet) + * Copyright (C) 2025 Dmitry Zhadinets + * Copyright (C) 2025 Google LLC * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by diff --git a/libflashrom.c b/libflashrom.c index a45e0d170..a98d4ba53 100644 --- a/libflashrom.c +++ b/libflashrom.c @@ -4,6 +4,7 @@ * Copyright (C) 2012, 2016 secunet Security Networks AG * (Written by Nico Huber for secunet) * Copyright (C) 2025 Dmitry Zhadinets + * Copyright 2025 Google LLC * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by @@ -342,11 +343,22 @@ int flashrom_flash_probe(struct flashrom_flashctx **const flashctx, memset(*flashctx, 0, sizeof(**flashctx)); for (i = 0; i < registered_master_count; ++i) { - int flash_idx = -1; - if (!ret || (flash_idx = probe_flash(®istered_masters[i], 0, *flashctx, 0, chip_name)) != -1) { + int flash_idx = ERROR_FLASHROM_PROBE_NO_CHIPS_FOUND; + if (!ret || (flash_idx = probe_flash(®istered_masters[i], 0, *flashctx, 0, chip_name)) + != ERROR_FLASHROM_PROBE_NO_CHIPS_FOUND) { + if (flash_idx == ERROR_FLASHROM_PROBE_INTERNAL_ERROR) { + ret = 1; + break; + } ret = 0; /* We found one chip, now check that there is no second match. */ - if (probe_flash(®istered_masters[i], flash_idx + 1, &second_flashctx, 0, chip_name) != -1) { + int second_flash_idx = + probe_flash(®istered_masters[i], flash_idx + 1, &second_flashctx, 0, chip_name); + if (second_flash_idx == ERROR_FLASHROM_PROBE_INTERNAL_ERROR) { + ret = 1; + break; + } + if (second_flash_idx != ERROR_FLASHROM_PROBE_NO_CHIPS_FOUND) { flashrom_layout_release(second_flashctx.default_layout); free(second_flashctx.chip); ret = 3; @@ -366,7 +378,7 @@ int flashrom_flash_probe_v2(struct flashrom_flashctx *flashctx, const struct flashrom_programmer *flashprog, const char *chip_name) { - int startchip; + int startchip = 0; unsigned int all_matched_count = 0; // start with no match found const char **matched_names = calloc(flashchips_size + 1, sizeof(char*)); @@ -377,7 +389,7 @@ int flashrom_flash_probe_v2(struct flashrom_flashctx *flashctx, 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) + if (startchip < 0) break; matched_names[all_matched_count] = context_for_probing->chip->name; @@ -396,16 +408,7 @@ int flashrom_flash_probe_v2(struct flashrom_flashctx *flashctx, 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; + int ret = (startchip == ERROR_FLASHROM_PROBE_INTERNAL_ERROR) ? -1 : (int) all_matched_count; return ret; } diff --git a/tests/libflashrom.c b/tests/libflashrom.c index 59ffeafd3..764a839ca 100644 --- a/tests/libflashrom.c +++ b/tests/libflashrom.c @@ -2,6 +2,7 @@ * This file is part of the flashrom project. * * Copyright 2025 Dmitry Zhadinets + * Copyright 2025 Google LLC * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by @@ -19,6 +20,7 @@ #include "tests.h" #include "libflashrom.h" #include "flash.h" +#include "programmer.h" static int test_log_callback(enum flashrom_log_level level, const char *format, va_list vargs) @@ -114,3 +116,58 @@ void flashrom_supported_programmers_test_success(void **state) assert_int_not_equal(ptr - array, 0); } + +#if CONFIG_DUMMY == 1 +typedef void* (map_flash_fn)(const char *descr, uintptr_t phys_addr, size_t len); +typedef int (spi_send_fn)(const struct flashctx *flash, unsigned int writecnt, + unsigned int readcnt, + const unsigned char *writearr, + unsigned char *readarr); + +static void *always_fail_map(const char *descr, uintptr_t phys_addr, size_t len) +{ + return ERROR_PTR; +} + +static int always_fail_spi_send_command(const struct flashctx *flash, unsigned int writecnt, + unsigned int readcnt, + const unsigned char *writearr, + unsigned char *readarr) +{ + return -1; +} + +void probe_v2_error_code_propagation(void **state) +{ + (void) state; /* unused */ + + struct flashrom_flashctx flashctx = { 0 }; + struct flashrom_programmer *flashprog; + const char **all_matched_names = NULL; + + assert_int_equal(0, flashrom_programmer_init(&flashprog, + programmer_dummy.name, + "bus=spi,emulate=W25Q128FV")); + + map_flash_fn *original_map_flash = registered_masters[0].spi.map_flash_region; + spi_send_fn *original_spi_send = registered_masters[0].spi.command; + /* This makes sure probe_flash fails. */ + registered_masters[0].spi.map_flash_region = &always_fail_map; + registered_masters[0].spi.command = &always_fail_spi_send_command; + + assert_int_equal(0 /* no chips found */, + flashrom_flash_probe_v2(&flashctx, &all_matched_names, + flashprog, + NULL)); + + /* restore programmer functions */ + registered_masters[0].spi.map_flash_region = original_map_flash; + registered_masters[0].spi.command = original_spi_send; + + assert_int_equal(0, flashrom_programmer_shutdown(flashprog)); + + flashrom_data_free(all_matched_names); +} +#else + SKIP_TEST(probe_v2_error_code_propagation) +#endif /* CONFIG_DUMMY */ diff --git a/tests/tests.c b/tests/tests.c index c43af3715..0b4b7adb5 100644 --- a/tests/tests.c +++ b/tests/tests.c @@ -483,6 +483,7 @@ int main(int argc, char *argv[]) cmocka_unit_test(flashrom_set_log_callback_v2_test_success), cmocka_unit_test(flashrom_set_log_level_test_success), cmocka_unit_test(flashrom_supported_programmers_test_success), + cmocka_unit_test(probe_v2_error_code_propagation), }; ret |= cmocka_run_group_tests_name("libflashrom.c tests", libflashrom_tests, NULL, NULL); diff --git a/tests/tests.h b/tests/tests.h index 6ec40568e..d9e54b774 100644 --- a/tests/tests.h +++ b/tests/tests.h @@ -36,6 +36,7 @@ void flashrom_set_log_callback_test_success(void **state); void flashrom_set_log_callback_v2_test_success(void **state); void flashrom_set_log_level_test_success(void **state); void flashrom_supported_programmers_test_success(void **state); +void probe_v2_error_code_propagation(void **state); /* spi25.c */ void spi_write_enable_test_success(void **state);