December 2023 - flashrom-gerrit

[S] Change in flashrom[main]: buspirate_spi: Add support for hiz output with pullups=off
by Dreg (Code Review) Dec. 1, 2023

Dec. 1, 2023

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 6: Code-Review+1 (1 comment) Patchset: PS6: > David, thank you for the patch! I have a question. […] Hello! good points, When someone enables 'pullups=on', the internal pull-ups are activated (powered from a VPU input pin), making the MOSI, MISO, etc., pins function as an open collector (hi-z) using these pull-ups. Therefore, 'pullups=on' inherently works as an open collector. Thus, setting both 'pullups=on' and 'hiz=on' is redundant but not incorrect. However, considering human error, I'm unsure if we should flag this as an error. What do you think? Would you like me to commit this change? BUT! A common and dangerous mistake is when people incorrectly use 'pullups' arg: 'pullups=1' or 'pullups=enable'. In such cases, flashrom continues to run without using the pull-ups or open collector, sending 3.3V through the SPI pins, which can damage 1.8V or 2.5V chips. Therefore, I believe if we detect a syntax error in 'pullups=' or 'hiz=', we should flag an error and not continue running flashrom. Would you like me to commit this change? -- 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: 6 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: Fri, 01 Dec 2023 18:35:42 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> 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) Dec. 1, 2023

Dec. 1, 2023

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: > Carly, thank you so much for the patch! You are right, there is a redundant verification. […] Anastasia, Thanks for taking the time to look this over. I will discuss this with Carly later today and try to schedule time to collect this data. Our company leverages flashrom pretty heavily and we're grateful for the project so are happy to contribute patches as we can. I think I agree that it makes sense to drop that `check_erased_range` call as it should be duplicating the smaller range checks in 341-344 but I would like us to make sure that we're not missing verification coverage there due to some weird loop quirk. The other option is to just call `check_erased_range` once for the entire range instead of multiple times on smaller chunks to avoid the overhead of the function calls, the multiple mallocs, and the flash reads.. though it does require more contiguous memory to do it just once which may put pressure on memory constrained systems. We can try benchmarking both. -- That said, I would like to make a case for why these proposed changes should still occur independent of the orphaned `check_erased_range` change. When programming an empty chip (scenario 3 in the testing info), it's not the orphaned `check_erased_range` that causes problems as the chip is blank and we never enter that loop, it's the `verity_range` over the entire layout after the writes are performed. In our case, this causes a ~40% reduction in programming performance (140sec -> 200sec) which would affect our board yields compared to flashrom 1.2. As mentioned in the commit message, `flashrom_flash_write` [already verifies writes when instructed](https://review.coreboot.org/plugins/gitiles/flashrom/+/refs/hea… and does so based on flags set in `flashctx` so the call here is both unavoidable (because it doesn't respect caller set flags) and redundant (because it duplicates what's already being done by `flashrom_flash_write`). While it's possible to keep the `verify_range` check in `erase_write`, it doesn't seem ideal as `flashrom_flash_write` already has a more "global" view and appropriately handles both full chip verification and verification by layout. If we kept it here in `erase_write`, it would need to be guarded by `flashctx` flags and not overlap with any checks in `flashrom_flash_write`. It would also make the verification logic disjointed and more difficult to maintain as they would no longer be colocated in `flashrom_flash_write`. -- As a side note, it could be argued that the erase path should also respect the verify flag(s), but I think that's a larger discussion to happen for a separate patchset and would be a change in behavior. [This FIXME here](https://review.coreboot.org/plugins/gitiles/flashrom/+/refs/heads/mai… seems to imply there's not much benefit in announcing erase failures, so performing verification seems like additional overhead for that operation, but I think it make senses to make any potentially destructive APIs, such as erase and write, consistent with respect to verification. So either they respect the verify flags in `flashctx` or API users, including the CLI, should independently invoke verification after calling erase/write. Currently (without this patch), for testing scenario 1 (overwriting an image with a new image) where we're erasing _and_ writing, I think we're verifying data 4 times: 1) `check_erased_range` unconditionally on small chunks 2) `check_erased_range` unconditionally on the entire erase block 3) `verify_range` unconditionally in `erase_write` after writing data 4) the verification flagged to be performed in `flashrom_flash_write` -- 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: Fri, 01 Dec 2023 13:05:27 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

[M] Change in flashrom[main]: Add Idaville platform into chipset enable list and add update IRC reg...
by Anastasia Klimchuk (Code Review) Dec. 1, 2023

Dec. 1, 2023

Attention is currently required from: Kevin Yu. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/79287?usp=email ) Change subject: Add Idaville platform into chipset enable list and add update IRC region support ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: I also found there is an earlier patch that you sent CB:67438 which very sadly got missed :\ I apologise for this. Let's pick that up, I don't want your work to be lost. The both patches are about Idaville support, are they compatible, do you need both? -- To view, visit https://review.coreboot.org/c/flashrom/+/79287?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: I15bb3bb368156b078547ce0bdea556c7ab87f729 Gerrit-Change-Number: 79287 Gerrit-PatchSet: 1 Gerrit-Owner: Kevin Yu <ykevin(a)celestica.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Kevin Yu <ykevin(a)celestica.com> Gerrit-Comment-Date: Fri, 01 Dec 2023 12:16:38 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[XS] Change in flashrom[main]: ati: add polaris22
by Anastasia Klimchuk (Code Review) Dec. 1, 2023

