February 2024 - flashrom-gerrit

Change in flashrom[master]: libflashrom.map: Add missing symbols
by Edward O'Callaghan (Code Review) April 7, 2025

April 7, 2025

Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/38993 ) Change subject: libflashrom.map: Add missing symbols ...................................................................... libflashrom.map: Add missing symbols The following symbols were missing at link-time from libflashrom: supported_flash_chips; supported_boards; supported_chipsets; Change-Id: I299928d62788c0abfe7f6cdd3e8ad8be0c49e45e Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M libflashrom.map 1 file changed, 3 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/93/38993/1 diff --git a/libflashrom.map b/libflashrom.map index d6dd24d..9430775 100644 --- a/libflashrom.map +++ b/libflashrom.map @@ -25,6 +25,9 @@ flashrom_set_log_callback; flashrom_shutdown; flashrom_supported_programmers; + flashrom_supported_flash_chips; + flashrom_supported_boards; + flashrom_supported_chipsets; flashrom_system_info; flashrom_version_info; local: *; -- To view, visit https://review.coreboot.org/c/flashrom/+/38993 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I299928d62788c0abfe7f6cdd3e8ad8be0c49e45e Gerrit-Change-Number: 38993 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: newchange

4 3

Change in flashrom[master]: tests/: Add programmer enumerate correspondence test
by Edward O'Callaghan (Code Review) Sept. 21, 2024

Sept. 21, 2024

Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/47356 ) Change subject: tests/: Add programmer enumerate correspondence test ...................................................................... tests/: Add programmer enumerate correspondence test The enumerate defines within programmer.h must be aligned with the programmer dispatch table within flashrom.c. The following unit-tests tries to improve coverage here to avoid issues like we saw when ENE and MEC1308 were upstreamed. BUG=none BRANCH=none TEST=meson test Change-Id: I13efc4561f743edf51326d4b408be8715b630377 Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M tests/flashrom.c M tests/tests.c M tests/tests.h 3 files changed, 94 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/56/47356/1 diff --git a/tests/flashrom.c b/tests/flashrom.c index 50464dd..9180922 100644 --- a/tests/flashrom.c +++ b/tests/flashrom.c @@ -15,6 +15,47 @@ #include <include/test.h> +/* must be before programmer.h include */ +#define CONFIG_INTERNAL 1 +#define CONFIG_DUMMY 1 +#define CONFIG_MEC1308 1 +#define CONFIG_NIC3COM 1 +#define CONFIG_NICREALTEK 1 +#define CONFIG_NICNATSEMI 1 +#define CONFIG_GFXNVIDIA 1 +#define CONFIG_RAIDEN 1 +#define CONFIG_DRKAISER 1 +#define CONFIG_SATASII 1 +#define CONFIG_ATAHPT 1 +#define CONFIG_ATAVIA 1 +#define CONFIG_ATAPROMISE 1 +#define CONFIG_IT8212 1 +#define CONFIG_FT2232_SPI 1 +#define CONFIG_SERPROG 1 +#define CONFIG_BUSPIRATE_SPI 1 +#define CONFIG_DEDIPROG 1 +#define CONFIG_DEVELOPERBOX_SPI 1 +#define CONFIG_ENE_LPC 1 +#define CONFIG_RAYER_SPI 1 +#define CONFIG_PONY_SPI 1 +#define CONFIG_NICINTEL 1 +#define CONFIG_NICINTEL_SPI 1 +#define CONFIG_NICINTEL_EEPROM 1 +#define CONFIG_OGP_SPI 1 +#define CONFIG_SATAMV 1 +#define CONFIG_LINUX_MTD 1 +#define CONFIG_LINUX_SPI 1 +#define CONFIG_LSPCON_I2C_SPI 1 +#define CONFIG_REALTEK_MST_I2C_SPI 1 +#define CONFIG_USBBLASTER_SPI 1 +#define CONFIG_MSTARDDC_SPI 1 +#define CONFIG_PICKIT2_SPI 1 +#define CONFIG_CH341A_SPI 1 +#define CONFIG_DIGILENT_SPI 1 +#define CONFIG_JLINK_SPI 1 +#define CONFIG_NI845X_SPI 1 +#define CONFIG_STLINKV3_SPI 1 + #include "programmer.h" void flashbuses_to_text_test_success(void **state) @@ -49,3 +90,54 @@ bustype = BUS_NONE; assert_string_equal(flashbuses_to_text(bustype), "None"); } + +void validate_prog_enumerates_test_success(void **state) +{ + (void) state; /* unused */ + + enum programmer prog = PROGRAMMER_INTERNAL; + const char *prog_names[] = { + "internal", + "dummy", + "mec1308", + "nic3com", + "nicrealtek", + "nicnatsemi", + "gfxnvidia", + "raiden_debug", + "drkaiser", + "satasii", + "atahpt", + "atavia", + "atapromise", + "it8212", + "ft2232_spi", + "serprog", + "buspirate_spi", + "dediprog", + "developerbox", + "ene_lpc", + "rayer_spi", + "pony_spi", + "nicintel", + "nicintel_spi", + "nicintel_eeprom", + "ogp_spi", + "satamv", + "linux_mtd", + "linux_spi", + "lspcon_i2c_spi", + "realtek_mst_i2c_spi", + "usbblaster_spi", + "mstarddc_spi", + "pickit2_spi", + "ch341a_spi", + "digilent_spi", + "jlink_spi", + "ni845x_spi", + "stlinkv3_spi", + }; + for (; prog < PROGRAMMER_INVALID; prog++) { + assert_string_equal(programmer_table[prog].name, prog_names[prog]); + } +} diff --git a/tests/tests.c b/tests/tests.c index 3124607..d2a22a5 100644 --- a/tests/tests.c +++ b/tests/tests.c @@ -46,6 +46,7 @@ const struct CMUnitTest flashrom_tests[] = { cmocka_unit_test(flashbuses_to_text_test_success), + cmocka_unit_test(validate_prog_enumerates_test_success), }; ret |= cmocka_run_group_tests_name("flashrom.c tests", flashrom_tests, NULL, NULL); diff --git a/tests/tests.h b/tests/tests.h index cb905fd..89fc7fa 100644 --- a/tests/tests.h +++ b/tests/tests.h @@ -27,6 +27,7 @@ /* flashrom.c */ void flashbuses_to_text_test_success(void **state); +void validate_prog_enumerates_test_success(void **state); /* spi25.c */ void spi_write_enable_test_success(void **state); -- To view, visit https://review.coreboot.org/c/flashrom/+/47356 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I13efc4561f743edf51326d4b408be8715b630377 Gerrit-Change-Number: 47356 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: newchange

