December 2022 - flashrom-gerrit

[M] Change in flashrom[master]: flashrom: Check for flash access restricitons in read_flash()
by Edward O'Callaghan (Code Review) 10 Dec '22

10 Dec '22

Attention is currently required from: Nikolai Artemiev. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/70514 ) Change subject: flashrom: Check for flash access restricitons in read_flash() ...................................................................... Patch Set 3: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/70514 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I22c795d7d08ef8bf773733d9952967b2fa2ef299 Gerrit-Change-Number: 70514 Gerrit-PatchSet: 3 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Sat, 10 Dec 2022 23:48:31 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: flashchips.c: Indent definition of W25Q512NW-IM properly
by Edward O'Callaghan (Code Review) 10 Dec '22

10 Dec '22

Attention is currently required from: Felix Singer. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/68151 ) Change subject: flashchips.c: Indent definition of W25Q512NW-IM properly ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/68151 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Icfd2a49383da0f8f0a4e3295aba81ce1d200652c Gerrit-Change-Number: 68151 Gerrit-PatchSet: 1 Gerrit-Owner: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Comment-Date: Sat, 10 Dec 2022 23:46:40 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: flashchips.c: Indent definition of W25Q512NW-IM properly
by Edward O'Callaghan (Code Review) 10 Dec '22

10 Dec '22

Attention is currently required from: Felix Singer. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/68151 ) Change subject: flashchips.c: Indent definition of W25Q512NW-IM properly ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: rebase on HEAD and let's get this merged? -- To view, visit https://review.coreboot.org/c/flashrom/+/68151 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Icfd2a49383da0f8f0a4e3295aba81ce1d200652c Gerrit-Change-Number: 68151 Gerrit-PatchSet: 1 Gerrit-Owner: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Comment-Date: Sat, 10 Dec 2022 23:46:35 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[L] Change in flashrom[master]: flashrom.c: Add wrapper function to use the erase algorithm
by Edward O'Callaghan (Code Review) 10 Dec '22

10 Dec '22

Attention is currently required from: Simon Buhrow, Nico Huber, Aarya, Anastasia Klimchuk. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/66104 ) Change subject: flashrom.c: Add wrapper function to use the erase algorithm ...................................................................... Patch Set 73: (1 comment) File flashrom.c: https://review.coreboot.org/c/flashrom/+/66104/comment/d3e27273_45d30e79 PS72, Line 1349: static int erase_by_layout > what you want to do is have both paths parallel to one another and atomically switch over to the nex […] A path to do this could be; - factor out the erase logic into its own C file from flashrom.c - put your implementation into another C file, - finally have the functions in flashrom.c be the entry-point to switch between those two? The benefits here would be, easy A/B testing, less disruptive, better long term vision on flashrom.c kitchsink syndrome, more modularity for the erasure path. -- To view, visit https://review.coreboot.org/c/flashrom/+/66104 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I29e3f2bd796759794184b125741a5abaac6f3ce8 Gerrit-Change-Number: 66104 Gerrit-PatchSet: 73 Gerrit-Owner: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: Simon Buhrow Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-CC: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Simon Buhrow Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Sat, 10 Dec 2022 23:44:16 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: flashrom.c:Add a function to get list of sectors that need erasing
by Edward O'Callaghan (Code Review) 10 Dec '22

10 Dec '22

Attention is currently required from: Simon Buhrow, Aarya. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/65844 ) Change subject: flashrom.c:Add a function to get list of sectors that need erasing ...................................................................... Patch Set 57: (1 comment) File flashrom.c: https://review.coreboot.org/c/flashrom/+/65844/comment/05d21115_9cc42441 PS56, Line 1203: { > That would complicate the recursion probably? Can you elaborate what you think would cause issue? Each function recuse has its own frame. -- To view, visit https://review.coreboot.org/c/flashrom/+/65844 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ic57ca1cca3d1646543f6b5939ba9c35db8d08256 Gerrit-Change-Number: 65844 Gerrit-PatchSet: 57 Gerrit-Owner: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: Simon Buhrow Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-CC: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Simon Buhrow Gerrit-Attention: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Comment-Date: Sat, 10 Dec 2022 23:39:40 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Comment-In-Reply-To: Aarya <aarya.chaumal(a)gmail.com> Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: flashrom.c:Add function to get a flattened view of the chip erase blocks
by Edward O'Callaghan (Code Review) 10 Dec '22