Dec. 1, 2023

Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/29222?usp=email ) Change subject: ati: add polaris22 ...................................................................... Patch Set 5: (1 comment) Patchset: PS3: > I have rebased the patches, but I sadly do not have old enough devices to test. Jiajie, thank you for your effort of rebasing! Not much that we can do without the devices to test. Original author abandoned the chain. On the other hand, from the comments someone else in Aug 2020 tested this, but it's a while ago, don't know if they return back. Let's keep it as is for now. Abandoned patches are not deleted, and can be restored. -- To view, visit https://review.coreboot.org/c/flashrom/+/29222?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: Ia4950bb098a2bb8dba7b10bb9b2b6012c79365ea Gerrit-Change-Number: 29222 Gerrit-PatchSet: 5 Gerrit-Owner: Luc Verhaegen <libv(a)skynet.be> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Chen <c(a)jia.je> Gerrit-Reviewer: Name of user not set #1003051 Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-CC: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Comment-Date: Fri, 01 Dec 2023 11:02:52 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Chen <c(a)jia.je> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

[S] Change in flashrom[main]: buspirate_spi: Add support for hiz output with pullups=off
by Anastasia Klimchuk (Code Review) Dec. 1, 2023

Dec. 1, 2023

Attention is currently required from: David Reguera Garcia, Thomas Heijligen. Anastasia Klimchuk 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 6: (1 comment) Patchset: PS6: David, thank you for the patch! I have a question. I read the commit message, which is explaining everything really well. It seems like having `pullups=on` and `hiz=on` at the same time is potentially error prone (human error as you said). Maybe we should restrict having these both params on? What could be the use case to have them both on? -- 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: 6 Gerrit-Owner: David Reguera Garcia <dreg(a)rootkit.es> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: David Reguera Garcia <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: David Reguera Garcia <dreg(a)rootkit.es> Gerrit-Comment-Date: Fri, 01 Dec 2023 10:05:21 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[XS] Change in flashrom[main]: MAINTAINERS: Add roccochen as supporter for flashrom_tester
by Anastasia Klimchuk (Code Review) Dec. 1, 2023

Dec. 1, 2023

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 roccochen as supporter for flashrom_tester ...................................................................... Patch Set 1: (4 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/79319/comment/496395fe_c957079a : PS1, Line 7: as supporter remove "as supporter" https://review.coreboot.org/c/flashrom/+/79319/comment/f07127a7_830a570c : PS1, Line 7: roccochen replace with: Hsuan-Ting Chen We use full names for commit titles in this file, so that it's easier to read commit history without looking into commit description. Patchset: PS1: Thank you Hsuan-Ting, and welcome to maintainers! Also have a read on Team page (if you haven't done this already) https://www.flashrom.org/about_flashrom/team.html especially the intro which applies to everyone and the section about "flashrom reviewers" group. File MAINTAINERS: https://review.coreboot.org/c/flashrom/+/79319/comment/36ada540_0cc358f1 : PS1, Line 199: M: Hsuan-ting Chen <roccochen(a)google.com> Put this line above previous, `M` entries come before `R` entries. `M` entry means stronger commitment: you subscribe to all code reviews for your area (which is flashrom_tester in this case), `R` entry means you are CCed to patches. -- 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: 1 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: Fri, 01 Dec 2023 08:30:30 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[XS] Change in flashrom[main]: erasure_layout: Remove redundant `verify_range` call from `erase_write`
by Anastasia Klimchuk (Code Review) Dec. 1, 2023

Dec. 1, 2023

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: (2 comments) Patchset: PS1: Carly, thank you so much for the patch! You are right, there is a redundant verification. Which is partially my mistake, because I removed redundant erase earlier (in CB:77747 ) and forgot to remove the correspondent redundant verification. Your idea is very good, but I think we need to remove a different verification instead, the one which corresponds to erase invocation removed in CB:77747. Which is in this Patchset 1 lines #353-359. Just to be clear: I think to remove lines #353-359 *instead*, so return back lines #388-394. It should have the same effect: one less verification, but logically removing the "orphaned" verification, and keep the one which follows the write. Would it be possible for you to re-run the test 3. with this? Thank you! PS1: > Testing Summary: […] This is great, I really appreciate detailed testing info. -- 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: Fri, 01 Dec 2023 08:16:16 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Carly Zlabek <carlyzlabek(a)gmail.com> Gerrit-MessageType: comment

1 0