December 2022 - flashrom-gerrit

[M] Change in flashrom[master]: tree/: Move programmer_delay() out of programmer state machine
by Anastasia Klimchuk (Code Review) 08 Dec '22

08 Dec '22

Attention is currently required from: Felix Singer, Nico Huber, Thomas Heijligen, Edward O'Callaghan, Angel Pons. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67393 ) Change subject: tree/: Move programmer_delay() out of programmer state machine ...................................................................... Patch Set 10: Code-Review+2 (2 comments) File flashrom.c: https://review.coreboot.org/c/flashrom/+/67393/comment/9f762481_7e4add37 PS9, Line 265: if (flash->mst->buses_supported & BUS_SPI) { : if (flash->mst->spi.delay) : flash->mst->spi.delay(flash, usecs); : } else if (flash->mst->buses_supported & BUS_PARALLEL) { : if (flash->mst->par.delay) : flash->mst->par.delay(flash, usecs); : } else : internal_delay(usecs); > Done? The difference was for the case when bus is defined, for example BUS_SPI, but custom delay function is NULL. Old logic would call internal_delay in that case, the logic in patchset 9 would call nothing. Latest patchset resolves this. I know delay functions have a void return type, but it is convenient to use return in this case to avoid more elses and elseifs. File serprog.c: https://review.coreboot.org/c/flashrom/+/67393/comment/41f8eb21_b32f0c5b PS9, Line 456: static void serprog_delay(const struct flashctx *flash, unsigned int usecs); > https://review.coreboot. […] I see, I missed `sp_check_opbuf_usage()` -- To view, visit https://review.coreboot.org/c/flashrom/+/67393 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Id059abb58b31a066a408009073912da2b224d40c Gerrit-Change-Number: 67393 Gerrit-PatchSet: 10 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> 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-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Thu, 08 Dec 2022 03:28:40 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: tests: Add llvm-cov option and run target for code coverage
by Anastasia Klimchuk (Code Review) 08 Dec '22

08 Dec '22

Attention is currently required from: Thomas Heijligen, Evan Benn. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/69268 ) Change subject: tests: Add llvm-cov option and run target for code coverage ...................................................................... Patch Set 16: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/69268 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Id6c73bff46e7b88d425956a80def97082b201f56 Gerrit-Change-Number: 69268 Gerrit-PatchSet: 16 Gerrit-Owner: Evan Benn <evanbenn(a)google.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Evan Benn <evanbenn(a)google.com> Gerrit-Comment-Date: Thu, 08 Dec 2022 02:19:17 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: tests: Add llvm-cov option and run target for code coverage
by Evan Benn (Code Review) 08 Dec '22

08 Dec '22

Attention is currently required from: Thomas Heijligen, Anastasia Klimchuk. Evan Benn has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/69268 ) Change subject: tests: Add llvm-cov option and run target for code coverage ...................................................................... Patch Set 16: (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/69268/comment/82c8611c_807b3b89 PS11, Line 14: TEST=meson test; ninja llvm-cov-tests > Great! Then the last thing left: add this to TEST= in commit message :) Done -- To view, visit https://review.coreboot.org/c/flashrom/+/69268 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Id6c73bff46e7b88d425956a80def97082b201f56 Gerrit-Change-Number: 69268 Gerrit-PatchSet: 16 Gerrit-Owner: Evan Benn <evanbenn(a)google.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Thu, 08 Dec 2022 02:08:08 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Comment-In-Reply-To: Evan Benn <evanbenn(a)google.com> Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: tests: Add llvm-cov option and run target for code coverage
by Evan Benn (Code Review) 08 Dec '22

08 Dec '22