10 Dec '22

Attention is currently required from: Simon Buhrow, Nico Huber, Paul Menzel, Aarya. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/65879 ) Change subject: flashrom.c:Add function to get a flattened view of the chip erase blocks ...................................................................... Patch Set 42: (8 comments) File flashrom.c: https://review.coreboot.org/c/flashrom/+/65879/comment/07d5a8df_b7c7f180 PS41, Line 1237: : struct erase_layout *create_erase_layout(struct flashctx *const flashctx); : > This function later becomes a static. […] Would it make sense for these functions to become part of layout.c perhaps? I feel like all this specific logic is fundamentally a abstract data structure part of the project you are annotating to later consume, walk and determine the set of actions to be taken for optimality of erasure in flashrom.c. File flashrom.c: https://review.coreboot.org/c/flashrom/+/65879/comment/2ce10007_6c565a1d PS42, Line 1240: s const https://review.coreboot.org/c/flashrom/+/65879/comment/fbf292a9_70ff2c3b PS42, Line 1242: s const https://review.coreboot.org/c/flashrom/+/65879/comment/8731a996_d5b83b1b PS42, Line 1243: layout `struct erase_layout *layout = ` and drop the cast? Also `calloc(1, ..` avoids uninitialised heap by zeroing it. https://review.coreboot.org/c/flashrom/+/65879/comment/64390aee_e65742e2 PS42, Line 1245: if (layout == NULL) ``` if (!ptr) return NULL; ``` is canonical. You also want the `msg_gerr("Out of memory!\n");` message in this branch to be consistent with the rest of the codebase. https://review.coreboot.org/c/flashrom/+/65879/comment/72662f1b_673af48e PS42, Line 1249: index doesn't `index` and `i` count in tandem? You can just use `i` and drop the `index`? https://review.coreboot.org/c/flashrom/+/65879/comment/ebb9fd9a_5aaf89dc PS42, Line 1250: (i should be fine to scope the `i` indexer to be lexically coupled to the loop construct with `for (size_t i = 0; ..` https://review.coreboot.org/c/flashrom/+/65879/comment/46c158ae_7577ea4b PS42, Line 1259: { `chip->block_erasers[i].eraseblocks[j]` as a intermediate to the loop pre-amble. -- To view, visit https://review.coreboot.org/c/flashrom/+/65879 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Iafe78de00daa55f7114bd4ce09465dd88074ece4 Gerrit-Change-Number: 65879 Gerrit-PatchSet: 42 Gerrit-Owner: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: Simon Buhrow Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-CC: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Simon Buhrow Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Comment-Date: Sat, 10 Dec 2022 23:37:30 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Comment-In-Reply-To: Aarya <aarya.chaumal(a)gmail.com> Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: flashrom.c:Add function to align region to sector boundaries
by Edward O'Callaghan (Code Review) 10 Dec '22

10 Dec '22

Attention is currently required from: Aarya. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/65642 ) Change subject: flashrom.c:Add function to align region to sector boundaries ...................................................................... Patch Set 58: (2 comments) File flashrom.c: https://review.coreboot.org/c/flashrom/+/65642/comment/2d6baeb6_5673e171 PS57, Line 1183: layout[i].layout_list[j] > intermediate to the inner for-loop construct? for example, something like; ``` for (size_t j = 0; j < layout[i].block_count; j++) { const struct eraseblock_data *ll = &layout[i].layout_list[j]; if (ll->start_addr <= *region_start) start_diff = (*region_start - ll->start_addr) > start_diff ? start_diff : (*region_start - ll->start_addr); [..] } ``` File flashrom.c: https://review.coreboot.org/c/flashrom/+/65642/comment/514afdb9_71d5afe1 PS58, Line 1181: int also indexing array's with `unsigned int` -- To view, visit https://review.coreboot.org/c/flashrom/+/65642 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I215ea4986aa23360fc65ff761f4e49c6069160ac Gerrit-Change-Number: 65642 Gerrit-PatchSet: 58 Gerrit-Owner: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: Simon Buhrow Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-CC: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Comment-Date: Sat, 10 Dec 2022 23:26:06 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: flashrom.c: Update check_block_eraser function to use probe opcode
by Edward O'Callaghan (Code Review) 10 Dec '22