6 8

Change in flashrom[master]: Makefile: Check for last line only from preprocessed output
by Khem Raj (Code Review) Aug. 1, 2024

Aug. 1, 2024

Khem Raj has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/43770 ) Change subject: Makefile: Check for last line only from preprocessed output ...................................................................... Makefile: Check for last line only from preprocessed output This started to fail with glibc 2.32 since glibc added additional attributes to functions in signal.h therefore existing regexp started to fail as it is not able to handle these functions e.g. extern int siginterrupt (int __sig, int __interrupt) __attribute__ ((__nothrow__ , __leaf__)) __attribute__ ((__deprecated__ ("Use sigaction with SA_RESTART instead"))); grep -v '^\#' | grep '"' | cut -f 2 -d'"' bit outside of fd_set selected Use sigaction with SA_RESTART instead arm So changing it to tail -1 | grep '"' | cut -f 2 -d'"' arm Produces the expected result, this was hidden until now Signed-off-by: Khem Raj <raj.khem(a)gmail.com> Change-Id: I123a046e142d54632f12d54e2aa09b0928c02b91 --- M Makefile 1 file changed, 3 insertions(+), 3 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/70/43770/1 diff --git a/Makefile b/Makefile index 803529f..3795681 100644 --- a/Makefile +++ b/Makefile @@ -106,7 +106,7 @@ # IMPORTANT: The following line must be placed before TARGET_OS is ever used # (of course), but should come after any lines setting CC because the line # below uses CC itself. -override TARGET_OS := $(strip $(call debug_shell,$(CC) $(CPPFLAGS) -E os.h 2>/dev/null | grep -v '^\#' | grep '"' | cut -f 2 -d'"')) +override TARGET_OS := $(strip $(call debug_shell,$(CC) $(CPPFLAGS) -E os.h 2>/dev/null | tail -1 | grep '"' | cut -f 2 -d'"')) ifeq ($(TARGET_OS), Darwin) override CPPFLAGS += -I/opt/local/include -I/usr/local/include @@ -460,8 +460,8 @@ # IMPORTANT: The following line must be placed before ARCH is ever used # (of course), but should come after any lines setting CC because the line # below uses CC itself. -override ARCH := $(strip $(call debug_shell,$(CC) $(CPPFLAGS) -E archtest.c 2>/dev/null | grep -v '^\#' | grep '"' | cut -f 2 -d'"')) -override ENDIAN := $(strip $(call debug_shell,$(CC) $(CPPFLAGS) -E endiantest.c 2>/dev/null | grep -v '^\#')) +override ARCH := $(strip $(call debug_shell,$(CC) $(CPPFLAGS) -E archtest.c 2>/dev/null | tail -1 | grep '"' | cut -f 2 -d'"')) +override ENDIAN := $(strip $(call debug_shell,$(CC) $(CPPFLAGS) -E endiantest.c 2>/dev/null | tail -1)) # Disable the internal programmer on unsupported architectures (everything but x86 and mipsel) ifneq ($(ARCH)-little, $(filter $(ARCH),x86 mips)-$(ENDIAN)) -- To view, visit https://review.coreboot.org/c/flashrom/+/43770 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I123a046e142d54632f12d54e2aa09b0928c02b91 Gerrit-Change-Number: 43770 Gerrit-PatchSet: 1 Gerrit-Owner: Khem Raj Gerrit-MessageType: newchange

