Attention is currently required from: Nico Huber, Thomas Heijligen, Arthur Heymans.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/63258 )
Change subject: ch341a_spi.c: Reset device before use
......................................................................
Patch Set 1: Code-Review+1
--
To view, visit https://review.coreboot.org/c/flashrom/+/63258
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Iff824c22dc871bb2d9504abe8b5ad4e4c191c4af
Gerrit-Change-Number: 63258
Gerrit-PatchSet: 1
Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Comment-Date: Sun, 03 Apr 2022 21:14:26 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Edward O'Callaghan.
Thomas Heijligen has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/63320 )
Change subject: .gitignore: Avoid committing libflashrom.a
......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1:
duplicate of CB:62826
This patch adds all *.a to gitignore, not only libflashrom.a which is mentioned in the commit message
--
To view, visit https://review.coreboot.org/c/flashrom/+/63320
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ie9ca7c6ab846d0b07c16798cdd3ff5aa0a748a9b
Gerrit-Change-Number: 63320
Gerrit-PatchSet: 1
Gerrit-Owner: 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-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Comment-Date: Sun, 03 Apr 2022 12:43:57 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Edward O'Callaghan, Light, Anastasia Klimchuk.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/63130 )
Change subject: pony_spi.c: Extract out get_params to simplify init
......................................................................
Patch Set 3:
(7 comments)
File pony_spi.c:
https://review.coreboot.org/c/flashrom/+/63130/comment/8833d157_f0edb292
PS3, Line 153: {
nit: add space before opening brace
https://review.coreboot.org/c/flashrom/+/63130/comment/10aeaede_615bf794
PS3, Line 167: bool have_device;
> From my point of view: Review doesn't scale well with the size of
> a patch. Even if it's a very simple patch hunk, every additional
> hunk can distract from the error-prone part of a patch. Doing fixes
> and simple changes like this in advance works well, FWIW.
+1
For me, it's very easy to miss something when reviewing commits that do multiple things at once. I'd rather review multiple commits where each does one and only one thing.
Whenever I want/need to make significant code changes, I draft in advance a "plan of action". I use several criteria to segment the changes into commits, one of them is "at most one thing/reason/idea per commit".
In this case, "simplify `pony_spi` init logic" and "use `bool` type for boolean variables" are two different things, and they are conceptually independent: disregarding merge conflicts, it's possible to do them in either order, or only do one of them. When accounting for merge conflicts, I start with the easiest (most likely to be approved) commits, so that I don't have to manually rebase them if another commit needs to be fixed.
That being said, there's no need to over-split things. For example, I'd retype both `have_device` and `have_prog` in the same commit, because the rationale is the same.
TL;DR: Please retype the variables in a separate commit (preferably before the "extract `get_params`" commit to streamline the submission process).
https://review.coreboot.org/c/flashrom/+/63130/comment/523207c5_d0f3c3b6
PS3, Line 171: return 1;
If the `dev` parameter is correct (i.e. `have_device` is true) but `get_params()` returns -1 because of an error with the `type` parameter, the serial port (what `serialport_shutdown()` releases) will leak.
https://review.coreboot.org/c/flashrom/+/63130/comment/3b064e8c_49fe92dc
PS3, Line 181: return 1;
Serial port can also leak here.
https://review.coreboot.org/c/flashrom/+/63130/comment/229d3eda_23804449
PS3, Line 190: serialport_shutdown
> I think you are mis-interpreting the diff presented by gerrit Anastasia.
This line is OK here, but it's missing in some other return paths.
https://review.coreboot.org/c/flashrom/+/63130/comment/54be3b14_cde22584
PS3, Line 232: true
This is unrelated. Please do it in a separate commit.
https://review.coreboot.org/c/flashrom/+/63130/comment/9797c619_2ea34563
PS3, Line 237: true
Ditto
--
To view, visit https://review.coreboot.org/c/flashrom/+/63130
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I364febc05c870683cbad114583762b0c006f4bac
Gerrit-Change-Number: 63130
Gerrit-PatchSet: 3
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Light <aarya.chaumal(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-CC: Felix Singer <felixsinger(a)posteo.net>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Light <aarya.chaumal(a)gmail.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Sun, 03 Apr 2022 11:26:14 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org>
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Edward O'Callaghan, Light, Anastasia Klimchuk.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/63130 )
Change subject: pony_spi.c: Extract out get_params to simplify init
......................................................................
Patch Set 3:
(1 comment)
Commit Message:
https://review.coreboot.org/c/flashrom/+/63130/comment/77310740_a9f5f7d3
PS3, Line 9: `commit caa0335114a81`
Nit: No markup needed. Maybe even add the commit message summary in () after it.
--
To view, visit https://review.coreboot.org/c/flashrom/+/63130
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I364febc05c870683cbad114583762b0c006f4bac
Gerrit-Change-Number: 63130
Gerrit-PatchSet: 3
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Light <aarya.chaumal(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Felix Singer <felixsinger(a)posteo.net>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Light <aarya.chaumal(a)gmail.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Sun, 03 Apr 2022 05:43:57 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Subrata Banik, Angel Pons.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62282 )
Change subject: ichspi: Add Jasper Lake support
......................................................................
Patch Set 10:
(2 comments)
Patchset:
PS4:
> > Unresolving then because it does look like a pch300 although I am not 100% on every tiny idiosyncr […]
You are right `NM` is confusing, can we comment it better? Possibly copy what you just said there near verbatim into the `ich_number_of_masters()` function?
OK, I picked to follow pch300 in `prettyprint_ich_descriptor_master()` and fixed that up. `ich_number_of_masters()` was left untouched.
I wonder how much of this declarative information about the differences could be encoded into just a big table instead of scattered over heaps of switch statements in the control flow itself? A lot of it doesn't seem dynamic just XXX has 3 and YYY has 4 of "thing".
Resolved?
File ich_descriptors.c:
https://review.coreboot.org/c/flashrom/+/62282/comment/62044ba6_c49c4d9d
PS8, Line 1061: CHIPSET_ELKHART_LAKE
> Google should have access too. Since ~SKL, both the SPI guide and FIT are […]
Long story about the access ACL's there.. Can talk out of band about it || summarised as 'a lot of nonsense'.
--
To view, visit https://review.coreboot.org/c/flashrom/+/62282
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ib942d0b8942fe0a991b2af0b187414818485153d
Gerrit-Change-Number: 62282
Gerrit-PatchSet: 10
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Sam McNally <sammc(a)google.com>
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-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Subrata Banik <subratabanik(a)google.com>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Comment-Date: Sun, 03 Apr 2022 00:30:15 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com>
Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Subrata Banik, Edward O'Callaghan, Angel Pons.
Hello Sam McNally, build bot (Jenkins), Subrata Banik, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/62282
to look at the new patch set (#10).
Change subject: ichspi: Add Jasper Lake support
......................................................................
ichspi: Add Jasper Lake support
Additionally, utilize CSSO (CPU Soft Strap Offset) to uniquely detect
the chipset when the CSSL (CPU Soft Strap Length) field default value
(0x03) on Jasper Lake is the same as Elkhart Lake.
BUG=b:221175960
TEST=dedede with `flashrom -p internal --flash-size`.
```
$ flashrom -VVV -p internal --ifd -i fd -i bios -r /tmp/filename.rom
<snip>
Enabling hardware sequencing by default for 100+ series PCH.
OK.
No board enable found matching coreboot IDs vendor="Google", model="Magolor".
The following protocols are supported: Programmer-specific.
Probing for Programmer Opaque flash chip, 0 kB: Chip identified: GD25Q127C/GD25Q128C
Hardware sequencing reports 1 attached SPI flash chip with a density of 16384 kB.
There is only one partition containing the whole address space (0x000000 - 0xffffff).
There are 4096 erase blocks with 4096 B each.
Added layout entry 00000000 - 00ffffff named complete flash
Found GigaDevice flash chip "GD25Q127C/GD25Q128C" (16384 kB, Programmer-specific) mapped at physical address 0x0000000000000000.
Found GigaDevice flash chip "GD25Q127C/GD25Q128C" (16384 kB, Programmer-specific).
This chip may contain one-time programmable memory. flashrom cannot read
and may never be able to write it, hence it may not be able to completely
clone the contents of this chip (see man page for details).
Reading Status register
Block protection is disabled.
Reading ich descriptor... Reading 4096 bytes starting at 0x000000.
done.
Assuming chipset 'Jasper Lake'.
Added layout entry 00000000 - 00000fff named fd
Added layout entry 00381000 - 00ffffff named bios
Added layout entry 00001000 - 00380fff named me
restore_power_management: Re-enabling power management.
Using regions: "bios", "fd".
Reading Status register
Block protection is disabled.
Reading flash... 0x381000-0xffffff:R Reading 13103104 bytes starting at 0x381000.
000000-0x0fff:R Reading 4096 bytes starting at 0x000000.
done.
restore_power_management: Re-enabling power management.
SUCCESS
Restoring PCI config space for 00:1f:5 reg 0xdc
restore_power_management: Re-enabling power management.
```
Change-Id: Ib942d0b8942fe0a991b2af0b187414818485153d
Signed-off-by: Edward O'Callaghan <quasisec(a)google.com>
---
M chipset_enable.c
M ich_descriptors.c
M ichspi.c
M programmer.h
M util/ich_descriptors_tool/ich_descriptors_tool.c
5 files changed, 36 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/82/62282/10
--
To view, visit https://review.coreboot.org/c/flashrom/+/62282
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ib942d0b8942fe0a991b2af0b187414818485153d
Gerrit-Change-Number: 62282
Gerrit-PatchSet: 10
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Sam McNally <sammc(a)google.com>
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-CC: Paul Menzel <paulepanter(a)mailbox.org>
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-MessageType: newpatchset
Attention is currently required from: Edward O'Callaghan, Light, Anastasia Klimchuk.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/63130 )
Change subject: pony_spi.c: Extract out get_params to simplify init
......................................................................
Patch Set 3:
(1 comment)
File pony_spi.c:
https://review.coreboot.org/c/flashrom/+/63130/comment/ac5dcf12_bccea0c8
PS3, Line 167: bool have_device;
> A bit of nit isn't it? Is there a solid technical reason here or just personal preference?
From my point of view: Review doesn't scale well with the size of
a patch. Even if it's a very simple patch hunk, every additional
hunk can distract from the error-prone part of a patch. Doing fixes
and simple changes like this in advance works well, FWIW.
--
To view, visit https://review.coreboot.org/c/flashrom/+/63130
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I364febc05c870683cbad114583762b0c006f4bac
Gerrit-Change-Number: 63130
Gerrit-PatchSet: 3
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Light <aarya.chaumal(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Felix Singer <felixsinger(a)posteo.net>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Light <aarya.chaumal(a)gmail.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Sat, 02 Apr 2022 14:19:45 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org>
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Hung-Te Lin, Nico Huber, Subrata Banik, Edward O'Callaghan.
Daniel Campello has uploaded a new patch set (#17) to the change originally created by Edward O'Callaghan. ( https://review.coreboot.org/c/flashrom/+/62213 )
Change subject: flashrom.c: Implement file-based locking semantics
......................................................................
flashrom.c: Implement file-based locking semantics
This upstreams the ChromiumOS implementation of file-based locking
for multiple instances of flashrom that could be spawned either
from libflashrom (perhaps by fwupd for example) and user cli
as another example. Since flashrom is programming singleton state
of hardware from userspace there is no way to exclusively own
the address space and therefore a file-based locking semantic
is considered here.
BUG=b:217629892,b:215255210
BRANCH=none
TEST=nm -gD /build/brya/usr/lib64/libflashrom.so | grep "flock"
Test futility update with multiple instances of flashrom running.
Change-Id: I19cb4e3bf14caeb67c3e8100a20395b264c5113a
Signed-off-by: Edward O'Callaghan <quasisec(a)google.com>
---
M Makefile
A big_lock.c
A big_lock.h
A file_lock.c
M flashrom.c
A ipc_lock.h
M meson.build
M tests/chip.c
M tests/include/test.h
M tests/lifecycle.c
M tests/meson.build
M tests/tests.c
12 files changed, 492 insertions(+), 17 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/13/62213/17
--
To view, visit https://review.coreboot.org/c/flashrom/+/62213
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I19cb4e3bf14caeb67c3e8100a20395b264c5113a
Gerrit-Change-Number: 62213
Gerrit-PatchSet: 17
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Hung-Te Lin <hungte(a)chromium.org>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Hung-Te Lin <hungte(a)chromium.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Subrata Banik <subratabanik(a)google.com>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-MessageType: newpatchset