September 2022 - flashrom-gerrit

[S] Change in flashrom[master]: internal.c: Pass `programmer_cfg` to `try_mtd()`
by Edward O'Callaghan (Code Review) 21 Sep '22

21 Sep '22

Attention is currently required from: Nico Huber, Angel Pons, Anastasia Klimchuk. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67752 ) Change subject: internal.c: Pass `programmer_cfg` to `try_mtd()` ...................................................................... Patch Set 1: Code-Review+2 (1 comment) Patchset: PS1: > Yeah, I agree. It should be submitted immediately. +1 merge! This got lost in a rebase or squash while amending for reviews. -- To view, visit https://review.coreboot.org/c/flashrom/+/67752 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I9e74bd68a1f9509a201dc518dbff96c27d68a3c3 Gerrit-Change-Number: 67752 Gerrit-PatchSet: 1 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> 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-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Wed, 21 Sep 2022 13:11:10 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Felix Singer <felixsinger(a)posteo.net> Comment-In-Reply-To: Thomas Heijligen <src(a)posteo.de> Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: internal.c: Pass `programmer_cfg` to `try_mtd()`
by Felix Singer (Code Review) 21 Sep '22

21 Sep '22

Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons, Anastasia Klimchuk. Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67752 ) Change subject: internal.c: Pass `programmer_cfg` to `try_mtd()` ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: > This should be merged soon!!! Yeah, I agree. It should be submitted immediately. -- To view, visit https://review.coreboot.org/c/flashrom/+/67752 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I9e74bd68a1f9509a201dc518dbff96c27d68a3c3 Gerrit-Change-Number: 67752 Gerrit-PatchSet: 1 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> 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-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Wed, 21 Sep 2022 13:08:29 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Thomas Heijligen <src(a)posteo.de> Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: internal.c: Pass `programmer_cfg` to `try_mtd()`
by Felix Singer (Code Review) 21 Sep '22

21 Sep '22

Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons, Anastasia Klimchuk. Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67752 ) Change subject: internal.c: Pass `programmer_cfg` to `try_mtd()` ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/67752 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I9e74bd68a1f9509a201dc518dbff96c27d68a3c3 Gerrit-Change-Number: 67752 Gerrit-PatchSet: 1 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> 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-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Wed, 21 Sep 2022 12:53:51 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: ichspi: Factor out common hwseq_xfer logic into helpers
by Subrata Banik (Code Review) 21 Sep '22

21 Sep '22

Attention is currently required from: Nico Huber, Subrata Banik, Angel Pons. Hello build bot (Jenkins), Nico Huber, Edward O'Callaghan, Angel Pons, Anastasia Klimchuk, Nikolai Artemiev, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/62869 to look at the new patch set (#27). Change subject: ichspi: Factor out common hwseq_xfer logic into helpers ...................................................................... ichspi: Factor out common hwseq_xfer logic into helpers List of changes: 1. Add a unified `execute SPI flash transfer` function that does: - Check the SCIP bit prior initiate new operation. - Start the transfer by setting address and length for transfer, finally set FGO bit. - Wait for the transaction to get completed/failed/timed out. 2. All HW Sequencing SPI operation uses `execute SPI flash transfer` function Note: The refactoring xfer logic here assumes setting `HSFC_FDBC to 0` while performing erase operation using `ich_hwseq_block_erase()`. But it does not impact the erase operations. BUG=b:223630977 TEST=Able to perform read-status/write-status/read/write/erase operation on PCH 600 series chipset (board name: google/kano). Signed-off-by: Subrata Banik <subratabanik(a)google.com> Change-Id: Ic9fd50841449e02f476a8834f4642d6ecad36dc3 --- M ichspi.c 1 file changed, 82 insertions(+), 90 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/69/62869/27 -- To view, visit https://review.coreboot.org/c/flashrom/+/62869 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ic9fd50841449e02f476a8834f4642d6ecad36dc3 Gerrit-Change-Number: 62869 Gerrit-PatchSet: 27 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> 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-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: newpatchset

1 0

[M] Change in flashrom[master]: ichspi: Factor out common hwseq_xfer logic into helpers
by Subrata Banik (Code Review) 21 Sep '22

