October 2024 - flashrom-gerrit

[XS] Change in flashrom[main]: erasure_layout: Erase larger block only when all sub-block need erase
by Anastasia Klimchuk (Code Review) 05 Oct '24

05 Oct '24

Attention is currently required from: Aarya, Bill XIE. Anastasia Klimchuk has posted comments on this change by Bill XIE. ( https://review.coreboot.org/c/flashrom/+/84614?usp=email ) Change subject: erasure_layout: Erase larger block only when all sub-block need erase ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: Bill, thank you so much for looking into the details of the erasure logic! And the patch with the test scenario, I appreciate so much! My first thought is that this needs a test, and the fact that existing tests are passing both before and after the patch means this scenario is not really tested. It should be. I am going to inspect existing cases (in tests/erase_func_algo.c) and either add or modify something. I will post here how it goes. As you said, this code is hardware independent, I have hopes to eventually get it fully covered with tests. Also, very important two points that you described in commit message. I think that p.2 should be fixed regardless of p.1, because it creates the situation of redundant double erase. I would call it "nested sub-blocks" instead of "double sub-blocks", what do you think? p.1 is a good question to discuss. My understanding, the idea was that if we have a region, and there are many small blocks inside that need erase, maybe it's faster to send one opcode once, and erase the whole region (instead of sending lots of opcodes for each small region). How to decide when "maybe it's faster" was a threshold of how much area is covered by small blocks. And before the patch the threshold was 50% , you changed it to 100%. I am thinking that maybe the optimal is in between? And anyway, I think p.2 needs to be fixed. Not in this patch. I now want to make a patch which changes the code to fix p.2 and also write a test which would fail on head but succeed on the patch. Will you give me some time for this? Also by the way, some time ago I discovered the opposite issue (smaller blocks selected instead of larger one), which I recorded as a failing test in CB:82723. Haven't started looking into it yet, but surely I want to. Just mentioning here because it belongs to the same topic. Oh, it's a long comment, to summarise the questions are > And before the patch the threshold was 50% , you changed it to 100%. I am thinking that maybe the optimal is in between? > I now want to make a patch which changes the code to fix p.2 and also write a test which would fail on head but succeed on the patch. Will you give me some time for this? -- To view, visit https://review.coreboot.org/c/flashrom/+/84614?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I9e10749186e395da67ec80e296119f33c3f83122 Gerrit-Change-Number: 84614 Gerrit-PatchSet: 1 Gerrit-Owner: Bill XIE <persmule(a)hardenedlinux.org> 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-Attention: Bill XIE <persmule(a)hardenedlinux.org> Gerrit-Attention: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Comment-Date: Sat, 05 Oct 2024 14:31:28 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No

1 0

[XS] Change in flashrom[main]: ichspi: Probe opcode in POSSIBLE_OPCODES[] as well
by Anastasia Klimchuk (Code Review) 05 Oct '24

05 Oct '24

Attention is currently required from: Bill XIE, Nikolai Artemiev, Peter Marheine. Anastasia Klimchuk has posted comments on this change by Bill XIE. ( https://review.coreboot.org/c/flashrom/+/84253?usp=email ) Change subject: ichspi: Probe opcode in POSSIBLE_OPCODES[] as well ...................................................................... Patch Set 5: Code-Review+2 (3 comments) File ichspi.c: https://review.coreboot.org/c/flashrom/+/84253/comment/ca73d0b1_ef2bbd2d?us… : PS1, Line 664: int oppos = 2; // use original JEDEC_BE_D8 offset > I made a separate patch CB:84567 in case it end up being useful. […] Okay I am closing this thread https://review.coreboot.org/c/flashrom/+/84253/comment/af921339_559b1464?us… : PS1, Line 1843: : static bool ich_spi_probe_opcode(const struct flashctx *flash, uint8_t opcode) : { : return find_opcode(curopcodes, opcode) >= 0; > Please show me your elegant solution to program other erasing opcodes on the fly, but not before CB:84567 and CB:84593. ;-) You win, you are more elegant :) I had in my mind checking inside `ich_spi_probe_opcode` for all erase opcodes, but I was thinking about hardcoded list of (JEDEC_SE, JEDEC_BE_52, JEDEC_BE_D8, JEDEC_CE_C7). You made it better by checking for POSSIBLE_OPCODES. Can we mark this thread as resolved? The only question that I have left, maybe you have a chance to test at the end of the chain? Thank you! https://review.coreboot.org/c/flashrom/+/84253/comment/7c42712f_f29733d0?us… : PS1, Line 2078: reg > However, is it okay to just use spi_master_ich9 here? (maybe after renaming it to "spi_master_ich79" […] Thanks for fixing! in CB:84593 -- To view, visit https://review.coreboot.org/c/flashrom/+/84253?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I3fc831fc072e2af9265835cb2f71bf8c222c6a64 Gerrit-Change-Number: 84253 Gerrit-PatchSet: 5 Gerrit-Owner: Bill XIE <persmule(a)hardenedlinux.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Bill XIE <persmule(a)hardenedlinux.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Comment-Date: Sat, 05 Oct 2024 09:15:01 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Bill XIE <persmule(a)hardenedlinux.org> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Comment-In-Reply-To: Nikolai Artemiev <nartemiev(a)google.com>

1 0

[S] Change in flashrom[main]: ichspi: Merge spi_master implementations for Intel ich
by Anastasia Klimchuk (Code Review) 05 Oct '24

05 Oct '24

Attention is currently required from: Bill XIE, Peter Marheine. Anastasia Klimchuk has posted comments on this change by Bill XIE. ( https://review.coreboot.org/c/flashrom/+/84593?usp=email ) Change subject: ichspi: Merge spi_master implementations for Intel ich ...................................................................... Patch Set 3: Code-Review+2 (1 comment) Patchset: PS3: > This looks good to me, it seems like probe_opcode was just accidentally left out of ich7. Yes I agree, it looks like probe_opcode should be in both structs, and yes with that they can be merged. I also did some reading of the git history, nothing there that would contradict. -- To view, visit https://review.coreboot.org/c/flashrom/+/84593?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I6a65c97e910622a55da7cef8a10de3af6a99c9e8 Gerrit-Change-Number: 84593 Gerrit-PatchSet: 3 Gerrit-Owner: Bill XIE <persmule(a)hardenedlinux.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Bill XIE <persmule(a)hardenedlinux.org> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Comment-Date: Sat, 05 Oct 2024 08:55:39 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Nikolai Artemiev <nartemiev(a)google.com>

1 0

[XS] Change in flashrom[main]: ichspi: const-correct POSSIBLE_OPCODES[]
by Anastasia Klimchuk (Code Review) 05 Oct '24

05 Oct '24

Attention is currently required from: Bill XIE. Anastasia Klimchuk has posted comments on this change by Bill XIE. ( https://review.coreboot.org/c/flashrom/+/84595?usp=email ) Change subject: ichspi: const-correct POSSIBLE_OPCODES[] ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/84595?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I217f8a9e50b9e2e9f2731adec89a46780874c754 Gerrit-Change-Number: 84595 Gerrit-PatchSet: 1 Gerrit-Owner: Bill XIE <persmule(a)hardenedlinux.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Bill XIE <persmule(a)hardenedlinux.org> Gerrit-Comment-Date: Sat, 05 Oct 2024 08:48:21 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes

1 0

[XS] Change in flashrom[main]: ichspi: Change the opcode position for reprogramming on the fly 2->4
by Anastasia Klimchuk (Code Review) 05 Oct '24

05 Oct '24

Attention is currently required from: Bill XIE. Anastasia Klimchuk has posted comments on this change by Anastasia Klimchuk. ( https://review.coreboot.org/c/flashrom/+/84567?usp=email ) Change subject: ichspi: Change the opcode position for reprogramming on the fly 2->4 ...................................................................... Patch Set 2: (1 comment) Patchset: PS2: Oh you put my patch first in the chain, this is so nice thank you! :) I think CB:84253 would fix the issue by itself, but I still think this patch is useful, it reduces the number of "reprogramming operations". I also wanted to ask, maybe you can test the chain? Or maybe you did it already. Thank you! -- To view, visit https://review.coreboot.org/c/flashrom/+/84567?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I6bc855daedf0af2e8de191f23a3512de3ebc3fef Gerrit-Change-Number: 84567 Gerrit-PatchSet: 2 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Bill XIE <persmule(a)hardenedlinux.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Bill XIE <persmule(a)hardenedlinux.org> Gerrit-Comment-Date: Sat, 05 Oct 2024 08:47:27 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No

1 0

[XS] Change in flashrom[main]: ichspi: Probe opcode in POSSIBLE_OPCODES[] as well
by Nikolai Artemiev (Code Review) 04 Oct '24

04 Oct '24

Attention is currently required from: Anastasia Klimchuk, Bill XIE, Peter Marheine. Nikolai Artemiev has posted comments on this change by Bill XIE. ( https://review.coreboot.org/c/flashrom/+/84253?usp=email ) Change subject: ichspi: Probe opcode in POSSIBLE_OPCODES[] as well ...................................................................... Patch Set 5: (1 comment) File ichspi.c: https://review.coreboot.org/c/flashrom/+/84253/comment/7b02b334_dbc4c0c5?us… : PS1, Line 1843: : static bool ich_spi_probe_opcode(const struct flashctx *flash, uint8_t opcode) : { : return find_opcode(curopcodes, opcode) >= 0; > Merging the masters seems fine to me. […] Oh, ignore my comment about rebasing on CB:84593, I see it's already the parent of this patch. I think I was confused by looking at an older patchset before. -- To view, visit https://review.coreboot.org/c/flashrom/+/84253?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I3fc831fc072e2af9265835cb2f71bf8c222c6a64 Gerrit-Change-Number: 84253 Gerrit-PatchSet: 5 Gerrit-Owner: Bill XIE <persmule(a)hardenedlinux.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Bill XIE <persmule(a)hardenedlinux.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Comment-Date: Fri, 04 Oct 2024 07:13:34 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Bill XIE <persmule(a)hardenedlinux.org> Comment-In-Reply-To: Nikolai Artemiev <nartemiev(a)google.com> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>

1 0

[XS] Change in flashrom[main]: ichspi: const-correct POSSIBLE_OPCODES[]
by Nikolai Artemiev (Code Review) 04 Oct '24

04 Oct '24

Attention is currently required from: Bill XIE. Nikolai Artemiev has posted comments on this change by Bill XIE. ( https://review.coreboot.org/c/flashrom/+/84595?usp=email ) Change subject: ichspi: const-correct POSSIBLE_OPCODES[] ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/84595?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I217f8a9e50b9e2e9f2731adec89a46780874c754 Gerrit-Change-Number: 84595 Gerrit-PatchSet: 1 Gerrit-Owner: Bill XIE <persmule(a)hardenedlinux.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Bill XIE <persmule(a)hardenedlinux.org> Gerrit-Comment-Date: Fri, 04 Oct 2024 07:12:40 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes

1 0

[XS] Change in flashrom[main]: ichspi: Change the opcode position for reprogramming on the fly 2->4
by Nikolai Artemiev (Code Review) 04 Oct '24

04 Oct '24

Attention is currently required from: Anastasia Klimchuk, Bill XIE. Nikolai Artemiev has posted comments on this change by Anastasia Klimchuk. ( https://review.coreboot.org/c/flashrom/+/84567?usp=email ) Change subject: ichspi: Change the opcode position for reprogramming on the fly 2->4 ...................................................................... Patch Set 2: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/84567?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I6bc855daedf0af2e8de191f23a3512de3ebc3fef Gerrit-Change-Number: 84567 Gerrit-PatchSet: 2 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Bill XIE <persmule(a)hardenedlinux.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Bill XIE <persmule(a)hardenedlinux.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Fri, 04 Oct 2024 04:24:06 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes

1 0

[XS] Change in flashrom[main]: ichspi: Probe opcode in POSSIBLE_OPCODES[] as well
by Nikolai Artemiev (Code Review) 04 Oct '24

04 Oct '24

Attention is currently required from: Anastasia Klimchuk, Bill XIE, Peter Marheine. Nikolai Artemiev has posted comments on this change by Bill XIE. ( https://review.coreboot.org/c/flashrom/+/84253?usp=email ) Change subject: ichspi: Probe opcode in POSSIBLE_OPCODES[] as well ...................................................................... Patch Set 5: Code-Review+2 (1 comment) File ichspi.c: https://review.coreboot.org/c/flashrom/+/84253/comment/ca6d3f7b_b5164420?us… : PS1, Line 1843: : static bool ich_spi_probe_opcode(const struct flashctx *flash, uint8_t opcode) : { : return find_opcode(curopcodes, opcode) >= 0; > I managed to make up a solution to program an erase opcode being probed if it is in the POSSIBLE_OPC […] Merging the masters seems fine to me. I have reviewed CB:84593 and I'll submit it if there are no objections soon. You can rebase this patch on top of that one. Also, I assume you originally encountered this problem on an ICH9 device, is that correct? ICH7 shouldn't have had this problem since it didn't have probe_opcode, so flashrom would just assume that all opcodes (including SE) are allowed. -- To view, visit https://review.coreboot.org/c/flashrom/+/84253?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I3fc831fc072e2af9265835cb2f71bf8c222c6a64 Gerrit-Change-Number: 84253 Gerrit-PatchSet: 5 Gerrit-Owner: Bill XIE <persmule(a)hardenedlinux.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Bill XIE <persmule(a)hardenedlinux.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Comment-Date: Fri, 04 Oct 2024 04:19:25 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Bill XIE <persmule(a)hardenedlinux.org> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Comment-In-Reply-To: Nikolai Artemiev <nartemiev(a)google.com>

1 0

[S] Change in flashrom[main]: ichspi: Merge spi_master implementations for Intel ich
by Nikolai Artemiev (Code Review) 04 Oct '24

04 Oct '24

Attention is currently required from: Bill XIE. Nikolai Artemiev has posted comments on this change by Bill XIE. ( https://review.coreboot.org/c/flashrom/+/84593?usp=email ) Change subject: ichspi: Merge spi_master implementations for Intel ich ...................................................................... Patch Set 3: (1 comment) Patchset: PS3: This looks good to me, it seems like probe_opcode was just accidentally left out of ich7. -- To view, visit https://review.coreboot.org/c/flashrom/+/84593?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I6a65c97e910622a55da7cef8a10de3af6a99c9e8 Gerrit-Change-Number: 84593 Gerrit-PatchSet: 3 Gerrit-Owner: Bill XIE <persmule(a)hardenedlinux.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Bill XIE <persmule(a)hardenedlinux.org> Gerrit-Comment-Date: Fri, 04 Oct 2024 04:07:27 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit October 2024