April 2024 - flashrom-gerrit - mail.coreboot.org

[L] Change in flashrom[main]: udelay: only use OS time for delays, except on DOS
by Peter Marheine (Code Review) April 25, 2024

April 25, 2024

Peter Marheine has submitted this change. ( https://review.coreboot.org/c/flashrom/+/81545?usp=email ) Change subject: udelay: only use OS time for delays, except on DOS ...................................................................... udelay: only use OS time for delays, except on DOS As proposed on the mailing list ("RFC: remove the calibrated delay loop" [1]), this removes the calibrated delay loop and uses OS-based timing functions for all delays because the calibrated delay loop can delay for shorter times than intended. When sleeping this now uses nanosleep() unconditionally, since usleep was only used on DOS (where DJGPP lacks nanosleep). When busy-looping, it uses clock_gettime() with CLOCK_MONOTONIC or CLOCK_REALTIME depending on availability, and gettimeofday() otherwise. The calibrated delay loop is retained for DOS only, because timer resolution on DJGPP is only about 50 milliseconds. Since typical delays in flashrom are around 10 microseconds, using OS timing there would regress performance by around 500x. The old implementation is reused with some branches removed based on the knowledge that timer resolution will not be better than about 50 milliseconds. Tested by reading and writing flash on several Intel and AMD systems: * Lenovo P920 (Intel C620, read/verify only) * "nissa" chromebook (Intel Alder Lake-N) * "zork" chromebook (AMD Zen+) [1]: https://mail.coreboot.org/hyperkitty/list/flashrom@flashrom.org/thread/HFH6… Signed-off-by: Peter Marheine <pmarheine(a)chromium.org> Change-Id: I7ac5450d194a475143698d65d64d8bcd2fd25e3f Reviewed-on: https://review.coreboot.org/c/flashrom/+/81545 Reviewed-by: Anastasia Klimchuk <aklm(a)chromium.org> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Brian Norris <briannorris(a)chromium.org> --- M Makefile M include/programmer.h M libflashrom.c M meson.build M tests/meson.build M tests/tests.c M tests/tests.h A tests/udelay.c M udelay.c A udelay_dos.c 10 files changed, 268 insertions(+), 178 deletions(-) Approvals: build bot (Jenkins): Verified Brian Norris: Looks good to me, but someone else must approve Anastasia Klimchuk: Looks good to me, approved diff --git a/Makefile b/Makefile index 1c0904b..9b94e22 100644 --- a/Makefile +++ b/Makefile @@ -388,9 +388,14 @@ ############################################################################### # Library code. -LIB_OBJS = libflashrom.o layout.o erasure_layout.o flashrom.o udelay.o parallel.o programmer.o programmer_table.o \ +LIB_OBJS = libflashrom.o layout.o erasure_layout.o flashrom.o parallel.o programmer.o programmer_table.o \ helpers.o helpers_fileio.o ich_descriptors.o fmap.o platform/endian_$(ENDIAN).o platform/memaccess.o +ifeq ($(TARGET_OS), DOS) + LIB_OBJS += udelay_dos.o +else + LIB_OBJS += udelay.o +endif ############################################################################### # Frontend related stuff. diff --git a/include/programmer.h b/include/programmer.h index 15df0d4..939c8de 100644 --- a/include/programmer.h +++ b/include/programmer.h @@ -219,8 +219,6 @@ #endif /* udelay.c */ -void myusec_delay(unsigned int usecs); -void myusec_calibrate_delay(void); void internal_sleep(unsigned int usecs); void default_delay(unsigned int usecs); diff --git a/libflashrom.c b/libflashrom.c index 4a59e2a..7c885cf 100644 --- a/libflashrom.c +++ b/libflashrom.c @@ -36,7 +36,6 @@ { if (perform_selfcheck && selfcheck()) return 1; - myusec_calibrate_delay(); return 0; } diff --git a/meson.build b/meson.build index 7c46de4..c4d7717 100644 --- a/meson.build +++ b/meson.build @@ -100,13 +100,19 @@ 'sst49lfxxxc.c', 'sst_fwhub.c', 'stm50.c', - 'udelay.c', 'w29ee011.c', 'w39.c', 'writeprotect.c', 'writeprotect_ranges.c', ) +# Select an appropriate delay implementation for the target OS +delay_src = files('udelay.c') +if target_machine.system() == 'dos' + delay_src = files('udelay_dos.c') +endif +srcs += delay_src + # check for required symbols if cc.has_function('clock_gettime') add_project_arguments('-DHAVE_CLOCK_GETTIME=1', language : 'c') diff --git a/tests/meson.build b/tests/meson.build index fd05166..e8de4b7 100644 --- a/tests/meson.build +++ b/tests/meson.build @@ -25,6 +25,7 @@ 'selfcheck.c', 'io_real.c', 'erase_func_algo.c', + 'udelay.c', ) if not programmer.get('dummy').get('active') diff --git a/tests/tests.c b/tests/tests.c index c38a66c..aa8353a 100644 --- a/tests/tests.c +++ b/tests/tests.c @@ -504,6 +504,11 @@ }; ret |= cmocka_run_group_tests_name("chip.c tests", chip_tests, NULL, NULL); + const struct CMUnitTest delay_tests[] = { + cmocka_unit_test(udelay_test_short), + }; + ret |= cmocka_run_group_tests_name("udelay.c tests", delay_tests, NULL, NULL); + size_t n_erase_tests; struct CMUnitTest *erase_func_algo_tests = get_erase_func_algo_tests(&n_erase_tests); ret |= _cmocka_run_group_tests("erase_func_algo.c tests", erase_func_algo_tests, n_erase_tests, NULL, NULL); diff --git a/tests/tests.h b/tests/tests.h index af5f968..c22da12 100644 --- a/tests/tests.h +++ b/tests/tests.h @@ -110,4 +110,7 @@ void erase_function_algo_test_success(void **state); void write_function_algo_test_success(void **state); +/* udelay.c */ +void udelay_test_short(void **state); + #endif /* TESTS_H */ diff --git a/tests/udelay.c b/tests/udelay.c new file mode 100644 index 0000000..277a15d --- /dev/null +++ b/tests/udelay.c @@ -0,0 +1,51 @@ +/* + * This file is part of the flashrom project. + * + * Copyright 2024 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 <include/test.h> +#include <stdint.h> +#include <sys/time.h> +#include <time.h> + +#include "programmer.h" +#include "tests.h" + +static uint64_t now_us(void) { +#if HAVE_CLOCK_GETTIME == 1 + struct timespec ts; + + clock_gettime(CLOCK_MONOTONIC, &ts); + return (ts.tv_nsec / 1000) + (ts.tv_sec * 1000000); +#else + struct timeval tv; + + gettimeofday(&tv, NULL); + return tv.tv_usec + (tv.tv_sec * 1000000); +#endif +} + +/* + * A short delay should delay for at least as long as requested, + * and more than 10x as long would be worrisome. + * + * This test could fail spuriously on a heavily-loaded system, or if we need + * to use gettimeofday() and a time change (such as DST) occurs during the + * test. + */ +void udelay_test_short(void **state) { + uint64_t start = now_us(); + default_delay(100); + uint64_t elapsed = now_us() - start; + + assert_in_range(elapsed, 100, 1000); +} diff --git a/udelay.c b/udelay.c index cb7a9db..453cb34 100644 --- a/udelay.c +++ b/udelay.c @@ -3,6 +3,7 @@ * * Copyright (C) 2000 Silicon Integrated System Corporation * Copyright (C) 2009,2010 Carl-Daniel Hailfinger + * Copyright (C) 2024 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 @@ -27,20 +28,25 @@ #include "flash.h" #include "programmer.h" -static bool use_clock_gettime = false; - #if HAVE_CLOCK_GETTIME == 1 -#ifdef _POSIX_MONOTONIC_CLOCK -static clockid_t clock_id = CLOCK_MONOTONIC; -#else -static clockid_t clock_id = CLOCK_REALTIME; -#endif - static void clock_usec_delay(int usecs) { + static clockid_t clock_id = +#ifdef _POSIX_MONOTONIC_CLOCK + CLOCK_MONOTONIC; +#else + CLOCK_REALTIME; +#endif + struct timespec now; - clock_gettime(clock_id, &now); + if (clock_gettime(clock_id, &now)) { + /* Fall back to realtime clock if monotonic doesn't work */ + if (clock_id != CLOCK_REALTIME && errno == EINVAL) { + clock_id = CLOCK_REALTIME; + clock_gettime(clock_id, &now); + } + } const long end_nsec = now.tv_nsec + usecs * 1000L; const struct timespec end = { @@ -52,183 +58,35 @@ } while (now.tv_sec < end.tv_sec || (now.tv_sec == end.tv_sec && now.tv_nsec < end.tv_nsec)); } -static int clock_check_res(void) -{ - struct timespec res; - if (!clock_getres(clock_id, &res)) { - if (res.tv_sec == 0 && res.tv_nsec <= 100) { - msg_pinfo("Using clock_gettime for delay loops (clk_id: %d, resolution: %ldns).\n", - (int)clock_id, res.tv_nsec); - use_clock_gettime = true; - return 1; - } - } else if (clock_id != CLOCK_REALTIME && errno == EINVAL) { - /* Try again with CLOCK_REALTIME. */ - clock_id = CLOCK_REALTIME; - return clock_check_res(); - } - return 0; -} #else -static inline void clock_usec_delay(int usecs) {} -static inline int clock_check_res(void) { return 0; } - -#endif /* HAVE_CLOCK_GETTIME == 1 */ - -/* loops per microsecond */ -static unsigned long micro = 1; - -__attribute__ ((noinline)) void myusec_delay(unsigned int usecs) -{ - unsigned long i; - for (i = 0; i < usecs * micro; i++) { - /* Make sure the compiler doesn't optimize the loop away. */ - __asm__ volatile ("" : : "rm" (i) ); - } -} - -static unsigned long measure_os_delay_resolution(void) -{ - unsigned long timeusec; +static void clock_usec_delay(unsigned int usecs) { struct timeval start, end; - unsigned long counter = 0; + unsigned long elapsed = 0; gettimeofday(&start, NULL); - timeusec = 0; - while (!timeusec && (++counter < 1000000000)) { + while (elapsed < usecs) { gettimeofday(&end, NULL); - timeusec = 1000000 * (end.tv_sec - start.tv_sec) + + elapsed = 1000000 * (end.tv_sec - start.tv_sec) + (end.tv_usec - start.tv_usec); /* Protect against time going forward too much. */ if ((end.tv_sec > start.tv_sec) && ((end.tv_sec - start.tv_sec) >= LONG_MAX / 1000000 - 1)) - timeusec = 0; + elapsed = 0; /* Protect against time going backwards during leap seconds. */ - if ((end.tv_sec < start.tv_sec) || (timeusec > LONG_MAX)) - timeusec = 0; + if ((end.tv_sec < start.tv_sec) || (elapsed > LONG_MAX)) + elapsed = 0; } - return timeusec; } -static unsigned long measure_delay(unsigned int usecs) -{ - unsigned long timeusec; - struct timeval start, end; - - gettimeofday(&start, NULL); - myusec_delay(usecs); - gettimeofday(&end, NULL); - timeusec = 1000000 * (end.tv_sec - start.tv_sec) + - (end.tv_usec - start.tv_usec); - /* Protect against time going forward too much. */ - if ((end.tv_sec > start.tv_sec) && - ((end.tv_sec - start.tv_sec) >= LONG_MAX / 1000000 - 1)) - timeusec = LONG_MAX; - /* Protect against time going backwards during leap seconds. */ - if ((end.tv_sec < start.tv_sec) || (timeusec > LONG_MAX)) - timeusec = 1; - - return timeusec; -} - -void myusec_calibrate_delay(void) -{ - if (clock_check_res()) - return; - - unsigned long count = 1000; - unsigned long timeusec, resolution; - int i, tries = 0; - - msg_pinfo("Calibrating delay loop... "); - resolution = measure_os_delay_resolution(); - if (resolution) { - msg_pdbg("OS timer resolution is %lu usecs, ", resolution); - } else { - msg_pinfo("OS timer resolution is unusable. "); - } - -recalibrate: - count = 1000; - while (1) { - timeusec = measure_delay(count); - if (timeusec > 1000000 / 4) - break; - if (count >= ULONG_MAX / 2) { - msg_pinfo("timer loop overflow, reduced precision. "); - break; - } - count *= 2; - } - tries ++; - - /* Avoid division by zero, but in that case the loop is shot anyway. */ - if (!timeusec) - timeusec = 1; - - /* Compute rounded up number of loops per microsecond. */ - micro = (count * micro) / timeusec + 1; - msg_pdbg("%luM loops per second, ", micro); - - /* Did we try to recalibrate less than 5 times? */ - if (tries < 5) { - /* Recheck our timing to make sure we weren't just hitting - * a scheduler delay or something similar. - */ - for (i = 0; i < 4; i++) { - if (resolution && (resolution < 10)) { - timeusec = measure_delay(100); - } else if (resolution && - (resolution < ULONG_MAX / 200)) { - timeusec = measure_delay(resolution * 10) * - 100 / (resolution * 10); - } else { - /* This workaround should be active for broken - * OS and maybe libpayload. The criterion - * here is horrible or non-measurable OS timer - * resolution which will result in - * measure_delay(100)=0 whereas a longer delay - * (1000 ms) may be sufficient - * to get a nonzero time measurement. - */ - timeusec = measure_delay(1000000) / 10000; - } - if (timeusec < 90) { - msg_pdbg("delay more than 10%% too short (got " - "%lu%% of expected delay), " - "recalculating... ", timeusec); - goto recalibrate; - } - } - } else { - msg_perr("delay loop is unreliable, trying to continue "); - } - - /* We're interested in the actual precision. */ - timeusec = measure_delay(10); - msg_pdbg("10 myus = %ld us, ", timeusec); - timeusec = measure_delay(100); - msg_pdbg("100 myus = %ld us, ", timeusec); - timeusec = measure_delay(1000); - msg_pdbg("1000 myus = %ld us, ", timeusec); - timeusec = measure_delay(10000); - msg_pdbg("10000 myus = %ld us, ", timeusec); - timeusec = measure_delay(resolution * 4); - msg_pdbg("%ld myus = %ld us, ", resolution * 4, timeusec); - - msg_pinfo("OK.\n"); -} +#endif /* HAVE_CLOCK_GETTIME == 1 */ /* Not very precise sleep. */ void internal_sleep(unsigned int usecs) { #if IS_WINDOWS Sleep((usecs + 999) / 1000); -#elif defined(__DJGPP__) - sleep(usecs / 1000000); - usleep(usecs % 1000000); #else nanosleep(&(struct timespec){usecs / 1000000, (usecs * 1000) % 1000000000UL}, NULL); #endif @@ -240,21 +98,14 @@ /* If the delay is >0.1 s, use internal_sleep because timing does not need to be so precise. */ if (usecs > 100000) { internal_sleep(usecs); - } else if (use_clock_gettime) { - clock_usec_delay(usecs); } else { - myusec_delay(usecs); + clock_usec_delay(usecs); } } #else #include <libpayload.h> -void myusec_calibrate_delay(void) -{ - get_cpu_speed(); -} - void default_delay(unsigned int usecs) { udelay(usecs); diff --git a/udelay_dos.c b/udelay_dos.c new file mode 100644 index 0000000..4a29f73 --- /dev/null +++ b/udelay_dos.c @@ -0,0 +1,171 @@ +/* + * This file is part of the flashrom project. + * + * Copyright (C) 2000 Silicon Integrated System Corporation + * Copyright (C) 2009,2010 Carl-Daniel Hailfinger + * + * 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; either version 2 of the License, or + * (at your option) any later version. + * + * 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 <stdbool.h> +#include <unistd.h> +#include <errno.h> +#include <time.h> +#include <sys/time.h> +#include <stdlib.h> +#include <limits.h> +#include "flash.h" +#include "programmer.h" + +/* loops per microsecond */ +static unsigned long micro = 1; + +__attribute__ ((noinline)) void myusec_delay(unsigned int usecs) +{ + unsigned long i; + for (i = 0; i < usecs * micro; i++) { + /* Make sure the compiler doesn't optimize the loop away. */ + __asm__ volatile ("" : : "rm" (i) ); + } +} + +static unsigned long measure_os_delay_resolution(void) +{ + unsigned long timeusec; + struct timeval start, end; + unsigned long counter = 0; + + gettimeofday(&start, NULL); + timeusec = 0; + + while (!timeusec && (++counter < 1000000000)) { + gettimeofday(&end, NULL); + timeusec = 1000000 * (end.tv_sec - start.tv_sec) + + (end.tv_usec - start.tv_usec); + /* Protect against time going forward too much. */ + if ((end.tv_sec > start.tv_sec) && + ((end.tv_sec - start.tv_sec) >= LONG_MAX / 1000000 - 1)) + timeusec = 0; + /* Protect against time going backwards during leap seconds. */ + if ((end.tv_sec < start.tv_sec) || (timeusec > LONG_MAX)) + timeusec = 0; + } + return timeusec; +} + +static unsigned long measure_delay(unsigned int usecs) +{ + unsigned long timeusec; + struct timeval start, end; + + gettimeofday(&start, NULL); + myusec_delay(usecs); + gettimeofday(&end, NULL); + timeusec = 1000000 * (end.tv_sec - start.tv_sec) + + (end.tv_usec - start.tv_usec); + /* Protect against time going forward too much. */ + if ((end.tv_sec > start.tv_sec) && + ((end.tv_sec - start.tv_sec) >= LONG_MAX / 1000000 - 1)) + timeusec = LONG_MAX; + /* Protect against time going backwards during leap seconds. */ + if ((end.tv_sec < start.tv_sec) || (timeusec > LONG_MAX)) + timeusec = 1; + + return timeusec; +} + +static void myusec_calibrate_delay(void) +{ + unsigned long count = 1000; + unsigned long timeusec, resolution; + int i, tries = 0; + + msg_pinfo("Calibrating delay loop... "); + /* Timing resolution on DJGPP is about 50ms, but measure it precisely. */ + resolution = measure_os_delay_resolution(); + if (resolution) { + msg_pdbg("OS timer resolution is %lu usecs, ", resolution); + } else { + msg_pinfo("OS timer resolution is unusable. "); + } + +recalibrate: + count = 1000; + while (1) { + timeusec = measure_delay(count); + if (timeusec > 1000000 / 4) + break; + if (count >= ULONG_MAX / 2) { + msg_pinfo("timer loop overflow, reduced precision. "); + break; + } + count *= 2; + } + tries ++; + + /* Avoid division by zero, but in that case the loop is shot anyway. */ + if (!timeusec) + timeusec = 1; + + /* Compute rounded up number of loops per microsecond. */ + micro = (count * micro) / timeusec + 1; + msg_pdbg("%luM loops per second, ", micro); + + /* Did we try to recalibrate less than 5 times? */ + if (tries < 5) { + /* Recheck our timing to make sure we weren't just hitting + * a scheduler delay or something similar. + */ + for (i = 0; i < 4; i++) { + timeusec = measure_delay(resolution * 10) * + 100 / (resolution * 10); + + if (timeusec < 90) { + msg_pdbg("delay more than 10%% too short (got " + "%lu%% of expected delay), " + "recalculating... ", timeusec); + goto recalibrate; + } + } + } else { + msg_perr("delay loop is unreliable, trying to continue "); + } + + /* We're interested in the actual precision. */ + timeusec = measure_delay(resolution * 4); + msg_pdbg("%ld myus = %ld us, ", resolution * 4, timeusec); + + msg_pinfo("OK.\n"); +} + +/* Not very precise sleep. */ +void internal_sleep(unsigned int usecs) +{ + sleep(usecs / 1000000); + usleep(usecs % 1000000); +} + +/* Precise delay. */ +void default_delay(unsigned int usecs) +{ + static bool calibrated = false; + + /* If the delay is >0.1 s, use internal_sleep because timing does not need to be so precise. */ + if (usecs > 100000) { + internal_sleep(usecs); + } else { + if (!calibrated) { + myusec_calibrate_delay(); + calibrated = true; + } + myusec_delay(usecs); + } +} -- To view, visit https://review.coreboot.org/c/flashrom/+/81545?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I7ac5450d194a475143698d65d64d8bcd2fd25e3f Gerrit-Change-Number: 81545 Gerrit-PatchSet: 10 Gerrit-Owner: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Brian Norris <briannorris(a)chromium.org> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

[L] Change in flashrom[main]: udelay: only use OS time for delays, except on DOS
by Peter Marheine (Code Review) April 25, 2024

April 25, 2024

Attention is currently required from: Brian Norris, Thomas Heijligen. Peter Marheine has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/81545?usp=email ) Change subject: udelay: only use OS time for delays, except on DOS ...................................................................... Patch Set 9: (1 comment) Patchset: PS9: > Peter, I think you can wait for let's say another week, in case someone else wants to have a look, a […] Time's up, see y'all on the other side! -- To view, visit https://review.coreboot.org/c/flashrom/+/81545?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I7ac5450d194a475143698d65d64d8bcd2fd25e3f Gerrit-Change-Number: 81545 Gerrit-PatchSet: 9 Gerrit-Owner: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Brian Norris <briannorris(a)chromium.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Brian Norris <briannorris(a)chromium.org> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Comment-Date: Thu, 25 Apr 2024 23:22:55 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

