- flashrom-gerrit - mail.coreboot.org

[S] Change in flashrom[main]: buspirate_spi: Add support for hiz output with pullups=off
by Dreg (Code Review) 06 Dec '23

06 Dec '23

Attention is currently required from: Anastasia Klimchuk, Thomas Heijligen. Dreg has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/79299?usp=email ) Change subject: buspirate_spi: Add support for hiz output with pullups=off ...................................................................... Patch Set 7: Code-Review+1 (1 comment) Patchset: PS6: > Hello! good points, […] I've prepared another code to make the use of psus, pullups and hi-z mode safer. Can you merge this and I'll open another PR with the changes? To me, they are different issues. Thx Anastasia -- To view, visit https://review.coreboot.org/c/flashrom/+/79299?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: I6d9909b23782ef3fcb1428fd127dae0d6b211855 Gerrit-Change-Number: 79299 Gerrit-PatchSet: 7 Gerrit-Owner: Dreg <dreg(a)rootkit.es> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Dreg <dreg(a)rootkit.es> 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: Wed, 06 Dec 2023 19:10:01 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Dreg <dreg(a)rootkit.es> 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 Paul Menzel (Code Review) 06 Dec '23

06 Dec '23

Attention is currently required from: Arthur Heymans, Brian Norris, Edward O'Callaghan. Paul Menzel 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/38b4d504_90285e03 : PS2, Line 7: Update Maybe more specific: > Drop minor version check in version check -- 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: 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: Brian Norris <briannorris(a)chromium.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Comment-Date: Wed, 06 Dec 2023 11:08:23 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[XS] Change in flashrom[main]: fmap: Update major/minor version check
by Hsuan-ting Chen (Code Review) 06 Dec '23

06 Dec '23

Attention is currently required from: Arthur Heymans, Brian Norris, Edward O'Callaghan. Hsuan-ting Chen 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: Code-Review+1 -- 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: 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-Attention: Brian Norris <briannorris(a)chromium.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Comment-Date: Wed, 06 Dec 2023 10:33:52 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[XS] Change in flashrom[main]: MAINTAINERS: Add Hsuan Ting Chen for flashrom_tester
by Anastasia Klimchuk (Code Review) 05 Dec '23

05 Dec '23

Attention is currently required from: Hsuan-ting Chen. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/79319?usp=email ) Change subject: MAINTAINERS: Add Hsuan Ting Chen for flashrom_tester ...................................................................... Patch Set 2: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/79319?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: I71139401b5f2c7c1b4e76ba67ff774e030a343aa Gerrit-Change-Number: 79319 Gerrit-PatchSet: 2 Gerrit-Owner: Hsuan-ting Chen <roccochen(a)google.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Evan Benn <evanbenn(a)gmail.com> Gerrit-Attention: Hsuan-ting Chen <roccochen(a)google.com> Gerrit-Comment-Date: Tue, 05 Dec 2023 06:13:19 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[XS] Change in flashrom[main]: erasure_layout: Remove redundant `verify_range` call from `erase_write`
by Vincent Fazio (Code Review) 04 Dec '23

04 Dec '23

Attention is currently required from: Aarya, Anastasia Klimchuk, Carly Zlabek. Vincent Fazio has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/79354?usp=email ) Change subject: erasure_layout: Remove redundant `verify_range` call from `erase_write` ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: > Vincent, Carly […] In order to keep the code consistent, I think we can drop #2 ``` // verify erase ret = check_erased_range(flashctx, start_addr, block_len); if (ret) { msg_cerr("Verifying flash. Erase failed for range %#"PRIx32" : %#"PRIx32", Abort.\n", start_addr, start_addr + block_len - 1); goto _end; } ``` This way the erase and verification are more tightly coupled and we avoid trying to verify ranges that are read/write protected. I'll try to schedule this work for this iteration so we can get this in sooner rather than later. -- As part of looking over #1 and #2, I developed some slight concerns about the loop on line 314 which is separate from this patchset. I don't know if it warrants an issue and further discussion, and I haven't spelunked into the erase function selection logic, but the concern I have there is we've already pre-calculated what erase functions we're using but `get_flash_region` could return a region with a shorter end range, meaning the selected erase block fn could erase part of the next region. So if we had a 32k erase block fn, it seems like if `region.end` forces the length of the erasable region to 4k, we don't actually select a 4k erase function and instead continue to use a 32k function. If the next region is write protected it seems like we'd have verification errors. I think this currently only affects masters that have `get_region` defined, so `opaque_master_ich_hwseq` is at risk here. This may have been less of a risk with the legacy path because, from what I remember, it always used the smallest working erase block function, however the new path adjusts the function used based on the amount of changed data and coalesces blocks when possible (I think). -- To view, visit https://review.coreboot.org/c/flashrom/+/79354?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: I638835facd9311979c4991cc4ca41a4b9e174bd5 Gerrit-Change-Number: 79354 Gerrit-PatchSet: 1 Gerrit-Owner: Carly Zlabek <carlyzlabek(a)gmail.com> Gerrit-Reviewer: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Vincent Fazio <vfazio(a)gmail.com> Gerrit-Attention: Carly Zlabek <carlyzlabek(a)gmail.com> Gerrit-Attention: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Mon, 04 Dec 2023 15:36:34 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Comment-In-Reply-To: Vincent Fazio <vfazio(a)gmail.com> Gerrit-MessageType: comment

