- flashrom-gerrit - mail.coreboot.org

[S] Change in flashrom[master]: dummyflasher: use new API to register shutdown function
by Alexander Goncharov (Code Review) 26 Jan '23

26 Jan '23

Attention is currently required from: Edward O'Callaghan, Anastasia Klimchuk. Alexander Goncharov has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/72408 ) Change subject: dummyflasher: use new API to register shutdown function ...................................................................... Patch Set 2: (1 comment) File dummyflasher.c: https://review.coreboot.org/c/flashrom/+/72408/comment/f5a78171_450db6e8 PS2, Line 910: > What if you try: […] 1) dummy_shutdown() has as an argument `void *data`, not `struct flashctx *flash`. It's just a pointer to the data. Of course, we can free the data, but we cannot make `rmst.(par/spi/opaque).data` equals NULL. If we do `emu_data = NULL`, it makes the _local_ pointer equals NULL. 2) When we talk about data ownership, the last owner of the data must release this data. Not the first, because it can bring us more problems. I'll try to figure out what I can add to the unit tests. -- To view, visit https://review.coreboot.org/c/flashrom/+/72408 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I0c67c25b0f53cd8c564c4ea0f09f2728e856f6ea Gerrit-Change-Number: 72408 Gerrit-PatchSet: 2 Gerrit-Owner: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Thu, 26 Jan 2023 09:10:30 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

[L] Change in flashrom[master]: flashrom.c: Add functions for new erase selction algorithm
by Edward O'Callaghan (Code Review) 26 Jan '23

26 Jan '23

Attention is currently required from: Simon Buhrow, Aarya, Anastasia Klimchuk. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/65844 ) Change subject: flashrom.c: Add functions for new erase selction algorithm ...................................................................... Patch Set 86: (8 comments) Patchset: PS86: relatively minor stuff now, we can still iterate in tree however the more const correctness the better. The more expensive you can be with types the better it communicates your intentions of your code to the compiler - e.g., modifying a loop upper bound you intended to be const. File erasure_layout.c: https://review.coreboot.org/c/flashrom/+/65844/comment/d94201de_4922fcfd PS86, Line 114: s `const` https://review.coreboot.org/c/flashrom/+/65844/comment/2bd74227_c520ebbf PS86, Line 130: size_t num; delete https://review.coreboot.org/c/flashrom/+/65844/comment/4565cf95_70abb9fd PS86, Line 133: ( `size_t num = 0;` https://review.coreboot.org/c/flashrom/+/65844/comment/68295e89_cb982826 PS86, Line 165: const unsigned int `const size_t`? https://review.coreboot.org/c/flashrom/+/65844/comment/8b486bf7_7f20a426 PS86, Line 166: unsigned int and this `size_t` too? https://review.coreboot.org/c/flashrom/+/65844/comment/d39e3d42_f18e6113 PS86, Line 216: int `const int` ? https://review.coreboot.org/c/flashrom/+/65844/comment/e15ee17e_8171ecc3 PS86, Line 217: int `const int` ? -- 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: 86 Gerrit-Owner: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> 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: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Simon Buhrow Gerrit-Attention: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Thu, 26 Jan 2023 08:03:49 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: flashrom.c: Add switch for legacy impl of erasure path
by Edward O'Callaghan (Code Review) 26 Jan '23

26 Jan '23

