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

[S] Change in flashrom[main]: udelay: clock_getres() does not provide clock_gettime() resolution
by Brian Norris (Code Review) 08 Mar '24

08 Mar '24

Attention is currently required from: Anastasia Klimchuk, Nikolai Artemiev, Stefan Reinauer. Brian Norris has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/80806?usp=email ) Change subject: udelay: clock_getres() does not provide clock_gettime() resolution ...................................................................... Patch Set 2: (2 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/80806/comment/a24825da_d6cc7b52 : PS2, Line 10: Linux makes this clearer > Brian, thank you for the work on this! And you even looked through old forgotten patches! […] It's a good question, and my answer is that I'm not sure. I never found the POSIX descriptions to be super precise here, but Linux docs seem to suggest that the Linux decisions are somewhat diverged from POSIX. So it's possible some POSIX-like system has different ideas of resolution and of clock_gettime() behavior. How would you like me to proceed with this question? Make educated guesses? (e.g., seems N/A on Windows; MacOS might be in [1]; ...) Or carve out `#ifdef __linux__` behavior here? Regarding guesses: I see that MacOS claims microsecond precision for CLOCK_MONOTONIC; and CLOCK_REALTIME seems less clear (possibly more-precise). So it's possible this is a regression for Mac. [1] https://opensource.apple.com/source/Libc/Libc-1439.141.1/gen/clock_gettime.… https://review.coreboot.org/c/flashrom/+/80806/comment/4694382e_5b4cfced : PS2, Line 42: https://review.coreboot.org/c/flashrom/+/39864 : https://review.coreboot.org/c/flashrom/+/44517 > In your opinion, if your patch goes ahead , are these old patches still needed? I suppose that depends on the underlying motivation and use case there. I assume that such patches are simply trying to reduce the time wastage in "calibration". If that's all that it is, and they're using a reasonable POSIX-like system, then no, they won't be needed. If such developers have systems that really can't use clock_gettime() for some reason (e.g., I think Windows + mingw won't have clock_gettime() at all? I don't have anything to test this with), then yes, they could still technically be desirable. -- 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: 2 Gerrit-Owner: Brian Norris <briannorris(a)chromium.org> Gerrit-Reviewer: Hsuan-ting Chen <roccochen(a)google.com> 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-CC: Anastasia Klimchuk <aklm(a)chromium.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: Fri, 08 Mar 2024 18:21:43 +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]: udelay: Lower the sleep vs delay threshold
by Brian Norris (Code Review) 08 Mar '24

08 Mar '24

Attention is currently required from: Anastasia Klimchuk. Brian Norris has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/80808?usp=email ) Change subject: udelay: Lower the sleep vs delay threshold ...................................................................... Patch Set 2: (1 comment) Patchset: PS2: > I just wanted to clarify, the diff will always be positive, so actual delay in us after this patch ( […] That's a good question. The pseudocode in my commit message shows absolute value, which means I lost that detail. But I ran some abbreviated experiments here and never saw a negative value. I can update the commit message. -- 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: 2 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: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Fri, 08 Mar 2024 18:21:12 +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: Don't throw around "delay 1 second" so lightly
by Brian Norris (Code Review) 08 Mar '24

08 Mar '24

Attention is currently required from: Anastasia Klimchuk, Nikolai Artemiev. 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 2: (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/80807/comment/8253b1ea_7abf526e : PS2, Line 13: https://review.coreboot.org/plugins/gitiles/flashrom/+/8ab49e72af8465d4527d… > Oh, gitiles is down at the moment, maybe you can tell the file and line? I would have a look! :) Ah, I noticed that a day or so after uploading this. That's unfortunate. It's just a commit link, showing where the progenitor of the lines I'm deleting came from. `git show 8ab49e72af8465d4527de2ec37b22cd44f7a1169` will give you the same info. The key isn't really the code anyway (it's identical, although it moved around a bit with the addition of libflashrom); it's the commit message, which includes: ``` Wait 1 second between erase and verify. This fixes a few reports where verify directly after erase had unpleasant side effects like corrupting flash or at least getting incorrect verify results. ``` -- 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: 2 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: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Fri, 08 Mar 2024 18:21:07 +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]: fmap: Update major/minor version check
by Anastasia Klimchuk (Code Review) 08 Mar '24

08 Mar '24

Anastasia Klimchuk has submitted this change. ( https://review.coreboot.org/c/flashrom/+/79060?usp=email ) Change subject: fmap: Update major/minor version check ...................................................................... fmap: Update major/minor version check 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) ERROR(); But this code was alleged (at introduction in [1]) to have come from cbfstool, and cbfstool doesn't bother with a minor version check. This check is only for finding the FMAP while searching the flash; it isn't actually here for integrity and compatibility purpose. Drop the MINOR version check; align with cbfstool on the MAJOR version check; and match the cbfstool comments for is_valid_fmap(), to emphasize the lack of precision. [1] Commit c82900b66142 ("Add support to get layout from fmap (e.g. coreboot rom)") BRANCH=none BUG=b:288327526 TEST=libflashrom + ChromiumOS flashmap Change-Id: I984835579d3b257a2462906f1f5091b179891bd0 Signed-off-by: Brian Norris <briannorris(a)chromium.org> Reviewed-on: https://review.coreboot.org/c/flashrom/+/79060 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Anastasia Klimchuk <aklm(a)chromium.org> --- M fmap.c 1 file changed, 2 insertions(+), 3 deletions(-) Approvals: build bot (Jenkins): Verified Anastasia Klimchuk: Looks good to me, approved diff --git a/fmap.c b/fmap.c index 7f5df2b..50cb969 100644 --- a/fmap.c +++ b/fmap.c @@ -47,14 +47,13 @@ return sizeof(*fmap) + (fmap->nareas * sizeof(struct fmap_area)); } +/* Make a best-effort assessment if the given fmap is real */ static int is_valid_fmap(const struct fmap *fmap) { if (memcmp(fmap, FMAP_SIGNATURE, strlen(FMAP_SIGNATURE)) != 0) return 0; /* strings containing the magic tend to fail here */ - if (fmap->ver_major > FMAP_VER_MAJOR) - return 0; - if (fmap->ver_minor > FMAP_VER_MINOR) + if (fmap->ver_major != FMAP_VER_MAJOR) return 0; /* a basic consistency check: flash address space size should be larger * than the size of the fmap data structure */ -- 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: 4 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-MessageType: merged

1 0

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

08 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 1: (1 comment) Patchset: PS1: > Do you have hint how we build this? […] You need to have `sphinx-build` installed, and after it is installed, docs will be built by default (you can use the command you mentioned). Also we have a doc about docs: https://www.flashrom.org/how_to_add_docs.html -- 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: 1 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-Attention: Hsuan-ting Chen <roccochen(a)google.com> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Comment-Date: Fri, 08 Mar 2024 12:27:20 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Hsuan-ting Chen <roccochen(a)google.com> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

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

08 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 1: (1 comment) Patchset: PS1: > Thank you so much for improving documentation! Do you have hint how we build this? I tried `meson compile -C builddir` but I cannot see anything? -- 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: 1 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-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Fri, 08 Mar 2024 10:26:31 +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]: classic_cli_manpage.rst: Update doc for custom_rst of raiden_debug_spi
by Anastasia Klimchuk (Code Review) 08 Mar '24

08 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 1: (3 comments) Patchset: PS1: Thank you so much for improving documentation! File doc/classic_cli_manpage.rst: https://review.coreboot.org/c/flashrom/+/81114/comment/e0d44405_233a6469 : PS1, Line 906: I want to visually separate the long text, so maybe do like this (two empty lines): ----- syntax, where: <empty line here> ``custom_rst=false`` is the implicit default timeout of 3ms <empty line here> and ``custom_rst=true`` set ``RAIDEN_DEBUG_SPI_REQ_ENABLE_AP_CUSTOM`` instead of ``RAIDEN_DEBUG_SPI_REQ_ENABLE_AP``. This custom reset modify the timeout from 3ms to 10ms and <... the rest of text> https://review.coreboot.org/c/flashrom/+/81114/comment/f7ceb566_1f1e4142 : PS1, Line 909: More Let's add empty line before this line (so that More information is visually separated from the previous text) -- 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: 1 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-Attention: Hsuan-ting Chen <roccochen(a)google.com> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Comment-Date: Fri, 08 Mar 2024 09:43:37 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

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

08 Mar '24

Attention is currently required from: Brian Norris. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/80808?usp=email ) The change is no longer submittable: All-Comments-Resolved is unsatisfied now. Change subject: udelay: Lower the sleep vs delay threshold ...................................................................... Patch Set 2: (1 comment) Patchset: PS2: I just wanted to clarify, the diff will always be positive, so actual delay in us after this patch (which uses sleep) can never be less than it was before with busy loop? So delay might get a bit longer, but never shorter, is this correct? -- 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: 2 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-Comment-Date: Fri, 08 Mar 2024 08:24:02 +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 Anastasia Klimchuk (Code Review) 08 Mar '24

08 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 2: (2 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/80807/comment/67aff59d_47e80b61 : PS2, Line 13: https://review.coreboot.org/plugins/gitiles/flashrom/+/8ab49e72af8465d4527d… Oh, gitiles is down at the moment, maybe you can tell the file and line? I would have a look! :) Patchset: PS1: > alright, I dropped the "RFC" tag 😊 I have a question, Nikolai you said `we can add a quirk flag for them` , do you mean some existing flag or new one? I agree the delay here looks too high level. Do you know which specific chip may, in theory, need this? Is it only for old chips? Maybe this can be a command-line flag, if it's really rarely needed. -- 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: 2 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: Fri, 08 Mar 2024 08:10:01 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Brian Norris <briannorris(a)chromium.org> Comment-In-Reply-To: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-MessageType: comment

1 0

[S] Change in flashrom[main]: udelay: clock_getres() does not provide clock_gettime() resolution
by Anastasia Klimchuk (Code Review) 08 Mar '24

08 Mar '24

Attention is currently required from: Brian Norris, Nikolai Artemiev, Stefan Reinauer. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/80806?usp=email ) The change is no longer submittable: All-Comments-Resolved is unsatisfied now. Change subject: udelay: clock_getres() does not provide clock_gettime() resolution ...................................................................... Patch Set 2: (2 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/80806/comment/0dbdfa47_34b8ee3b : PS2, Line 10: Linux makes this clearer Brian, thank you for the work on this! And you even looked through old forgotten patches! I have a question about OSes: I understand this totally makes sense and was tested on Linux, do you know what the situation could be on other OSes? I understand the idea is, if `clock_gettime()` exists let's use it. Seems legit, are there any potential issues in non-Linux? https://review.coreboot.org/c/flashrom/+/80806/comment/634d8d6e_b2f3c540 : PS2, Line 42: https://review.coreboot.org/c/flashrom/+/39864 : https://review.coreboot.org/c/flashrom/+/44517 In your opinion, if your patch goes ahead , are these old patches still needed? -- 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: 2 Gerrit-Owner: Brian Norris <briannorris(a)chromium.org> Gerrit-Reviewer: Hsuan-ting Chen <roccochen(a)google.com> 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-CC: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Brian Norris <briannorris(a)chromium.org> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Fri, 08 Mar 2024 07:52:23 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit March 2024