21 Sep '22

Attention is currently required from: Nico Huber, Subrata Banik, Angel Pons. Hello build bot (Jenkins), Nico Huber, Edward O'Callaghan, Angel Pons, Anastasia Klimchuk, Nikolai Artemiev, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/62869 to look at the new patch set (#26). Change subject: ichspi: Factor out common hwseq_xfer logic into helpers ...................................................................... ichspi: Factor out common hwseq_xfer logic into helpers List of changes: 1. Add a unified `execute SPI flash transfer` function that does: - Check the SCIP bit prior initiate new operation. - Start the transfer by setting address and length for transfer, finally set FGO bit. - Wait for the transaction to get completed/failed/timed out. 2. All HW Sequencing SPI operation uses `execute SPI flash transfer` function Note: The refactoring xfer logic here assumes setting `HSFC_FDBC to 0` while performing erase operation using `ich_hwseq_block_erase()`. But it does not impact the erase operations. BUG=b:223630977 TEST=Able to perform read-status/write-status/read/write/erase operation on PCH 600 series chipset (board name: google/kano). Signed-off-by: Subrata Banik <subratabanik(a)google.com> Change-Id: Ic9fd50841449e02f476a8834f4642d6ecad36dc3 --- M ichspi.c 1 file changed, 83 insertions(+), 90 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/69/62869/26 -- To view, visit https://review.coreboot.org/c/flashrom/+/62869 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ic9fd50841449e02f476a8834f4642d6ecad36dc3 Gerrit-Change-Number: 62869 Gerrit-PatchSet: 26 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> 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-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: newpatchset

1 0

[M] Change in flashrom[master]: flashchips.c: enable WP for to 7 entries of MX chips
by Maciej Pijanowski (Code Review) 21 Sep '22

21 Sep '22

Attention is currently required from: Nico Huber, Angel Pons, Nikolai Artemiev, Sergii Dmytruk. Maciej Pijanowski has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/66216 ) Change subject: flashchips.c: enable WP for to 7 entries of MX chips ...................................................................... Patch Set 6: Code-Review+1 (1 comment) Patchset: PS6: Tested on: MX25L3206E and MX25L6406E Some results are here: https://github.com/flashrom/flashrom/issues/185#issuecomment-1253434497 -- To view, visit https://review.coreboot.org/c/flashrom/+/66216 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ib3db9d39ffacd3e9e44de92c6cfb6c3ecc8615bd Gerrit-Change-Number: 66216 Gerrit-PatchSet: 6 Gerrit-Owner: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Maciej Pijanowski <maciej.pijanowski(a)3mdeb.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Wed, 21 Sep 2022 09:19:45 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: test_build.sh: Identify runs for Coverity Scan
by Anastasia Klimchuk (Code Review) 21 Sep '22

21 Sep '22

Attention is currently required from: Patrick Georgi. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67739 ) Change subject: test_build.sh: Identify runs for Coverity Scan ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/67739 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I75147799b1c3213866e343a0384c94d0a1f5c249 Gerrit-Change-Number: 67739 Gerrit-PatchSet: 1 Gerrit-Owner: Patrick Georgi <patrick(a)coreboot.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Patrick Georgi <patrick(a)coreboot.org> Gerrit-Comment-Date: Wed, 21 Sep 2022 08:37:44 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: internal.c: Pass `programmer_cfg` to `try_mtd()`
by Thomas Heijligen (Code Review) 21 Sep '22

21 Sep '22

Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons, Anastasia Klimchuk. Thomas Heijligen has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67752 ) Change subject: internal.c: Pass `programmer_cfg` to `try_mtd()` ...................................................................... Patch Set 1: Code-Review+2 (1 comment) Patchset: PS1: This should be merged soon!!! -- To view, visit https://review.coreboot.org/c/flashrom/+/67752 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I9e74bd68a1f9509a201dc518dbff96c27d68a3c3 Gerrit-Change-Number: 67752 Gerrit-PatchSet: 1 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com> 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-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Wed, 21 Sep 2022 08:04:18 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: cli_classic.c: Early init of layout obscures invalid memory access
by Angel Pons (Code Review) 21 Sep '22

