Anastasia Klimchuk has submitted this change. ( https://review.coreboot.org/c/flashrom/+/81106?usp=email )
Change subject: doc: Update arbitration team to be flashrom specific
......................................................................
doc: Update arbitration team to be flashrom specific
Same approach as it was before: founder and current project lead.
Change-Id: I1043b9499ab22da5ec981592d7b4311f027c4b5f
Signed-off-by: Anastasia Klimchuk <aklm(a)flashrom.org>
Reviewed-on: https://review.coreboot.org/c/flashrom/+/81106
Reviewed-by: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org>
---
M doc/about_flashrom/code_of_conduct.rst
1 file changed, 1 insertion(+), 3 deletions(-)
Approvals:
build bot (Jenkins): Verified
Stefan Reinauer: Looks good to me, approved
diff --git a/doc/about_flashrom/code_of_conduct.rst b/doc/about_flashrom/code_of_conduct.rst
index da94e29..2967095 100644
--- a/doc/about_flashrom/code_of_conduct.rst
+++ b/doc/about_flashrom/code_of_conduct.rst
@@ -126,10 +126,8 @@
Our arbitration team consists of the following people
+* Anastasia Klimchuk <aklm(a)flashrom.org> (Australia)
* Stefan Reinauer <stefan.reinauer(a)coreboot.org> (USA)
-* Patrick Georgi <patrick(a)coreboot.org> (Germany)
-* Ronald Minnich <rminnich(a)coreboot.org> (USA)
-* Martin Roth <martin(a)coreboot.org> (USA)
License and attribution
=======================
--
To view, visit https://review.coreboot.org/c/flashrom/+/81106?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: I1043b9499ab22da5ec981592d7b4311f027c4b5f
Gerrit-Change-Number: 81106
Gerrit-PatchSet: 2
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(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-MessageType: merged
Attention is currently required from: Brian Norris.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/80772?usp=email )
Change subject: cli_classic: Defer flashrom_init calibration until after options parsing
......................................................................
Patch Set 2: Code-Review+2
--
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: 2
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-Attention: Brian Norris <briannorris(a)chromium.org>
Gerrit-Comment-Date: Sat, 09 Mar 2024 09:07:09 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Brian Norris, Hsuan-ting Chen, Nikolai Artemiev.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/80771?usp=email )
Change subject: linux_mtd: Provide no-op delay implementation
......................................................................
Patch Set 2: Code-Review+2
(1 comment)
Patchset:
PS1:
> Sure! The BUG= and TEST= thing seems like more of a ChromiumOS thing than a coreboot. […]
Very useful, thank you.
The tag `TEST=` itself is not that important, but the info about testing is. However, it's often good to have structure and tags, I like structure.
--
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: 2
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-Attention: Brian Norris <briannorris(a)chromium.org>
Gerrit-Attention: Hsuan-ting Chen <roccochen(a)google.com>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Comment-Date: Sat, 09 Mar 2024 09:06:46 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: Brian Norris <briannorris(a)chromium.org>
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Anastasia Klimchuk, Hsuan-ting Chen, Nikolai Artemiev.
Brian Norris has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/80771?usp=email )
Change subject: linux_mtd: Provide no-op delay implementation
......................................................................
Patch Set 2:
(1 comment)
Patchset:
PS1:
> Do you have any testing info that you can add to commit message? I strongly suspect that there is in […]
Sure! The BUG= and TEST= thing seems like more of a ChromiumOS thing than a coreboot.org thing, but I guess coreboot.org doesn't seem to mind them.
The details I had in the non-public bug were essentially a more verbose version of my last paragraph (1.1 seconds time waste). But I've added a few more details of the exact operation and stats.
--
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: 2
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-Attention: Hsuan-ting Chen <roccochen(a)google.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Comment-Date: Fri, 08 Mar 2024 23:25:37 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Anastasia Klimchuk, Nikolai Artemiev.
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 3:
(1 comment)
Patchset:
PS2:
> I can update the commit message.
Done
--
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: 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: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Fri, 08 Mar 2024 23:25:09 +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>
Gerrit-MessageType: comment
Attention is currently required from: Anastasia Klimchuk, Nikolai Artemiev.
Hello Nikolai Artemiev, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/80808?usp=email
to look at the new patch set (#3).
The following approvals got outdated and were removed:
Code-Review+2 by Nikolai Artemiev, Verified+1 by build bot (Jenkins)
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>
---
M udelay.c
1 file changed, 2 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/08/80808/3
--
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: 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: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: newpatchset
Attention is currently required from: Brian Norris, Hsuan-ting Chen, Nikolai Artemiev.
Hello Anastasia Klimchuk, Hsuan-ting Chen, Nikolai Artemiev, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/80771?usp=email
to look at the new patch set (#2).
The following approvals got outdated and were removed:
Code-Review+2 by Hsuan-ting Chen, Code-Review+2 by Nikolai Artemiev, Verified+1 by build bot (Jenkins)
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>
---
M linux_mtd.c
1 file changed, 9 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/71/80771/2
--
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: 2
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-Attention: Brian Norris <briannorris(a)chromium.org>
Gerrit-Attention: Hsuan-ting Chen <roccochen(a)google.com>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-MessageType: newpatchset
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
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
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