10 Dec '22

Attention is currently required from: Simon Buhrow, Nico Huber, Thomas Heijligen, Aarya. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/66717 ) Change subject: flashrom.c: Update check_block_eraser function to use probe opcode ...................................................................... Patch Set 21: (1 comment) Patchset: PS21: merge? -- To view, visit https://review.coreboot.org/c/flashrom/+/66717 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I6591a84ae1fe5bc1648051cc30b9393450033852 Gerrit-Change-Number: 66717 Gerrit-PatchSet: 21 Gerrit-Owner: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Simon Buhrow Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Simon Buhrow Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Comment-Date: Sat, 10 Dec 2022 23:16:10 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: spi.c: Add AT45 & SF25F erasefn opcode mapping
by Edward O'Callaghan (Code Review) 10 Dec '22

10 Dec '22

Attention is currently required from: Simon Buhrow, Thomas Heijligen, Aarya. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67717 ) Change subject: spi.c: Add AT45 & SF25F erasefn opcode mapping ...................................................................... Patch Set 8: (1 comment) Patchset: PS6: > I'm fine with both. Well do you want to squash CB:67715 && CB:67716 into this and merge to get this tucked away into HEAD. I think it is pretty safe to get some of this merged and behind us rather than sitting in review forever. -- To view, visit https://review.coreboot.org/c/flashrom/+/67717 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I798a91f1e20b63662715c68e6d43d03fc6005d51 Gerrit-Change-Number: 67717 Gerrit-PatchSet: 8 Gerrit-Owner: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Simon Buhrow Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Attention: Simon Buhrow Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Comment-Date: Sat, 10 Dec 2022 23:15:27 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Thomas Heijligen <src(a)posteo.de> Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: flashrom.c:Add a function to get list of sectors that need erasing
by Aarya (Code Review) 10 Dec '22

10 Dec '22

Attention is currently required from: Simon Buhrow, Edward O'Callaghan. Aarya has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/65844 ) Change subject: flashrom.c:Add a function to get list of sectors that need erasing ...................................................................... Patch Set 56: (5 comments) File flashrom.c: https://review.coreboot.org/c/flashrom/+/65844/comment/6d32e3b6_7b2805a8 PS56, Line 1203: { > `layout[function_index]. […] That would complicate the recursion probably? https://review.coreboot.org/c/flashrom/+/65844/comment/0d139f69_ce6d20cc PS56, Line 1204: ) : { > `) {` Done https://review.coreboot.org/c/flashrom/+/65844/comment/074d9e65_1ed456f9 PS56, Line 1214: } : else { > `} else {` Done https://review.coreboot.org/c/flashrom/+/65844/comment/47520377_e772a495 PS56, Line 1220: r( > nit: `for (` Done https://review.coreboot.org/c/flashrom/+/65844/comment/20861912_8360099d PS56, Line 1225: (sub_block_end - sub_block_start + 1)/2 > hmm, maybe good to use a const intermediate here for clarity. Done -- To view, visit https://review.coreboot.org/c/flashrom/+/65844 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ic57ca1cca3d1646543f6b5939ba9c35db8d08256 Gerrit-Change-Number: 65844 Gerrit-PatchSet: 56 Gerrit-Owner: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: Simon Buhrow Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-CC: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Simon Buhrow Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Comment-Date: Sat, 10 Dec 2022 21:26:09 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: comment

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit December 2022