1 0

[XS] Change in flashrom[main]: MAINTAINERS: Add Hsuan Ting Chen for flashrom_tester
by Hsuan-ting Chen (Code Review) 04 Dec '23

04 Dec '23

Attention is currently required from: Anastasia Klimchuk. Hsuan-ting Chen has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/79319?usp=email ) Change subject: MAINTAINERS: Add Hsuan Ting Chen for flashrom_tester ...................................................................... Patch Set 2: (4 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/79319/comment/38e93658_8e8f22d9 : PS1, Line 7: roccochen > replace with: Hsuan-Ting Chen […] Done https://review.coreboot.org/c/flashrom/+/79319/comment/75975020_0895fb85 : PS1, Line 7: as supporter > remove "as supporter" Done Patchset: PS1: > Thank you Hsuan-Ting, and welcome to maintainers! […] Done File MAINTAINERS: https://review.coreboot.org/c/flashrom/+/79319/comment/0f4ceca1_9f3b4785 : PS1, Line 199: M: Hsuan-ting Chen <roccochen(a)google.com> > Put this line above previous, `M` entries come before `R` entries. […] Done -- To view, visit https://review.coreboot.org/c/flashrom/+/79319?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: I71139401b5f2c7c1b4e76ba67ff774e030a343aa Gerrit-Change-Number: 79319 Gerrit-PatchSet: 2 Gerrit-Owner: Hsuan-ting Chen <roccochen(a)google.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Evan Benn <evanbenn(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Mon, 04 Dec 2023 10:09:14 +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]: MAINTAINERS: Add Hsuan Ting Chen for flashrom_tester
by Hsuan-ting Chen (Code Review) 04 Dec '23

04 Dec '23

Attention is currently required from: Hsuan-ting Chen. Hello Anastasia Klimchuk, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/79319?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: MAINTAINERS: Add Hsuan Ting Chen for flashrom_tester ...................................................................... MAINTAINERS: Add Hsuan Ting Chen for flashrom_tester BUG=none BRANCH=none TEST=~/coreboot/util/scripts/get_maintainer.pl -f util/flashrom_tester/build.rs Change-Id: I71139401b5f2c7c1b4e76ba67ff774e030a343aa Signed-off-by: Hsuan Ting Chen <roccochen(a)google.com> --- M MAINTAINERS 1 file changed, 1 insertion(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/19/79319/2 -- To view, visit https://review.coreboot.org/c/flashrom/+/79319?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: I71139401b5f2c7c1b4e76ba67ff774e030a343aa Gerrit-Change-Number: 79319 Gerrit-PatchSet: 2 Gerrit-Owner: Hsuan-ting Chen <roccochen(a)google.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Evan Benn <evanbenn(a)gmail.com> Gerrit-Attention: Hsuan-ting Chen <roccochen(a)google.com> Gerrit-MessageType: newpatchset

1 0

[XS] Change in flashrom[main]: erasure_layout: Remove redundant `verify_range` call from `erase_write`
by Anastasia Klimchuk (Code Review) 04 Dec '23

04 Dec '23

Attention is currently required from: Aarya, Carly Zlabek, Vincent Fazio. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/79354?usp=email ) Change subject: erasure_layout: Remove redundant `verify_range` call from `erase_write` ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: Vincent, Carly > Our company leverages flashrom pretty heavily and we're grateful for the project so are happy to contribute patches as we can. This is really nice, thank you so much! Contributions are most welcome! This is the best way to help flashrom: by doing contributions, patches and testing. Also one more way to help could be to review other patches. If you see someone else's patch which is relevant for you, you can add comments. Just add yourself to the patch and add comments or votes! I really appreciate your detailed message, I read it carefully and then went through the code and in short: I think you are right. And if there are not one but two redundant verifications, I think the best would be to remove both in one patch (this patch), and then test the patch as a whole. Your other point is also correct: I can see that `erase_write` code does not check verification flags and doing verification unconditionally. This can be a good idea to fix too, but yes that would be a separate patch. Specifically for this patch, I agree either 1) or 2) can be removed, and then also 3) , so that if you get to detailed testing and benchmarking, you would test the full solution. Because testing takes your time and effort, so better spend this effort on the full solution. Between 1) and 2) there can be one more point of difference: if an error happened, 1) will discover it earlier and fail early (`goto _end`) vs 2) will go through the whole region even if the first byte failed. > I would like us to make sure that we're not missing verification coverage there due to some weird loop quirk. A note on this: if it helps, and if it can be useful for you, there is a unit test which focuses exactly on various test cases of erasing and writing: `tests/erase_func_algo.c` and you are welcome to add test cases to it. Test has a mock chip with 16 bytes memory, sets up the initial state of memory and what to write. Mock chip logs all invocations of read, erase, write. You can construct any test cases and various layouts. (for example I am adding here https://review.coreboot.org/c/flashrom/+/78985) If you end up adding some new test case which is useful, send it as a patch! But even without a patch, it might be useful to run locally many times. Won't test the hardware of course, but will test the logic of erase / write. Thinking about weird loop quirk: verification on lines #341-344 works precisely on the same memory range as `erasefn` above it on #337. So if theoretically, there is a case with missing verification coverage, this would mean missing erase coverage which would be much bigger issue. Good news: such a test case can be added into a unit test and this way we protect against regressions. I initially created a test with the mock chip of 16 bytes size, so far it has been enough to describe test cases I could come up with. But if there is a test case which needs 32 bytes and 16 is not enough, mock chip can be extended. -- To view, visit https://review.coreboot.org/c/flashrom/+/79354?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: I638835facd9311979c4991cc4ca41a4b9e174bd5 Gerrit-Change-Number: 79354 Gerrit-PatchSet: 1 Gerrit-Owner: Carly Zlabek <carlyzlabek(a)gmail.com> Gerrit-Reviewer: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Vincent Fazio <vfazio(a)gmail.com> Gerrit-Attention: Carly Zlabek <carlyzlabek(a)gmail.com> Gerrit-Attention: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Attention: Vincent Fazio <vfazio(a)gmail.com> Gerrit-Comment-Date: Mon, 04 Dec 2023 09:26:10 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Comment-In-Reply-To: Vincent Fazio <vfazio(a)gmail.com> Gerrit-MessageType: comment

