December 2021 - flashrom-gerrit

Change in flashrom[master]: flashrom: Convert do_read() into a libflashrom user
by Anastasia Klimchuk (Code Review) 14 Dec '21

14 Dec '21

Attention is currently required from: Edward O'Callaghan, Daniel Campello, Angel Pons. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59291 ) Change subject: flashrom: Convert do_read() into a libflashrom user ...................................................................... Patch Set 4: (1 comment) File flashrom.c: https://review.coreboot.org/c/flashrom/+/59291/comment/5108cb42_bb9bca79 PS4, Line 2173: msg_cinfo("FAILED.\n"); This message is now left orphaned. Before, there was a message `msg_cinfo("Reading flash... ")` in the beginning of read_flash_to_file, and this one goes together with it. So if memory allocation does fail, the output was Reading flash... Memory allocation failed! FAILED Now `msg_cinfo("Reading flash... ")` happens in flashrom_image_read, but flashrom_image_read is called below. So if memory allocation does fail, the output will be Memory allocation failed! FAILED Maybe this orphaned FAILED can be removed? Do we consider memory allocation as a part of Reading flash or is it a pre-process step? -- To view, visit https://review.coreboot.org/c/flashrom/+/59291 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I4b690b688acf9d5deb46e8642a252a2132ea8c73 Gerrit-Change-Number: 59291 Gerrit-PatchSet: 4 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Daniel Campello <campello(a)chromium.org> 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: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Daniel Campello <campello(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Tue, 14 Dec 2021 03:11:22 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: dediprog: wait for spi bulk read xfers to finish
by Rick Altherr (Code Review) 14 Dec '21

14 Dec '21

Rick Altherr has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/60087 ) Change subject: dediprog: wait for spi bulk read xfers to finish ...................................................................... dediprog: wait for spi bulk read xfers to finish dediprog_bulk_read_poll()'s finish argument allows it to be used in two distinct cases: where dediprog_bulk_read_poll will be called as part of a loop (finish=0) and where dediprog_bulk_read_poll should wait for all outstanding transfers to finish (finish=1). In both cases, dediprog_bulk_read_poll() calls libusb to process events with a 10 second timeout. After dediprog_spi_bulk_read() has queued the last transfers, it calls dediprog_bulk_read_poll() with finish=0 when it should be finish=1. finish=0 just happens to work because frequently the transfers finish in the 10 second timeout. Signed-off-by: Rick Altherr <rick(a)oxidecomputer.com> Change-Id: If7cb541742c8620358c8e04275d8316131b2d1ab --- M dediprog.c 1 file changed, 1 insertion(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/87/60087/1 diff --git a/dediprog.c b/dediprog.c index 939673c..8a05a7c 100644 --- a/dediprog.c +++ b/dediprog.c @@ -526,7 +526,7 @@ goto err_free; } /* Wait for transfers to finish. */ - if (dediprog_bulk_read_poll(dp_data->usb_ctx, &status, 0)) + if (dediprog_bulk_read_poll(dp_data->usb_ctx, &status, 1)) goto err_free; /* Check if everything has been transmitted. */ if ((status.finished_idx < count) || status.error) -- To view, visit https://review.coreboot.org/c/flashrom/+/60087 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: If7cb541742c8620358c8e04275d8316131b2d1ab Gerrit-Change-Number: 60087 Gerrit-PatchSet: 1 Gerrit-Owner: Rick Altherr <kc8apf(a)kc8apf.net> Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: writeprotect: print chip range truth table for debugging
by Anastasia Klimchuk (Code Review) 14 Dec '21

14 Dec '21

Attention is currently required from: Nikolai Artemiev. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59909 ) Change subject: writeprotect: print chip range truth table for debugging ...................................................................... Patch Set 3: Code-Review+1 (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/59909/comment/1538b347_e2a3c645 PS3, Line 13: -V So it will print the table with `-V` and won't print in usual (non-verbose) mode , right? -- To view, visit https://review.coreboot.org/c/flashrom/+/59909 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I2cda441229ffdcabff27906f7804efba82baa6bc Gerrit-Change-Number: 59909 Gerrit-PatchSet: 3 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Tue, 14 Dec 2021 00:08:55 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: flashrom.c: extract operation only uses layout files
by Anastasia Klimchuk (Code Review) 13 Dec '21

13 Dec '21

Attention is currently required from: Edward O'Callaghan, Daniel Campello, Angel Pons. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59921 ) Change subject: flashrom.c: extract operation only uses layout files ...................................................................... Patch Set 4: (1 comment) Patchset: PS4: > I am not sure what the concern is since read_flash_to_file() has a singular call site It has two call sites (not one) at this patch, and only after CB:59291 it becomes a singular call site. So this argument actually supports my side :) > CB:59291 is simply a matter of inlining read_flash_to_file() to the call site CB:59291 does more than that, it eliminates a call site from cli_classic and then inlines to one remaining call site. I understand that after both patches are merged, the end result is the same no matter in what sequence both patches merged. But at the moment two patches are not in a chain, and I see from you both comments that you both prefer merge one (this one) as priority and the other one sometime later. In my eyes, just merging this one patch leaves tree in a state where I don't want it to be. CB:59291 would fix the state to be normal again. That's why I want two patches be in a chain and merged together. I understand there is a bug that needs to be fixed. I very much want to fix the bug. Not only because I discovered it, but also because I am probably the only person on the planet who is actually blocked by the bug (my work is blocked CB:59532). So yes I want to fix the bug. > a refactor patch that simply inlines a function It does more than that, it leaves tree in a better state, where bug fix looks good. A very practical question: how far CB:59291 is from being merged? I think it's very close. -- To view, visit https://review.coreboot.org/c/flashrom/+/59921 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ibc9a4e2966385863345f06662521d6d0e4685121 Gerrit-Change-Number: 59921 Gerrit-PatchSet: 4 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: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Daniel Campello <campello(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Mon, 13 Dec 2021 23:52:54 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No 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

1 0

Change in flashrom[master]: flashrom.c: Validate before allocate in verify_range()
by Anastasia Klimchuk (Code Review) 13 Dec '21

13 Dec '21

Attention is currently required from: Nico Huber, Edward O'Callaghan. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59372 ) Change subject: flashrom.c: Validate before allocate in verify_range() ...................................................................... Patch Set 1: Code-Review+2 (1 comment) Patchset: PS1: Sorry for delay, this is nice and useful patch! I should have looked long time ago, but I got buried under heaps of other stuff :( If you could run this from command line once, that would be great. On the other hand, successful path of erase operation has a unit test. -- To view, visit https://review.coreboot.org/c/flashrom/+/59372 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Iae886f203d1c59ae9a89421f7483a4ec3f747256 Gerrit-Change-Number: 59372 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(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-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Comment-Date: Mon, 13 Dec 2021 22:44:41 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: cli_classic.c: Convert do_erase() to libflashrom call
by Daniel Campello (Code Review) 13 Dec '21

13 Dec '21

Attention is currently required from: Edward O'Callaghan, Angel Pons. Daniel Campello has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/60068 ) Change subject: cli_classic.c: Convert do_erase() to libflashrom call ...................................................................... Patch Set 1: Code-Review+1 -- To view, visit https://review.coreboot.org/c/flashrom/+/60068 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I8566164e7dbad69cf478b24208014f10fb99e4d0 Gerrit-Change-Number: 60068 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Daniel Campello <campello(a)chromium.org> 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-Comment-Date: Mon, 13 Dec 2021 15:21:10 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: flashrom.c: Move do_*() helpers into cli_classic.c
by Edward O'Callaghan (Code Review) 13 Dec '21

13 Dec '21

Attention is currently required from: Anastasia Klimchuk. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/60070 ) Change subject: flashrom.c: Move do_*() helpers into cli_classic.c ...................................................................... Patch Set 2: Code-Review-2 (1 comment) File cli_classic.c: https://review.coreboot.org/c/flashrom/+/60070/comment/f99bda8a_4db503d2 PS2, Line 156: : static cli_common is maybe a better place. -- To view, visit https://review.coreboot.org/c/flashrom/+/60070 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: If1112155e2421e0178fd73f847cbb80868387433 Gerrit-Change-Number: 60070 Gerrit-PatchSet: 2 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Mon, 13 Dec 2021 04:43:34 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: build: Remove cli_classic need for internal symbols [WIP]
by Edward O'Callaghan (Code Review) 13 Dec '21