[XS] Change in flashrom[main]: flashrom_tester: Correct "WP screw" message
by Brian Norris (Code Review) April 25, 2024

April 25, 2024

Attention is currently required from: Anastasia Klimchuk, Evan Benn, Hsuan-ting Chen, Thomas Heijligen. Hello Anastasia Klimchuk, Evan Benn, Hsuan-ting Chen, Thomas Heijligen, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/82083?usp=email to look at the new patch set (#5). Change subject: flashrom_tester: Correct "WP screw" message ...................................................................... flashrom_tester: Correct "WP screw" message flashrom_tester prints hints on how to modify hardware write protect state as follows: ... > connect the battery (and/or open the WP screw) ... > disconnect the battery (and/or open the WP screw) ... The first advice should be "[...] close the WP screw". TEST=`flashrom_tester --flashrom_binary=$(which flashrom) \ internal Erase_and_Write Fail_to_verify` Change-Id: I45f06db51e92e68bf724b13bdf5b31bba511d270 Signed-off-by: Brian Norris <briannorris(a)chromium.org> --- M util/flashrom_tester/src/utils.rs 1 file changed, 3 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/83/82083/5 -- To view, visit https://review.coreboot.org/c/flashrom/+/82083?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I45f06db51e92e68bf724b13bdf5b31bba511d270 Gerrit-Change-Number: 82083 Gerrit-PatchSet: 5 Gerrit-Owner: Brian Norris <briannorris(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Evan Benn <evanbenn(a)gmail.com> Gerrit-Reviewer: Hsuan-ting Chen <roccochen(a)google.com> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Evan Benn <evanbenn(a)gmail.com> Gerrit-Attention: Hsuan-ting Chen <roccochen(a)google.com> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

[XS] Change in flashrom[main]: flashrom_tester: Correct "WP screw" message
by Brian Norris (Code Review) April 25, 2024

April 25, 2024

Attention is currently required from: Evan Benn, Hsuan-ting Chen. Hello Evan Benn, Hsuan-ting Chen, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/82083?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: flashrom_tester: Correct "WP screw" message ...................................................................... flashrom_tester: Correct "WP screw" message flashrom_tester prints hints on how to modify hardware write protect state as follows: ... > connect the battery (and/or open the WP screw) ... > disconnect the battery (and/or open the WP screw) ... The first advice should be "[...] close the WP screw". TEST=`flashrom_tester --flashrom_binary=$(which flashrom) \ internal Erase_and_Write Fail_to_verify` Change-Id: I45f06db51e92e68bf724b13bdf5b31bba511d270 Signed-off-by: Brian Norris <briannorris(a)chromium.org> --- M README.rst M util/flashrom_tester/src/utils.rs 2 files changed, 4 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/83/82083/4 -- To view, visit https://review.coreboot.org/c/flashrom/+/82083?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I45f06db51e92e68bf724b13bdf5b31bba511d270 Gerrit-Change-Number: 82083 Gerrit-PatchSet: 4 Gerrit-Owner: Brian Norris <briannorris(a)chromium.org> Gerrit-Reviewer: Evan Benn <evanbenn(a)gmail.com> Gerrit-Reviewer: Hsuan-ting Chen <roccochen(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Evan Benn <evanbenn(a)gmail.com> Gerrit-Attention: Hsuan-ting Chen <roccochen(a)google.com> Gerrit-MessageType: newpatchset

1 0

[S] Change in flashrom[main]: flashchips: Add support for MXIC MX25L3239E
by Anastasia Klimchuk (Code Review) April 25, 2024

April 25, 2024

Attention is currently required from: DZ, Nikolai Artemiev, Stefan Reinauer. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/81563?usp=email ) Change subject: flashchips: Add support for MXIC MX25L3239E ...................................................................... Patch Set 6: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/81563?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: Ic7a848028fe937deb1bf83ef2a9dddf1330334b6 Gerrit-Change-Number: 81563 Gerrit-PatchSet: 6 Gerrit-Owner: DZ <danielzhang(a)mxic.com.cn> 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: DZ <danielzhang(a)mxic.com.cn> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Thu, 25 Apr 2024 09:16:52 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in flashrom[main]: flashchips: Add support for MXIC MX25L3239E
by DZ (Code Review) April 25, 2024