1 0

[XS] Change in flashrom[main]: dummyflasher: Fix JEDEC_WRSR2 and JEDEC_WRSR3 emulation
by Anastasia Klimchuk (Code Review) 04 Dec '23

04 Dec '23

Anastasia Klimchuk has abandoned this change. ( https://review.coreboot.org/c/flashrom/+/79390?usp=email ) Change subject: dummyflasher: Fix JEDEC_WRSR2 and JEDEC_WRSR3 emulation ...................................................................... Abandoned the patch is wrong -- To view, visit https://review.coreboot.org/c/flashrom/+/79390?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: Ia0e51a9b8dbd1f67cb947e3ec73f64b6d30cbb97 Gerrit-Change-Number: 79390 Gerrit-PatchSet: 1 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: abandon

1 0

[XS] Change in flashrom[main]: dummyflasher: Fix JEDEC_WRSR2 and JEDEC_WRSR3 emulation
by Anastasia Klimchuk (Code Review) 04 Dec '23

04 Dec '23

Attention is currently required from: Sergii Dmytruk. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/79390?usp=email ) Change subject: dummyflasher: Fix JEDEC_WRSR2 and JEDEC_WRSR3 emulation ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: > Look, for example, at `spi_write_register()`: […] Thank you for explanation! I studied the code carefully, and now I see. -- To view, visit https://review.coreboot.org/c/flashrom/+/79390?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: Ia0e51a9b8dbd1f67cb947e3ec73f64b6d30cbb97 Gerrit-Change-Number: 79390 Gerrit-PatchSet: 1 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Mon, 04 Dec 2023 01:14:55 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-MessageType: comment

1 0

2025

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit