November 2022 - flashrom-gerrit

[S] Change in flashrom[master]: libflashrom: Define macro to detect v1.3 header
by Evan Benn (Code Review) 28 Nov '22

28 Nov '22

Attention is currently required from: Felix Singer, Edward O'Callaghan, Angel Pons, Anastasia Klimchuk. Evan Benn has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/70027 ) Change subject: libflashrom: Define macro to detect v1.3 header ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: > I talked to aklm about unblocking the release. […] I'm not against doing it this way, but I'm wondering how this gets used. Do we keep this macro forever? or drop it at v1.4 and add a v1.4 macro? It seems like I as a library user might want to know something like; version is at least 1.4, or version is between 1.5 and 1.6. That sounds complicated with a symbolic macro like this. `__STDC_VERSION__` is a case study, it is defined to a number (date), so I can test `#if __STDC_VERSION__ > 202201` Maybe we could do `#define LIBFLASHROM_VERSION` $MAJOR$MINOR? -- To view, visit https://review.coreboot.org/c/flashrom/+/70027 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Iac35ff167272e1bc3d5434f7b47099f53bfceab7 Gerrit-Change-Number: 70027 Gerrit-PatchSet: 1 Gerrit-Owner: 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: Evan Benn <evanbenn(a)google.com> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> 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: Mon, 28 Nov 2022 22:21:27 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Comment-In-Reply-To: Evan Benn <evanbenn(a)google.com> Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: libflashrom: Define macro to detect v1.3 header
by Edward O'Callaghan (Code Review) 28 Nov '22

28 Nov '22

Attention is currently required from: Felix Singer, Angel Pons, Anastasia Klimchuk, Evan Benn. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/70027 ) Change subject: libflashrom: Define macro to detect v1.3 header ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: > :O it's assigned to me! […] I talked to aklm about unblocking the release. This is minimal viable to allow for user code to know which API rev it is at least # including. -- To view, visit https://review.coreboot.org/c/flashrom/+/70027 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Iac35ff167272e1bc3d5434f7b47099f53bfceab7 Gerrit-Change-Number: 70027 Gerrit-PatchSet: 1 Gerrit-Owner: 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: Evan Benn <evanbenn(a)google.com> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Evan Benn <evanbenn(a)google.com> Gerrit-Comment-Date: Mon, 28 Nov 2022 22:05:54 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Evan Benn <evanbenn(a)google.com> Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: libflashrom: Define macro to detect v1.3 header
by Edward O'Callaghan (Code Review) 28 Nov '22

28 Nov '22

Attention is currently required from: Felix Singer, Angel Pons, Anastasia Klimchuk. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/70027 ) Change subject: libflashrom: Define macro to detect v1.3 header ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/70027 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Iac35ff167272e1bc3d5434f7b47099f53bfceab7 Gerrit-Change-Number: 70027 Gerrit-PatchSet: 1 Gerrit-Owner: 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: Evan Benn <evanbenn(a)google.com> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Mon, 28 Nov 2022 22:04:21 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: bindings: Add FLASHROM_WP_ERR_UNSUPPORTED_STATE
by Felix Singer (Code Review) 28 Nov '22

28 Nov '22

Attention is currently required from: Evan Benn. Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/69864 ) Change subject: bindings: Add FLASHROM_WP_ERR_UNSUPPORTED_STATE ...................................................................... Patch Set 2: (1 comment) Patchset: PS1: > Just tell me ;) CB:70079 😊 […] It works CB:70099 \o/ -- To view, visit https://review.coreboot.org/c/flashrom/+/69864 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I2adaad6552a601147e779c85a2a51d2e39b91c28 Gerrit-Change-Number: 69864 Gerrit-PatchSet: 2 Gerrit-Owner: Evan Benn <evanbenn(a)google.com> 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-CC: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Evan Benn <evanbenn(a)google.com> Gerrit-Comment-Date: Mon, 28 Nov 2022 17:07:59 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Felix Singer <felixsinger(a)posteo.net> Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Comment-In-Reply-To: Evan Benn <evanbenn(a)google.com> Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: tree/: Make probe_opcode() flashctx argument const
by Nikolai Artemiev (Code Review) 28 Nov '22

28 Nov '22

Attention is currently required from: Edward O'Callaghan, Angel Pons, Nikolai Artemiev. Hello build bot (Jenkins), Edward O'Callaghan, Angel Pons, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/70030 to look at the new patch set (#2). Change subject: tree/: Make probe_opcode() flashctx argument const ...................................................................... tree/: Make probe_opcode() flashctx argument const Probing an opcode generally shouldn't involve mutating the flashctx state and currently no probe_opcode functions do that. Make the flashctx arg const so that call sites don't need to have a non-const pointer. BUG=b:253715389,b:253713774 BRANCH=none TEST=ninja test Change-Id: I19e98be50d682de2d2715417f8b7b8c62b871617 Signed-off-by: Nikolai Artemiev <nartemiev(a)google.com> --- M dummyflasher.c M ichspi.c M include/programmer.h M spi.c 4 files changed, 26 insertions(+), 6 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/30/70030/2 -- To view, visit https://review.coreboot.org/c/flashrom/+/70030 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I19e98be50d682de2d2715417f8b7b8c62b871617 Gerrit-Change-Number: 70030 Gerrit-PatchSet: 2 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: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-MessageType: newpatchset

1 0

[S] Change in flashrom[master]: tree/: Make probe_opcode() flashctx argument const
by Nikolai Artemiev (Code Review) 28 Nov '22

28 Nov '22

Attention is currently required from: Edward O'Callaghan, Angel Pons. Nikolai Artemiev has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/70030 ) Change subject: tree/: Make probe_opcode() flashctx argument const ...................................................................... Patch Set 1: This change is ready for review. -- To view, visit https://review.coreboot.org/c/flashrom/+/70030 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I19e98be50d682de2d2715417f8b7b8c62b871617 Gerrit-Change-Number: 70030 Gerrit-PatchSet: 1 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-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Mon, 28 Nov 2022 06:54:21 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: writeprotect,ichspi,spi25: handle register access constraints
by Nikolai Artemiev (Code Review) 28 Nov '22

28 Nov '22

Attention is currently required from: Subrata Banik, Edward O'Callaghan, Angel Pons. Nikolai Artemiev has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/69129 ) Change subject: writeprotect,ichspi,spi25: handle register access constraints ...................................................................... Patch Set 6: (1 comment) This change is ready for review. File spi25_statusreg.c: https://review.coreboot.org/c/flashrom/+/69129/comment/0342556a_3b83e048 PS5, Line 110: (struct flashctx *) > Isn't this casting away `const` from the data pointed by `flash`? Indeed, this was a hack to get it working in the cros fork that I forgot about. I've created a patch to make probe_opcode take a const pointer so we can drop the case here. -- To view, visit https://review.coreboot.org/c/flashrom/+/69129 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I2145749dcc51f4556550650dab5aa1049f879c45 Gerrit-Change-Number: 69129 Gerrit-PatchSet: 6 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.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: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Mon, 28 Nov 2022 06:53:35 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: internal.c: laptop_ok global state can become stale
by Edward O'Callaghan (Code Review) 28 Nov '22

28 Nov '22

Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/70026 ) Change subject: internal.c: laptop_ok global state can become stale ...................................................................... Patch Set 2: (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/70026/comment/ec8b2da8_1d2535c0 PS1, Line 30: BUG=b:260518132,b:260518132 > These are both the same. Done -- To view, visit https://review.coreboot.org/c/flashrom/+/70026 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I2c00c351904307eeb1488c5dfaffc91d6468ee25 Gerrit-Change-Number: 70026 Gerrit-PatchSet: 2 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Sam McNally <sammc(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-CC: Felix Singer <felixsinger(a)posteo.net> Gerrit-Comment-Date: Mon, 28 Nov 2022 06:49:50 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Sam McNally <sammc(a)google.com> Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: internal.c: laptop_ok global state can become stale
by Edward O'Callaghan (Code Review) 28 Nov '22

28 Nov '22

Attention is currently required from: Edward O'Callaghan. Hello Sam McNally, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/70026 to look at the new patch set (#2). Change subject: internal.c: laptop_ok global state can become stale ...................................................................... internal.c: laptop_ok global state can become stale Craask and similar DUT's are erroneously probing random second chips. ``` Found chipset "Intel Alder Lake-N". Enabling flash write... SPI Configuration is locked down. FREG0: Flash Descriptor region (0x00000000-0x00000fff) is read-write. FREG1: BIOS region (0x003a0000-0x00ffffff) is read-write. FREG2: Management Engine region (0x00001000-0x0039ffff) is read-write. OK. Found Winbond flash chip "W25Q128.V..M" (16384 kB, Programmer-specific) on host. Warning: Setting BIOS Control at 0xdc from 0x8b to 0x89 failed. New value is 0x8b. Found MoselVitelic flash chip "V29C51000T" (64 kB, Parallel) mapped at physical address 0x00000000ffff0000. ``` This seems to be due to `laptop_ok` becoming a stale global state after the first operation leading to probing on unrelated buses. Therefore unconditionally reset the global state upon entry into the internal driver. BUG=b:260518132,b:260151917 TEST=Craask reportly no longer finds duplicate chip. Change-Id: I2c00c351904307eeb1488c5dfaffc91d6468ee25 Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M internal.c 1 file changed, 37 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/26/70026/2 -- To view, visit https://review.coreboot.org/c/flashrom/+/70026 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I2c00c351904307eeb1488c5dfaffc91d6468ee25 Gerrit-Change-Number: 70026 Gerrit-PatchSet: 2 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Sam McNally <sammc(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-CC: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

[M] Change in flashrom[master]: ichspi.c: Read chip ID and use it to populate `flash->chip`
by Nikolai Artemiev (Code Review) 28 Nov '22

28 Nov '22

Attention is currently required from: Edward O'Callaghan, Angel Pons. Nikolai Artemiev 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 3: (12 comments) This change is ready for review. Commit Message: https://review.coreboot.org/c/flashrom/+/69195/comment/6a35c849_e0cabe29 PS2, Line 7: ichspi.c: Read chip ID and use it to identify chip > Knowing the chip shouldn't matter for hwseq, as we're not sending direct commands anyway. […] Two flash chips is an interesting case, I think it will always read the ID of the first chip but I don't have any hardware to test with. https://review.coreboot.org/c/flashrom/+/69195/comment/d353879c_5a095212 PS2, Line 9: BUG=b:253715389,b:253713774 > Ah, so downstream has some extra functionality, probably to control WP on platforms where only hwseq […] Done https://review.coreboot.org/c/flashrom/+/69195/comment/e1f58459_2e107666 PS2, Line 11: TEST=builds > I think you can include this was actually tested here across the DUT pool? Done Commit Message: https://review.coreboot.org/c/flashrom/+/69195/comment/471a1d1c_65f04e34 PS3, Line 21: FIX: w,E Something about this change causes erase to fail: ``` Found Winbond flash chip "W25Q128.V..M" (16384 kB, Programmer-specific) on internal. Erasing and writing flash chip... Transaction error between offset 0x00000000 and 0x00000000 (= 0x00000000 + 0)! Looking for another erase function. Looking for another erase function. Looking for another erase function. Looking for another erase function. Looking for another erase function. Looking for another erase function. Looking for another erase function. No usable erase functions left. FAILED! ``` Looks like it's trying to do SPI erase instead of hwseq now. We don't have this issue in cros flashrom so there's probably some additional logic that makes it work, maybe action_descriptor since it mutates erasers a bit. I'll keep debugging. File ichspi.c: https://review.coreboot.org/c/flashrom/+/69195/comment/b9ad68a8_da0a28ee PS2, Line 1472: entry == NULL > `!entry` Done https://review.coreboot.org/c/flashrom/+/69195/comment/bb65953b_71300045 PS2, Line 1475: return 0; > `return -1;` Done https://review.coreboot.org/c/flashrom/+/69195/comment/6efbaad6_c65cce11 PS2, Line 1476: } else { > no need for a `else` as you have a early return in the `if` branch. Done https://review.coreboot.org/c/flashrom/+/69195/comment/ed6ebbd5_6161eede PS2, Line 1486: flash->chip->tested = entry->tested; > drop line, it is overwritten in the eulogy of `ich_hwseq_probe()`, the call site of this function. Wouldn't it be better to keep this and delete the line that overwrites it? That's what I've done in the latest patchset but let me know if I'm missing something. https://review.coreboot.org/c/flashrom/+/69195/comment/e36e1217_fa5f9d57 PS2, Line 1492: return 1; > `return 0;` Done https://review.coreboot.org/c/flashrom/+/69195/comment/36f44c0c_0c4b8fcc PS2, Line 1502: != 1 > `< 0` Done https://review.coreboot.org/c/flashrom/+/69195/comment/09d8b060_d1e3cda4 PS2, Line 1503: msg_perr("Unable to read flash chip ID\n"); > drop as you already have a `msg_perr()` call in the func. Done https://review.coreboot.org/c/flashrom/+/69195/comment/4df269a8_f21d7b0e PS2, Line 1552: TEST_OK_PREW; > https://review.coreboot.org/c/flashrom/+/69518 […] Done -- 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: 3 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Mon, 28 Nov 2022 06:24:06 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> 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 November 2022