April 25, 2024

Attention is currently required from: Anastasia Klimchuk, Nikolai Artemiev, Stefan Reinauer. DZ has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/81563?usp=email ) Change subject: flashchips: Add support for MXIC MX25L3239E ...................................................................... Patch Set 6: (1 comment) File flashchips.c: https://review.coreboot.org/c/flashrom/+/81563/comment/3746a5f3_41b7b410 : PS3, Line 9523: 1024B > Daniel, I noticed on this patch (and few others that you currently have under review), you left unre […] Done -- To view, visit https://review.coreboot.org/c/flashrom/+/81563?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: Ic7a848028fe937deb1bf83ef2a9dddf1330334b6 Gerrit-Change-Number: 81563 Gerrit-PatchSet: 6 Gerrit-Owner: DZ <danielzhang(a)mxic.com.cn> 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: Thu, 25 Apr 2024 01:01:25 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

[XS] Change in flashrom[main]: flashrom_tester: Correct "WP screw" message
by Brian Norris (Code Review) April 24, 2024

April 24, 2024

Attention is currently required from: Evan Benn, Hsuan-ting Chen. Brian Norris has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/82083?usp=email ) Change subject: flashrom_tester: Correct "WP screw" message ...................................................................... Patch Set 3: (1 comment) Patchset: PS3: jenkins is rebuilding version 2. I don't know how to get it to rebuilds version 3 -- To view, visit https://review.coreboot.org/c/flashrom/+/82083?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I45f06db51e92e68bf724b13bdf5b31bba511d270 Gerrit-Change-Number: 82083 Gerrit-PatchSet: 3 Gerrit-Owner: Brian Norris <briannorris(a)chromium.org> Gerrit-Reviewer: Evan Benn <evanbenn(a)gmail.com> Gerrit-Reviewer: Hsuan-ting Chen <roccochen(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Evan Benn <evanbenn(a)gmail.com> Gerrit-Attention: Hsuan-ting Chen <roccochen(a)google.com> Gerrit-Comment-Date: Wed, 24 Apr 2024 21:51:56 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[XS] Change in flashrom[main]: flashrom: Don't throw around "delay 1 second" so lightly
by Brian Norris (Code Review) April 24, 2024