13 Dec '21

Attention is currently required from: Edward O'Callaghan. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/60074 ) Change subject: build: Remove cli_classic need for internal symbols [WIP] ...................................................................... Patch Set 1: Code-Review-2 -- To view, visit https://review.coreboot.org/c/flashrom/+/60074 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ib04293c14f53c8e798c7d3e35483067520668161 Gerrit-Change-Number: 60074 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Comment-Date: Mon, 13 Dec 2021 04:39:25 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: build: Remove cli_classic need for internal symbols [WIP]
by Edward O'Callaghan (Code Review) 13 Dec '21

13 Dec '21

Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/60074 ) Change subject: build: Remove cli_classic need for internal symbols [WIP] ...................................................................... build: Remove cli_classic need for internal symbols [WIP] cli_classic should now only use the libflashrom interface. BUG=b:208132085 TEST=`make && meson/ninja`. Change-Id: Ib04293c14f53c8e798c7d3e35483067520668161 Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M Makefile M meson.build 2 files changed, 2 insertions(+), 4 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/74/60074/1 diff --git a/Makefile b/Makefile index b196d98..ecffcd6 100644 --- a/Makefile +++ b/Makefile @@ -865,8 +865,8 @@ @+$(MAKE) -C util/ich_descriptors_tool/ HOST_OS=$(HOST_OS) TARGET_OS=$(TARGET_OS) endif -$(PROGRAM)$(EXEC_SUFFIX): $(OBJS) - $(CC) $(LDFLAGS) -o $(PROGRAM)$(EXEC_SUFFIX) $(OBJS) $(LIBS) $(PCILIBS) $(FEATURE_LIBS) $(USBLIBS) $(USB1LIBS) $(JAYLINKLIBS) $(NI845X_LIBS) +$(PROGRAM)$(EXEC_SUFFIX): $(CLI_OBJS) libflashrom.a + $(CC) $(LDFLAGS) -o $(PROGRAM)$(EXEC_SUFFIX) $(CLI_OBJS) libflashrom.a $(LIBS) $(PCILIBS) $(FEATURE_LIBS) $(USBLIBS) $(USB1LIBS) $(JAYLINKLIBS) $(NI845X_LIBS) libflashrom.a: $(LIBFLASHROM_OBJS) $(AR) rcs $@ $^ diff --git a/meson.build b/meson.build index 85f40ba..73ce68d 100644 --- a/meson.build +++ b/meson.build @@ -470,7 +470,6 @@ cargs += '-DCONFIG_DEFAULT_PROGRAMMER_ARGS="' + config_default_programmer_args + '"' -# we can't just link_with libflashrom as we require all the internal symbols... executable( 'flashrom', sources : [ @@ -478,7 +477,6 @@ 'cli_classic.c', 'cli_common.c', 'cli_output.c', - 'flashrom.c', ], dependencies : [ deps, -- To view, visit https://review.coreboot.org/c/flashrom/+/60074 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ib04293c14f53c8e798c7d3e35483067520668161 Gerrit-Change-Number: 60074 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: flashrom.c: Move do_*() helpers into cli_classic.c
by Edward O'Callaghan (Code Review) 13 Dec '21

13 Dec '21

Attention is currently required from: Anastasia Klimchuk. Hello build bot (Jenkins), Anastasia Klimchuk, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/60070 to look at the new patch set (#2). Change subject: flashrom.c: Move do_*() helpers into cli_classic.c ...................................................................... flashrom.c: Move do_*() helpers into cli_classic.c These helpers are only used by the CLI logic and so we localise them here to move towards cli_classic being a pure libflashrom user. BUG=b:208132085 TEST=`make` Change-Id: If1112155e2421e0178fd73f847cbb80868387433 Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M cli_classic.c M flash.h M flashrom.c 3 files changed, 98 insertions(+), 102 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/70/60070/2 -- To view, visit https://review.coreboot.org/c/flashrom/+/60070 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: If1112155e2421e0178fd73f847cbb80868387433 Gerrit-Change-Number: 60070 Gerrit-PatchSet: 2 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit December 2021