December 2023 - flashrom-gerrit

[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

[M] Change in flashrom[main]: dummyflasher: make RO status of SPI_SR_WEL explicit
by Sergii Dmytruk (Code Review) 03 Dec '23

03 Dec '23

Sergii Dmytruk has abandoned this change. ( https://review.coreboot.org/c/flashrom/+/60224?usp=email ) Change subject: dummyflasher: make RO status of SPI_SR_WEL explicit ...................................................................... Abandoned Doesn't affect functionality -- To view, visit https://review.coreboot.org/c/flashrom/+/60224?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: I5dce77e22a1f9cd29f40ec0881d2b1238a985e97 Gerrit-Change-Number: 60224 Gerrit-PatchSet: 1 Gerrit-Owner: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-MessageType: abandon

1 0

[XS] Change in flashrom[main]: dummyflasher: Fix JEDEC_WRSR2 and JEDEC_WRSR3 emulation
by Sergii Dmytruk (Code Review) 03 Dec '23

03 Dec '23

Attention is currently required from: Anastasia Klimchuk. Sergii Dmytruk 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: Code-Review-1 (1 comment) Patchset: PS1: Look, for example, at `spi_write_register()`: ```c write_cmd[0] = JEDEC_WRSR; write_cmd[1] = value; // ... write_cmd[0] = JEDEC_WRSR2; write_cmd[1] = value; // ... write_cmd[0] = JEDEC_WRSR3; write_cmd[1] = value; ``` Current code is correct. -- 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: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Sun, 03 Dec 2023 13:29:15 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[XS] Change in flashrom[main]: programmer: Use C linkage when compiling with C++
by Anastasia Klimchuk (Code Review) 03 Dec '23

03 Dec '23

Anastasia Klimchuk has abandoned this change. ( https://review.coreboot.org/c/flashrom/+/63001?usp=email ) Change subject: programmer: Use C linkage when compiling with C++ ...................................................................... Abandoned The patch looks abandoned with unresolved comments and no responses for >1yr. Patch can be restored later if needed. -- To view, visit https://review.coreboot.org/c/flashrom/+/63001?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: I7e12ead58dee07313d756f1afcc687ba12b6392b Gerrit-Change-Number: 63001 Gerrit-PatchSet: 1 Gerrit-Owner: Tom Hughes <tomhughes(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-MessageType: abandon

1 0

[XS] Change in flashrom[main]: layout: space needed between literal and identifier
by Anastasia Klimchuk (Code Review) 03 Dec '23

03 Dec '23

Anastasia Klimchuk has abandoned this change. ( https://review.coreboot.org/c/flashrom/+/63000?usp=email ) Change subject: layout: space needed between literal and identifier ...................................................................... Abandoned The patch looks abandoned with unresolved comments and no responses for >1yr. Patch can be restored later if needed. -- To view, visit https://review.coreboot.org/c/flashrom/+/63000?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: I04f5572d6c2dc27b7c54ee6ee97874a7a1940229 Gerrit-Change-Number: 63000 Gerrit-PatchSet: 1 Gerrit-Owner: Tom Hughes <tomhughes(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-MessageType: abandon

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit December 2023