April 24, 2024

Attention is currently required from: Anastasia Klimchuk, Nikolai Artemiev, Peter Marheine. Brian Norris has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/80807?usp=email ) Change subject: flashrom: Don't throw around "delay 1 second" so lightly ...................................................................... Patch Set 5: (1 comment) Patchset: PS1: > I think that recommendation should be captured in some kind of changelog so there's "real" documentation of the change, though. Done > Erase operation verifies the chip straight away after erasing. Without any delay. (Also without respecting the noverify flags, but this we will fix later). > From the chip point of view, erase is almost the same as write, is that correct? > So maybe the erase flow is kind of a "proof-of-concept" for what this patch is doing? Yeah, you might have a point. I noted this in the commit message now. > I completely forgot that write and verify can be separated in a manual workflow, so silly of me. This changes the situation, maybe we don't need a special option. Cool, less work for me :) > > do you want me to start such a mailing list thread, or were you suggesting I do this? > > Brian, you gave me two options which are the same :) Haha, of course I had to fudge my words :) Anyway, I've done both of them now :) https://mail.coreboot.org/hyperkitty/list/flashrom@flashrom.org/thread/SFV3… -- To view, visit https://review.coreboot.org/c/flashrom/+/80807?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: Ie09651fede3f9f03425244c94a2da8bae00315fc Gerrit-Change-Number: 80807 Gerrit-PatchSet: 5 Gerrit-Owner: Brian Norris <briannorris(a)chromium.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-CC: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Comment-Date: Wed, 24 Apr 2024 21:18:37 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Brian Norris <briannorris(a)chromium.org> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Comment-In-Reply-To: Nikolai Artemiev <nartemiev(a)google.com> Comment-In-Reply-To: Peter Marheine <pmarheine(a)chromium.org> Gerrit-MessageType: comment

