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

[M] Change in flashrom[main]: Add support for Idaville HCC platform and iRC regions
by Kevin Yu (Code Review) 15 Mar '24

15 Mar '24

Kevin Yu has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/81258?usp=email ) Change subject: Add support for Idaville HCC platform and iRC regions ...................................................................... Patch Set 2: This change is ready for review. -- To view, visit https://review.coreboot.org/c/flashrom/+/81258?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: I61b810e4fa2aba8d92483bb4744561e67f46b959 Gerrit-Change-Number: 81258 Gerrit-PatchSet: 2 Gerrit-Owner: Kevin Yu <ykevin(a)celestica.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Fri, 15 Mar 2024 07:36:26 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[XS] Change in flashrom[main]: serprog: Fix scan-build warning of resource leak
by Stefan Reinauer (Code Review) 15 Mar '24

15 Mar '24

Attention is currently required from: Anastasia Klimchuk, Riku Viitanen. Stefan Reinauer has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/81032?usp=email ) Change subject: serprog: Fix scan-build warning of resource leak ...................................................................... Patch Set 3: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/81032?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: Id9cf211de3c482f702adebfcfa274a183c83a33f Gerrit-Change-Number: 81032 Gerrit-PatchSet: 3 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Riku Viitanen <riku.viitanen(a)protonmail.com> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Riku Viitanen <riku.viitanen(a)protonmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Fri, 15 Mar 2024 02:38:27 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[XS] Change in flashrom[main]: serprog: Fix scan-build warning of resource leak
by Anastasia Klimchuk (Code Review) 15 Mar '24

15 Mar '24

Attention is currently required from: Riku Viitanen, Stefan Reinauer. Hello Riku Viitanen, Stefan Reinauer, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/81032?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: serprog: Fix scan-build warning of resource leak ...................................................................... serprog: Fix scan-build warning of resource leak Warning found by the latest scan-build run: *** CID 1534883: (RESOURCE_LEAK) /serprog.c: 853 in serprog_init() 847 "by programmer!\n", cs_num8); 848 goto init_err_cleanup_exit; 849 } 850 } 851 bt = serprog_buses_supported; 852 if (sp_docommand(S_CMD_S_BUSTYPE, 1, &bt, 0, NULL)) >>>CID 1534883: (RESOURCE_LEAK) >>>Variable "cs" going out of scope leaks the storage it points to. 853 goto init_err_cleanup_exit; 854 } Follow up on commit e8c350f55e596aae3ab2bbc210b68389e2301a6c Change-Id: Id9cf211de3c482f702adebfcfa274a183c83a33f Signed-off-by: Anastasia Klimchuk <aklm(a)flashrom.org> --- M serprog.c 1 file changed, 4 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/32/81032/3 -- To view, visit https://review.coreboot.org/c/flashrom/+/81032?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: Id9cf211de3c482f702adebfcfa274a183c83a33f Gerrit-Change-Number: 81032 Gerrit-PatchSet: 3 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Riku Viitanen <riku.viitanen(a)protonmail.com> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Riku Viitanen <riku.viitanen(a)protonmail.com> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-MessageType: newpatchset

1 0

[XS] Change in flashrom[main]: udelay: Lower the sleep vs delay threshold
by Anastasia Klimchuk (Code Review) 14 Mar '24

14 Mar '24

Anastasia Klimchuk has submitted this change. ( https://review.coreboot.org/c/flashrom/+/80808?usp=email ) Change subject: udelay: Lower the sleep vs delay threshold ...................................................................... udelay: Lower the sleep vs delay threshold By default, we busy-loop (a.k.a., "delay") for most delay values, and only allow sleeping for large delays. But busy-looping is expensive, as it wastes CPU cycles. In a simple program that runs a bunch of samples of [1] over 1000 samples, I find that for 0.1 s (100000 us): 64x2 AMD CPU (CONFIG_HZ=250 / CONFIG_NO_HZ_FULL=y): min diff: 60 us max diff: 831 us mean diff: 135 us 4+2 Mediatek MT8183 CPU (CONFIG_HZ=1000 / CONFIG_NO_HZ_IDLE=y / sysctl kernel.timer_highres=1): min diff: 70 us max diff: 1556 us mean diff: 146 us 4+2 Mediatek MT8183 CPU (CONFIG_HZ=1000 / CONFIG_NO_HZ_IDLE=y / sysctl kernel.timer_highres=0): min diff: 94 us max diff: 7222 us mean diff: 1201 us i.e., maximum 1.5% error, typically ~0.1% error with high resolution timers. Max 7% error, typical 1% error with low resolution timers. The error is always in the positive direction (i.e., sleep longer than the requested delay, not shorter than the request). This seems reasonable. [1] Stripped / pseudocode: clock_gettime(CLOCK_MONOTONIC, before); nanosleep({ .tv_nsec = usecs * 1000 }, NULL); clock_gettime(CLOCK_MONOTONIC, after); diff = abs((after - before) / 1000 - usecs)); Change-Id: Ifd4821c66c5564f7c975c08769a6742f645e9be0 Signed-off-by: Brian Norris <briannorris(a)chromium.org> Reviewed-on: https://review.coreboot.org/c/flashrom/+/80808 Reviewed-by: Anastasia Klimchuk <aklm(a)chromium.org> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> --- M udelay.c 1 file changed, 2 insertions(+), 2 deletions(-) Approvals: Anastasia Klimchuk: Looks good to me, approved build bot (Jenkins): Verified diff --git a/udelay.c b/udelay.c index 82ddcbd..cb7a9db 100644 --- a/udelay.c +++ b/udelay.c @@ -237,8 +237,8 @@ /* Precise delay. */ void default_delay(unsigned int usecs) { - /* If the delay is >1 s, use internal_sleep because timing does not need to be so precise. */ - if (usecs > 1000000) { + /* 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); -- To view, visit https://review.coreboot.org/c/flashrom/+/80808?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: Ifd4821c66c5564f7c975c08769a6742f645e9be0 Gerrit-Change-Number: 80808 Gerrit-PatchSet: 4 Gerrit-Owner: Brian Norris <briannorris(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

[XS] Change in flashrom[main]: cli_classic: Defer flashrom_init calibration until after options parsing
by Anastasia Klimchuk (Code Review) 14 Mar '24

14 Mar '24

Anastasia Klimchuk has submitted this change. ( 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> Reviewed-on: https://review.coreboot.org/c/flashrom/+/80772 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Hsuan-ting Chen <roccochen(a)google.com> Reviewed-by: Nikolai Artemiev <nartemiev(a)google.com> Reviewed-by: Anastasia Klimchuk <aklm(a)chromium.org> --- M cli_classic.c 1 file changed, 4 insertions(+), 4 deletions(-) Approvals: Anastasia Klimchuk: Looks good to me, approved build bot (Jenkins): Verified Nikolai Artemiev: Looks good to me, approved Hsuan-ting Chen: Looks good to me, approved 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: 3 Gerrit-Owner: Brian Norris <briannorris(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Hsuan-ting Chen <roccochen(a)google.com> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

[XS] Change in flashrom[main]: linux_mtd: Provide no-op delay implementation
by Anastasia Klimchuk (Code Review) 14 Mar '24

14 Mar '24

Anastasia Klimchuk has submitted this change. ( 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 TEST=`elogtool add 0xa7` on a linux_mtd system (such as a Mediatek Chromebook): Time improvement - reduces from ~1.5s wall clock to about 0.4s CPU usage: -90%, as most of the CPU cycles (before this change) are spent in needless magic-delay busy loops Change-Id: I3ac69d3fd7cfc689c5b32c130d044516ed846c29 Signed-off-by: Brian Norris <briannorris(a)chromium.org> Reviewed-on: https://review.coreboot.org/c/flashrom/+/80771 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Anastasia Klimchuk <aklm(a)chromium.org> --- M linux_mtd.c 1 file changed, 9 insertions(+), 0 deletions(-) Approvals: build bot (Jenkins): Verified Anastasia Klimchuk: Looks good to me, approved 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: 3 Gerrit-Owner: Brian Norris <briannorris(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Hsuan-ting Chen <roccochen(a)google.com> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

[S] Change in flashrom[main]: classic_cli_manpage.rst: Update doc for custom_rst of raiden_debug_spi
by Anastasia Klimchuk (Code Review) 14 Mar '24

14 Mar '24

Attention is currently required from: Hsuan-ting Chen, Thomas Heijligen. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/81114?usp=email ) Change subject: classic_cli_manpage.rst: Update doc for custom_rst of raiden_debug_spi ...................................................................... Patch Set 2: Code-Review+1 (2 comments) Patchset: PS2: I just found one typo, sorry missed that earlier, otherwise that's it File doc/classic_cli_manpage.rst: https://review.coreboot.org/c/flashrom/+/81114/comment/ae11a99e_8b0b232e : PS2, Line 911: modify will modify (because the rest of the sentence is in future tense) -- To view, visit https://review.coreboot.org/c/flashrom/+/81114?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: Ie2b084a3ed9bf40f91bfa81dbc95ec69d99d5ad0 Gerrit-Change-Number: 81114 Gerrit-PatchSet: 2 Gerrit-Owner: Hsuan-ting Chen <roccochen(a)google.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Hsuan Ting Chen <roccochen(a)chromium.org> Gerrit-Attention: Hsuan-ting Chen <roccochen(a)google.com> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Comment-Date: Thu, 14 Mar 2024 10:12:04 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in flashrom[main]: classic_cli_manpage.rst: Update doc for custom_rst of raiden_debug_spi
by Hsuan-ting Chen (Code Review) 13 Mar '24

13 Mar '24

Attention is currently required from: Anastasia Klimchuk, Thomas Heijligen. Hsuan-ting Chen has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/81114?usp=email ) Change subject: classic_cli_manpage.rst: Update doc for custom_rst of raiden_debug_spi ...................................................................... Patch Set 2: (2 comments) File doc/classic_cli_manpage.rst: https://review.coreboot.org/c/flashrom/+/81114/comment/a11eb3ba_2a67cab4 : PS1, Line 906: > I want to visually separate the long text, so maybe do like this (two empty lines): […] Done https://review.coreboot.org/c/flashrom/+/81114/comment/d744f651_7f9e7810 : PS1, Line 909: More > Let's add empty line before this line (so that More information is visually separated from the previ […] Done -- To view, visit https://review.coreboot.org/c/flashrom/+/81114?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: Ie2b084a3ed9bf40f91bfa81dbc95ec69d99d5ad0 Gerrit-Change-Number: 81114 Gerrit-PatchSet: 2 Gerrit-Owner: Hsuan-ting Chen <roccochen(a)google.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Hsuan Ting Chen <roccochen(a)chromium.org> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Wed, 13 Mar 2024 22:47:45 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

[S] Change in flashrom[main]: classic_cli_manpage.rst: Update doc for custom_rst of raiden_debug_spi
by Hsuan-ting Chen (Code Review) 13 Mar '24

13 Mar '24

Attention is currently required from: Hsuan-ting Chen, Thomas Heijligen. Hello Anastasia Klimchuk, Thomas Heijligen, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/81114?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: classic_cli_manpage.rst: Update doc for custom_rst of raiden_debug_spi ...................................................................... classic_cli_manpage.rst: Update doc for custom_rst of raiden_debug_spi Update technical details for custom_rst of raiden_debug_spi to help users better understand their configuration options. BUG=b:161745002 BRANCH=none TEST=`meson compile -C testdir` and view ./testdir/doc/html/classic_cli_manpage.html Change-Id: Ie2b084a3ed9bf40f91bfa81dbc95ec69d99d5ad0 Signed-off-by: Hsuan Ting Chen <roccochen(a)google.com> --- M doc/classic_cli_manpage.rst 1 file changed, 10 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/14/81114/2 -- To view, visit https://review.coreboot.org/c/flashrom/+/81114?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: Ie2b084a3ed9bf40f91bfa81dbc95ec69d99d5ad0 Gerrit-Change-Number: 81114 Gerrit-PatchSet: 2 Gerrit-Owner: Hsuan-ting Chen <roccochen(a)google.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Hsuan Ting Chen <roccochen(a)chromium.org> Gerrit-Attention: Hsuan-ting Chen <roccochen(a)google.com> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-MessageType: newpatchset

1 0

[XS] Change in flashrom[main]: flashrom: Don't throw around "delay 1 second" so lightly
by Anastasia Klimchuk (Code Review) 11 Mar '24

11 Mar '24

Attention is currently required from: Brian Norris, Nikolai Artemiev. Anastasia Klimchuk 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 3: (1 comment) Patchset: PS1: > I have a question, Nikolai you said `we can add a quirk flag for them` , do you mean some existing f […] My main concern is that there may be some chips in the flashchips database which need this delay, but we don't know which ones and no way to check exactly. Or at least, there were chips like that a decade ago. If there is any idea, which kind of chips can be needing delay, that would be useful info. Side effects of `corrupting flash or at least getting incorrect verify results` sounds bad, especially for a chip which was supported and marked as tested, and this now suddenly breaks. However, how old such a chip could be, can such side effects be repro now, decade later? -- 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: 3 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-Attention: Brian Norris <briannorris(a)chromium.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Mon, 11 Mar 2024 11:56:01 +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> Gerrit-MessageType: comment

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit March 2024