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
Attention is currently required from: Edward O'Callaghan, Anastasia Klimchuk.
Daniel Campello has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62312 )
Change subject: tests/: wrap flock() and ftruncate()
......................................................................
Patch Set 11: Code-Review+1
(1 comment)
Commit Message:
https://review.coreboot.org/c/flashrom/+/62312/comment/71042151_ca94a9bc
PS6, Line 7: wrap flock() and ftruncate()
> I think this should be its own commit and merged regardless. […]
I am not sure about this, why stop on only I/O ops and not write mocks for every syscall?
--
To view, visit https://review.coreboot.org/c/flashrom/+/62312
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I57e7843a4d94c0a7200690d4c1d9e4194b9f83c5
Gerrit-Change-Number: 62312
Gerrit-PatchSet: 11
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Daniel Campello <campello(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: Sam McNally <sammc(a)google.com>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Sat, 02 Apr 2022 13:55:35 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org>
Comment-In-Reply-To: Daniel Campello <campello(a)chromium.org>
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Anastasia Klimchuk.
Daniel Campello has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/63193 )
Change subject: tests: use MOCK_FD instead of NON_ZERO
......................................................................
Patch Set 3:
(1 comment)
Commit Message:
https://review.coreboot.org/c/flashrom/+/63193/comment/7b884d31_ebb7c75d
PS3, Line 9: return a non-negative value for
: the file descriptor expected from open operations.
> Actually, was this intentional? […]
This could not be intentional. It would mean that open() would always fail unless a mock is defined for the test. At least we know that the linux_spi is using the default open wrapper with /dev/null as path (if the code checked for (fd < 0) this would have caused the test to fail)
--
To view, visit https://review.coreboot.org/c/flashrom/+/63193
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ib6bac051852aea2465665a6fd669b7f5e3772985
Gerrit-Change-Number: 63193
Gerrit-PatchSet: 3
Gerrit-Owner: Daniel Campello <campello(a)chromium.org>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Sat, 02 Apr 2022 13:53:22 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Sam McNally.
Daniel Campello has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/63227 )
Change subject: tests: assert pathname and flags when calling open()
......................................................................
Patch Set 6:
(2 comments)
File tests/tests.c:
https://review.coreboot.org/c/flashrom/+/63227/comment/a404b6ba_abd3579a
PS6, Line 77: wrap_open_and_friends
> this may have unexpected behavior when `(get_io()->open != NULL)&&(get_io()->open_state != NULL)`.
open_state in not used when open != NULL (this function returns from within this if case)
https://review.coreboot.org/c/flashrom/+/63227/comment/19990c0d_b3d6b86e
PS6, Line 81: )
> `&& !get_io()->open)`
see above
--
To view, visit https://review.coreboot.org/c/flashrom/+/63227
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ib46ca5b854c8453ec02ae09f3151cd4d25f988eb
Gerrit-Change-Number: 63227
Gerrit-PatchSet: 6
Gerrit-Owner: Daniel Campello <campello(a)chromium.org>
Gerrit-Reviewer: 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: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Sam McNally <sammc(a)google.com>
Gerrit-Comment-Date: Sat, 02 Apr 2022 13:51:01 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
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.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62282 )
Change subject: ichspi: Add Jasper Lake support
......................................................................
Patch Set 9:
(2 comments)
Patchset:
PS4:
> Unresolving then because it does look like a pch300 although I am not 100% on every tiny idiosyncrasy.
>
> If `ich_number_of_masters()` needs adjusting then so does the switch in `prettyprint_ich_descriptor_master()` too.
NM has always been vaguely defined. The original assumption was that it's
0 based. This didn't work for some niche platforms, IIRC starting with
Lewisburg. AIUI, the value was never interpreted by the hardware and only
for FIT's convenience. It seems possible that not even FIT cares about it
anymore and we are chasing ghosts (i.e. we might have adapted the code to
make it work while we actually read a garbage value).
One possible way out of the dilemma would be to let ich_number_of_masters()
return static values per platform (or just make it a constant array). Simply
use the maximum a descriptor generation supports. From the SPI guides it
looks like the arrays like FLMSTR are not dynamic, i.e. always have the
same length for a given generation. We should have a look at some descrip-
tors to confirm that.
Probably the same for NR on older platforms.
>
> Thing is, that has:
> ```
> const char *const master_names[] = {
> "BIOS", "ME", "GbE", "unknown", "EC",
> };
> ```
> but the datasheet says on page 42 - [BIOS, ME, GbE, EC] and states there are 4 masters. Nothing indicates if to interpret "number of masters" from the flash descriptor as so called- 0-based or 1-based. Therefore I just assumed `cont->NM`.
If in doubt, it's often good to assume that nothing changed. What makes
things harder is that we can't derive how optional things are considered
like that "EC" master. I don't think I've ever worked with a board that
makes use of it.
>
> Open to some guidance here? My concern is that I got those two functions wrong.
Sometimes it's easiest to do what the existing code for the closest
relative does. If it needs to be fixed later, it at least avoids
arguments like "why did JSL differ, let's better not touch the code".
File ich_descriptors.c:
https://review.coreboot.org/c/flashrom/+/62282/comment/428fe818_20fc729b
PS8, Line 1061: CHIPSET_ELKHART_LAKE
> Thanks Nico that is super useful ! I didn't have access to EHL I was asking if Subrata could ask a c […]
Google should have access too. Since ~SKL, both the SPI guide and FIT are
part of the CSE/(CS)ME/TXE firmware packages. It's not always easy to find
on intel.com, took me hours to find one for JSL. For newer platforms the
following search strings might give results "... Lake CSE" "... Lake CSME"
"... Lake BKC". It can also help to know the version for a given platform,
for this it's best to query the experts:
https://www.win-raid.com/t596f39-Intel-Converged-Security-Management-Engine…
(still not easy to keep track of codenames, who would have thought that
the EHL PCH is Mule Creek Canyon?)
--
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: 9
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-Comment-Date: Sat, 02 Apr 2022 13:45:20 +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: Thomas Heijligen, Arthur Heymans.
Paul Menzel 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: 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: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Comment-Date: Sat, 02 Apr 2022 08:23:09 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Edward O'Callaghan, Daniel Campello, Anastasia Klimchuk.
Sam McNally has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62312 )
Change subject: tests/: wrap flock() and ftruncate()
......................................................................
Patch Set 11:
(1 comment)
File tests/tests.c:
https://review.coreboot.org/c/flashrom/+/62312/comment/12329192_bdb5e71f
PS11, Line 285: int __wrap_flock(int fd, int operation)
These should go after __wrap_ferror().
--
To view, visit https://review.coreboot.org/c/flashrom/+/62312
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I57e7843a4d94c0a7200690d4c1d9e4194b9f83c5
Gerrit-Change-Number: 62312
Gerrit-PatchSet: 11
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Daniel Campello <campello(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: Sam McNally <sammc(a)google.com>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Daniel Campello <campello(a)chromium.org>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Sat, 02 Apr 2022 01:59:15 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Daniel Campello, Anastasia Klimchuk.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62312 )
Change subject: tests/: wrap flock() and ftruncate()
......................................................................
Patch Set 10:
(1 comment)
Commit Message:
https://review.coreboot.org/c/flashrom/+/62312/comment/1ee368bb_a3d687e9
PS6, Line 7: wrap flock() and ftruncate()
> Done
I think this should be its own commit and merged regardless. All the I/O ops should be stubbed out so real variants are not accidentally called with fake fd values. If that is how the test harness works then it shouldn't have singularities that make your leg explode while writing a unit-test, sounds like a lot of foot-gun if doing on ad hoc.
--
To view, visit https://review.coreboot.org/c/flashrom/+/62312
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I57e7843a4d94c0a7200690d4c1d9e4194b9f83c5
Gerrit-Change-Number: 62312
Gerrit-PatchSet: 10
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Daniel Campello <campello(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Daniel Campello <campello(a)chromium.org>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Sat, 02 Apr 2022 01:50:43 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Daniel Campello <campello(a)chromium.org>
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Edward O'Callaghan, Anastasia Klimchuk.
Hello build bot (Jenkins), Daniel Campello, Anastasia Klimchuk,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/62312
to look at the new patch set (#10).
Change subject: tests/: wrap flock() and ftruncate()
......................................................................
tests/: wrap flock() and ftruncate()
Avoid calling real I/O functions accidentally with fake fd values.
BUG=b:217629892,b:215255210
TEST=`ninja test`.
Change-Id: I57e7843a4d94c0a7200690d4c1d9e4194b9f83c5
Signed-off-by: Edward O'Callaghan <quasisec(a)google.com>
---
M tests/meson.build
M tests/tests.c
2 files changed, 14 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/12/62312/10
--
To view, visit https://review.coreboot.org/c/flashrom/+/62312
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I57e7843a4d94c0a7200690d4c1d9e4194b9f83c5
Gerrit-Change-Number: 62312
Gerrit-PatchSet: 10
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Daniel Campello <campello(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: newpatchset