- flashrom-gerrit - mail.coreboot.org

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

07 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 5: (1 comment) File layout.c: https://review.coreboot.org/c/flashrom/+/70006/comment/52ae100b_e51cfcea PS5, Line 134: colon - arg `if (colon && !colon[1]) {` is no longer checked before this operation. -- 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: 5 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: Wed, 07 Dec 2022 22:26:23 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No 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) 07 Dec '22

07 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 5: (3 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/70006/comment/f36b5b30_3c2d576b 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. It refactors, if you are going to do this I suggest splitting the patch into perhaps two patches. The first should factor out all this C-typical string parsing hand grenade type stuff into its own static function. The second patch can refactor the inadequate error handling of ternary operators acting on heap allocating functions. Also if you are going to unroll the ternary operators, do all of them to make the function procedurally obvious. File layout.c: https://review.coreboot.org/c/flashrom/+/70006/comment/40111dcb_a81f9fde PS5, Line 136: Could not allocate memory CB:69472 https://review.coreboot.org/c/flashrom/+/70006/comment/e65aa596_442440a1 PS5, Line 149: Could not allocate memory\ CB:69472 -- 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: 5 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: Wed, 07 Dec 2022 22:23:02 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: test_build.sh: Add build_rust
by Evan Benn (Code Review) 07 Dec '22

07 Dec '22

Attention is currently required from: Felix Singer, Patrick Georgi. Evan Benn has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/69770 ) Change subject: test_build.sh: Add build_rust ...................................................................... Patch Set 2: (2 comments) File test_build.sh: https://review.coreboot.org/c/flashrom/+/69770/comment/9e3df2bb_0aba3378 PS1, Line 87: do_for_rust test --offline > this is not great as a user that has cargo but hasnt run this before needs to manually run the same […] Discussed with felix on irc, felix pointed out that rust doesnt go online if things are in the cache anyway. So this will work the same with or without --offline. https://review.coreboot.org/c/flashrom/+/69770/comment/ba29e975_9ae407a1 PS1, Line 94: hash cargo && build_rust > when jenkins has cargo I would make this […] Done -- To view, visit https://review.coreboot.org/c/flashrom/+/69770 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I728f5b856a9161b87b96207f4c3965e7493c33d1 Gerrit-Change-Number: 69770 Gerrit-PatchSet: 2 Gerrit-Owner: Evan Benn <evanbenn(a)google.com> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Patrick Georgi <patrick(a)coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Patrick Georgi <patrick(a)coreboot.org> Gerrit-Comment-Date: Wed, 07 Dec 2022 22:15:46 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No 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) 07 Dec '22

