September 2022 - flashrom-gerrit

[S] Change in flashrom[master]: ichspi: Add Intel Idaville HCC support
by Kevin Yu (Code Review) 13 Sep '22

13 Sep '22

Attention is currently required from: Thomas Heijligen, Anastasia Klimchuk. Hello Felix Singer, build bot (Jenkins), Thomas Heijligen, Angel Pons, Anastasia Klimchuk, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/67438 to look at the new patch set (#4). Change subject: ichspi: Add Intel Idaville HCC support ...................................................................... ichspi: Add Intel Idaville HCC support Add Cedar Fork PCH device IDs to enable flashrom on Idaville HCC platforms. TEST=read/write BIOS on Intel Morocity CRB Board. Signed-off-by: Kevin Yu <ykevin(a)celestica.com> Change-Id: I3d73b4796f610953d38506794eb20791871ec31f --- M chipset_enable.c 1 file changed, 16 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/38/67438/4 -- To view, visit https://review.coreboot.org/c/flashrom/+/67438 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I3d73b4796f610953d38506794eb20791871ec31f Gerrit-Change-Number: 67438 Gerrit-PatchSet: 4 Gerrit-Owner: Kevin Yu <ykevin(a)celestica.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

[S] Change in flashrom[master]: ichspi: Add Intel Idaville HCC support
by Kevin Yu (Code Review) 13 Sep '22

13 Sep '22

Attention is currently required from: Thomas Heijligen, Anastasia Klimchuk. Kevin Yu has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67438 ) Change subject: ichspi: Add Intel Idaville HCC support ...................................................................... Patch Set 3: (1 comment) Patchset: PS3: Hi Our platform is Idaville,in this spec :"The Intel® Converged Security and Management Engine IOMMU Hardware Issue – CVE-2019-0090 and CVE-2020-0566" ,and its corresponding PCH/SOC is CDF. -- To view, visit https://review.coreboot.org/c/flashrom/+/67438 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I3d73b4796f610953d38506794eb20791871ec31f Gerrit-Change-Number: 67438 Gerrit-PatchSet: 3 Gerrit-Owner: Kevin Yu <ykevin(a)celestica.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Tue, 13 Sep 2022 03:15:32 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: flashrom_tester: Fix cargo check and clippy warnings
by Peter Marheine (Code Review) 13 Sep '22

13 Sep '22

Attention is currently required from: Evan Benn. Peter Marheine has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67472 ) Change subject: flashrom_tester: Fix cargo check and clippy warnings ...................................................................... Patch Set 3: Code-Review+1 -- To view, visit https://review.coreboot.org/c/flashrom/+/67472 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I50c5af61e06df1bb6956f347cb6806a7eca6ce0e Gerrit-Change-Number: 67472 Gerrit-PatchSet: 3 Gerrit-Owner: Evan Benn <evanbenn(a)google.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Evan Benn <evanbenn(a)google.com> Gerrit-Comment-Date: Tue, 13 Sep 2022 01:06:32 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: spi: Make 'default_spi_send_multicommand' the default unless defined
by Peter Marheine (Code Review) 13 Sep '22

13 Sep '22

Attention is currently required from: Thomas Heijligen, Edward O'Callaghan. Peter Marheine has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67481 ) Change subject: spi: Make 'default_spi_send_multicommand' the default unless defined ...................................................................... Patch Set 1: Code-Review+1 -- To view, visit https://review.coreboot.org/c/flashrom/+/67481 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I6cc24bf982da3d5251d391eb397db43dd10280e8 Gerrit-Change-Number: 67481 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Comment-Date: Tue, 13 Sep 2022 01:05:28 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: spi: Make 'default_spi_write_aai' the default unless defined
by Peter Marheine (Code Review) 13 Sep '22

13 Sep '22

Attention is currently required from: Thomas Heijligen, Edward O'Callaghan. Peter Marheine has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67479 ) Change subject: spi: Make 'default_spi_write_aai' the default unless defined ...................................................................... Patch Set 1: Code-Review+1 -- To view, visit https://review.coreboot.org/c/flashrom/+/67479 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I7f14aaea0edcf0c08cea0e9cd27d58152707fb2a Gerrit-Change-Number: 67479 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Comment-Date: Tue, 13 Sep 2022 01:05:08 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: tests: Add workaround to allow tests mock fileno on FreeBSD
by Peter Marheine (Code Review) 13 Sep '22

13 Sep '22

Attention is currently required from: Edward O'Callaghan, Anastasia Klimchuk. Peter Marheine has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67312 ) Change subject: tests: Add workaround to allow tests mock fileno on FreeBSD ...................................................................... Patch Set 6: Code-Review+1 -- To view, visit https://review.coreboot.org/c/flashrom/+/67312 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I3789ea9107a4cf8033cf59bb96d3c82aa58de026 Gerrit-Change-Number: 67312 Gerrit-PatchSet: 6 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-CC: Idwer Vollering <vidwer(a)gmail.com> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Tue, 13 Sep 2022 01:04:36 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: tree/: Move programmer_delay() out of programmer state machine
by Edward O'Callaghan (Code Review) 13 Sep '22

