Attention is currently required from: Subrata Banik, Angel Pons, Nikolai Artemiev.
Hello build bot (Jenkins), Subrata Banik, Edward O'Callaghan, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/69195
to look at the new patch set (#11).
Change subject: ichspi.c: Read chip ID and use it to populate `flash->chip`
......................................................................
ichspi.c: Read chip ID and use it to populate `flash->chip`
Read the flash chip vendor/device ID using hardware sequencing, find the
corresponding flashchip entry, and copy it over to `flash->chip`.
Identifying the chip was not previously required as ICH hardware
sequencing handles chip-level details related to read/write/erase ops.
However writeprotect operations require the chip entry to be identified
so that chip->reg_bits can be used to compute status register values.
BUG=b:253715389,b:253713774
BRANCH=none
TEST=flashrom on dedede (JSL) identifies "W25Q128.V..M" chip
TEST=flashrom -{r,v} on dedede
TEST=write/erase bios region on dedede:
flashrom -{E,w} --layout <(echo '0x381000:0xffffff bios') -i bios
Change-Id: Ia408e1e45dc6f53c0934afd6558e301abfa48ee6
Signed-off-by: Nikolai Artemiev <nartemiev(a)google.com>
---
M ichspi.c
1 file changed, 95 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/95/69195/11
--
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: 11
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-Attention: Subrata Banik <subratabanik(a)google.com>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-MessageType: newpatchset
Attention is currently required from: Edward O'Callaghan, Nikolai Artemiev.
Hello build bot (Jenkins), Nico Huber, Edward O'Callaghan, Anastasia Klimchuk,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/70342
to look at the new patch set (#2).
Change subject: flashchips.c: remove WREN from GD25Q256D enter 4BA sequence
......................................................................
flashchips.c: remove WREN from GD25Q256D enter 4BA sequence
As noted in a comment on
`commit 86fc9cf7ab221bc54ef6f10252e296fc2d7a22d2`, the GD25Q256D
datasheet indicates that the chip does not require a WREN command to
enter 4BA mode.
Testing has confirmed that a WREN command is not required, so change the
flashchip feature flags from FEATURE_4BA_WREN to FEATURE_4BA.
Ticket: https://ticket.coreboot.org/issues/356
BUG=none
BRANCH=none
TEST=read/write/erase/verify GD25Q256D flash with FT2232H programmer
Change-Id: I96e48933f33c52c0d10a0d4cb7f7e07c1fceab99
Signed-off-by: Nikolai Artemiev <nartemiev(a)google.com>
---
M flashchips.c
1 file changed, 25 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/42/70342/2
--
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: 2
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-MessageType: newpatchset
Attention is currently required from: Edward O'Callaghan.
Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/68153 )
Change subject: libflashrom.c: Invert if conditions to improve the readability
......................................................................
Patch Set 6:
(1 comment)
File libflashrom.c:
https://review.coreboot.org/c/flashrom/+/68153/comment/17e2a7af_3968d9ab
PS3, Line 107: (enum flashrom_test_state)
> There are duplicate definitions for this. `test_state` in flash. […]
So yes, the cast is needed for now. I'm setting this to resolved since it's unrelated.
--
To view, visit https://review.coreboot.org/c/flashrom/+/68153
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I4021d8802cd041dcca29a226af0798ebd9c5a81b
Gerrit-Change-Number: 68153
Gerrit-PatchSet: 6
Gerrit-Owner: Felix Singer <felixsinger(a)posteo.net>
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-Comment-Date: Tue, 06 Dec 2022 23:03:34 +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>
Gerrit-MessageType: comment
Attention is currently required from: Edward O'Callaghan.
Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/68153 )
Change subject: libflashrom.c: Invert if conditions to improve the readability
......................................................................
Patch Set 6:
(2 comments)
File libflashrom.c:
https://review.coreboot.org/c/flashrom/+/68153/comment/c8f0df98_93257f04
PS3, Line 103: (;
> optional: while we are here could we just scope the indexer into the loop construct. […]
Done
https://review.coreboot.org/c/flashrom/+/68153/comment/aee63750_06fa30dc
PS3, Line 107: (enum flashrom_test_state)
> is this cast needed?
There are duplicate definitions for this. `test_state` in flash.h and `flashrom_test_state` in libflashrom.h. The latter is only used for the functions I'm cleaning up here.
--
To view, visit https://review.coreboot.org/c/flashrom/+/68153
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I4021d8802cd041dcca29a226af0798ebd9c5a81b
Gerrit-Change-Number: 68153
Gerrit-PatchSet: 6
Gerrit-Owner: Felix Singer <felixsinger(a)posteo.net>
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-Comment-Date: Tue, 06 Dec 2022 22:57: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: Felix Singer.
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/+/68153
to look at the new patch set (#6).
Change subject: libflashrom.c: Invert if conditions to improve the readability
......................................................................
libflashrom.c: Invert if conditions to improve the readability
Invert some if conditions to improve the readability of the code.
Instead of running some code if the specific condition applies, error
out early and reduce the indentation levels.
Also, while at it, move the initializers for these for-loops iterators
into their constructs. They are only used by them.
Change-Id: I4021d8802cd041dcca29a226af0798ebd9c5a81b
Signed-off-by: Felix Singer <felixsinger(a)posteo.net>
---
M libflashrom.c
1 file changed, 53 insertions(+), 36 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/53/68153/6
--
To view, visit https://review.coreboot.org/c/flashrom/+/68153
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I4021d8802cd041dcca29a226af0798ebd9c5a81b
Gerrit-Change-Number: 68153
Gerrit-PatchSet: 6
Gerrit-Owner: Felix Singer <felixsinger(a)posteo.net>
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: Felix Singer <felixsinger(a)posteo.net>
Gerrit-MessageType: newpatchset
Attention is currently required from: Felix Singer.
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/+/68153
to look at the new patch set (#5).
Change subject: libflashrom.c: Invert if conditions to improve the readability
......................................................................
libflashrom.c: Invert if conditions to improve the readability
Invert some if conditions to improve the readability of the code.
Instead of running some code if the specific condition applies, error
out early and reduce the indentation levels.
Also, while at it, move the initializers for these for-loops iterators
into their constructs. They are only used by them.
Change-Id: I4021d8802cd041dcca29a226af0798ebd9c5a81b
Signed-off-by: Felix Singer <felixsinger(a)posteo.net>
---
M libflashrom.c
1 file changed, 53 insertions(+), 36 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/53/68153/5
--
To view, visit https://review.coreboot.org/c/flashrom/+/68153
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I4021d8802cd041dcca29a226af0798ebd9c5a81b
Gerrit-Change-Number: 68153
Gerrit-PatchSet: 5
Gerrit-Owner: Felix Singer <felixsinger(a)posteo.net>
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: Felix Singer <felixsinger(a)posteo.net>
Gerrit-MessageType: newpatchset
Attention is currently required from: Felix Singer.
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/+/68153
to look at the new patch set (#4).
Change subject: libflashrom.c: Invert if conditions to improve the readability
......................................................................
libflashrom.c: Invert if conditions to improve the readability
Invert some if conditions to improve the readability of the code.
Instead of running some code if the specific condition applies, error
out early and reduce the indentation levels.
Also, while at it, move the initializers for these for-loops iterators
into their constructs. They are only used by them.
Change-Id: I4021d8802cd041dcca29a226af0798ebd9c5a81b
Signed-off-by: Felix Singer <felixsinger(a)posteo.net>
---
M libflashrom.c
1 file changed, 53 insertions(+), 36 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/53/68153/4
--
To view, visit https://review.coreboot.org/c/flashrom/+/68153
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I4021d8802cd041dcca29a226af0798ebd9c5a81b
Gerrit-Change-Number: 68153
Gerrit-PatchSet: 4
Gerrit-Owner: Felix Singer <felixsinger(a)posteo.net>
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: Felix Singer <felixsinger(a)posteo.net>
Gerrit-MessageType: newpatchset
Attention is currently required from: Felix Singer, Thomas Heijligen.
Alexander Goncharov has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/68247 )
Change subject: util: add bash completion script
......................................................................
Patch Set 10:
(7 comments)
Patchset:
PS10:
As part of rebasing, I replaced --image (this option is deprecated) with --include
File Makefile:
https://review.coreboot.org/c/flashrom/+/68247/comment/ce44ac9b_5eb3e208
PS9, Line 589: ACTIVE_PROGRAMMERS += internal
> It took a little until I understood this programmer list is needed for the substitution. […]
Yes, it should be mentioned, thank you. Done.
https://review.coreboot.org/c/flashrom/+/68247/comment/c604bafa_2b814ac7
PS9, Line 1032: $(PROGRAM).8 $(PROGRAM).8.html $(PROGRAM).bash $(BUILD_DETAILS_FILE)
> One tab too much
What do you mean? I just added `$(PROGRAM).bash`
https://review.coreboot.org/c/flashrom/+/68247/comment/2ed7f8fe_81105ed9
PS9, Line 1041: .bash
> That's just the file in the source directory. […]
From [Bash Completion FAQ](https://github.com/scop/bash-completion/blob/master/README.md#faq):
```
The completion filename for command `foo` in this directory should be either `foo`, or `foo.bash`.
```
So, we only have two naming options available. I cannot use a name without the suffix due to a name conflict (As Felix mentioned above, the flashrom binary has the same name). Unfortunately, a custom prefix isn't supported, so we have no choice.
Felix, your idea is what I wanted to do initially. But there's a problem with meson, please have a look at another Thomas comment #646.
File meson.build:
https://review.coreboot.org/c/flashrom/+/68247/comment/e4d7faf8_5c6d71e0
PS9, Line 624: if get_option('classic_cli').disabled()
> When `get_option('bash_completion').auto()` and `get_option('classic_cli'). […]
Thanks for catching it!
https://review.coreboot.org/c/flashrom/+/68247/comment/697a8222_540120ac
PS9, Line 646: .bash
> same here
No way. If the output file is `flashrom`, the bash-completion file will be overwritten by the executable.
`install` command (in the makefile) lets me choose DESTINATION, and meson only copies the file in one place - `install_dir`/`output`.
File util/flashrom.bash-completion.tmpl:
https://review.coreboot.org/c/flashrom/+/68247/comment/37b51484_679828be
PS9, Line 43: OPTS="--help
> Missing \ at the end of each line.
It makes no difference for this task. Also, those `\` at the end of lines look ugly :(
--
To view, visit https://review.coreboot.org/c/flashrom/+/68247
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ie68bc91c3cea4de2ffdbeffd07e48edd8d5590e1
Gerrit-Change-Number: 68247
Gerrit-PatchSet: 10
Gerrit-Owner: Alexander Goncharov <chat(a)joursoir.net>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Comment-Date: Tue, 06 Dec 2022 21:30:11 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Felix Singer <felixsinger(a)posteo.net>
Comment-In-Reply-To: Thomas Heijligen <src(a)posteo.de>
Gerrit-MessageType: comment
Felix Singer has submitted this change. ( https://review.coreboot.org/c/flashrom/+/70243 )
Change subject: util/lint: Introduce linter for git sign-off-by line
......................................................................
util/lint: Introduce linter for git sign-off-by line
For compliance and to check if the commit message matches the
development guidelines, add a linter checking for the sign-off-by line
in the commit message. Also, hook it up to the test_build.sh script
which is used for CI builds.
These scripts were copied from the coreboot repository and were adjusted
so that they work for flashrom.
Original-Signed-off-by: Felix Singer <felixsinger(a)posteo.net>
Original-Reviewed-on: https://review.coreboot.org/c/flashrom/+/70079
Original-Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org>
Original-Reviewed-by: Edward O'Callaghan <quasisec(a)chromium.org>
Change-Id: I03f1827803f8492d60a0a44174d5822c2265bfcb
Signed-off-by: Felix Singer <felixsinger(a)posteo.net>
Reviewed-on: https://review.coreboot.org/c/flashrom/+/70243
Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org>
Reviewed-by: Edward O'Callaghan <quasisec(a)chromium.org>
---
M test_build.sh
A util/lint/helper_functions.sh
A util/lint/lint-extended-020-signed-off-by
3 files changed, 101 insertions(+), 0 deletions(-)
Approvals:
build bot (Jenkins): Verified
Edward O'Callaghan: Looks good to me, approved
diff --git a/test_build.sh b/test_build.sh
index 69d3a34..813d0b9 100755
--- a/test_build.sh
+++ b/test_build.sh
@@ -26,6 +26,11 @@
fi
+run_linter() {
+ ./util/lint/lint-extended-020-signed-off-by
+}
+
+
build_make () {
make clean
make -j $(nproc) CONFIG_EVERYTHING=yes
@@ -67,5 +72,7 @@
}
+run_linter
+
build_make
build_meson
diff --git a/util/lint/helper_functions.sh b/util/lint/helper_functions.sh
new file mode 100644
index 0000000..7abdab8
--- /dev/null
+++ b/util/lint/helper_functions.sh
@@ -0,0 +1,45 @@
+#!/usr/bin/env sh
+#
+# SPDX-License-Identifier: GPL-2.0-only
+
+# This file is sourced by the linters so that each one doesn't have to
+# specify these routines individually
+
+LC_ALL=C export LC_ALL
+
+if [ -z "$GIT" ]; then
+ GIT="$(command -v git)"
+else
+ # If git is specified, Do a basic check that it runs and seems like
+ # it's actually git
+ if ! "${GIT}" --version | grep -q git; then
+ echo "Error: ${GIT} does not seem to be valid."
+ exit 1;
+ fi
+fi
+
+if [ "$(${GIT} rev-parse --is-inside-work-tree 2>/dev/null)" = "true" ]; then
+ IN_GIT_TREE=1
+else
+ IN_GIT_TREE=0
+fi
+
+if [ "${IN_GIT_TREE}" -eq 1 ] && [ -z "${GIT}" ]; then
+ echo "This test needs git to run. Please install it, then run this test again."
+ exit 1
+fi
+
+# Use git ls-files if the code is in a git repo, otherwise use find.
+if [ "${IN_GIT_TREE}" -eq 1 ]; then
+ FIND_FILES="${GIT} ls-files"
+else
+ FIND_FILES="find "
+ FINDOPTS="-type f"
+fi
+
+# Use git grep if the code is in a git repo, otherwise use grep.
+if [ "${IN_GIT_TREE}" -eq 1 ]; then
+ GREP_FILES="${GIT} grep"
+else
+ GREP_FILES="grep -r"
+fi
diff --git a/util/lint/lint-extended-020-signed-off-by b/util/lint/lint-extended-020-signed-off-by
new file mode 100755
index 0000000..ef62a45
--- /dev/null
+++ b/util/lint/lint-extended-020-signed-off-by
@@ -0,0 +1,23 @@
+#!/usr/bin/env sh
+# SPDX-License-Identifier: GPL-2.0-or-later
+#
+# DESCR: Check for a signed-off-by line on the latest commit
+
+
+LINTDIR="$(
+ cd -- "$(dirname "$0")" > /dev/null 2>&1 || return
+ pwd -P
+)"
+
+# shellcheck source=helper_functions.sh
+. "${LINTDIR}/helper_functions.sh"
+
+if [ "${IN_GIT_TREE}" -eq 0 ]; then
+ exit 0
+fi
+
+# This test is mainly for the jenkins server
+if ! ${GIT} log -n 1 | grep -q '[[:space:]]\+Signed-off-by: '; then
+ echo "No Signed-off-by line in commit message"
+ exit 1
+fi
--
To view, visit https://review.coreboot.org/c/flashrom/+/70243
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: 1.3.x
Gerrit-Change-Id: I03f1827803f8492d60a0a44174d5822c2265bfcb
Gerrit-Change-Number: 70243
Gerrit-PatchSet: 2
Gerrit-Owner: Felix Singer <felixsinger(a)posteo.net>
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-MessageType: merged