Attention is currently required from: Thomas Heijligen, Evan Benn. Hello build bot (Jenkins), Thomas Heijligen, Edward O'Callaghan, Anastasia Klimchuk, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/69268 to look at the new patch set (#16). Change subject: tests: Add llvm-cov option and run target for code coverage ...................................................................... tests: Add llvm-cov option and run target for code coverage Code coverage can be requested with -Dllvm_cov and run with ninja llvm-cov-tests or llvm-cov-cli. BUG=b:187647884 BRANCH=None TEST=meson test; ninja llvm-cov-tests TEST=ran test_build.sh with coverage enabled TEST=jenkins ran test_build.sh with coverage disabled Change-Id: Id6c73bff46e7b88d425956a80def97082b201f56 Signed-off-by: Evan Benn <evanbenn(a)chromium.org> --- M Documentation/building.md M meson.build M meson_options.txt A scripts/llvm-cov M tests/meson.build 5 files changed, 46 insertions(+), 9 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/68/69268/16 -- To view, visit https://review.coreboot.org/c/flashrom/+/69268 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Id6c73bff46e7b88d425956a80def97082b201f56 Gerrit-Change-Number: 69268 Gerrit-PatchSet: 16 Gerrit-Owner: Evan Benn <evanbenn(a)google.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Evan Benn <evanbenn(a)google.com> Gerrit-MessageType: newpatchset

1 0

[M] Change in flashrom[master]: tests: Add llvm-cov option and run target for code coverage
by Anastasia Klimchuk (Code Review) 08 Dec '22

08 Dec '22

Attention is currently required from: Thomas Heijligen, Evan Benn. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/69268 ) Change subject: tests: Add llvm-cov option and run target for code coverage ...................................................................... Patch Set 15: Code-Review+1 (2 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/69268/comment/7886a47a_381bbaaa PS11, Line 14: TEST=meson test; ninja llvm-cov-tests > yes I also ran without coverage (and so does jenkins! V+1) Great! Then the last thing left: add this to TEST= in commit message :) File meson.build: https://review.coreboot.org/c/flashrom/+/69268/comment/d11d7e89_ae9d9579 PS14, Line 625: run_target('llvm-cov-cli', command : ['scripts/llvm-cov', classic_cli]) > This is declaring a run target, its not running anything. […] Oh I see. I agree looks better (less confusing) inside the if -- To view, visit https://review.coreboot.org/c/flashrom/+/69268 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Id6c73bff46e7b88d425956a80def97082b201f56 Gerrit-Change-Number: 69268 Gerrit-PatchSet: 15 Gerrit-Owner: Evan Benn <evanbenn(a)google.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Evan Benn <evanbenn(a)google.com> Gerrit-Comment-Date: Thu, 08 Dec 2022 01:26:48 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Comment-In-Reply-To: Evan Benn <evanbenn(a)google.com> Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: layout: Check return values for strdup in register_include_arg
by Edward O'Callaghan (Code Review) 08 Dec '22

08 Dec '22