1 0

[XS] Change in flashrom[main]: flashrom: Don't throw around "delay 1 second" so lightly
by Brian Norris (Code Review) April 24, 2024

April 24, 2024

Attention is currently required from: Brian Norris, Nikolai Artemiev, Peter Marheine. Hello Nikolai Artemiev, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/80807?usp=email to look at the new patch set (#5). Change subject: flashrom: Don't throw around "delay 1 second" so lightly ...................................................................... flashrom: Don't throw around "delay 1 second" so lightly Waiting a full second is a very long time, especially when default_delay() chooses to busy-loop. This code has been around for a decade, with vague references to user reports: commit 8ab49e72af8465d4527de2ec37b22cd44f7a1169 Date: Wed Aug 19 13:55:34 2009 +0000 Disallow erase/write for known bad chips so people won't try without a clear understanding Still, this logic does not belong in the high-level library logic, used by all programmers and all chips. If there is a timing issue, it should either be encoded in the appropriate programmer or flashchip timing. Now, this change has some (hopefully [1] tiny) chance of regression, so we have the following considerations: 1. emergency_help_message() already provides documentation on how to contact support, in case we need to handle any user-reported regressions. 2. If there is any regression here, it's only in the --verify code; so we can always provide workarounds for testing this, to determine whether this change may have been at fault. For example, something like: flashrom --write /my/new/image.bin --noverify sleep 1 flashrom --read /tmp/bar.bin cmp /my/new/image.bin /tmp/bar.bin If such problems occur, we can collect system/programmer/chip info to try to encode a more targeted delay into the appropriate chip/programmer implementation, and avoid penalizing the entire project like this. 3. we already have (embedded in erase_write()) erase verification that performs no such delay. So depending on the type of timing error that this delay was attempting to cover, we may have some proof that this delay is no longer necessary (or at least, that whatever systems were needing this delay in the first place are no longer caring about flashrom). [1] Famous last words. BUG=b:325539928 TEST=`flashrom_tester --flashrom_binary=$(which flashrom) \ internal Erase_and_Write Fail_to_verify`, TEST=`vpd -i RW_VPD -s foo=bar; vpd -i RW_VPD -l; \ vpd -i RW_VPD -d foo; vpd -i RW_VPD -l` TEST=`elogtool list; elogtool add 0xa7; elogtool list` on (at least) 2 systems: #1: Kukui/Kakadu rev2 - MTD programmer / kernel 5.10.215-24542-g0515a679eb42 / CrOS ~ 15857.0.0 #2: Zork/Dirinboz rev2 - chip name: vendor="Winbond" name="W25Q128.JW.DTR" / BIOS: Google_Dirinboz.13434.688.0 / kernel 5.4.267-21940-g67f70e251a74 / CrOS ~ 15753.43.0 Change-Id: Ie09651fede3f9f03425244c94a2da8bae00315fc Signed-off-by: Brian Norris <briannorris(a)chromium.org> --- M flashrom.c 1 file changed, 0 insertions(+), 3 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/07/80807/5 -- To view, visit https://review.coreboot.org/c/flashrom/+/80807?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: Ie09651fede3f9f03425244c94a2da8bae00315fc Gerrit-Change-Number: 80807 Gerrit-PatchSet: 5 Gerrit-Owner: Brian Norris <briannorris(a)chromium.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-CC: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Attention: Brian Norris <briannorris(a)chromium.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

[XS] Change in flashrom[main]: flashrom: Don't throw around "delay 1 second" so lightly
by Brian Norris (Code Review) April 24, 2024

April 24, 2024

Attention is currently required from: Brian Norris, Nikolai Artemiev, Peter Marheine. Hello Nikolai Artemiev, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/80807?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: flashrom: Don't throw around "delay 1 second" so lightly ...................................................................... flashrom: Don't throw around "delay 1 second" so lightly Waiting a full second is a very long time, especially when default_delay() chooses to busy-loop. This code has been around for a decade, with vague references to user reports: commit 8ab49e72af8465d4527de2ec37b22cd44f7a1169 Date: Wed Aug 19 13:55:34 2009 +0000 Disallow erase/write for known bad chips so people won't try without a clear understanding Still, this logic does not belong in the high-level library logic, used by all programmers and all chips. If there is a timing issue, it should either be encoded in the appropriate programmer or flashchip timing. Now, this change has some (hopefully [1] tiny) chance of regression, so we have the following considerations: 1. emergency_help_message() already provides documentation on how to contact support, in case we need to handle any user-reported regressions. 2. If there is any regression here, it's only in the --verify code; so we can always provide workarounds for testing this, to determine whether this change may have been at fault. For example, something like: flashrom --read /tmp/foo.bin flashrom --write /my/new/image.bin --noverify sleep 1 flashrom --read /tmp/bar.bin cmp /tmp/foo.bin /tmp/bar.bin If such problems occur, we can collect system/programmer/chip info to try to encode a more targeted delay into the appropriate chip/programmer implementation, and avoid penalizing the entire project like this. 3. we already have (embedded in erase_write()) erase verification that performs no such delay. So depending on the type of timing error that this delay was attempting to cover, we may have some proof that this delay is no longer necessary (or at least, that whatever systems were needing this delay in the first place are no longer caring about flashrom). [1] Famous last words. BUG=b:325539928 TEST=`flashrom_tester --flashrom_binary=$(which flashrom) \ internal Erase_and_Write Fail_to_verify`, TEST=`vpd -i RW_VPD -s foo=bar; vpd -i RW_VPD -l; \ vpd -i RW_VPD -d foo; vpd -i RW_VPD -l` TEST=`elogtool list; elogtool add 0xa7; elogtool list` on (at least) 2 systems: #1: Kukui/Kakadu rev2 - MTD programmer / kernel 5.10.215-24542-g0515a679eb42 / CrOS ~ 15857.0.0 #2: Zork/Dirinboz rev2 - chip name: vendor="Winbond" name="W25Q128.JW.DTR" / BIOS: Google_Dirinboz.13434.688.0 / kernel 5.4.267-21940-g67f70e251a74 / CrOS ~ 15753.43.0 Change-Id: Ie09651fede3f9f03425244c94a2da8bae00315fc Signed-off-by: Brian Norris <briannorris(a)chromium.org> --- M flashrom.c 1 file changed, 0 insertions(+), 3 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/07/80807/4 -- To view, visit https://review.coreboot.org/c/flashrom/+/80807?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: Ie09651fede3f9f03425244c94a2da8bae00315fc Gerrit-Change-Number: 80807 Gerrit-PatchSet: 4 Gerrit-Owner: Brian Norris <briannorris(a)chromium.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-CC: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Attention: Brian Norris <briannorris(a)chromium.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-MessageType: newpatchset

1 0