4 5

Change in flashrom[master]: Add -- to make clean
by Ryan O'Leary (Code Review) Aug. 1, 2024

Aug. 1, 2024

Ryan O'Leary has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/37776 ) Change subject: Add -- to make clean ...................................................................... Add -- to make clean Somehow, I ended up with a file name "-c.d". I probably made it by mistake. This change lets "make clean" be a bit more resilient to files such as these. Change-Id: I2517ffac975f3df75f706350a07f189a98a11b7c Signed-off-by: Ryan O'Leary <ryanoleary(a)google.com> --- M Makefile 1 file changed, 1 insertion(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/76/37776/1 diff --git a/Makefile b/Makefile index 518d41b..fa66910 100644 --- a/Makefile +++ b/Makefile @@ -1129,7 +1129,7 @@ # This includes all frontends and libflashrom. # We don't use EXEC_SUFFIX here because we want to clean everything. clean: - rm -f $(PROGRAM) $(PROGRAM).exe libflashrom.a *.o *.d $(PROGRAM).8 $(PROGRAM).8.html $(BUILD_DETAILS_FILE) + rm -f -- $(PROGRAM) $(PROGRAM).exe libflashrom.a *.o *.d $(PROGRAM).8 $(PROGRAM).8.html $(BUILD_DETAILS_FILE) @+$(MAKE) -C util/ich_descriptors_tool/ clean distclean: clean -- To view, visit https://review.coreboot.org/c/flashrom/+/37776 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I2517ffac975f3df75f706350a07f189a98a11b7c Gerrit-Change-Number: 37776 Gerrit-PatchSet: 1 Gerrit-Owner: Ryan O'Leary <ryanoleary(a)google.com> Gerrit-MessageType: newchange

6 8

Change in flashrom[master]: dummyflasher.c: Use programmer logger functions
by Angel Pons (Code Review) April 11, 2024

April 11, 2024

Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/46523 ) Change subject: dummyflasher.c: Use programmer logger functions ...................................................................... dummyflasher.c: Use programmer logger functions All but three log messages use the programmer logger. Change the three outliers that were using the chip logger accordingly. Change-Id: Ia8668e05df2da739e6bb4c7d0fddad86e8d054a3 Signed-off-by: Angel Pons <th3fanbus(a)gmail.com> --- M dummyflasher.c 1 file changed, 3 insertions(+), 3 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/23/46523/1 diff --git a/dummyflasher.c b/dummyflasher.c index 6426ad2..d233db4 100644 --- a/dummyflasher.c +++ b/dummyflasher.c @@ -610,7 +610,7 @@ } for (i = 0; i < data->spi_ignorelist_size; i++) { if (writearr[0] == data->spi_ignorelist[i]) { - msg_cdbg("Ignoring ignorelisted SPI command 0x%02x\n", + msg_pdbg("Ignoring ignorelisted SPI command 0x%02x\n", data->spi_ignorelist[i]); /* Return success because the command does not fail, * it is simply ignored. @@ -1050,7 +1050,7 @@ * Search "total_size * 1024" in code. */ flash->chip->total_size = emu_data->emu_chip_size / 1024; - msg_cdbg("%s: set flash->total_size to %dK bytes.\n", __func__, + msg_pdbg("%s: set flash->total_size to %dK bytes.\n", __func__, flash->chip->total_size); /* Update the first count of each of the block_erasers. */ @@ -1061,7 +1061,7 @@ eraser->eraseblocks[0].count = 1; eraser->eraseblocks[0].size = emu_data->emu_chip_size; - msg_cdbg("%s: eraser.size=%d, .count=%d\n", + msg_pdbg("%s: eraser.size=%d, .count=%d\n", __func__, eraser->eraseblocks[0].size, eraser->eraseblocks[0].count); } -- To view, visit https://review.coreboot.org/c/flashrom/+/46523 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ia8668e05df2da739e6bb4c7d0fddad86e8d054a3 Gerrit-Change-Number: 46523 Gerrit-PatchSet: 1 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: newchange

3 3

[S] Change in flashrom[main]: udelay: clock_getres() does not provide clock_gettime() resolution
by Brian Norris (Code Review) March 1, 2024

March 1, 2024

Brian Norris has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/80806?usp=email ) Change subject: udelay: clock_getres() does not provide clock_gettime() resolution ...................................................................... udelay: clock_getres() does not provide clock_gettime() resolution POSIX doesn't give a clear definition of how clock_getres() and clock_gettime() relate, but Linux makes this clearer. See: https://git.kernel.org/linus/01679b5db7172b898be325ff272e10aebd412911 posix-timers: Document sys_clock_getres() correctly The deleted comments are quite clear: "Clock resolution is used to round up timer and interval times, NOT to report clock times" The new comments instead tell how the resolution is only related to clock_settime() and to the precision of *timers* -- not to the precision of clock_gettime() itself. This is particularly relevant depending on the Linux sysctl `sysctl kernel.timer_highres`. When enabled, resolution is always 1 nanosecond; when disabled, it follow the CONFIG_HZ of the kernel in question. But in neither case does this result actually assist determining the quality of the use case in question: busy-looping over clock_gettime(). In practice, any system with a typical CPU clock will have good enough clock_gettime() resolution, and trusting clock_getres() for this is actively misleading (and, wasteful on systems with kernel.timer_highres==0). Any system that doesn't trust the precision of clock_gettime() should simply be building with HAS_CLOCK_GETTIME=no. Therefore, avoid ever looking at clock_getres(), and instead always use clock_gettime() if we're allowed to. This saves a bunch of wasted time in "calibration", which is itself not actually accurate in the presence of hybrid CPUs or CPU DFS. Notably, calibration time-wastage has been the subject of a few past stalled efforts: https://review.coreboot.org/c/flashrom/+/39864 https://review.coreboot.org/c/flashrom/+/44517 BUG=b:325539928 TEST=flashrom never resorts to "Calibrating delay loop" when clock_gettime is available TEST=flashrom works at the same speed (wall clock time) with `sysctl kernel.timer_highres=1` and `sysctl kernel.timer_highres=0` Change-Id: Icdc6e184eacb14d3bd27029e3fc75be4431d82b4 Signed-off-by: Brian Norris <briannorris(a)chromium.org> --- M udelay.c 1 file changed, 5 insertions(+), 22 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/06/80806/1 diff --git a/udelay.c b/udelay.c index 82ddcbd..0005d39 100644 --- a/udelay.c +++ b/udelay.c @@ -27,10 +27,10 @@ #include "flash.h" #include "programmer.h" -static bool use_clock_gettime = false; - #if HAVE_CLOCK_GETTIME == 1 +static const bool use_clock_gettime = true; + #ifdef _POSIX_MONOTONIC_CLOCK static clockid_t clock_id = CLOCK_MONOTONIC; #else @@ -51,28 +51,11 @@ clock_gettime(clock_id, &now); } 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 const bool use_clock_gettime = false; + static inline void clock_usec_delay(int usecs) {} -static inline int clock_check_res(void) { return 0; } #endif /* HAVE_CLOCK_GETTIME == 1 */ @@ -135,7 +118,7 @@ void myusec_calibrate_delay(void) { - if (clock_check_res()) + if (use_clock_gettime) return; unsigned long count = 1000; -- To view, visit https://review.coreboot.org/c/flashrom/+/80806?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: Icdc6e184eacb14d3bd27029e3fc75be4431d82b4 Gerrit-Change-Number: 80806 Gerrit-PatchSet: 1 Gerrit-Owner: Brian Norris <briannorris(a)chromium.org> Gerrit-MessageType: newchange

1 0

[M] Change in flashrom[main]: doc: Add doc how to support flashrom project
by Peter Marheine (Code Review) Feb. 28, 2024

Feb. 28, 2024

Attention is currently required from: Anastasia Klimchuk, Angel Pons, Stefan Reinauer, Thomas Heijligen. Peter Marheine has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/80729?usp=email ) Change subject: doc: Add doc how to support flashrom project ...................................................................... Patch Set 2: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/80729?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: I59a4f5978bc8ffa8ca3a3dc3f15c770ef5fcedce Gerrit-Change-Number: 80729 Gerrit-PatchSet: 2 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Patrick Georgi <patrick(a)coreboot.org> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Tue, 27 Feb 2024 23:08:10 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[XS] Change in flashrom[main]: cli_classic: Defer flashrom_init calibration until after options parsing
by Brian Norris (Code Review) Feb. 27, 2024

Feb. 27, 2024

Brian Norris has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/80772?usp=email ) Change subject: cli_classic: Defer flashrom_init calibration until after options parsing ...................................................................... cli_classic: Defer flashrom_init calibration until after options parsing flashrom_init() may run through an expensive delay calibration phase, depending on the system timer behavior (e.g., if high resolution timers are disabled). This is wholly unnecessary if we're only going to dump usage output or similar. Defer the calibration until we've finished our CLI overhead. BUG=b:325539928 TEST=`flashrom --help` with `sysctl kernel.timer_highres` == 0 Change-Id: I57949ab511df04c6922008fa8cb12467154e2c5e Signed-off-by: Brian Norris <briannorris(a)chromium.org> --- M cli_classic.c 1 file changed, 4 insertions(+), 4 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/72/80772/1 diff --git a/cli_classic.c b/cli_classic.c index 60f3fd5..ffded5f 100644 --- a/cli_classic.c +++ b/cli_classic.c @@ -917,10 +917,6 @@ print_version(); print_banner(); - /* FIXME: Delay calibration should happen in programmer code. */ - if (flashrom_init(1)) - exit(1); - setbuf(stdout, NULL); parse_options(argc, argv, optstring, long_options, &options); @@ -1005,6 +1001,10 @@ } } + /* FIXME: Delay calibration should happen in programmer code. */ + if (flashrom_init(1)) + exit(1); + if (programmer_init(options.prog, options.pparam)) { msg_perr("Error: Programmer initialization failed.\n"); ret = 1; -- To view, visit https://review.coreboot.org/c/flashrom/+/80772?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: I57949ab511df04c6922008fa8cb12467154e2c5e Gerrit-Change-Number: 80772 Gerrit-PatchSet: 1 Gerrit-Owner: Brian Norris <briannorris(a)chromium.org> Gerrit-MessageType: newchange

1 0

[XS] Change in flashrom[main]: linux_mtd: Provide no-op delay implementation
by Brian Norris (Code Review) Feb. 27, 2024

Feb. 27, 2024

Brian Norris has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/80771?usp=email ) Change subject: linux_mtd: Provide no-op delay implementation ...................................................................... linux_mtd: Provide no-op delay implementation Flashrom has several magic programmer_delay() calls scattered throughout its codebase (see cli_classic.s/main() and flashrom.c/flashrom_image_write(), at least). These delays are superfluous for the linux_mtd programmer, because it's an opaque programmer in which all protocol details are handled by the kernel driver. Stub out the delay function, so we don't waste up to 1.1 seconds of needless delay time, depending on the operation and CLI vs libflashrom usage. BUG=b:325539928 Change-Id: I3ac69d3fd7cfc689c5b32c130d044516ed846c29 Signed-off-by: Brian Norris <briannorris(a)chromium.org> --- M linux_mtd.c 1 file changed, 9 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/71/80771/1 diff --git a/linux_mtd.c b/linux_mtd.c index 495db9a..eea0cf2 100644 --- a/linux_mtd.c +++ b/linux_mtd.c @@ -429,6 +429,14 @@ return FLASHROM_WP_ERR_RANGE_LIST_UNAVAILABLE; } +static void linux_mtd_nop_delay(const struct flashctx *flash, unsigned int usecs) +{ + /* + * Ignore delay requests. The Linux MTD framework brokers all flash + * protocol, including timing, resets, etc. + */ +} + static const struct opaque_master linux_mtd_opaque_master = { /* max_data_{read,write} don't have any effect for this programmer */ .max_data_read = MAX_DATA_UNSPECIFIED, @@ -441,6 +449,7 @@ .wp_read_cfg = linux_mtd_wp_read_cfg, .wp_write_cfg = linux_mtd_wp_write_cfg, .wp_get_ranges = linux_mtd_wp_get_available_ranges, + .delay = linux_mtd_nop_delay, }; /* Returns 0 if setup is successful, non-zero to indicate error */ -- To view, visit https://review.coreboot.org/c/flashrom/+/80771?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: I3ac69d3fd7cfc689c5b32c130d044516ed846c29 Gerrit-Change-Number: 80771 Gerrit-PatchSet: 1 Gerrit-Owner: Brian Norris <briannorris(a)chromium.org> Gerrit-MessageType: newchange