13 Sep '22

Attention is currently required from: Felix Singer, Nico Huber. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67393 ) Change subject: tree/: Move programmer_delay() out of programmer state machine ...................................................................... Patch Set 4: (2 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/67393/comment/973a6532_9fa9229f PS4, Line 13: > I'm missing the reason for the change. Especially because some […] I am sorry I cannot follow your mind absolutely here as it goes off on too many speculative directions however I will have a crack at it; From what I could understand from the above is the following; * Reason for the change - make flashrom reentrant on the `programmer` handle because `programmer_delay()` does not need to be a closure upon `programmer` anymore. * Something regressed but it is unclear what you are actually stating is regressing? * Programmers that have multiple masters need redundant code but this seems speculative as that obviously isn't happening here. You seem to be alluding to dummyflasher as perhaps a example or ichspi as perhaps another. Next paragraph I could not parse, clearly `programmer` global problem got fixed in the context of `programmer_delay()` here. I guess this paragraph is perhaps speculating beyond the scope of this patch? Final paragraph seems to be the same theme of talking well beyond the scope of the patch to resolve design defects at the libflashrom entry-point and how it relates to core flashrom structures that carry the programmer state. While co-related to this patch certainly, that topic is vastly beyond the scope of resolving the very specific sub-problem of `programmer_delay()` in the overall `programmer` singleton pattern issue. Thus, probably more a suited as a topic for a bug and/or mailing list post if you see another control flow structure that can be derived. By looking at this patch and the subsequent patches you will notice the underlying theme of using a declarative+procedural mindset over a OOP one in addressing the issue and ultimately that would be my interpretation of how to resolve the above issue. File flashrom.c: https://review.coreboot.org/c/flashrom/+/67393/comment/7ee2a7b2_0da90163 PS4, Line 224: flash->mst->par.delay(usecs); > Actually, if there will be common information to store, we could […] This did occur to me however given the current structure this isn't easy as `&mst` isn't so trivially accessible within the context of a driver implementation as drivers register into `&mst->{par,spi,opaque}`. Therefore it was more pragmatic to go in this direction first instead of trying to force the code into ivory towers of structures that currently are ill suited to the current control flow even if they do indeed conceptually make more sense. -- To view, visit https://review.coreboot.org/c/flashrom/+/67393 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Id059abb58b31a066a408009073912da2b224d40c Gerrit-Change-Number: 67393 Gerrit-PatchSet: 4 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Alexander Goncharov <chat(a)joursoir.net> Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Comment-Date: Tue, 13 Sep 2022 00:04:31 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: drivers/: Make 'internal_delay' the default unless defined
by Edward O'Callaghan (Code Review) 12 Sep '22

12 Sep '22

Attention is currently required from: Nico Huber, Angel Pons, Nikolai Artemiev. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67391 ) Change subject: drivers/: Make 'internal_delay' the default unless defined ...................................................................... Patch Set 3: (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/67391/comment/be07fd24_c6b55b55 PS3, Line 13: and paves way : to remove the 'programmer' global handle. > I can't connect the dots either. […] Because 42 programmers had to dispatch `internal_delay()` via deference though `programmer->delay()` and now the dispatch to `internal_delay()` is direct under the condition of nullity. Therefore this decreases the overall cyclomatic complexity of the control flow graph of the `programmer` global handle. -- To view, visit https://review.coreboot.org/c/flashrom/+/67391 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I17460bc2c0aebcbb48c8dfa052b260991525cc49 Gerrit-Change-Number: 67391 Gerrit-PatchSet: 3 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Mon, 12 Sep 2022 23:42:06 +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> Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: flashrom.c: Remove programmer_param global state
by Edward O'Callaghan (Code Review) 12 Sep '22

12 Sep '22

Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/66659 ) ( 13 is the latest approved patch-set. No files were changed between the latest approved patch-set and the submitted one. )Change subject: flashrom.c: Remove programmer_param global state ...................................................................... flashrom.c: Remove programmer_param global state By leveraging the programmer_cfg state machine passed into programmer init() entry-points we may now directly pass programmer parameterisation values and thus rid ourseleves of the singleton pattern around programmer_param. Change-Id: I778609e370e44ad2b63b8baa4984ac03ff4124d8 Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> Reviewed-on: https://review.coreboot.org/c/flashrom/+/66659 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Anastasia Klimchuk <aklm(a)chromium.org> --- M flashrom.c 1 file changed, 45 insertions(+), 25 deletions(-) Approvals: build bot (Jenkins): Verified Anastasia Klimchuk: Looks good to me, approved diff --git a/flashrom.c b/flashrom.c index b68ba55..ec16d04 100644 --- a/flashrom.c +++ b/flashrom.c @@ -38,7 +38,6 @@ const char *chip_to_probe = NULL; static const struct programmer_entry *programmer = NULL; -static const char *programmer_param = NULL; /* * Programmers supporting multiple buses can have differing size limits on @@ -126,10 +125,28 @@ return rc; } +static int get_param_residue(const char *prog_params, int ret) +{ + if (prog_params && strlen(prog_params)) { + if (ret != 0) { + /* It is quite possible that any unhandled programmer parameter would have been valid, + * but an error in actual programmer init happened before the parameter was evaluated. + */ + msg_pwarn("Unhandled programmer parameters (possibly due to another failure): %s\n", prog_params); + } else { + /* Actual programmer init was successful, but the user specified an invalid or unusable + * (for the current programmer configuration) parameter. + */ + msg_perr("Unhandled programmer parameters: %s\n", prog_params); + msg_perr("Aborting.\n"); + ret = ERROR_FATAL; + } + } + return ret; +} + int programmer_init(const struct programmer_entry *prog, const char *param) { - int ret; - if (prog == NULL) { msg_perr("Invalid programmer specified!\n"); return -1; @@ -150,25 +167,11 @@ /* Default to allowing writes. Broken programmers set this to 0. */ programmer_may_write = true; - programmer_param = param; - msg_pdbg("Initializing %s programmer\n", programmer->name); - ret = programmer->init(NULL); - if (programmer_param && strlen(programmer_param)) { - if (ret != 0) { - /* It is quite possible that any unhandled programmer parameter would have been valid, - * but an error in actual programmer init happened before the parameter was evaluated. - */ - msg_pwarn("Unhandled programmer parameters (possibly due to another failure): %s\n", - programmer_param); - } else { - /* Actual programmer init was successful, but the user specified an invalid or unusable - * (for the current programmer configuration) parameter. - */ - msg_perr("Unhandled programmer parameters: %s\n", programmer_param); - msg_perr("Aborting.\n"); - ret = ERROR_FATAL; - } - } + msg_pdbg("Initializing %s programmer\n", prog->name); + const struct programmer_cfg cfg = { .params = strdup(param) }; + int ret = prog->init(&cfg); + ret = get_param_residue(cfg.params, ret); + free(cfg.params); return ret; } @@ -188,7 +191,6 @@ ret |= shutdown_fn[i].func(shutdown_fn[i].data); } - programmer_param = NULL; registered_master_count = 0; return ret; @@ -227,7 +229,7 @@ * needle and remove everything from the first occurrence of needle to the next * delimiter from haystack. */ -static char *extract_param(const char *const *haystack, const char *needle, const char *delim) +static char *extract_param(char *const *haystack, const char *needle, const char *delim) { char *param_pos, *opt_pos, *rest; char *opt = NULL; @@ -285,7 +287,7 @@ char *extract_programmer_param_str(const struct programmer_cfg *cfg, const char *param_name) { - return extract_param(&programmer_param, param_name, ","); + return extract_param(&cfg->params, param_name, ","); } static int check_block_eraser(const struct flashctx *flash, int k, int log) -- To view, visit https://review.coreboot.org/c/flashrom/+/66659 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I778609e370e44ad2b63b8baa4984ac03ff4124d8 Gerrit-Change-Number: 66659 Gerrit-PatchSet: 15 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: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

[L] Change in flashrom[master]: flashrom.c: Add wrapper function to use the erase algorithm
by Aarya (Code Review) 12 Sep '22

12 Sep '22

Attention is currently required from: Simon Buhrow, Nico Huber, Thomas Heijligen, Paul Menzel. Aarya has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/66104 ) Change subject: flashrom.c: Add wrapper function to use the erase algorithm ...................................................................... Patch Set 52: (1 comment) File flashrom.c: https://review.coreboot.org/c/flashrom/+/66104/comment/4f7bf6ac_09a7534d PS52, Line 1057: flashctx->chip->page_size > `flashctx->chip->gran` […] gran is just an enum, so I don't think it is needed here -- To view, visit https://review.coreboot.org/c/flashrom/+/66104 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I29e3f2bd796759794184b125741a5abaac6f3ce8 Gerrit-Change-Number: 66104 Gerrit-PatchSet: 52 Gerrit-Owner: Aarya <aarya.chaumal(a)gmail.com> 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-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Simon Buhrow Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Comment-Date: Mon, 12 Sep 2022 19:44:10 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Thomas Heijligen <src(a)posteo.de> Gerrit-MessageType: comment

1 0

2025

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit September 2022