Attention is currently required from: Felix Singer, Thomas Heijligen, Aarya. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/71119 ) Change subject: flashrom.c: Add switch for legacy impl of erasure path ...................................................................... Patch Set 13: (2 comments) Patchset: PS10: > ` Ack File flashrom.c: https://review.coreboot.org/c/flashrom/+/71119/comment/4b479c5d_340910ad PS12, Line 37: use_legacy_erase_path > `use_legacy_erase_path` is not modified anywhere. […] You _could_ add it as a build flag however this is easy enough to toggle with a one-line commit too. I believe Thomas is catching up on email however he could perhaps help advise with working towards making the new algorithm the default. My suggestion once you have your work landed on HEAD is to send a email to the mailing list/IRC channel and try to engage some folks to help with testing. The other part is to try and build up the portfolio of unit-test coverage around this new algorithm that instrument the fundamental data structures and procedures mapped over them at play here. Furthermore the unit-tests will help mature not just the functional code quality but also the code structure. -- To view, visit https://review.coreboot.org/c/flashrom/+/71119 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ib5660db0067c1c799dcb5c8e83b4a4826b236442 Gerrit-Change-Number: 71119 Gerrit-PatchSet: 13 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Aarya <aarya.chaumal(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: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Comment-Date: Thu, 26 Jan 2023 07:52:36 +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 new erase_by_layout and walk_by_layout implementations
by Edward O'Callaghan (Code Review) 26 Jan '23

26 Jan '23

Attention is currently required from: Simon Buhrow, Thomas Heijligen, Aarya, Anastasia Klimchuk. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/71173 ) Change subject: flashrom.c: Add new erase_by_layout and walk_by_layout implementations ...................................................................... Patch Set 21: Code-Review+2 (2 comments) File flashrom.c: https://review.coreboot.org/c/flashrom/+/71173/comment/b626580a_b69a82f5 PS6, Line 1701: (uint8_t*) > The newcontents buffer may b written while aligning boundaries. […] OK, but we should fix the `const void *` in another patch. https://review.coreboot.org/c/flashrom/+/71173/comment/2729f8c7_5f41731c PS6, Line 1708: free_erase_layout(erase_layout, erasefn_count); > `free_erase_layout(erase_layout, count_usable_erasers(flashctx));` […] Ack -- To view, visit https://review.coreboot.org/c/flashrom/+/71173 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Id79ae943eb9d6a817da28381db477725834faaf6 Gerrit-Change-Number: 71173 Gerrit-PatchSet: 21 Gerrit-Owner: Aarya <aarya.chaumal(a)gmail.com> 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: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Simon Buhrow Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Thu, 26 Jan 2023 07:46:59 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes 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

[L] Change in flashrom[master]: flashrom.c: Add functions for new erase selction algorithm
by Edward O'Callaghan (Code Review) 26 Jan '23

26 Jan '23

Attention is currently required from: Simon Buhrow, Aarya, Anastasia Klimchuk. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/65844 ) Change subject: flashrom.c: Add functions for new erase selction algorithm ...................................................................... Patch Set 86: Code-Review+2 -- 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: 86 Gerrit-Owner: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> 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: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Simon Buhrow Gerrit-Attention: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Thu, 26 Jan 2023 07:45:17 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: flashrom.c: Add switch for legacy impl of erasure path
by Edward O'Callaghan (Code Review) 26 Jan '23

26 Jan '23

Attention is currently required from: Felix Singer, Thomas Heijligen. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/71119 ) Change subject: flashrom.c: Add switch for legacy impl of erasure path ...................................................................... Patch Set 13: -Code-Review -- To view, visit https://review.coreboot.org/c/flashrom/+/71119 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ib5660db0067c1c799dcb5c8e83b4a4826b236442 Gerrit-Change-Number: 71119 Gerrit-PatchSet: 13 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Aarya <aarya.chaumal(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: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Comment-Date: Thu, 26 Jan 2023 07:43:35 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: flashrom.c: Fix skip flag typo in read_flash()
by Nikolai Artemiev (Code Review) 26 Jan '23

26 Jan '23

Attention is currently required from: Edward O'Callaghan, Angel Pons, Anastasia Klimchuk. Nikolai Artemiev has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/72454 ) Change subject: flashrom.c: Fix skip flag typo in read_flash() ...................................................................... Patch Set 1: Code-Review+1 -- To view, visit https://review.coreboot.org/c/flashrom/+/72454 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie04cd62020ca29775fc66a81d2fadd32a5aab5cd Gerrit-Change-Number: 72454 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Elyes Haouas <ehaouas(a)noos.fr> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Sam McNally <sammc(a)google.com> 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: Thu, 26 Jan 2023 05:32:55 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: flashrom.c: Fix skip flag typo in read_flash()
by Elyes Haouas (Code Review) 26 Jan '23

26 Jan '23

Attention is currently required from: Edward O'Callaghan, Angel Pons, Anastasia Klimchuk. Elyes Haouas has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/72454 ) Change subject: flashrom.c: Fix skip flag typo in read_flash() ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/72454 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie04cd62020ca29775fc66a81d2fadd32a5aab5cd Gerrit-Change-Number: 72454 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Elyes Haouas <ehaouas(a)noos.fr> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-CC: Sam McNally <sammc(a)google.com> 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: Thu, 26 Jan 2023 04:11:22 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: flashrom.c: Fix skip flag typo in read_flash()
by Edward O'Callaghan (Code Review) 26 Jan '23

26 Jan '23

Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/72454 ) Change subject: flashrom.c: Fix skip flag typo in read_flash() ...................................................................... flashrom.c: Fix skip flag typo in read_flash() Only picked up by internal test infra in ChromeOS when instrumenting futility(1). The following error was incured, ``` >> Starting firmware updater. >> Target image: images/bios-kindred.ro-12672-141-0.rw-12672-141-0.bin (RO:Google_Kindred.12672.141.0, RW/A:Google_Kindred.12672.141.0, RW/B:Google_Kindred.12672.141.0). INFO: update_firmware: Loading current system firmware... INFO: load_system_firmware: flashrom -r <IMAGE> -p host Warning: Setting BIOS Control at 0xdc from 0x8b to 0x89 failed. New value is 0x8b. At least some flash regions are read protected. You have to use a flash layout and include only accessible regions. For write operations, you'll additionally need the --noverify-all switch. See manpage for more details. read_flash: cannot read inside Management Engine region (0x001000..0x3fffff). Read operation failed! ERROR: do_update: Cannot load system active firmware. ``` despite the appropriate flag being set within futility(1). BUG=b:266748702 TEST=cros fw updates. Change-Id: Ie04cd62020ca29775fc66a81d2fadd32a5aab5cd Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M flashrom.c 1 file changed, 34 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/54/72454/1 diff --git a/flashrom.c b/flashrom.c index 67b71f0..f873300 100644 --- a/flashrom.c +++ b/flashrom.c @@ -573,7 +573,7 @@ uint8_t *rbuf = buf + addr - start; if (region.read_prot) { - if (flash->flags.skip_unwritable_regions) { + if (flash->flags.skip_unreadable_regions) { msg_gdbg("%s: cannot read inside %s region (%#08x..%#08x), " "filling (%#08x..%#08x) with erased value instead.\n", __func__, region.name, region.start, region.end - 1, -- To view, visit https://review.coreboot.org/c/flashrom/+/72454 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie04cd62020ca29775fc66a81d2fadd32a5aab5cd Gerrit-Change-Number: 72454 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: newchange

1 0

[M] Change in flashrom[master]: flashrom: Check for flash access restricitons in read_flash()
by Edward O'Callaghan (Code Review) 26 Jan '23

26 Jan '23

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 5: (1 comment) File flashrom.c: https://review.coreboot.org/c/flashrom/+/70514/comment/b2a98682_2ae9a5ad PS5, Line 550: unwritable shouldn't this be 'unreadable' based on the commit message, this CB:70128 and the identifier? -- 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: 5 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> 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: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Wed, 25 Jan 2023 23:35:53 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit