January 2018 - flashrom-gerrit

Change in flashrom[master]: make args for -r/-w/-v non-positional and optional
by Nico Huber (Code Review) 20 Jan '18

20 Jan '18

Nico Huber has posted comments on this change. ( https://review.coreboot.org/23022 ) Change subject: make args for -r/-w/-v non-positional and optional ...................................................................... Patch Set 7: Code-Review-1 (1 comment) It's moving into the right direction. But I'd still like to have that discussion about CrOS interface compatibility. If we decide to uncon- ditionally keep that (over possibly better interface choices), that's a very influental decision for the project. And shouldn't be made in a review. Just imagine how fast this change could have been done for flashrom proper if we'd just kept the idea (per region files) and implemented it consistently with good old flashrom quality. I'd estimate a third of the time we already spent so far for gathering old commits, poli- shing, discussing. https://review.coreboot.org/#/c/23022/7/cli_classic.c File cli_classic.c: https://review.coreboot.org/#/c/23022/7/cli_classic.c@347 PS7, Line 347: char ** argv_copy = malloc((argc + 1) * sizeof(*argv_copy)); I'm not fully awake yet. Though, can't you just use argv + optind instead of a copy? Either way you don't seem to reset `optind`, so the copied `argv[j]` with `j < optind` won't be considered by getopt_long() anyway. Which is just fine. But makes this code a hell to follow... -- To view, visit https://review.coreboot.org/23022 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iefbcb7dc4fefe26f5afd1292dfd5c1687fa62803 Gerrit-Change-Number: 23022 Gerrit-PatchSet: 7 Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Louis Yung-Chieh Lo <yjlou(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Sat, 20 Jan 2018 13:19:25 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: Yes

1 0

Change in flashrom[master]: layout: Add -i <region>[:<file>] support.
by Arthur Heymans (Code Review) 20 Jan '18

20 Jan '18

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/23021 ) Change subject: layout: Add -i <region>[:<file>] support. ...................................................................... Patch Set 8: still needs to address some issues -- To view, visit https://review.coreboot.org/23021 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic5465659605d8431d931053967b40290195cfd99 Gerrit-Change-Number: 23021 Gerrit-PatchSet: 8 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: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Nicola Corna <nicola(a)corna.info> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Sat, 20 Jan 2018 09:48:52 +0000 Gerrit-HasComments: No Gerrit-HasLabels: No

1 0

Change in flashrom[master]: make args for -r/-w/-v non-positional and optional
by build bot (Jenkins) (Code Review) 20 Jan '18

20 Jan '18

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/23022 ) Change subject: make args for -r/-w/-v non-positional and optional ...................................................................... Patch Set 7: Verified+1 Build Successful https://qa.coreboot.org/job/flashrom-customrules/1037/ : SUCCESS https://qa.coreboot.org/job/flashrom_gerrit/885/ : SUCCESS -- To view, visit https://review.coreboot.org/23022 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iefbcb7dc4fefe26f5afd1292dfd5c1687fa62803 Gerrit-Change-Number: 23022 Gerrit-PatchSet: 7 Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Louis Yung-Chieh Lo <yjlou(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Sat, 20 Jan 2018 09:48:26 +0000 Gerrit-HasComments: No Gerrit-HasLabels: Yes

1 0

Change in flashrom[master]: layout: Add -i <region>[:<file>] support.
by build bot (Jenkins) (Code Review) 20 Jan '18

20 Jan '18

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/23021 ) Change subject: layout: Add -i <region>[:<file>] support. ...................................................................... Patch Set 8: Verified+1 Build Successful https://qa.coreboot.org/job/flashrom-customrules/1036/ : SUCCESS https://qa.coreboot.org/job/flashrom_gerrit/884/ : SUCCESS -- To view, visit https://review.coreboot.org/23021 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic5465659605d8431d931053967b40290195cfd99 Gerrit-Change-Number: 23021 Gerrit-PatchSet: 8 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: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Nicola Corna <nicola(a)corna.info> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Sat, 20 Jan 2018 09:48:17 +0000 Gerrit-HasComments: No Gerrit-HasLabels: Yes

1 0

Change in flashrom[master]: [WIP]Add support to get layout from a binary fmap file
by build bot (Jenkins) (Code Review) 20 Jan '18

20 Jan '18

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/23203 ) Change subject: [WIP]Add support to get layout from a binary fmap file ...................................................................... Patch Set 3: Verified+1 Build Successful https://qa.coreboot.org/job/flashrom-customrules/1035/ : SUCCESS https://qa.coreboot.org/job/flashrom_gerrit/883/ : SUCCESS -- To view, visit https://review.coreboot.org/23203 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0e7fad38ed79a84d41358e1f175c36d255786c12 Gerrit-Change-Number: 23203 Gerrit-PatchSet: 3 Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Sat, 20 Jan 2018 09:48:05 +0000 Gerrit-HasComments: No Gerrit-HasLabels: Yes

