Attention is currently required from: David Hendricks, Edward O'Callaghan, Daniel Campello.
Daniel Campello has uploaded a new patch set (#17) to the change originally created by David Hendricks. ( https://review.coreboot.org/c/flashrom/+/23021 )
Change subject: layout: Add -i <region>[:<file>] support
......................................................................
layout: Add -i <region>[:<file>] support
Add an optional sub-parameter to the -i parameter to allow building the
image to be written from multiple files. This will also allow regions to
be read from flash and written to separate image files.
This is a rebase of a patch that was ported from chromiumos. A lot of
things have changed, but the idea is the same.
Original patch by Louis Yung-Chieh Lo <yjlou(a)chromium.org>:
Summary: Support -i partition:file feature for both read and write.
Commit: 9c7525f
Review URL: http://codereview.chromium.org/6611015
Ported version by Stefan Tauner <stefan.tauner(a)student.tuwien.ac.at>
and Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006(a)gmx.net>:
Summary: [PATCH 2/6] layout: Add -i <region>[:<file>] support.
Review URL: https://mail.coreboot.org/pipermail/flashrom/2013-October/011729.html
Change-Id: Ic5465659605d8431d931053967b40290195cfd99
Signed-off-by: David Hendricks <dhendrix(a)chromium.org>
Signed-off-by: Stefan Tauner <stefan.tauner(a)student.tuwien.ac.at>
Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006(a)gmx.net>
Signed-off-by: Arthur Heymans <arthur(a)aheymans.xyz>
Signed-off-by: Nico Huber <nico.huber(a)secunet.com>
Signed-off-by: Edward O'Callaghan <quasisec(a)google.com>
Signed-off-by: Daniel Campello <campello(a)chromium.org>
Co-Authored-by: Edward O'Callaghan <quasisec(a)google.com>
Co-Authored-by: Daniel Campello <campello(a)chromium.org>
---
M cli_classic.c
M flashrom.8.tmpl
M flashrom.c
M layout.c
M layout.h
5 files changed, 196 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/21/23021/17
--
To view, visit https://review.coreboot.org/c/flashrom/+/23021
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ic5465659605d8431d931053967b40290195cfd99
Gerrit-Change-Number: 23021
Gerrit-PatchSet: 17
Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006(a)gmx.net>
Gerrit-Reviewer: Daniel Campello <campello(a)chromium.org>
Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Nicola Corna <nicola(a)corna.info>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Daniel Campello <campello(a)chromium.org>
Gerrit-MessageType: newpatchset
Attention is currently required from: David Hendricks, Edward O'Callaghan.
Daniel Campello has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/23021 )
Change subject: layout: Add -i <region>[:<file>] support
......................................................................
Patch Set 16:
(2 comments)
File flashrom.c:
https://review.coreboot.org/c/flashrom/+/23021/comment/3d6558a8_8d4a203c
PS16, Line 1337: %s
This can just be changed to "the expected size" for all cases to avoid the extra complicated interface.
File layout.c:
https://review.coreboot.org/c/flashrom/+/23021/comment/4a3143c8_27b92e8f
PS16, Line 293: layout->entries[i].included = true;
I think this was removed by mistake.
--
To view, visit https://review.coreboot.org/c/flashrom/+/23021
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ic5465659605d8431d931053967b40290195cfd99
Gerrit-Change-Number: 23021
Gerrit-PatchSet: 16
Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006(a)gmx.net>
Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Nicola Corna <nicola(a)corna.info>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Daniel Campello <campello(a)chromium.org>
Gerrit-Attention: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Comment-Date: Tue, 13 Apr 2021 02:05:43 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: David Hendricks, Edward O'Callaghan.
Daniel Campello has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/23021 )
Change subject: layout: Add -i <region>[:<file>] support
......................................................................
Patch Set 16:
(1 comment)
File flashrom.c:
https://review.coreboot.org/c/flashrom/+/23021/comment/f0d5371a_ca67881f
PS16, Line 2720: get_num_include_args_with_files(get_layout(flash)) > 0
This is not needed. If no regions are included with files the read_buf_from_include_args() will be a noop.
--
To view, visit https://review.coreboot.org/c/flashrom/+/23021
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ic5465659605d8431d931053967b40290195cfd99
Gerrit-Change-Number: 23021
Gerrit-PatchSet: 16
Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006(a)gmx.net>
Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Nicola Corna <nicola(a)corna.info>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Daniel Campello <campello(a)chromium.org>
Gerrit-Attention: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Comment-Date: Tue, 13 Apr 2021 01:37:06 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Anastasia Klimchuk has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/52284 )
Change subject: linux_spi.c: Separate shutdown from failed init cleanup
......................................................................
linux_spi.c: Separate shutdown from failed init cleanup
Shutdown function was covering two different jobs here:
1) the actual shutdown which is run at the end of the driver's
lifecycle and 2) cleanup in cases when initialisation failed.
Now, shutdown is only doing its main job (#1), and the driver
itself is doing cleanup when init fails (#2).
The good thing is that now resources are released/closed immediately
in cases when init fails (vs shutdown function which was run at some
point later), and the driver leaves clean space after itself if init fails.
And very importantly this unlocks API change which plans to move
register_shutdown inside register master API, see this
https://review.coreboot.org/c/flashrom/+/51761
TEST=builds
BUG=b:140394053
Change-Id: I1c8da2878cd0e85a1e43ba9b4b8e6f3d9f38ae5c
Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org>
---
M linux_spi.c
1 file changed, 18 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/84/52284/1
diff --git a/linux_spi.c b/linux_spi.c
index b3cbf97..ae5c318 100644
--- a/linux_spi.c
+++ b/linux_spi.c
@@ -167,6 +167,7 @@
/* SPI mode 0 (beware this also includes: MSB first, CS active low and others */
const uint8_t mode = SPI_MODE_0;
const uint8_t bits = 8;
+ int ret = 0;
p = extract_programmer_param("spispeed");
if (p && strlen(p)) {
@@ -200,34 +201,44 @@
}
free(dev);
- if (register_shutdown(linux_spi_shutdown, NULL))
- return 1;
- /* We rely on the shutdown function for cleanup from here on. */
-
if (ioctl(fd, SPI_IOC_WR_MAX_SPEED_HZ, &speed_hz) == -1) {
msg_perr("%s: failed to set speed to %"PRIu32"Hz: %s\n",
__func__, speed_hz, strerror(errno));
- return 1;
+ ret = 1;
+ goto init_err;
}
msg_pdbg("Using %"PRIu32"kHz clock\n", speed_hz / 1000);
if (ioctl(fd, SPI_IOC_WR_MODE, &mode) == -1) {
msg_perr("%s: failed to set SPI mode to 0x%02x: %s\n",
__func__, mode, strerror(errno));
- return 1;
+ ret = 1;
+ goto init_err;
}
if (ioctl(fd, SPI_IOC_WR_BITS_PER_WORD, &bits) == -1) {
msg_perr("%s: failed to set the number of bits per SPI word to %u: %s\n",
__func__, bits == 0 ? 8 : bits, strerror(errno));
- return 1;
+ ret = 1;
+ goto init_err;
}
max_kernel_buf_size = get_max_kernel_buf_size();
msg_pdbg("%s: max_kernel_buf_size: %zu\n", __func__, max_kernel_buf_size);
+ if (register_shutdown(linux_spi_shutdown, NULL)) {
+ ret = 1;
+ goto init_err;
+ }
register_spi_master(&spi_master_linux);
return 0;
+
+init_err:
+ if (fd != -1) {
+ close(fd);
+ fd = -1;
+ }
+ return ret;
}
#endif // CONFIG_LINUX_SPI == 1
--
To view, visit https://review.coreboot.org/c/flashrom/+/52284
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I1c8da2878cd0e85a1e43ba9b4b8e6f3d9f38ae5c
Gerrit-Change-Number: 52284
Gerrit-PatchSet: 1
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: newchange
Attention is currently required from: Jack Rosenthal.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52215 )
Change subject: linux_mtd: prevent corruption of flash when stdout/stderr is closed
......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1:
> > Generally, as this seems to be a problem of the CLI (flashrom_print_cb() using stdout/stderr I guess), I'd prefer a patch for the CLI, e.g. check if fstat() works on them or something like
> that. Other programmers might suffer the same.
>
> Are you aware of any other programmers that will write file contents directly to the flash like Linux MTD? Would be useful to add the same to them...
I don't think there are any. There are other programmers that use file I/O,
though, e.g. anything TTY using. Chances seem low that they'd recognize
text output as commands, I guess?
>
> Patching just the CLI would not protect libflashrom users, unfortunately.
I'm not aware of any non-CLI code writing to stdout/stderr. That would
be a bug. I just grepped and all I could find was serprog flushing stdout.
That seems not very critical but should be fixed anyway.
--
To view, visit https://review.coreboot.org/c/flashrom/+/52215
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I751c9dd88ad1d30283b94bd2185b4f8f25569c8f
Gerrit-Change-Number: 52215
Gerrit-PatchSet: 1
Gerrit-Owner: Jack Rosenthal <jrosenth(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Jack Rosenthal <jrosenth(a)chromium.org>
Gerrit-Comment-Date: Mon, 12 Apr 2021 22:34:23 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Jack Rosenthal <jrosenth(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber.
Jack Rosenthal has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52215 )
Change subject: linux_mtd: prevent corruption of flash when stdout/stderr is closed
......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1:
> Generally, as this seems to be a problem of the CLI (flashrom_print_cb() using stdout/stderr I guess), I'd prefer a patch for the CLI, e.g. check if fstat() works on them or something like
that. Other programmers might suffer the same.
Are you aware of any other programmers that will write file contents directly to the flash like Linux MTD? Would be useful to add the same to them...
Patching just the CLI would not protect libflashrom users, unfortunately.
--
To view, visit https://review.coreboot.org/c/flashrom/+/52215
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I751c9dd88ad1d30283b94bd2185b4f8f25569c8f
Gerrit-Change-Number: 52215
Gerrit-PatchSet: 1
Gerrit-Owner: Jack Rosenthal <jrosenth(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
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-Comment-Date: Mon, 12 Apr 2021 21:44:12 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/51487 )
Change subject: Add unit test to run init/shutdown for enabled drivers
......................................................................
Patch Set 2:
(1 comment)
Patchset:
PS2:
Hello my reviewers and cc! What do people think of the latest version that has mocks in a separate compilation units? I can say it works, tests are working, which is really good.
I thought to split this into 3 smaller patches, but on the other hand if you think this can go together in one patch, also fine.
Thank you!
--
To view, visit https://review.coreboot.org/c/flashrom/+/51487
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I3af612defe1af3850dfc1626a208d873e3a3eddc
Gerrit-Change-Number: 51487
Gerrit-PatchSet: 2
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
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-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: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Comment-Date: Mon, 12 Apr 2021 21:42:36 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber.
Jack Rosenthal has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52215 )
Change subject: linux_mtd: prevent corruption of flash when stdout/stderr is closed
......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1:
The issue we saw here was in the cgpt utility, for editing the RW_GPT region on some devices. For years (since 2015 when this code landed) it had this ugly code in attempt to squelch the stdout of flashrom:
int fd_flags = fcntl(1, F_GETFD);
// Close stdout on exec so that flashrom does not muck up cgpt's output.
if (0 != fcntl(1, F_SETFD, FD_CLOEXEC))
Warning("Can't stop flashrom from mucking up our output\n");
if (ForkExecL(temp_dir_template, FLASHROM_PATH, "-i", "RW_GPT:rw_gpt", "-r",
NULL) != 0) {
For years, this code was actually sending the output to a lock file that got opened before /dev/mtd0 (see https://chromium.googlesource.com/chromiumos/third_party/flashrom/+/HEAD/bi… in our fork of flashrom). But upon switching from Makefile to meson build systems, we dropped our config option for the lock file, and this was now being written to /dev/mtd0.
Quite honestly, this is one of the more fascinating bugs I've seen in my career... I would have never expected the kernel to re-use fd 0, 1, 2 for another random file if those got closed, but I guess it does.
> Or maybe there's even something to fix in your libc.
Fixing this in glibc would probably break something :P
I actually doubt glibc would even want a "fix" which checks stdin, stdout, stderr are open before main. Sure, the library assumes it, but that's because POSIX says it can ;)
Anyway, this is a lot of a story to dump into the commit message. I think what I have there is detailed enough for the commit already ... if you think some detail here could be added to it, LMK.
--
To view, visit https://review.coreboot.org/c/flashrom/+/52215
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I751c9dd88ad1d30283b94bd2185b4f8f25569c8f
Gerrit-Change-Number: 52215
Gerrit-PatchSet: 1
Gerrit-Owner: Jack Rosenthal <jrosenth(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
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-Comment-Date: Mon, 12 Apr 2021 21:41:35 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Gerrit-MessageType: comment