1 0

[XS] Change in flashrom[main]: fmap: Update major/minor version check
by Brian Norris (Code Review) Feb. 27, 2024

Feb. 27, 2024

Attention is currently required from: Anastasia Klimchuk. Brian Norris has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/79060?usp=email ) Change subject: fmap: Update major/minor version check ...................................................................... Patch Set 2: (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/79060/comment/9a18351f_755b13f2 : PS2, Line 9: It's not valid to separately check the major and minor versions. The : proper minor check would be something like: : : if (fmap->ver_major == FMAP_VER_MAJOR && : fmap->ver_minor > FMAP_VER_MINOR) Sorry for some delay. I've mostly moved on from this, because the primary bug was that libfmap was using the wrong version. Now that I've fixed that up, everyone I'm aware of is using major=1 minor=1, so none of these ugly comparisons have much more impact at the moment. I guess the real question would be what should happen if anybody ever bumps the minor version for some reason. (Would they still be backward compatible? If so, then why bump it at all?) I'm not aware of any versioning policy for this, but if look at "semantic versioning" (https://semver.org/) I believe their definitions ("Minor version Y (x.Y.z | x > 0) MUST be incremented if new, backward compatible functionality is introduced") would suggest we should ignore minor versions for the purpose of compatibility. > Brian, I like this idea! Maybe you can move the code from commit message into the code? :) The rest of my commit message describes why I don't: because cbfstool only has a major check, and I preferred to not develop a 3rd form of this check. > I looked into the chromiumos patch you linked, it still checks both major and minor versions. Yeah, I got enough run-around here that I fixed my problems elsewhere, and punted on the question of version comparisons. I still think it's bad to leave flashrom as it is, while libfmap was less important. (Or, I can go change the ChromiumOS libfmap much more easily after y'all upstream folks agree on how flashrom and/or cbfstool should look.) > The diff I see is that the check is more strict (both major and minor version expected to be exact equal) while initial fmap.c checks that the versions are not greater than. > > I can agree to make the check more strict, but I don't see why checking minor version has to be dropped, I think it can stay? > > I admit I haven't looked into cbfstool code, is there a way you can link a relevant piece? (if you think it's useful. if not then not) Sure, here you go! https://review.coreboot.org/plugins/gitiles/coreboot/+/354a54ac84a934c1ee60… ``` /* strings containing the magic tend to fail here */ if (fmap->ver_major != FMAP_VER_MAJOR) return 0; ``` Summary reasons for dropping the minor: 1. I was matching cbfstool 2. I don't want a 3rd version of this comparison (perhaps I can change cbfstool too if you'd like) 3. it's unclear what a version bump of any kind would mean w.r.t. compatibility; but minor version changes are considered backwards compatible by some definitions But I don't have strong opinions. At this point, I might just as well drop the CL, as it no longer has much significance despite the existing code being logically strange. -- To view, visit https://review.coreboot.org/c/flashrom/+/79060?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: I984835579d3b257a2462906f1f5091b179891bd0 Gerrit-Change-Number: 79060 Gerrit-PatchSet: 2 Gerrit-Owner: Brian Norris <briannorris(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Hsuan-ting Chen <roccochen(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Hsuan Ting Chen <roccochen(a)chromium.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Tue, 27 Feb 2024 21:35:38 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0