21 Sep '22

Attention is currently required from: Thomas Heijligen, Edward O'Callaghan, Nikolai Artemiev. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67723 ) Change subject: cli_classic.c: Early init of layout obscures invalid memory access ...................................................................... Patch Set 4: Code-Review-1 (2 comments) File cli_classic.c: https://review.coreboot.org/c/flashrom/+/67723/comment/eeea74ca_25893b67 PS2, Line 541: struct flashrom_layout *layout = NULL; > The issue is not that NULL means 'no layout' but rather it is the attempted use of a layout before perhaps there is one available. By early initialisation one obscures this compiler warning that a branch didn't explicitly handle the case 'no layout' before usage. Squelching a compiler warning that can indicate possibly issue with procedural control flow isn't a good idea which made the bug the previous commit in the chain go unnoticed. Hmmm, I'm not sure if I follow. Did CB:67722 fix the problem so that this change would not cause a compiler warnerror? > Ultimately `layout` can still end up as NULL if no layout provided/found, so anything that uses it (e.g. get_region_range call below) should check it is non-null. I agree. > But we can't really get the compiler to enforce that since it only checks the variable has been initialized, and it doesn't check for initialized-to-non-null. I don't think we should rely on the (which?) compiler to warn about this. I'd rather have explicit NULL checks. > I'm ok with leaving layout set to NULL here instead of setting it to NULL in the new `else` block below. I don't think we should have an else clause, as it's much harder to prove that the variable has been initialized. > An alternative would be to delete `default_layout` in `struct flashctx` and assign `flash->layout` to a default layout in `probe_flash()`. Then users can override the default `flash->layout` value if they want, and flash->layout will always be a valid layout. My main concern is that the cli_classic main function is extremely long, and with variables declared at the top of the file, one could accidentally use the layout before it's supposed to be initialized. I think there's a clean way to get around this: move the part of the code that initializes the layout into a function, so that one can declare the layout pointer as constant (`struct flashrom_layout *const layout = foo();`) and be sure that it has been initialized before it can be used. Currently, the layout from file is handled much earlier than layout from IFD or fmap, so there's cases when the layout pointer may be valid in only a few cases. > Anyway I'm going to move this patch to the end since the other two are sufficient to fix the segfault. Sounds good, thanks. https://review.coreboot.org/c/flashrom/+/67723/comment/5011262d_d84568ac PS2, Line 1110: msg_gdbg("Valid layout could not be found without image.\n"); > Maybe we should change it to "No layout provided or found in image", it's really just for debugging. I don't think you can print a correct message with this else clause. What happens if you provide the layout as a file (`-l foo.layout`)? I just noticed that its command-line parameter is processed much earlier than the other layout sources (IFD, fmap), so I would expect that this patch as-is breaks this functionality. Update: I just tested it (and I found I broke stuff yesterday; CB:67752 fixes it) and indeed, if you try using a layout from a file and reach this code, the else branch will be taken even though a valid layout exists. If you still want to print a message for debugging, you'd have to check if the layout pointer is NULL: ``` if (!layout) msg_gdbg("Not using a layout\n"); ``` -- To view, visit https://review.coreboot.org/c/flashrom/+/67723 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I3df61b873da27f6945d60916a1c3713dedfc3924 Gerrit-Change-Number: 67723 Gerrit-PatchSet: 4 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Wed, 21 Sep 2022 07:59:50 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Comment-In-Reply-To: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: flashrom.c: Drop `programmer_param` global variable
by Angel Pons (Code Review) 21 Sep '22

21 Sep '22

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67649 ) Change subject: flashrom.c: Drop `programmer_param` global variable ...................................................................... Patch Set 2: (1 comment) Patchset: PS2: > Hmmmmm, this breaks the internal programmer at least... Ah, CB:67752 should take care of the internal programmer. -- To view, visit https://review.coreboot.org/c/flashrom/+/67649 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I59397451ea625bd431b15848bad5ec7cb926f22d Gerrit-Change-Number: 67649 Gerrit-PatchSet: 2 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Wed, 21 Sep 2022 07:50:39 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit September 2022