March 2025 - flashrom-gerrit - mail.coreboot.org

[S] Change in flashrom[main]: flashchips: Add Macronix MX77U25650FZ4I42
by Dolan Liu (Code Review) 19 Mar '25

19 Mar '25

Attention is currently required from: Anastasia Klimchuk, Nikolai Artemiev, Stefan Reinauer. Dolan Liu has posted comments on this change by Dolan Liu. ( https://review.coreboot.org/c/flashrom/+/86348?usp=email ) Change subject: flashchips: Add Macronix MX77U25650FZ4I42 ...................................................................... Patch Set 2: (1 comment) Patchset: PS2: > ya, thanks for commetns, we are asking HW team to request the datasheet, i will attached the link he […] Hi Anastasia, we have try to ask Macronix to request datasheet link, but they were reject because the RPMC device need theirs internal approval or license limited, and it was have watermakr with company confidential. sorry for that, may need you apply to Macronix... -- To view, visit https://review.coreboot.org/c/flashrom/+/86348?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I7866b2db343f4eb2bc194400ceca099d3af3b87d Gerrit-Change-Number: 86348 Gerrit-PatchSet: 2 Gerrit-Owner: Dolan Liu <liuyong5(a)huaqin.corp-partner.google.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Wed, 19 Mar 2025 06:42:25 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Dolan Liu <liuyong5(a)huaqin.corp-partner.google.com> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>

1 0

[M] Change in flashrom[main]: libflashrom: Added API for capabilitible chips
by Dmitry Zhadinets (Code Review) 19 Mar '25

19 Mar '25

Attention is currently required from: Anastasia Klimchuk, Dmitry Zhadinets. Hello Anastasia Klimchuk, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/86922?usp=email to look at the new patch set (#2). The following approvals got outdated and were removed: Verified-1 by build bot (Jenkins) Change subject: libflashrom: Added API for capabilitible chips ...................................................................... libflashrom: Added API for capabilitible chips There were no options available to obtain the list of programmers, compatible chips for the programmer, and layout region names. The implementation is based on flashrom_supported_flash_chips. Arrays of constant strings are returned, and the array must be freed using flashrom_data_free. BTW it would be good to add a notice about freeing memory to the other flashrom_supported* functions. Testing: Both unit tests and CLI tools serve as libflashrom clients. All unit tests run successfully. Change-Id: I13c951f9404ae8b042cdb572e6117a09ac231747 Signed-off-by: Dmitry Zhadinets <dzhadinets(a)gmail.com> --- M include/libflashrom.h M libflashrom.c M libflashrom.map M tests/libflashrom.c M tests/tests.c 5 files changed, 51 insertions(+), 6 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/22/86922/2 -- To view, visit https://review.coreboot.org/c/flashrom/+/86922?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: newpatchset Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I13c951f9404ae8b042cdb572e6117a09ac231747 Gerrit-Change-Number: 86922 Gerrit-PatchSet: 2 Gerrit-Owner: Dmitry Zhadinets <dzhadinets(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Dmitry Zhadinets <dzhadinets(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>

1 0

[M] Change in flashrom[main]: libflashrom: Added API for capabilitible chips
by Dmitry Zhadinets (Code Review) 19 Mar '25

19 Mar '25

Attention is currently required from: Anastasia Klimchuk. Dmitry Zhadinets has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/86922?usp=email ) Change subject: libflashrom: Added API for capabilitible chips ...................................................................... libflashrom: Added API for capabilitible chips There were no options available to obtain the list of programmers, compatible chips for the programmer, and layout region names. The implementation is based on flashrom_supported_flash_chips. Arrays of constant strings are returned, and the array must be freed using flashrom_data_free. BTW it would be good to add a notice about freeing memory to the other flashrom_supported* functions. Testing: Both unit tests and CLI tools serve as libflashrom clients. All unit tests run successfully. Change-Id: I13c951f9404ae8b042cdb572e6117a09ac231747 Signed-off-by: Dmitry Zhadinets <dzhadinets(a)gmail.com> --- M include/libflashrom.h M libflashrom.c M libflashrom.map M tests/libflashrom.c M tests/tests.c 5 files changed, 51 insertions(+), 6 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/22/86922/1 diff --git a/include/libflashrom.h b/include/libflashrom.h index e3893ac..7f90fa4 100644 --- a/include/libflashrom.h +++ b/include/libflashrom.h @@ -227,6 +227,14 @@ */ /** + * @brief Returns list of supported chip names fo selected programmer + * @param[in] flashprog The flash programmer used to access the chip. + * @return List of supported flash chips, or NULL if an error occurred. + * The pointer must be freed after using flashrom_data_free + */ +const char ** flashrom_programmer_capabilities(const struct flashrom_programmer *const flashprog); + +/** * @brief Probe for a flash chip. * * Probes for a flash chip and returns a flash context, that can be used diff --git a/libflashrom.c b/libflashrom.c index 9f177d7..76ab32a 100644 --- a/libflashrom.c +++ b/libflashrom.c @@ -279,7 +279,30 @@ return programmer_shutdown(); } -/* TODO: flashrom_programmer_capabilities()? */ +const char ** flashrom_programmer_capabilities(const struct flashrom_programmer *const flashprog) +{ + (void) flashprog; /* unused */ + unsigned int chipcount = 0; + const char ** result = malloc((flashchips_size + 1) * sizeof(char*)); + struct flashrom_flashctx flashctx = { 0 }; + for (int j = 0; j < registered_master_count; j++) { + int startchip = 0; + + while (chipcount < flashchips_size) { + startchip = probe_flash(&registered_masters[j], startchip, &flashctx, 0, NULL); + if (startchip == -1) + break; + result[chipcount] = flashctx.chip->name; + chipcount++; + startchip++; + flashrom_layout_release(flashctx.default_layout); + free(flashctx.chip); + } + } + result = realloc(result, (chipcount + 1) * sizeof(char*)); + result[chipcount] = 0; + return result; +} int flashrom_flash_probe(struct flashrom_flashctx **const flashctx, const struct flashrom_programmer *const flashprog, diff --git a/libflashrom.map b/libflashrom.map index 8e4635d..165a217 100644 --- a/libflashrom.map +++ b/libflashrom.map @@ -21,6 +21,7 @@ flashrom_layout_read_from_ifd; flashrom_layout_release; flashrom_layout_set; + flashrom_programmer_capabilities flashrom_programmer_init; flashrom_programmer_shutdown; flashrom_set_log_callback; @@ -30,6 +31,7 @@ flashrom_supported_boards; flashrom_supported_chipsets; flashrom_supported_flash_chips; + flashrom_supported_programmers flashrom_version_info; flashrom_wp_cfg_new; flashrom_wp_cfg_release; diff --git a/tests/libflashrom.c b/tests/libflashrom.c index 3d726c8..173c8a9 100644 --- a/tests/libflashrom.c +++ b/tests/libflashrom.c @@ -28,4 +28,16 @@ while (*(ptr++)){} flashrom_data_free(array); assert_int_not_equal(ptr - array, 0); + +#if CONFIG_DUMMY == 1 + struct flashrom_programmer *flashprog = NULL; + flashrom_programmer_init(&flashprog, "dummy", "emulate=SST25VF040.REMS"); + array = flashrom_programmer_capabilities(flashprog); + ptr = array; + assert_non_null(array); + while (*(ptr++)){} + flashrom_data_free(array); + assert_int_equal(ptr - array, 2); + flashrom_programmer_shutdown(flashprog); +#endif } diff --git a/tests/tests.c b/tests/tests.c index 2a4f597..49d8c0a 100644 --- a/tests/tests.c +++ b/tests/tests.c @@ -438,11 +438,6 @@ }; ret |= cmocka_run_group_tests_name("flashrom.c tests", flashrom_tests, NULL, NULL); - const struct CMUnitTest libflashrom_tests[] = { - cmocka_unit_test(enumerators_test_success), - }; - ret |= cmocka_run_group_tests_name("libflashrom.c tests", libflashrom_tests, NULL, NULL); - const struct CMUnitTest spi25_tests[] = { cmocka_unit_test(spi_write_enable_test_success), cmocka_unit_test(spi_write_disable_test_success), @@ -458,6 +453,11 @@ }; ret |= cmocka_run_group_tests_name("spi25.c tests", spi25_tests, NULL, NULL); + const struct CMUnitTest libflashrom_tests[] = { + cmocka_unit_test(enumerators_test_success), + }; + ret |= cmocka_run_group_tests_name("libflashrom.c tests", libflashrom_tests, NULL, NULL); + const struct CMUnitTest lifecycle_tests[] = { cmocka_unit_test(dummy_basic_lifecycle_test_success), cmocka_unit_test(dummy_probe_lifecycle_test_success), -- To view, visit https://review.coreboot.org/c/flashrom/+/86922?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: newchange Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I13c951f9404ae8b042cdb572e6117a09ac231747 Gerrit-Change-Number: 86922 Gerrit-PatchSet: 1 Gerrit-Owner: Dmitry Zhadinets <dzhadinets(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>

1 0

[M] Change in flashrom[main]: libflashrom: Added API to enumerate supported programmers
by Dmitry Zhadinets (Code Review) 19 Mar '25

19 Mar '25

Dmitry Zhadinets has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/86921?usp=email ) Change subject: libflashrom: Added API to enumerate supported programmers ...................................................................... libflashrom: Added API to enumerate supported programmers Change-Id: Ib5275b742b849183b1fe701900040fee369a1d78 Signed-off-by: Dmitry Zhadinets <dzhadinets(a)gmail.com> --- M include/libflashrom.h M libflashrom.c A tests/libflashrom.c M tests/meson.build M tests/tests.c M tests/tests.h 6 files changed, 58 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/21/86921/1 diff --git a/include/libflashrom.h b/include/libflashrom.h index 41c7424..e3893ac 100644 --- a/include/libflashrom.h +++ b/include/libflashrom.h @@ -162,6 +162,12 @@ */ const char *flashrom_version_info(void); /** + * @brief Returns list of supported programmers + * @return List of supported programmers, or NULL if an error occurred. + * The pointer must be freed after using flashrom_data_free + */ +const char ** flashrom_supported_programmers(void); +/** * @brief Returns list of supported flash chips * @return List of supported flash chips, or NULL if an error occurred */ diff --git a/libflashrom.c b/libflashrom.c index 8cc38c4..9f177d7 100644 --- a/libflashrom.c +++ b/libflashrom.c @@ -144,6 +144,18 @@ return flashrom_version; } +const char ** flashrom_supported_programmers(void) +{ + const char ** result = malloc(sizeof(char*) * (programmer_table_size + 1)); + if (!result) + return NULL; + for (unsigned int i = 0; i < programmer_table_size; ++i) { + result[i] = programmer_table[i]->name; + } + result[programmer_table_size] = 0; + return result; +} + struct flashrom_flashchip_info *flashrom_supported_flash_chips(void) { struct flashrom_flashchip_info *supported_flashchips = diff --git a/tests/libflashrom.c b/tests/libflashrom.c new file mode 100644 index 0000000..3d726c8 --- /dev/null +++ b/tests/libflashrom.c @@ -0,0 +1,31 @@ +/* + * This file is part of the flashrom project. + * + * Copyright 2020 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 + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + + #include <stdlib.h> + + #include <include/test.h> + #include "tests.h" + #include "libflashrom.h" + +void enumerators_test_success(void **state) +{ + (void) state; /* unused */ + const char** array = flashrom_supported_programmers(); + const char** ptr = array; + assert_non_null(array); + while (*(ptr++)){} + flashrom_data_free(array); + assert_int_not_equal(ptr - array, 0); +} diff --git a/tests/meson.build b/tests/meson.build index e788b80..77dab0f 100644 --- a/tests/meson.build +++ b/tests/meson.build @@ -17,6 +17,7 @@ 'libusb_wraps.c', 'helpers.c', 'flashrom.c', + 'libflashrom.c', 'spi25.c', 'lifecycle.c', 'layout.c', diff --git a/tests/tests.c b/tests/tests.c index 72b4868..2a4f597 100644 --- a/tests/tests.c +++ b/tests/tests.c @@ -438,6 +438,11 @@ }; ret |= cmocka_run_group_tests_name("flashrom.c tests", flashrom_tests, NULL, NULL); + const struct CMUnitTest libflashrom_tests[] = { + cmocka_unit_test(enumerators_test_success), + }; + ret |= cmocka_run_group_tests_name("libflashrom.c tests", libflashrom_tests, NULL, NULL); + const struct CMUnitTest spi25_tests[] = { cmocka_unit_test(spi_write_enable_test_success), cmocka_unit_test(spi_write_disable_test_success), diff --git a/tests/tests.h b/tests/tests.h index 85c4f52..04a6e34 100644 --- a/tests/tests.h +++ b/tests/tests.h @@ -31,6 +31,9 @@ /* flashrom.c */ void flashbuses_to_text_test_success(void **state); +/* libflashrom.c */ +void enumerators_test_success(void **state); + /* spi25.c */ void spi_write_enable_test_success(void **state); void spi_write_disable_test_success(void **state); -- To view, visit https://review.coreboot.org/c/flashrom/+/86921?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: newchange Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: Ib5275b742b849183b1fe701900040fee369a1d78 Gerrit-Change-Number: 86921 Gerrit-PatchSet: 1 Gerrit-Owner: Dmitry Zhadinets <dzhadinets(a)gmail.com>

1 0

[M] Change in flashrom[main]: libflashrom: Update the API for Logger Callback
by Peter Marheine (Code Review) 17 Mar '25

17 Mar '25

Attention is currently required from: Anastasia Klimchuk, Dmitry Zhadinets. Peter Marheine has posted comments on this change by Dmitry Zhadinets. ( https://review.coreboot.org/c/flashrom/+/86875?usp=email ) Change subject: libflashrom: Update the API for Logger Callback ...................................................................... Patch Set 4: (3 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/86875/comment/45aeb5f8_2d474f9d?us… : PS2, Line 7: libflashrom: Update the API for Logger Callback > Honestly, I've copied the title from your patch about the progress. […] I agree that the new API should appear fully-formed, not with two different ABIs (that is, there should be one commit that creates the new API in its final form). The only question to answer is whether we want to delegate formatting to the user or do it in the library. File libflashrom.c: https://review.coreboot.org/c/flashrom/+/86875/comment/92bb8a11_c8a58df8?us… : PS2, Line 59: /* Dynamic messages and double formatting may not be ideal, : as they are slow. It would be better to limit the message : size to a fixed value (e.g., char message[1024] = {0};). */ : char *message; : va_list args_attempt; : va_copy(args_attempt, args); : int size = vsnprintf(NULL, 0, format, args_attempt); : va_end(args_attempt); : if (size < 0) : return size; : message = malloc(size+1); : if (!message) : return -ENOMEM; : vsnprintf(message, size + 1, format, args); : global_log_callback_v2(level, message, global_log_user_data); : free(message); : return 0; > So, this is the main thing to discuss. […] I think it's better to do the formatting inside libflashrom, and just pass strings. Currently we don't even clearly document that it's a printf-style format string, but even if we were to do that it's not always easy for callers to deal with varargs or printf: if somebody were using FFI bindings to, say, Python or Rust then it would be difficult to handle a `va_list` and printf-style format string without adding some custom-built C code as a shim. https://review.coreboot.org/c/flashrom/+/86875/comment/34ca3b37_8c3ccc3e?us… : PS2, Line 79: : global_log_callback = flashrom_log_callback_v2_handler; > *Set means _replace_ here, flashrom_set_log_callback checks nothing I agree that the current approach (having only one callback stored, and expressing the old API in terms of the new one) is the better way to do it. That was less feasible with the progress API, but I think it works well here. In an application that might support both the old and new APIs, there's no issue in knowing which is in use because the prototype for the v2 function will be missing if it's not supported at build-time and otherwise v2 is assumed to work. -- To view, visit https://review.coreboot.org/c/flashrom/+/86875?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: Iea738bd371fa3d69b9cf222c89ee67490d30af39 Gerrit-Change-Number: 86875 Gerrit-PatchSet: 4 Gerrit-Owner: Dmitry Zhadinets <dzhadinets(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Dmitry Zhadinets <dzhadinets(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Mon, 17 Mar 2025 22:46:00 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Dmitry Zhadinets <dzhadinets(a)gmail.com> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>

1 0

[M] Change in flashrom[main]: libflashrom: Update the API for Logger Callback
by Dmitry Zhadinets (Code Review) 17 Mar '25

17 Mar '25

Attention is currently required from: Anastasia Klimchuk, Dmitry Zhadinets, Peter Marheine. Hello Anastasia Klimchuk, Peter Marheine, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/86875?usp=email to look at the new patch set (#4). The following approvals got outdated and were removed: Verified-1 by build bot (Jenkins) Change subject: libflashrom: Update the API for Logger Callback ...................................................................... libflashrom: Update the API for Logger Callback The initial implementation does not account for user_data, requiring the calling application to use a global scope. This may lead to issues related to object lifecycle management and other architectural concerns. This patch adds user_data to the user’s log callback. Moreover, it performs message formatting, so the application only needs to pass the formatted string to the selected output. The change does not break the existing logging API but extends it. A new API version is introduced with the v2 suffix. Testing: Both unit tests and CLI tools serve as libflashrom clients. All unit tests run successfully. Change-Id: Iea738bd371fa3d69b9cf222c89ee67490d30af39 Signed-off-by: Dmitry Zhadinets <dzhadinets(a)gmail.com> --- M include/libflashrom.h M libflashrom.c M libflashrom.map A tests/libflashrom.c M tests/meson.build M tests/tests.c M tests/tests.h 7 files changed, 128 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/75/86875/4 -- To view, visit https://review.coreboot.org/c/flashrom/+/86875?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: newpatchset Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: Iea738bd371fa3d69b9cf222c89ee67490d30af39 Gerrit-Change-Number: 86875 Gerrit-PatchSet: 4 Gerrit-Owner: Dmitry Zhadinets <dzhadinets(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Dmitry Zhadinets <dzhadinets(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org>

1 0

[M] Change in flashrom[main]: libflashrom: Update the API for Logger Callback
by Dmitry Zhadinets (Code Review) 17 Mar '25

17 Mar '25

Attention is currently required from: Anastasia Klimchuk, Dmitry Zhadinets, Peter Marheine. Dmitry Zhadinets has posted comments on this change by Dmitry Zhadinets. ( https://review.coreboot.org/c/flashrom/+/86875?usp=email ) Change subject: libflashrom: Update the API for Logger Callback ...................................................................... Patch Set 3: (2 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/86875/comment/fd737ea1_ed20fb1a?us… : PS2, Line 15: Testing: Both unit tests and CLI tools serve as libflashrom clients. : All unit tests run successfully. > Yes, but you are not adding new tests: so v2 is not tested. […] Done I did not find any tests of API https://review.coreboot.org/c/flashrom/+/86875/comment/fd8f6d1f_f75e2ca6?us… : PS2, Line 9: The initial implementation does not account for user_data, requiring the calling application to use a global scope. This may lead to issues related to object lifecycle management and other architectural concerns. : : This patch adds user_data to the user’s log callback. Moreover, it performs message formatting, so the application only needs to pass the formatted string to the selected output. : : The change does not break the existing logging API but extends it. A new API version is introduced with the v2 suffix. : : Testing: Both unit tests and CLI tools serve as libflashrom clients. : All unit tests run successfully. > We wrap commit message by 72 chars width, I feel these lines are longer […] Done -- To view, visit https://review.coreboot.org/c/flashrom/+/86875?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: Iea738bd371fa3d69b9cf222c89ee67490d30af39 Gerrit-Change-Number: 86875 Gerrit-PatchSet: 3 Gerrit-Owner: Dmitry Zhadinets <dzhadinets(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Dmitry Zhadinets <dzhadinets(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Comment-Date: Mon, 17 Mar 2025 20:59:47 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>

1 0

[M] Change in flashrom[main]: libflashrom: Update the API for Logger Callback
by Dmitry Zhadinets (Code Review) 17 Mar '25

17 Mar '25

Attention is currently required from: Anastasia Klimchuk, Peter Marheine. Hello Anastasia Klimchuk, Peter Marheine, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/86875?usp=email to look at the new patch set (#3). The following approvals got outdated and were removed: Verified+1 by build bot (Jenkins) Change subject: libflashrom: Update the API for Logger Callback ...................................................................... libflashrom: Update the API for Logger Callback The initial implementation does not account for user_data, requiring the calling application to use a global scope. This may lead to issues related to object lifecycle management and other architectural concerns. This patch adds user_data to the user’s log callback. Moreover, it performs message formatting, so the application only needs to pass the formatted string to the selected output. The change does not break the existing logging API but extends it. A new API version is introduced with the v2 suffix. Testing: Both unit tests and CLI tools serve as libflashrom clients. All unit tests run successfully. Change-Id: Iea738bd371fa3d69b9cf222c89ee67490d30af39 Signed-off-by: Dmitry Zhadinets <dzhadinets(a)gmail.com> --- M include/libflashrom.h M libflashrom.c M libflashrom.map A tests/libflashrom.c M tests/meson.build M tests/tests.c M tests/tests.h 7 files changed, 125 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/75/86875/3 -- To view, visit https://review.coreboot.org/c/flashrom/+/86875?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: newpatchset Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: Iea738bd371fa3d69b9cf222c89ee67490d30af39 Gerrit-Change-Number: 86875 Gerrit-PatchSet: 3 Gerrit-Owner: Dmitry Zhadinets <dzhadinets(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org>

1 0

[M] Change in flashrom[main]: libflashrom: Update the API for Logger Callback
by Dmitry Zhadinets (Code Review) 17 Mar '25

17 Mar '25

Attention is currently required from: Anastasia Klimchuk, Peter Marheine. Dmitry Zhadinets has posted comments on this change by Dmitry Zhadinets. ( https://review.coreboot.org/c/flashrom/+/86875?usp=email ) Change subject: libflashrom: Update the API for Logger Callback ...................................................................... Patch Set 2: (1 comment) File libflashrom.c: https://review.coreboot.org/c/flashrom/+/86875/comment/889ee33e_32699c42?us… : PS2, Line 79: : global_log_callback = flashrom_log_callback_v2_handler; > I've implemented the new callback utilizing the old one. […] *Set means _replace_ here, flashrom_set_log_callback checks nothing -- To view, visit https://review.coreboot.org/c/flashrom/+/86875?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: Iea738bd371fa3d69b9cf222c89ee67490d30af39 Gerrit-Change-Number: 86875 Gerrit-PatchSet: 2 Gerrit-Owner: Dmitry Zhadinets <dzhadinets(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Comment-Date: Mon, 17 Mar 2025 16:14:37 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Dmitry Zhadinets <dzhadinets(a)gmail.com> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>

1 0

[M] Change in flashrom[main]: libflashrom: Update the API for Logger Callback
by Dmitry Zhadinets (Code Review) 17 Mar '25

17 Mar '25

Attention is currently required from: Anastasia Klimchuk, Peter Marheine. Dmitry Zhadinets has posted comments on this change by Dmitry Zhadinets. ( https://review.coreboot.org/c/flashrom/+/86875?usp=email ) Change subject: libflashrom: Update the API for Logger Callback ...................................................................... Patch Set 2: (3 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/86875/comment/b5bb1771_919181a8?us… : PS2, Line 7: libflashrom: Update the API for Logger Callback > You need to be more concrete, and also one commit should do one thing. […] Honestly, I've copied the title from your patch about the progress. But let me explain. Update API means here: to add new function, new prototype of callback function, and probably to add flashrom_set_loglevel (this is another commit if it will be implemented). BTW. please see my question bellow about it. All of these are about logging and is in the single patch. I do not think it is good to have 2 patches initially with callback prototype with vargs and then with char* you may like my approach or not. this is what we are talking about File libflashrom.c: https://review.coreboot.org/c/flashrom/+/86875/comment/2655f3e5_8207a85d?us… : PS2, Line 59: /* Dynamic messages and double formatting may not be ideal, : as they are slow. It would be better to limit the message : size to a fixed value (e.g., char message[1024] = {0};). */ : char *message; : va_list args_attempt; : va_copy(args_attempt, args); : int size = vsnprintf(NULL, 0, format, args_attempt); : va_end(args_attempt); : if (size < 0) : return size; : message = malloc(size+1); : if (!message) : return -ENOMEM; : vsnprintf(message, size + 1, format, args); : global_log_callback_v2(level, message, global_log_user_data); : free(message); : return 0; > I am not sure this belongs to the library. […] So, this is the main thing to discuss. the prototype of the logger callback is like glib has https://docs.gtk.org/glib/callback.LogFunc.html. And many other libraries. I wanted to add log domain as well (it is convenient way to highlight where the message is from and adds good way to filter logs) but it is mostly used in the libraries with huge number of logs and, moreover, will lead to the big change in the flashrom itself. This is not me wants to use the formatting. This is how it is used everywhere, from my point of view. Usually, libraries have API like ... format, varg ... to provide an option for some custom formatting of the datatypes within the fmt. Does flashrom have some specific messages with custom args that can be treated by the client some other way than POSIX's vsnprintf do? I can do not know something. Please correct me if I'm wrong. even flashrom_print_cb works the same way. Moreover it looks like there is a bug here in update_line_state. because of does not care about vargs. Will it be ok if format is "some %s", and vargs is "string\n"? As far as I see there is no such cases in the code but it's a potential glitch in the terminal https://review.coreboot.org/c/flashrom/+/86875/comment/6500966c_3355c191?us… : PS2, Line 79: : global_log_callback = flashrom_log_callback_v2_handler; > I don't think you need it here: the v2 should set `global_log_callback_v2` only […] I've implemented the new callback utilizing the old one. And did not change something in the mechanism of publishing messages itself. If I would do so, just imaging what change will be in print function with my prototype of callback (with formatting) and maybe somewhere else (I'm not familiar with the whole codebase of flashrom yet). And yes, this is an idea there are two options for callback (v1 and v2), and only one of them can be used. Set means replays here, flashrom_set_log_callback checks nothing -- To view, visit https://review.coreboot.org/c/flashrom/+/86875?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: Iea738bd371fa3d69b9cf222c89ee67490d30af39 Gerrit-Change-Number: 86875 Gerrit-PatchSet: 2 Gerrit-Owner: Dmitry Zhadinets <dzhadinets(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Comment-Date: Mon, 17 Mar 2025 16:01:59 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>

1 0

2025

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit March 2025