April 2021 - flashrom-gerrit - mail.coreboot.org

Change in flashrom[master]: layout: Add -i <region>[:<file>] support
by Edward O'Callaghan (Code Review) 20 Apr '21

20 Apr '21

Attention is currently required from: Sam McNally, Nico Huber, Nicola Corna, Jack Rosenthal, Paul Menzel, Stefan Reinauer, David Hendricks, Daniel Campello, Arthur Heymans, Carl-Daniel Hailfinger. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/23021 ) Change subject: layout: Add -i <region>[:<file>] support ...................................................................... Patch Set 34: Code-Review+2 -- 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: 34 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: Jack Rosenthal <jrosenth(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: Sam McNally <sammc(a)google.com> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Sam McNally <sammc(a)google.com> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Nicola Corna <nicola(a)corna.info> Gerrit-Attention: Jack Rosenthal <jrosenth(a)chromium.org> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Attention: Daniel Campello <campello(a)chromium.org> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006(a)gmx.net> Gerrit-Comment-Date: Tue, 20 Apr 2021 02:35:42 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: flashrom.c: allow - as filename for stdin/stdout
by Edward O'Callaghan (Code Review) 20 Apr '21

20 Apr '21

Attention is currently required from: Nico Huber, Daniel Campello, Angel Pons. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52383 ) Change subject: flashrom.c: allow - as filename for stdin/stdout ...................................................................... Patch Set 12: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/52383 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I97889cfdf7ba9a257e182c4ee2b20075cfa58d4d Gerrit-Change-Number: 52383 Gerrit-PatchSet: 12 Gerrit-Owner: Daniel Campello <campello(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Jack Rosenthal <jrosenth(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Daniel Campello <campello(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Tue, 20 Apr 2021 02:33:36 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: cli_classic.c: reorder writeprotect operation processing
by Edward O'Callaghan (Code Review) 20 Apr '21

20 Apr '21

Attention is currently required from: Sam McNally, Daniel Campello. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52530 ) Change subject: cli_classic.c: reorder writeprotect operation processing ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/52530 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I7480d3f947aaaf30093d056226fe0c402763efdc Gerrit-Change-Number: 52530 Gerrit-PatchSet: 1 Gerrit-Owner: Daniel Campello <campello(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Sam McNally <sammc(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Sam McNally <sammc(a)google.com> Gerrit-Attention: Daniel Campello <campello(a)chromium.org> Gerrit-Comment-Date: Tue, 20 Apr 2021 02:32:49 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: cli_classic.c: implement set_wp_region operation
by Edward O'Callaghan (Code Review) 20 Apr '21

20 Apr '21

Attention is currently required from: Sam McNally, Daniel Campello, Nikolai Artemiev. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52531 ) Change subject: cli_classic.c: implement set_wp_region operation ...................................................................... Patch Set 1: Code-Review+2 (1 comment) Patchset: PS1: Hey Nikolai, could you take a look over this please as it overlaps with your writeprotect work. -- To view, visit https://review.coreboot.org/c/flashrom/+/52531 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ibad68a038ab38b9986b0d8b5f5eb6c73b20ef381 Gerrit-Change-Number: 52531 Gerrit-PatchSet: 1 Gerrit-Owner: Daniel Campello <campello(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Sam McNally <sammc(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Sam McNally <sammc(a)google.com> Gerrit-Attention: Daniel Campello <campello(a)chromium.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Tue, 20 Apr 2021 02:32:02 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ft2232_spi.c: Pack WREN and op in one ftdi_write_data() call
by Edward O'Callaghan (Code Review) 20 Apr '21

20 Apr '21

Attention is currently required from: Alan Green, Simon Buhrow, Nico Huber, Samir Ibradžić. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/40477 ) Change subject: ft2232_spi.c: Pack WREN and op in one ftdi_write_data() call ...................................................................... Patch Set 25: (1 comment) File ft2232_spi.c: https://review.coreboot.org/c/flashrom/+/40477/comment/6d22a01e_86bdefc1 PS25, Line 189: ft2232_spi_send_command > There's much repeated code between send_command and send_multicommand. […] Regarding this idea. Probably a suitable intermediate change would be to factor common logic out into a internal static function ft2232_spi_send_command_common() that backs both the concrete callback implementations. Such a refactor definitely seems to be a patch with scope to support its own merits. If it is indeed reasonable for the two callback wrappers to be folded up into common then so be it but the common factorisation and fold step seem to be distinct to me. -- To view, visit https://review.coreboot.org/c/flashrom/+/40477 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie4a07499ec5ef0af23818593f45dc427285a9e8a Gerrit-Change-Number: 40477 Gerrit-PatchSet: 25 Gerrit-Owner: Simon Buhrow Gerrit-Reviewer: Alan Green <avg(a)google.com> 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: Samir Ibradžić <sibradzic(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-CC: Patrick Georgi <pgeorgi(a)google.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Alan Green <avg(a)google.com> Gerrit-Attention: Simon Buhrow Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Samir Ibradžić <sibradzic(a)gmail.com> Gerrit-Comment-Date: Tue, 20 Apr 2021 02:26:44 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Alan Green <avg(a)google.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: flashrom.c: Fix up stale FIXME comment when doit() was removed
by Edward O'Callaghan (Code Review) 20 Apr '21

20 Apr '21

Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/52365 ) Change subject: flashrom.c: Fix up stale FIXME comment when doit() was removed ...................................................................... flashrom.c: Fix up stale FIXME comment when doit() was removed Once upon a time flashrom had a entry point function called doit(). Excise the last mention of it here so that we may never mention it again. BUG=none TEST=none Change-Id: I40d815b7154456c323b4230cd3fed2cc2e8e3641 Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> Reviewed-on: https://review.coreboot.org/c/flashrom/+/52365 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Anastasia Klimchuk <aklm(a)chromium.org> Reviewed-by: Paul Menzel <paulepanter(a)mailbox.org> Reviewed-by: Angel Pons <th3fanbus(a)gmail.com> --- M flashrom.c 1 file changed, 2 insertions(+), 2 deletions(-) Approvals: build bot (Jenkins): Verified Paul Menzel: Looks good to me, but someone else must approve Angel Pons: Looks good to me, approved Anastasia Klimchuk: Looks good to me, but someone else must approve diff --git a/flashrom.c b/flashrom.c index c89abad..056fc57 100644 --- a/flashrom.c +++ b/flashrom.c @@ -2189,8 +2189,8 @@ return ret; } -/* FIXME: This function signature needs to be improved once doit() has a better - * function signature. +/* FIXME: This function signature needs to be improved once prepare_flash_access() + * has a better function signature. */ static int chip_safety_check(const struct flashctx *flash, int force, int read_it, int write_it, int erase_it, int verify_it) -- To view, visit https://review.coreboot.org/c/flashrom/+/52365 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I40d815b7154456c323b4230cd3fed2cc2e8e3641 Gerrit-Change-Number: 52365 Gerrit-PatchSet: 3 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: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

Change in flashrom[master]: flashrom.c: Fix up stale FIXME comment when doit() was removed
by Edward O'Callaghan (Code Review) 20 Apr '21

20 Apr '21

Attention is currently required from: Paul Menzel. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52365 ) Change subject: flashrom.c: Fix up stale FIXME comment when doit() was removed ...................................................................... Patch Set 2: (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/52365/comment/86e8fe8f_36dd39cc PS1, Line 7: Fixup > Nit: The verb is spelled with a space: Fix up Done -- To view, visit https://review.coreboot.org/c/flashrom/+/52365 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I40d815b7154456c323b4230cd3fed2cc2e8e3641 Gerrit-Change-Number: 52365 Gerrit-PatchSet: 2 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: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Comment-Date: Tue, 20 Apr 2021 02:19:34 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: flashrom.c: Fix up stale FIXME comment when doit() was removed
by Edward O'Callaghan (Code Review) 20 Apr '21

20 Apr '21

Attention is currently required from: Edward O'Callaghan. Hello build bot (Jenkins), Paul Menzel, Angel Pons, Anastasia Klimchuk, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/52365 to look at the new patch set (#2). Change subject: flashrom.c: Fix up stale FIXME comment when doit() was removed ...................................................................... flashrom.c: Fix up stale FIXME comment when doit() was removed Once upon a time flashrom had a entry point function called doit(). Excise the last mention of it here so that we may never mention it again. BUG=none TEST=none Change-Id: I40d815b7154456c323b4230cd3fed2cc2e8e3641 Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M flashrom.c 1 file changed, 2 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/65/52365/2 -- To view, visit https://review.coreboot.org/c/flashrom/+/52365 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I40d815b7154456c323b4230cd3fed2cc2e8e3641 Gerrit-Change-Number: 52365 Gerrit-PatchSet: 2 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: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: programmer.h,chipset_enable.c: Make penable fn ptr more descript
by Edward O'Callaghan (Code Review) 20 Apr '21

20 Apr '21

Attention is currently required from: Nico Huber, Angel Pons, Anastasia Klimchuk. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52364 ) Change subject: programmer.h,chipset_enable.c: Make penable fn ptr more descript ...................................................................... Patch Set 2: (3 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/52364/comment/d4b4bb2f_d3ffff53 PS1, Line 7: Make penable cb more descript > It's not a callback anyway. It's a function pointer used for […] Reading a bit too deeply into such a small change are we not, I guess I went a bit far with such detail in the commit message myself. I only said "cb" to try and keep the short line short. I went with 'fn ptr' instead as Nico points out its more semantically correct to say its a function pointer given the control flow. https://review.coreboot.org/c/flashrom/+/52364/comment/49850b8d_c0067490 PS1, Line 15: use > typo use*d* Done File programmer.h: https://review.coreboot.org/c/flashrom/+/52364/comment/3e6539c6_6daceff9 PS1, Line 228: enable_flash_xxx > How about `run` or anything else that doesn't repeat `enable`. I think I much prefer the current pattern as "_xxx" as, in programming 'xxx' is typically recognised as a find-and-replace pattern and thus, it makes it super easy to identify that `enable_flash_sis5511()` is a concrete realisation for an example. Any other name incurs more mental indirection which entirely is the motivation of the change to remove. It is easy for us given that we are familiar with the code however imagine someone who has never looked at Flashrom before, see's this struct, greps the field name or struct identifier. The developer would very quickly conclude the connection with the consistent naming without any more effort than a pattern match. TL;DR the mundane `enable_flash_xxx` identifier leads to both consistent naming and minimal mental indirection with maximal grep friendliness. -- To view, visit https://review.coreboot.org/c/flashrom/+/52364 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I8b3a4ec51ea488c40c20ccafea883f8dea93577d Gerrit-Change-Number: 52364 Gerrit-PatchSet: 2 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-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Tue, 20 Apr 2021 02:17:47 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: programmer.h,chipset_enable.c: Make penable fn ptr more descript
by Edward O'Callaghan (Code Review) 20 Apr '21

20 Apr '21

Attention is currently required from: Edward O'Callaghan, Angel Pons, Anastasia Klimchuk. Hello build bot (Jenkins), Nico Huber, Angel Pons, Anastasia Klimchuk, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/52364 to look at the new patch set (#2). Change subject: programmer.h,chipset_enable.c: Make penable fn ptr more descript ...................................................................... programmer.h,chipset_enable.c: Make penable fn ptr more descript The enable_flash callback function pointer embedded within the penable struct is used by the chipset_enable entry-point to dispatch to the correct concrete implementation. The handles have the canonical form `flash_enable_<chipset-name>()` and therefore `doit()` is hard to comprehend while groking code and seeing to what it refers to. It is also a little more confusing as there used to be another doit() in flashrom once upon a time. BUG=none TEST=builds Change-Id: I8b3a4ec51ea488c40c20ccafea883f8dea93577d Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M chipset_enable.c M programmer.h 2 files changed, 2 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/64/52364/2 -- To view, visit https://review.coreboot.org/c/flashrom/+/52364 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I8b3a4ec51ea488c40c20ccafea883f8dea93577d Gerrit-Change-Number: 52364 Gerrit-PatchSet: 2 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-CC: Paul Menzel <paulepanter(a)mailbox.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

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit April 2021