Attention is currently required from: David Hendricks, Angel Pons, Anastasia Klimchuk. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/70006 ) Change subject: layout: Check return values for strdup in register_include_arg ...................................................................... Patch Set 6: Code-Review+2 (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/70006/comment/4113b576_f953261f PS5, Line 9: strdup return values should be checked for NULL to catch the : potential error case of out of memory. > First of all, good point about what the patch is doing: indeed, so I updated commit message. […] It looks much better. I still would strongly suggest the separate function to complete the work here, which is only a minor step upon this however this patch is more inline with my suggestions. -- To view, visit https://review.coreboot.org/c/flashrom/+/70006 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I6c22196be6847a8c9704f1de936604a51b4b8a28 Gerrit-Change-Number: 70006 Gerrit-PatchSet: 6 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Thu, 08 Dec 2022 00:54:49 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: tree/: Change chip restore data type from uint8_t to void ptr
by Nikolai Artemiev (Code Review) 08 Dec '22

08 Dec '22

Attention is currently required from: Edward O'Callaghan. Nikolai Artemiev has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/70349 ) Change subject: tree/: Change chip restore data type from uint8_t to void ptr ...................................................................... Patch Set 5: (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/70349/comment/3682f9c7_0b0417c3 PS4, Line 21: TEST=todo > test. Done -- To view, visit https://review.coreboot.org/c/flashrom/+/70349 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I311b468a4b0349f4da9584c12b36af6ec2394527 Gerrit-Change-Number: 70349 Gerrit-PatchSet: 5 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Comment-Date: Thu, 08 Dec 2022 00:41:13 +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]: layout: Check return values for strdup in register_include_arg
by Anastasia Klimchuk (Code Review) 08 Dec '22

08 Dec '22

Attention is currently required from: David Hendricks, Edward O'Callaghan, Angel Pons. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/70006 ) Change subject: layout: Check return values for strdup in register_include_arg ...................................................................... Patch Set 6: (4 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/70006/comment/c413b325_39b4026a PS5, Line 9: strdup return values should be checked for NULL to catch the : potential error case of out of memory. > This patch does more than this. […] First of all, good point about what the patch is doing: indeed, so I updated commit message. Secondly, about splitting into two: I would definitely do the part of "extracting code into static functions" as a separate patch. Can we have this one as first, and extracting code into functions as a second? I did the part of "refactor the inadequate error handling of ternary operators acting on heap allocating functions" in this patch, since that was immediate concern described in the ticket. Overall, patch looks better than it was in the previous version, thank you for ideas! File layout.c: https://review.coreboot.org/c/flashrom/+/70006/comment/304b54ea_52658a1c PS5, Line 134: colon - arg > `if (colon && !colon[1]) {` is no longer checked before this operation. Done https://review.coreboot.org/c/flashrom/+/70006/comment/6ac2c2db_439b2783 PS5, Line 136: Could not allocate memory > CB:69472 Done https://review.coreboot.org/c/flashrom/+/70006/comment/4a1ad134_288dab2b PS5, Line 149: Could not allocate memory\ > CB:69472 Done -- To view, visit https://review.coreboot.org/c/flashrom/+/70006 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I6c22196be6847a8c9704f1de936604a51b4b8a28 Gerrit-Change-Number: 70006 Gerrit-PatchSet: 6 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Thu, 08 Dec 2022 00:41:03 +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: Supplement `chip->unlock()` calls with wp unlocking
by Nikolai Artemiev (Code Review) 08 Dec '22

08 Dec '22

Attention is currently required from: Edward O'Callaghan. Nikolai Artemiev has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/69517 ) Change subject: flashrom.c: Supplement `chip->unlock()` calls with wp unlocking ...................................................................... Patch Set 8: (4 comments) File flashrom.c: https://review.coreboot.org/c/flashrom/+/69517/comment/07c8b3d9_3885c720 PS7, Line 1687: if (ret != FLASHROM_WP_OK) : return -1; : : return 0; > ternary operator seems more concise here. Done https://review.coreboot.org/c/flashrom/+/69517/comment/3a76eb2c_83fabe59 PS7, Line 1696: unlocked_cfg > be consistent `unlocked_wp_cfg` Done https://review.coreboot.org/c/flashrom/+/69517/comment/719c77ca_217f959a PS7, Line 1695: struct flashrom_wp_cfg *original_wp_cfg = NULL; : struct flashrom_wp_cfg *unlocked_cfg = NULL; > Do these need to be initialised, if done unnecessarily it can obscure static analysis. Done https://review.coreboot.org/c/flashrom/+/69517/comment/f65fe723_b22d974e PS7, Line 1708: return -1; > are you leaking `original_wp_cfg` on the return here? […] I fixed resource leaks, in the end I refactored some of this code into a separate function. -- To view, visit https://review.coreboot.org/c/flashrom/+/69517 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I74b3f5d3a17749ea60485b916b2d87467a5d8b2f Gerrit-Change-Number: 69517 Gerrit-PatchSet: 8 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Comment-Date: Thu, 08 Dec 2022 00:40:56 +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]: layout: Check return values for strdup in register_include_arg
by Anastasia Klimchuk (Code Review) 08 Dec '22

08 Dec '22

Attention is currently required from: David Hendricks, Angel Pons, Anastasia Klimchuk. Hello Felix Singer, build bot (Jenkins), David Hendricks, Edward O'Callaghan, Angel Pons, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/70006 to look at the new patch set (#6). Change subject: layout: Check return values for strdup in register_include_arg ...................................................................... layout: Check return values for strdup in register_include_arg strdup return values should be checked for NULL to catch the potential error case of out of memory. This patch re-writes ternary conditionals so that strdup return values could be checked for all branches fof execution. Follow up on commit 45d50a101e8073191e6d88143990ed91d3bfe815 Ticket: https://ticket.coreboot.org/issues/372 Change-Id: I6c22196be6847a8c9704f1de936604a51b4b8a28 Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org> --- M layout.c 1 file changed, 39 insertions(+), 4 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/06/70006/6 -- To view, visit https://review.coreboot.org/c/flashrom/+/70006 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I6c22196be6847a8c9704f1de936604a51b4b8a28 Gerrit-Change-Number: 70006 Gerrit-PatchSet: 6 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit December 2022