07 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 14: (2 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/69268/comment/bfeace61_dcfba292 PS11, Line 14: TEST=meson test; ninja llvm-cov-tests > I meant to say, test scenario == "build flashrom and run tests with coverage disabled" […] yes I also ran without coverage (and so does jenkins! V+1) File meson.build: https://review.coreboot.org/c/flashrom/+/69268/comment/5bb531b2_b1ceb172 PS14, Line 625: run_target('llvm-cov-cli', command : ['scripts/llvm-cov', classic_cli]) > But why this is always run? It should only run when feature is enabled? This is declaring a run target, its not running anything. It can go inside a feature if, it will be less confusing to a user inspecting the ninja run targets. thanks -- 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: 14 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: Wed, 07 Dec 2022 22:11:07 +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) 07 Dec '22

07 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 (#15). 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 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, 45 insertions(+), 9 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/68/69268/15 -- 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-MessageType: newpatchset

1 0

[S] Change in flashrom[master]: README: Add information about meson and link build instructions
by Edward O'Callaghan (Code Review) 07 Dec '22

07 Dec '22

Attention is currently required from: David Hendricks, Anastasia Klimchuk. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/70176 ) Change subject: README: Add information about meson and link build instructions ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/70176 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I2a27d8f2ba42e18be2485ae95bec1b4c874bb4f7 Gerrit-Change-Number: 70176 Gerrit-PatchSet: 1 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> 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: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Wed, 07 Dec 2022 22:09:46 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

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

07 Dec '22

Attention is currently required from: Nikolai Artemiev. Sergii Dmytruk 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 4: Code-Review+1 -- 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: 4 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: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Wed, 07 Dec 2022 18:54:38 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: flashchips.c: remove WREN from GD25Q256D enter 4BA sequence
by Nico Huber (Code Review) 07 Dec '22

07 Dec '22

Attention is currently required from: Edward O'Callaghan, Nikolai Artemiev, Eric Lai. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/70342 ) Change subject: flashchips.c: remove WREN from GD25Q256D enter 4BA sequence ...................................................................... Patch Set 3: Code-Review+1 (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/70342/comment/4e48c80b_36b65dc5 PS1, Line 19: FT2232H > Ok interesting, I wrote a little test case: […] Many thanks for the elaborate testing! -- To view, visit https://review.coreboot.org/c/flashrom/+/70342 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I96e48933f33c52c0d10a0d4cb7f7e07c1fceab99 Gerrit-Change-Number: 70342 Gerrit-PatchSet: 3 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.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: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Eric Lai <eric_lai(a)quanta.corp-partner.google.com> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Eric Lai <eric_lai(a)quanta.corp-partner.google.com> Gerrit-Comment-Date: Wed, 07 Dec 2022 11:53:13 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: ichspi.c: Read chip ID and use it to populate `flash->chip`
by Angel Pons (Code Review) 07 Dec '22

07 Dec '22

Attention is currently required from: Nikolai Artemiev. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/69195 ) Change subject: ichspi.c: Read chip ID and use it to populate `flash->chip` ...................................................................... Patch Set 12: (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/69195/comment/59696b58_79997a2e PS2, Line 7: ichspi.c: Read chip ID and use it to identify chip > Subrata can you take a look too please. There aren't that many systems with 2 flash chips, but they exist. Ivy Bridge and Haswell ThinkPads have a 8 MiB chip and a 4 MiB chip, and flashrom detects a single 12 MiB opaque flash chip. The problem with sending WP commands in such situations is that you have 2 chips to deal with. No idea how hwseq handles this. -- To view, visit https://review.coreboot.org/c/flashrom/+/69195 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ia408e1e45dc6f53c0934afd6558e301abfa48ee6 Gerrit-Change-Number: 69195 Gerrit-PatchSet: 12 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: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Wed, 07 Dec 2022 10:55:15 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com> 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

[M] Change in flashrom[master]: ichspi.c: Read chip ID and use it to populate `flash->chip`
by Nico Huber (Code Review) 07 Dec '22

07 Dec '22

Attention is currently required from: Angel Pons, Nikolai Artemiev. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/69195 ) Change subject: ichspi.c: Read chip ID and use it to populate `flash->chip` ...................................................................... Patch Set 12: (6 comments) File ichspi.c: https://review.coreboot.org/c/flashrom/+/69195/comment/229372ca_f2c15230 PS12, Line 1500: flash->chip->tested = TEST_OK_PREWB; What if ich_hwseq_get_flash_id() bailed? https://review.coreboot.org/c/flashrom/+/69195/comment/26c5d96f_52e3d538 PS12, Line 135: #define HSFC_CYCLE_RDID HSFC_FCYCLE_MASK(6) This belongs to PCH100_HSFC_FCYCLE*. It doesn't fit in the 2 bits for ICH9. (ICH9_HSFC_FCYCLE_BIT_WIDTH is a mask, not a number of bits, apparently). https://review.coreboot.org/c/flashrom/+/69195/comment/4d7ff62c_95702594 PS12, Line 1404: if ((chip->manufacture_id == mfg_id) && (chip->model_id == model_id)) Should check if it's a SPI flash and if the database numbers are for RDID. https://review.coreboot.org/c/flashrom/+/69195/comment/d7deac47_99b4b57a PS12, Line 1466: const int len = sizeof(data); Why 4? https://review.coreboot.org/c/flashrom/+/69195/comment/699cafbf_a65018c0 PS12, Line 1474: if (ich_exec_sync_hwseq_xfer(flash, HSFC_CYCLE_RDID, 1, len, ich_generation, Should check if RDID cycle is supported (since PCH100). https://review.coreboot.org/c/flashrom/+/69195/comment/e43ed110_ed398c89 PS12, Line 1494: return -1; Does this break normal read/erase/write operation if there is no matching entry? -- To view, visit https://review.coreboot.org/c/flashrom/+/69195 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ia408e1e45dc6f53c0934afd6558e301abfa48ee6 Gerrit-Change-Number: 69195 Gerrit-PatchSet: 12 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: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Wed, 07 Dec 2022 10:49:22 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit