Attention is currently required from: Nico Huber, Edward O'Callaghan, Nikolai Artemiev.
Anastasia Klimchuk 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 1: Code-Review+1
(2 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/70342/comment/6ed53cd6_7c97f7a5
PS1, Line 15: Coreboot issue
Ticket
(instead of "Coreboot issue")
Patchset:
PS1:
Thank you so much Nikolai! Could you please add a topic "for_1.3.x" .
--
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: 1
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-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Comment-Date: Mon, 05 Dec 2022 02:22:45 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Anastasia Klimchuk has submitted this change. ( https://review.coreboot.org/c/flashrom/+/70003 )
(
3 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
)Change subject: cli, manpage: Clean up occurrences of old image argument
......................................................................
cli, manpage: Clean up occurrences of old image argument
--include argument was introduced and replaced --image argument in
commit 45d50a101e8073191e6d88143990ed91d3bfe815
This patch cleans up remaining few places where old `--image`
argument was mentioned so that now all the documentation has
`--include`. --image is deprecated.
Both old --image and new --include have the same short version -i
and it remains the same. The code remains the same since the code
handles --include already.
Tested by running
flashrom -h
man ./flashrom.8.tmpl
Ticket: https://ticket.coreboot.org/issues/372
Change-Id: If457e2b8548034868105b515125d7e8b4d5f6134
Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org>
Reviewed-on: https://review.coreboot.org/c/flashrom/+/70003
Reviewed-by: Edward O'Callaghan <quasisec(a)chromium.org>
Reviewed-by: Felix Singer <felixsinger(a)posteo.net>
Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org>
---
M cli_classic.c
M flashrom.8.tmpl
2 files changed, 36 insertions(+), 3 deletions(-)
Approvals:
build bot (Jenkins): Verified
Felix Singer: Looks good to me, approved
Edward O'Callaghan: Looks good to me, approved
diff --git a/cli_classic.c b/cli_classic.c
index 5cc14a0..0b0944f 100644
--- a/cli_classic.c
+++ b/cli_classic.c
@@ -71,8 +71,9 @@
" --fmap read ROM layout from fmap embedded in ROM\n"
" --fmap-file <fmapfile> read ROM layout from fmap in <fmapfile>\n"
" --ifd read layout from an Intel Firmware Descriptor\n"
- " -i | --image <region>[:<file>] only read/write image <region> from layout\n"
+ " -i | --include <region>[:<file>] only read/write image <region> from layout\n"
" (optionally with data from <file>)\n"
+ " --image <region>[:<file>] deprecated, please use --include\n"
" -o | --output <logfile> log output to <logfile>\n"
" --flash-contents <ref-file> assume flash contents to be <ref-file>\n"
" -L | --list-supported print supported devices\n"
@@ -619,7 +620,8 @@
{"ifd", 0, NULL, OPTION_IFD},
{"fmap", 0, NULL, OPTION_FMAP},
{"fmap-file", 1, NULL, OPTION_FMAP_FILE},
- {"image", 1, NULL, 'i'},
+ {"image", 1, NULL, 'i'}, // (deprecated): back compatibility.
+ {"include", 1, NULL, 'i'},
{"flash-contents", 1, NULL, OPTION_FLASH_CONTENTS},
{"flash-name", 0, NULL, OPTION_FLASH_NAME},
{"flash-size", 0, NULL, OPTION_FLASH_SIZE},
diff --git a/flashrom.8.tmpl b/flashrom.8.tmpl
index e672869..4c43323 100644
--- a/flashrom.8.tmpl
+++ b/flashrom.8.tmpl
@@ -49,7 +49,7 @@
(\fB\-\-flash\-name\fR|\fB\-\-flash\-size\fR|
[\fB\-E\fR|\fB\-x\fR|\fB\-r\fR <file>|\fB\-w\fR <file>|\fB\-v\fR <file>]
[(\fB\-l\fR <file>|\fB\-\-ifd\fR|\fB \-\-fmap\fR|\fB\-\-fmap-file\fR <file>)
- [\fB\-i\fR <image>[:<file>]]]
+ [\fB\-i\fR <include>[:<file>]]]
[\fB\-\-wp\-status\fR] [\fB\-\-wp\-list\fR] [\fB\-\-wp\-enable\fR|\fB\-\-wp\-disable\fR]
[\fB\-\-wp\-range\fR <start>,<length>|\fB\-\-wp\-region\fR <region>]
[\fB\-n\fR] [\fB\-N\fR] [\fB\-f\fR])]
--
To view, visit https://review.coreboot.org/c/flashrom/+/70003
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: If457e2b8548034868105b515125d7e8b4d5f6134
Gerrit-Change-Number: 70003
Gerrit-PatchSet: 5
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: 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: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-MessageType: merged
Nikolai Artemiev has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/70342 )
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 CB:35480, the GD25Q256D datasheet indicates
that the chip does not requre a WREN command to enter 4BA mode.
Testing has confirmed that a WREN command is not requried, so change the
flashchip feature flags from FEATURE_4BA_WREN to FEATURE_4BA.
Coreboot issue: 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, 23 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/42/70342/1
diff --git a/flashchips.c b/flashchips.c
index 6f65389..7ea8601 100644
--- a/flashchips.c
+++ b/flashchips.c
@@ -6849,7 +6849,7 @@
.model_id = GIGADEVICE_GD25Q256D,
.total_size = 32768,
.page_size = 256,
- .feature_bits = FEATURE_WRSR_WREN | FEATURE_OTP | FEATURE_4BA_WREN |
+ .feature_bits = FEATURE_WRSR_WREN | FEATURE_OTP | FEATURE_4BA |
FEATURE_WRSR_EXT2 | FEATURE_WRSR2 | FEATURE_WRSR3,
.tested = TEST_OK_PREWB,
.probe = PROBE_SPI_RDID,
--
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: 1
Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-MessageType: newchange
Attention is currently required from: Felix Singer, Edward O'Callaghan, Angel Pons.
Anastasia Klimchuk 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 4:
(4 comments)
File layout.c:
https://review.coreboot.org/c/flashrom/+/70006/comment/f07643a8_a4ca4b9a
PS2, Line 139: || file == NULL
> Line 144 causes a memory leak now. […]
Oh yes, thanks for spotting memory leak! I missed that, fixed by `goto error`.
The option you said prefer (moving the code) does not compile, I tried that at some point earlier locally (between patchset 2 and 3), the reason is:
```
../layout.c: In function ‘register_include_arg’:
../layout.c:170:9: error: ‘name’ may be used uninitialized [-Werror=maybe-uninitialized]
170 | free(name);
| ^~~~~~~~~~
```
(line number might be out of date).
Handling the `name` first, `file` second solves the issue. This also means all error paths (for name, file and below) are treated by `goto error`, looks nice and consistent.
File layout.c:
https://review.coreboot.org/c/flashrom/+/70006/comment/b2316f42_28504fd3
PS3, Line 135: if (name == NULL) {
> nit: `!name`
Done
https://review.coreboot.org/c/flashrom/+/70006/comment/cee1d3e0_a50fe7c7
PS3, Line 142: if(!colon[1]) {
> mising space `if (`
Done
https://review.coreboot.org/c/flashrom/+/70006/comment/a4347779_12ec7822
PS3, Line 148: if (file == NULL) {
> nit: `!file`
Done
--
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: 4
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: 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-Comment-Date: Mon, 05 Dec 2022 02:03:54 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Felix Singer <felixsinger(a)posteo.net>
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Edward O'Callaghan, Angel Pons, Anastasia Klimchuk.
Hello Felix Singer, build bot (Jenkins), Edward O'Callaghan, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/70006
to look at the new patch set (#4).
Change subject: layout: Check return values for strdup in register_include_arg
......................................................................
layout: Check return values for strdup in register_include_arg
strdup return values should be checked for NULL to catch the
potential error case of out of memory.
Follow up on
commit 45d50a101e8073191e6d88143990ed91d3bfe815
Ticket: https://ticket.coreboot.org/issues/372
Change-Id: I6c22196be6847a8c9704f1de936604a51b4b8a28
Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org>
---
M layout.c
1 file changed, 39 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/06/70006/4
--
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: 4
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: Felix Singer <felixsinger(a)posteo.net>
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: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: newpatchset
Attention is currently required from: Subrata Banik, Angel Pons, Nikolai Artemiev.
Edward O'Callaghan 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 9:
(1 comment)
Commit Message:
https://review.coreboot.org/c/flashrom/+/69195/comment/ae975696_a7b26501
PS2, Line 7: ichspi.c: Read chip ID and use it to identify chip
> > > Maybe we should just print a warning if >1 chip is detected? […]
In flashrom.c the top level of the CFG is:
```
@@ -915,7 +915,7 @@ int probe_flash(struct registered_master *mst, int startchip, struct flashctx *f
if (!w29ee011_can_override(flash->chip->name, chip_to_probe))
goto notfound;
- if (probe_func(flash) != 1)
```
you could return '2' from this function to mean two chips found and handle it in the above.
The actionable here, however, is likely just read the descriptor to find the number of flashes and print that when it is >0 (i.e., >1chip) but just assume the first chip.
--
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: 9
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-Comment-Date: Sun, 04 Dec 2022 23:45:25 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com>
Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>
Comment-In-Reply-To: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-MessageType: comment
Attention is currently required from: Simon Buhrow, Thomas Heijligen, Aarya.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67717 )
Change subject: spi.c: Add AT45 & SF25F erasefn opcode mapping
......................................................................
Patch Set 6:
(3 comments)
Patchset:
PS6:
I would just squash this and the two previous patches together. There is no benefit to having them separate and actually introduces the possibility of subtle things leaking though review like that NULL.
File spi.c:
https://review.coreboot.org/c/flashrom/+/67717/comment/e2e3c4d2_66e1fe1c
PS6, Line 229: Assuming 0x00
..
https://review.coreboot.org/c/flashrom/+/67717/comment/2adc1ada_10f71bcd
PS6, Line 229: 0x00
`NULL`
--
To view, visit https://review.coreboot.org/c/flashrom/+/67717
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I798a91f1e20b63662715c68e6d43d03fc6005d51
Gerrit-Change-Number: 67717
Gerrit-PatchSet: 6
Gerrit-Owner: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Simon Buhrow
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Aarya <aarya.chaumal(a)gmail.com>
Gerrit-Attention: Simon Buhrow
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Aarya <aarya.chaumal(a)gmail.com>
Gerrit-Comment-Date: Sun, 04 Dec 2022 23:18:30 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Edward O'Callaghan, Angel Pons, Anastasia Klimchuk.
Felix Singer 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 3:
(4 comments)
File layout.c:
https://review.coreboot.org/c/flashrom/+/70006/comment/cfe0ec42_17bcabf6
PS2, Line 139: || file == NULL
> Yes I forgot that indeed file is optional parameter. […]
Line 144 causes a memory leak now. If a colon is found but the second parameter (the file name) isn't given, then `name` doesn't get freed because line 144 just returns. That's why I suggested to leave that code where it is and to just extend that if-block, so that we error out early as possible.
You can either replace that return with `goto error` or you move lines 140-152 below line 131, where they were before. I prefer the latter.
File layout.c:
https://review.coreboot.org/c/flashrom/+/70006/comment/1511c23a_63309098
PS3, Line 135: if (name == NULL) {
nit: `!name`
https://review.coreboot.org/c/flashrom/+/70006/comment/9ff374eb_419a66a6
PS3, Line 142: if(!colon[1]) {
mising space `if (`
https://review.coreboot.org/c/flashrom/+/70006/comment/b94b0165_679e6b6f
PS3, Line 148: if (file == NULL) {
nit: `!file`
--
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: 3
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: Felix Singer <felixsinger(a)posteo.net>
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: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Sat, 03 Dec 2022 15:09:59 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Felix Singer <felixsinger(a)posteo.net>
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: comment
Aarya has abandoned this change. ( https://review.coreboot.org/c/flashrom/+/67354 )
Change subject: spi.c: Add erasefn and opcodes for AT45 and S25F to lookup list
......................................................................
Abandoned
CB:67717 does the job
--
To view, visit https://review.coreboot.org/c/flashrom/+/67354
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ief25932120f940dea0ffe161a79219deecb2e8ab
Gerrit-Change-Number: 67354
Gerrit-PatchSet: 12
Gerrit-Owner: Aarya <aarya.chaumal(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Simon Buhrow
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: abandon