1 0

Change in flashrom[master]: make args for -r/-w/-v non-positional and optional
by Arthur Heymans (Code Review) 20 Jan '18

20 Jan '18

Arthur Heymans has uploaded a new patch set (#7) to the change originally created by David Hendricks. ( https://review.coreboot.org/23022 ) Change subject: make args for -r/-w/-v non-positional and optional ...................................................................... make args for -r/-w/-v non-positional and optional Ported from chromiumos: https://chromium-review.googlesource.com/#/c/60515/ This makes a filename following -r, -w, and -v operations non- positional. The first argument that does not begin with a hyphen (-) is the filename for -r/-w/-v. If no such argument exists, then -r/-w/-v options apply to files specified for individually included regions via -i. This has a few side-effects: 1. It allows us to omit the ROM-sized filename entirely when a filename is provided for a particular region when using -i. For example, "flashrom -i FOO:foo.bin -r". 2. It allows, for better or worse, the filename be specified anywhere on the command line after the program name. Our scripts and docs should still specify the file after -r/-w/-v for clarity. For our purposes, if a filename is given for every included region (-i region:filename) then the user does not need to specify a filename after -r/-w/-v. However, if any -i option does not specify a filename, then a file must be specified after -r/-w/-v. The syntax will be backwards compatible for now so that one can still mix -i options with and without the added filename specifier for each region. BUG=chromium:263495 BRANCH=none TEST=manual (see notes below) 1. Write random data to RW_UNUSED region (on snow in this case) without requiring an argument to -w. See that only RW_UNUSED is erased and written, and that verify works: dd if=/dev/urandom of=rw_unused.bin bs=1k count=1 conv=notrunc flashrom -p host -i RW_UNUSED:rw_unused.bin -w -V flashrom -p host -i RW_UNUSED:rw_unused.bin -v -V 2. Same as above, but specify a dummy file to test syntax backwards compatibility: dd if=/dev/urandom of=rw_unused.bin bs=1k count=1 conv=notrunc dd if=/dev/urandom of=random_4M.bin bs=1M count=4 flashrom -p host -i RW_UNUSED:rw_unused.bin -w random_4M.bin -V flashrom -p host -i RW_UNUSED:rw_unused.bin -v random_4M.bin -V 3. Test that dumping RW_UNUSED and GBB regions without -r arg dumps two files and that they can be used to verify the content: flashrom -p host -i RW_UNUSED:rw_unused.bin -i GBB:gbb.bin -r flashrom -p host -i RW_UNUSED:rw_unused.bin -i GBB:gbb.bin -v 4. Same as above, but with dummy file: flashrom -p host -i RW_UNUSED:rw_unused.bin -i GBB:gbb.bin -r x.bin flashrom -p host -i RW_UNUSED:rw_unused.bin -i GBB:gbb.bin -v x.bin Change-Id: Iefbcb7dc4fefe26f5afd1292dfd5c1687fa62803 Reviewed-on: https://gerrit.chromium.org/gerrit/60515 Reviewed-by: Yung-Chieh Lo <yjlou(a)chromium.org> Commit-Queue: David Hendricks <dhendrix(a)chromium.org> Tested-by: David Hendricks <dhendrix(a)chromium.org> Signed-off-by: Arthur Heymans <arthur(a)aheymans.xyz> --- M cli_classic.c M flashrom.8.tmpl 2 files changed, 117 insertions(+), 23 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/22/23022/7 -- To view, visit https://review.coreboot.org/23022 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iefbcb7dc4fefe26f5afd1292dfd5c1687fa62803 Gerrit-Change-Number: 23022 Gerrit-PatchSet: 7 Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Louis Yung-Chieh Lo <yjlou(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>

1 0

Change in flashrom[master]: layout: Add -i <region>[:<file>] support.
by Arthur Heymans (Code Review) 20 Jan '18

20 Jan '18

Arthur Heymans has uploaded a new patch set (#8) to the change originally created by David Hendricks. ( https://review.coreboot.org/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> --- M cli_classic.c M dummyflasher.c M flash.h M flashrom.8.tmpl M flashrom.c M layout.c M layout.h 7 files changed, 262 insertions(+), 41 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/21/23021/8 -- To view, visit https://review.coreboot.org/23021 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ic5465659605d8431d931053967b40290195cfd99 Gerrit-Change-Number: 23021 Gerrit-PatchSet: 8 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: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Nicola Corna <nicola(a)corna.info> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>

1 0

Change in flashrom[master]: layout: Add -i <region>[:<file>] support.
by David Hendricks (Code Review) 19 Jan '18

19 Jan '18

David Hendricks has posted comments on this change. ( https://review.coreboot.org/23021 ) Change subject: layout: Add -i <region>[:<file>] support. ...................................................................... Patch Set 7: (1 comment) https://review.coreboot.org/#/c/23021/7/flashrom.c File flashrom.c: https://review.coreboot.org/#/c/23021/7/flashrom.c@2647 PS7, Line 2647: * processed and merged into newcontents since -i files take priority. > This comment still makes no sense to me. I'd guess it was […] Looking back, this seems to have been messed up in translation. My fault. The comment is also confusing given how the code is structured in the upstream codebase with Nico's libflashrom patches merged. The `else` is incorrect and in do_verify(). We should always read_buf_from_include_args() if `-i region:filename` is used. We should read the full file if `filename` is specified for -w/-v, and then read the `-i region:file` args to update the content in the buffer. For reference, refer to lines 2058-2091 and 2124-2134 in https://chromium.googlesource.com/chromiumos/third_party/flashrom/+/master/… -- To view, visit https://review.coreboot.org/23021 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic5465659605d8431d931053967b40290195cfd99 Gerrit-Change-Number: 23021 Gerrit-PatchSet: 7 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: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Nicola Corna <nicola(a)corna.info> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Fri, 19 Jan 2018 21:25:31 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: No

1 0

Change in flashrom[master]: make args for -r/-w/-v non-positional and optional
by Arthur Heymans (Code Review) 19 Jan '18

19 Jan '18

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/23022 ) Change subject: make args for -r/-w/-v non-positional and optional ...................................................................... Patch Set 6: > I'm missing a check for spurious arguments. I have no idea how to > implement it (straightforward) together with the non-positional > file > names. Currently, if you have a typo in your command line (e.g. > forget > a dash before an option), things may just be ignored... > > So why make it non-positional? Is there a way to change the argument amount on -r/-w/-v since without this change, using -i region:file one always needs a filename next to -r/-w/-v which sort of defeats the purpose of the previous patch. -- To view, visit https://review.coreboot.org/23022 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iefbcb7dc4fefe26f5afd1292dfd5c1687fa62803 Gerrit-Change-Number: 23022 Gerrit-PatchSet: 6 Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Louis Yung-Chieh Lo <yjlou(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Fri, 19 Jan 2018 11:10:01 +0000 Gerrit-HasComments: No Gerrit-HasLabels: No

1 0

Change in flashrom[master]: buspirate_spi: Add support for variable serial speeds
by Nico Huber (Code Review) 17 Jan '18

17 Jan '18

Nico Huber has posted comments on this change. ( https://review.coreboot.org/23057 ) Change subject: buspirate_spi: Add support for variable serial speeds ...................................................................... Patch Set 6: Code-Review+1 (4 comments) Tried to find some documentation on the matter which raised more questions. Code looks fine, so far. https://review.coreboot.org/#/c/23057/4/buspirate_spi.c File buspirate_spi.c: https://review.coreboot.org/#/c/23057/4/buspirate_spi.c@497 PS4, Line 497: bp_commbuf[0] = 0x00; > Good catch, I'll fix that. […] Yes, I meant msg_pinfo() but was in doubt anyway. msg_pdbg() is just fine. https://review.coreboot.org/#/c/23057/6/buspirate_spi.c File buspirate_spi.c: https://review.coreboot.org/#/c/23057/6/buspirate_spi.c@443 PS6, Line 443: 5.5 Where is this written? This page talks about 5.2: http://dangerousprototypes.com/docs/Bus_Pirate_menu_options_guide#B_Set_PC_… https://review.coreboot.org/#/c/23057/6/buspirate_spi.c@451 PS6, Line 451: " Continue at your own risk.\n"); I did some research, v2 can work with the same firmware and uses the same microcontroller and USB converter as v3. So that should be fine. Maybe remove the warning for v2 but keep it at default 115200 because we can't test it? https://review.coreboot.org/#/c/23057/6/buspirate_spi.c@473 PS6, Line 473: sleep(1); Is this specified somewhere? It seems incredibly long. -- To view, visit https://review.coreboot.org/23057 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3706f17a94fdf056063f2ad4a5f0a219665cdcbf Gerrit-Change-Number: 23057 Gerrit-PatchSet: 6 Gerrit-Owner: Shawn Anastasio <shawnanastasio(a)yahoo.com> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Shawn Anastasio <shawnanastasio(a)yahoo.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Wed, 17 Jan 2018 11:49:49 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: Yes

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit January 2018