diff --git a/cli_classic.c b/cli_classic.c index 7a7125b0f..29f21cca4 100644 --- a/cli_classic.c +++ b/cli_classic.c @@ -1050,7 +1050,6 @@ static void free_options(struct cli_options *options) int main(int argc, char *argv[]) { const struct flashchip *chip = NULL; - struct flashctx context = {0}; /* holds the active detected chip and other info */ char *tempstr = NULL; int i, j; int ret = 0; @@ -1058,6 +1057,13 @@ int main(int argc, char *argv[]) const char **all_matched_names = NULL; time_t time_start, time_end; + struct flashctx *context = NULL; /* holds the active detected chip and other info */ + ret = flashrom_create_context(&context); + if (ret) { + msg_gerr("Failed to allocate flash context. Aborting"); + return 1; + } + struct cli_options options = { 0 }; static const char optstring[] = "r::Rw::v::nNVEfc:l:i:p:Lzho:x"; static const struct option long_options[] = { @@ -1216,7 +1222,7 @@ int main(int argc, char *argv[]) msg_pdbg("The following protocols are supported: %s.\n", tempstr ? tempstr : "?"); free(tempstr); - all_matched_count = flashrom_flash_probe_v2(&context, &all_matched_names, + all_matched_count = flashrom_flash_probe_v2(context, &all_matched_names, NULL, options.chip_to_probe); if (all_matched_count == -1) { /* -1 is the ret code which means "something went wrong". @@ -1229,7 +1235,7 @@ int main(int argc, char *argv[]) if (all_matched_count > 1) { msg_cinfo("Multiple flash chip definitions match the detected chip(s): \"%s\"", - context.chip->name); + context->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 option.\n"); @@ -1264,7 +1270,7 @@ int main(int argc, char *argv[]) int force_probe_ret = ERROR_FLASHROM_PROBE_NO_CHIPS_FOUND; for (j = 0; j < registered_master_count; j++) { mst = ®istered_masters[j]; - force_probe_ret = probe_flash(mst, 0, &context, 1, options.chip_to_probe); + force_probe_ret = probe_flash(mst, 0, context, 1, options.chip_to_probe); if (force_probe_ret >= 0) break; } @@ -1275,31 +1281,31 @@ int main(int argc, char *argv[]) goto out_shutdown; } msg_cinfo("Please note that forced reads most likely contain garbage.\n"); - flashrom_flag_set(&context, FLASHROM_FLAG_FORCE, options.force); - ret = do_read(&context, options.filename); - free(context.chip); + flashrom_flag_set(context, FLASHROM_FLAG_FORCE, options.force); + ret = do_read(context, options.filename); + free(context->chip); goto out_shutdown; } ret = 1; goto out_shutdown; } else if (!options.chip_to_probe) { /* repeat for convenience when looking at foreign logs */ - tempstr = flashbuses_to_text(context.chip->bustype); + tempstr = flashbuses_to_text(context->chip->bustype); msg_gdbg("Found %s flash chip \"%s\" (%d kB, %s).\n", - context.chip->vendor, context.chip->name, context.chip->total_size, + context->chip->vendor, context->chip->name, context->chip->total_size, tempstr ? tempstr : "?"); free(tempstr); } struct cli_progress cli_progress = {0}; if (options.show_progress) - flashrom_set_progress_callback_v2(&context, &flashrom_progress_cb, &cli_progress); + flashrom_set_progress_callback_v2(context, &flashrom_progress_cb, &cli_progress); - print_chip_support_status(context.chip); + print_chip_support_status(context->chip); - unsigned int limitexceeded = count_max_decode_exceedings(&context, &max_rom_decode); + unsigned int limitexceeded = count_max_decode_exceedings(context, &max_rom_decode); if (limitexceeded > 0 && !options.force) { - enum chipbustype commonbuses = context.mst->buses_supported & context.chip->bustype; + enum chipbustype commonbuses = context->mst->buses_supported & context->chip->bustype; /* Sometimes chip and programmer have more than one bus in common, * and the limit is not exceeded on all buses. Tell the user. */ @@ -1397,10 +1403,10 @@ int main(int argc, char *argv[]) } if (options.flash_name) { - if (context.chip->vendor && context.chip->name) { + if (context->chip->vendor && context->chip->name) { printf("vendor=\"%s\" name=\"%s\"\n", - context.chip->vendor, - context.chip->name); + context->chip->vendor, + context->chip->name); } else { ret = -1; } @@ -1408,7 +1414,7 @@ int main(int argc, char *argv[]) } if (options.flash_size) { - printf("%zu\n", flashrom_flash_getsize(&context)); + printf("%zu\n", flashrom_flash_getsize(context)); goto out_shutdown; } @@ -1417,10 +1423,10 @@ int main(int argc, char *argv[]) msg_ginfo("Invalid input of sacrifice ratio, valid 0-50. Fallback to default value 0.\n"); options.sacrifice_ratio = 0; } - context.sacrifice_ratio = options.sacrifice_ratio; + context->sacrifice_ratio = options.sacrifice_ratio; } - if (options.ifd && (flashrom_layout_read_from_ifd(&options.layout, &context, NULL, 0) || + if (options.ifd && (flashrom_layout_read_from_ifd(&options.layout, context, NULL, 0) || process_include_args(options.layout, options.include_args))) { ret = 1; goto out_shutdown; @@ -1445,7 +1451,7 @@ int main(int argc, char *argv[]) goto out_shutdown; } - if (flashrom_layout_read_fmap_from_buffer(&options.layout, &context, fmapfile_buffer, fmapfile_size) || + if (flashrom_layout_read_fmap_from_buffer(&options.layout, context, fmapfile_buffer, fmapfile_size) || process_include_args(options.layout, options.include_args)) { ret = 1; free(fmapfile_buffer); @@ -1454,8 +1460,8 @@ int main(int argc, char *argv[]) free(fmapfile_buffer); } else if (options.fmap) { /* Read layout from ROM fmap */ - if (flashrom_layout_read_fmap_from_rom(&options.layout, &context, 0, - flashrom_flash_getsize(&context)) || + if (flashrom_layout_read_fmap_from_rom(&options.layout, context, 0, + flashrom_flash_getsize(context)) || process_include_args(options.layout, options.include_args)) { ret = 1; goto out_shutdown; @@ -1482,8 +1488,8 @@ int main(int argc, char *argv[]) goto out_release; } /* Read layout from file fmap */ - if (flashrom_layout_read_fmap_from_buffer(&file_layout, &context, file_buffer, - flashrom_flash_getsize(&context)) || + if (flashrom_layout_read_fmap_from_buffer(&file_layout, context, file_buffer, + flashrom_flash_getsize(context)) || process_include_args(file_layout, options.include_args)) { ret = 1; free(file_buffer); @@ -1501,7 +1507,7 @@ int main(int argc, char *argv[]) msg_cinfo("FMAP layouts match.\n"); } } - flashrom_layout_set(&context, options.layout); + flashrom_layout_set(context, options.layout); if (any_wp_op) { if (options.set_wp_region && options.wp_region) { @@ -1519,7 +1525,7 @@ int main(int argc, char *argv[]) options.set_wp_range = true; } ret = wp_cli( - &context, + context, options.enable_wp, options.disable_wp, options.print_wp_status, @@ -1532,33 +1538,33 @@ int main(int argc, char *argv[]) goto out_release; } - flashrom_flag_set(&context, FLASHROM_FLAG_FORCE, options.force); + flashrom_flag_set(context, FLASHROM_FLAG_FORCE, options.force); #if CONFIG_INTERNAL == 1 - flashrom_flag_set(&context, FLASHROM_FLAG_FORCE_BOARDMISMATCH, force_boardmismatch); + flashrom_flag_set(context, FLASHROM_FLAG_FORCE_BOARDMISMATCH, force_boardmismatch); #endif - flashrom_flag_set(&context, FLASHROM_FLAG_VERIFY_AFTER_WRITE, !options.dont_verify_it); - flashrom_flag_set(&context, FLASHROM_FLAG_VERIFY_WHOLE_CHIP, !options.dont_verify_all); + flashrom_flag_set(context, FLASHROM_FLAG_VERIFY_AFTER_WRITE, !options.dont_verify_it); + flashrom_flag_set(context, FLASHROM_FLAG_VERIFY_WHOLE_CHIP, !options.dont_verify_all); /* FIXME: We should issue an unconditional chip reset here. This can be * done once we have a .reset function in struct flashchip. * Give the chip time to settle. */ - programmer_delay(&context, 100000); + programmer_delay(context, 100000); if (options.read_it) - ret = do_read(&context, options.filename); + ret = do_read(context, options.filename); else if (options.extract_it) - ret = do_extract(&context); + ret = do_extract(context); else if (options.erase_it) { - ret = flashrom_flash_erase(&context); + ret = flashrom_flash_erase(context); } else if (options.write_it) - ret = do_write(&context, options.filename, options.referencefile); + ret = do_write(context, options.filename, options.referencefile); else if (options.verify_it) - ret = do_verify(&context, options.filename); + ret = do_verify(context, options.filename); #if CONFIG_RPMC_ENABLED == 1 if (any_rpmc_op && ret == 0) { - ret = rpmc_cli(&context, + ret = rpmc_cli(context, options.rpmc_root_key_file, options.rpmc_key_data, options.rpmc_counter_address, @@ -1576,9 +1582,8 @@ out_release: out_shutdown: flashrom_programmer_shutdown(NULL); out: - flashrom_layout_release(context.default_layout); - free(context.chip); flashrom_data_free(all_matched_names); + flashrom_flash_release(context); free_options(&options); diff --git a/doc/release_notes/devel.rst b/doc/release_notes/devel.rst index 5eab95d24..f0fe3c8f5 100644 --- a/doc/release_notes/devel.rst +++ b/doc/release_notes/devel.rst @@ -63,3 +63,19 @@ New programmers =============== * Nvidia System Management Agent + +libflashrom +=========== + +The real life usage of ``flashrom_flash_probe_v2`` discovered the issue, the error messages as below:: + + error: variable 'flashctx' has initializer but incomplete type + error: storage size of 'flashctx' isn't known + +New API method ``flashrom_create_context`` is created to fix this. +``flashrom_create_context`` does create and all initialisations needed for flash context. +It need to be called prior to any other API calls that require flash context. + +The issue was a good motivation to write a new test which is built and runs as an external +client of libflashrom. The test runs in a separate test executable to achieve this. +As a bonus, test code in ``tests/external_client.c`` is an example how to use libflashrom API. diff --git a/include/libflashrom.h b/include/libflashrom.h index 46d710b70..1e804b774 100644 --- a/include/libflashrom.h +++ b/include/libflashrom.h @@ -50,6 +50,19 @@ int flashrom_init(int perform_selfcheck); * @return 0 on success */ int flashrom_shutdown(void); + +struct flashrom_flashctx; + +/** + * @brief Create flash context. + * + * @param[out] flashctx Points to a pointer of type struct flashrom_flashctx + * that will be initialised to be used for further operations. *flashctx + * has to be freed by the caller with @ref flashrom_flash_release.* + * @return 0 on success + */ +int flashrom_create_context(struct flashrom_flashctx **const flashctx); + enum flashrom_log_level { FLASHROM_MSG_ERROR = 0, FLASHROM_MSG_WARN = 1, @@ -117,7 +130,6 @@ struct flashrom_progress { void *user_data; }; -struct flashrom_flashctx; typedef void(flashrom_progress_callback)(struct flashrom_flashctx *flashctx); /** diff --git a/libflashrom.c b/libflashrom.c index b5902d7c9..4ea0881a5 100644 --- a/libflashrom.c +++ b/libflashrom.c @@ -49,6 +49,23 @@ int flashrom_shutdown(void) return 0; /* TODO: nothing to do? */ } +int flashrom_create_context(struct flashrom_flashctx **const flashctx) +{ + struct flashrom_flashctx *const out = malloc(sizeof(struct flashrom_flashctx)); + if (!out) + return 1; + + /* Init of values inside context. */ + *out = (struct flashrom_flashctx) { + .virtual_memory = (chipaddr)ERROR_PTR, + .virtual_registers = (chipaddr)ERROR_PTR, + }; + + *flashctx = out; + + return 0; +} + void flashrom_set_log_level(enum flashrom_log_level level) { global_log_level = level; diff --git a/libflashrom.map b/libflashrom.map index 358280ad3..742f569ea 100644 --- a/libflashrom.map +++ b/libflashrom.map @@ -1,5 +1,6 @@ LIBFLASHROM_1.0 { global: + flashrom_create_context; flashrom_data_free; flashrom_flag_get; flashrom_flag_set; diff --git a/meson.build b/meson.build index d6dd6b6ef..aa66470af 100644 --- a/meson.build +++ b/meson.build @@ -791,6 +791,14 @@ if get_option('tests').auto() or get_option('tests').enabled() ], ) + # This is a special dependency for tests that run as external client of libflashrom + # External client does not know internal symbols, includes libflashrom.h, + # and only uses what is in the public API. + libflashrom_test_dep = declare_dependency( + include_directories : include_dir, + link_with : libflashrom + ) + if cmocka_dep.found() subdir('tests') endif diff --git a/tests/client_tests.c b/tests/client_tests.c new file mode 100644 index 000000000..413d6d766 --- /dev/null +++ b/tests/client_tests.c @@ -0,0 +1,26 @@ +/* + * This file is part of the flashrom project. + * + * SPDX-License-Identifier: GPL-2.0-only + * SPDX-FileCopyrightText: 2025 Google LLC + */ + +#include "client_tests.h" + +int main(int argc, char *argv[]) +{ + int ret = 0; + + if (argc > 1) + cmocka_set_test_filter(argv[1]); + + cmocka_set_message_output(CM_OUTPUT_STDOUT); + + const struct CMUnitTest external_client_tests[] = { + cmocka_unit_test(flashrom_init_probe_erase_shutdown), + }; + ret |= cmocka_run_group_tests_name("external_client.c tests", + external_client_tests, NULL, NULL); + + return ret; +} diff --git a/tests/client_tests.h b/tests/client_tests.h new file mode 100644 index 000000000..7ff4ba85d --- /dev/null +++ b/tests/client_tests.h @@ -0,0 +1,16 @@ +/* + * This file is part of the flashrom project. + * + * SPDX-License-Identifier: GPL-2.0-only + * SPDX-FileCopyrightText: 2025 Google LLC + */ + +#ifndef CLIENT_TESTS_H +#define CLIENT_TESTS_H + +#include + +/* external_client.c */ +void flashrom_init_probe_erase_shutdown(void **state); + +#endif /* CLIENT_TESTS_H */ diff --git a/tests/dummyflasher.c b/tests/dummyflasher.c index 18a75a79e..ceb3d554c 100644 --- a/tests/dummyflasher.c +++ b/tests/dummyflasher.c @@ -257,8 +257,7 @@ static void dummy_test_shutdown(struct flashrom_flashctx *flashctx, io_mock_register(NULL); - flashrom_layout_release(flashctx->default_layout); - free(flashctx->chip); + flashrom_flash_release(flashctx); } void dummy_probe_and_read(void **state) @@ -266,21 +265,22 @@ void dummy_probe_and_read(void **state) (void) state; /* unused */ struct flashrom_programmer *flashprog = NULL; - struct flashrom_flashctx flashctx = { 0 }; + struct flashrom_flashctx *flashctx = NULL; + assert_int_equal(0, flashrom_create_context(&flashctx)); - dummy_test_init_and_probe(&flashctx, flashprog); + dummy_test_init_and_probe(flashctx, flashprog); const unsigned long image_size = 16384 * KiB; unsigned char *buf = calloc(image_size, sizeof(unsigned char)); assert_non_null(buf); printf("Testing flashrom_image_read ...\n"); - assert_int_equal(0, flashrom_image_read(&flashctx, buf, image_size)); + assert_int_equal(0, flashrom_image_read(flashctx, buf, image_size)); printf("... flashrom_image_read is successful.\n"); free(buf); - dummy_test_shutdown(&flashctx, flashprog); + dummy_test_shutdown(flashctx, flashprog); } void dummy_probe_and_write(void **state) @@ -288,21 +288,22 @@ void dummy_probe_and_write(void **state) (void) state; /* unused */ struct flashrom_programmer *flashprog = NULL; - struct flashrom_flashctx flashctx = { 0 }; + struct flashrom_flashctx *flashctx = NULL; + assert_int_equal(0, flashrom_create_context(&flashctx)); - dummy_test_init_and_probe(&flashctx, flashprog); + dummy_test_init_and_probe(flashctx, flashprog); const unsigned long image_size = 16384 * KiB; uint8_t *const newcontents = malloc(image_size); assert_non_null(newcontents); printf("Testing flashrom_image_write ...\n"); - assert_int_equal(0, flashrom_image_write(&flashctx, newcontents, image_size, NULL)); + assert_int_equal(0, flashrom_image_write(flashctx, newcontents, image_size, NULL)); printf("... flashrom_image_write is successful.\n"); free(newcontents); - dummy_test_shutdown(&flashctx, flashprog); + dummy_test_shutdown(flashctx, flashprog); } void dummy_probe_and_erase(void **state) @@ -310,15 +311,16 @@ void dummy_probe_and_erase(void **state) (void) state; /* unused */ struct flashrom_programmer *flashprog = NULL; - struct flashrom_flashctx flashctx = { 0 }; + struct flashrom_flashctx *flashctx = NULL; + assert_int_equal(0, flashrom_create_context(&flashctx)); - dummy_test_init_and_probe(&flashctx, flashprog); + dummy_test_init_and_probe(flashctx, flashprog); printf("Testing flashrom_flash_erase ...\n"); - assert_int_equal(0, flashrom_flash_erase(&flashctx)); + assert_int_equal(0, flashrom_flash_erase(flashctx)); printf("... flashrom_flash_erase is successful.\n"); - dummy_test_shutdown(&flashctx, flashprog); + dummy_test_shutdown(flashctx, flashprog); } #else diff --git a/tests/external_client.c b/tests/external_client.c new file mode 100644 index 000000000..e125a9386 --- /dev/null +++ b/tests/external_client.c @@ -0,0 +1,84 @@ +/* + * This file is part of the flashrom project. + * + * SPDX-License-Identifier: GPL-2.0-only + * SPDX-FileCopyrightText: 2025 Google LLC + */ + +/* + * The purpose of this test is to use libflashrom API as an external client. + * + * Therefore, the test *must not* include any flashrom-internal headers. + * In meson files, test depends on a special `libflashrom_test_dep` and runs + * in a separate test executable `flashrom_client_tests`. + */ + +#include +#include +#include "client_tests.h" +#include "libflashrom.h" + +#define DUMMYFLASHER_NAME "dummy" + +void flashrom_init_probe_erase_shutdown(void **state) +{ + (void) state; /* unused */ + + assert_int_equal(0, flashrom_init(1)); + printf("flashrom_init with selfcheck: OK\n"); + + struct flashrom_programmer *flashprog = NULL; + struct flashrom_flashctx *flashctx; + const char **all_matched_names = NULL; + + assert_int_equal(0, flashrom_create_context(&flashctx)); + printf("flashrom_create_context: OK\n"); + + const char **progs_names = flashrom_supported_programmers(); + assert_non_null(progs_names); + + const char **ptr = progs_names; + bool dummyflasher_support = false; + do { + if (!strcmp(*ptr, DUMMYFLASHER_NAME)) { + dummyflasher_support = true; + break; + } + } while (*(++ptr)); + flashrom_data_free(progs_names); + + if (dummyflasher_support) { + printf("dummyflasher supported: OK\n"); + + assert_int_equal(0, flashrom_programmer_init( + &flashprog, + DUMMYFLASHER_NAME, + "bus=spi,emulate=W25Q128FV")); + printf("flashrom_programmer_init for dummy with params 'bus=spi,emulate=W25Q128FV': OK\n"); + + assert_int_equal(1, flashrom_flash_probe_v2( + flashctx, + &all_matched_names, + flashprog, + NULL)); + printf("flashrom_flash_probe_v2 found 1 chip: OK\n"); + assert_int_equal(0, strcmp("W25Q128.V", all_matched_names[0])); + printf("chip name matches 'W25Q128.V': OK\n"); + assert_int_equal(16777216, flashrom_flash_getsize(flashctx)); + printf("chip size 16M: OK\n"); + + assert_int_equal(0, flashrom_flash_erase(flashctx)); + printf("flashrom_flash_erase: OK\n"); + + assert_int_equal(0, flashrom_programmer_shutdown(flashprog)); + printf("flashrom_programmer_shutdown: OK\n"); + } else { + printf("WARNING: dummyflasher is disabled, cannot test probe and erase\n"); + } + + flashrom_flash_release(flashctx); + printf("flashrom_flash_release: OK\n"); + + assert_int_equal(0, flashrom_shutdown()); + printf("flashrom_shutdown: OK\n"); +} diff --git a/tests/libflashrom.c b/tests/libflashrom.c index 5ae4b2d96..cebf5c27b 100644 --- a/tests/libflashrom.c +++ b/tests/libflashrom.c @@ -133,7 +133,9 @@ void probe_v2_error_code_propagation(void **state) { (void) state; /* unused */ - struct flashrom_flashctx flashctx = { 0 }; + struct flashrom_flashctx *flashctx = NULL; + assert_int_equal(0, flashrom_create_context(&flashctx)); + struct flashrom_programmer *flashprog; const char **all_matched_names = NULL; @@ -148,7 +150,7 @@ void probe_v2_error_code_propagation(void **state) 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, + flashrom_flash_probe_v2(flashctx, &all_matched_names, flashprog, NULL)); @@ -159,6 +161,7 @@ void probe_v2_error_code_propagation(void **state) assert_int_equal(0, flashrom_programmer_shutdown(flashprog)); flashrom_data_free(all_matched_names); + flashrom_flash_release(flashctx); } #else SKIP_TEST(probe_v2_error_code_propagation) diff --git a/tests/lifecycle.c b/tests/lifecycle.c index c39e8e0db..5a265207a 100644 --- a/tests/lifecycle.c +++ b/tests/lifecycle.c @@ -13,11 +13,12 @@ static void probe_chip_v2(const struct programmer_entry *prog, const char **expected_matched_names, unsigned int expected_matched_count) { - struct flashrom_flashctx flashctx = { 0 }; + struct flashrom_flashctx *flashctx = NULL; + assert_int_equal(0, flashrom_create_context(&flashctx)); 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, + 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++) @@ -26,14 +27,13 @@ static void probe_chip_v2(const struct programmer_entry *prog, 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)); + 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); + flashrom_flash_release(flashctx); } static void run_lifecycle(void **state, const struct io_mock *io, const struct programmer_entry *prog, diff --git a/tests/meson.build b/tests/meson.build index b98fa16cc..34af21375 100644 --- a/tests/meson.build +++ b/tests/meson.build @@ -134,3 +134,18 @@ test('cmocka test flashrom', flashrom_tests) if get_option('llvm_cov').enabled() run_target('llvm-cov-tests', command : ['../scripts/llvm-cov', flashrom_tests]) endif + +# Creating a second test executable which runs tests as if it is an external +# flashrom client using libflashrom via public API. + +client_test_srcs = files( + 'external_client.c', + 'client_tests.c', + ) + +flashrom_client_tests = executable('flashrom_client_tests', + client_test_srcs, + export_dynamic : true, + dependencies : [cmocka_dep, libflashrom_test_dep], +) +test('flashrom client tests', flashrom_client_tests)