- flashrom-gerrit - mail.coreboot.org

[S] Change in flashrom[main]: Extract cli_output declarations to a separate header.
by Antonio Vázquez Blanco (Code Review) 21 Nov '24

21 Nov '24

Attention is currently required from: Anastasia Klimchuk. Antonio Vázquez Blanco has posted comments on this change by Antonio Vázquez Blanco. ( https://review.coreboot.org/c/flashrom/+/85134?usp=email ) Change subject: Extract cli_output declarations to a separate header. ...................................................................... Patch Set 1: (2 comments) Patchset: PS1: Confirmation please! :) File include/cli_output.h: https://review.coreboot.org/c/flashrom/+/85134/comment/88635bd4_60a91ae7?us… : PS1, Line 4: * SPDX-FileCopyrightText: 2024 Antonio Vázquez Blanco <antoniovazquezblanco(a)gmail.com> : * SPDX-License-Identifier: GPL-2.0-or-later > I would ask you the same thing as in previous patch: if you can keep our usual license header for th […] Just for clarification, this is a newly added file. This header is not a change with respect with a previously existing copyright text and thus I believe this is not a modification of an original copyright text. That being said, I can copy the copyright text from any other file onto this one. Would you rather I copy the full license text from other file? Thanks! -- To view, visit https://review.coreboot.org/c/flashrom/+/85134?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I4209d5ed205ca14c39e83aa923e103b7282a7059 Gerrit-Change-Number: 85134 Gerrit-PatchSet: 1 Gerrit-Owner: Antonio Vázquez Blanco <antoniovazquezblanco(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Thu, 21 Nov 2024 17:43:49 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>

1 0

[S] Change in flashrom[1.3.x]: libflashrom: Fix comparison of layout entries
by Anastasia Klimchuk (Code Review) 21 Nov '24

21 Nov '24

Anastasia Klimchuk has abandoned this change. ( https://review.coreboot.org/c/flashrom/+/72433?usp=email ) Change subject: libflashrom: Fix comparison of layout entries ...................................................................... Abandoned Follow https://ticket.coreboot.org/issues/570 to track the work on main branch -- To view, visit https://review.coreboot.org/c/flashrom/+/72433?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: abandon Gerrit-Project: flashrom Gerrit-Branch: 1.3.x Gerrit-Change-Id: I125d3892d9efc68e8fc19eef559c82d46c3bdc94 Gerrit-Change-Number: 72433 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>

1 0

[S] Change in flashrom[main]: libflashrom: Fix comparison of layout entries
by Anastasia Klimchuk (Code Review) 21 Nov '24

21 Nov '24

Anastasia Klimchuk has abandoned this change. ( https://review.coreboot.org/c/flashrom/+/85201?usp=email ) Change subject: libflashrom: Fix comparison of layout entries ...................................................................... Abandoned Follow https://ticket.coreboot.org/issues/570 to track the work on main branch -- To view, visit https://review.coreboot.org/c/flashrom/+/85201?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: abandon Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I125d3892d9efc68e8fc19eef559c82d46c3bdc94 Gerrit-Change-Number: 85201 Gerrit-PatchSet: 1 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>

1 0

[S] Change in flashrom[main]: libflashrom: Fix comparison of layout entries
by Anastasia Klimchuk (Code Review) 21 Nov '24

21 Nov '24

Anastasia Klimchuk has posted comments on this change by Anastasia Klimchuk. ( https://review.coreboot.org/c/flashrom/+/85201?usp=email ) Change subject: libflashrom: Fix comparison of layout entries ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: > Cherry-picked from 1.3.x, looks like a reasonable fix but was never reviewed. […] `romentry` struct has changed significantly since the original change based on 1.3.x was created, layout comparison function needs to be re-written. Another question is to also check for other instances of the issue. Work will be tracked under https://ticket.coreboot.org/issues/570 -- To view, visit https://review.coreboot.org/c/flashrom/+/85201?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I125d3892d9efc68e8fc19eef559c82d46c3bdc94 Gerrit-Change-Number: 85201 Gerrit-PatchSet: 1 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Thu, 21 Nov 2024 02:15:07 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>

1 0

[M] Change in flashrom[main]: cli_classic.c: Make -r/-w/-v argument optional when using -i
by Anastasia Klimchuk (Code Review) 21 Nov '24

21 Nov '24

Attention is currently required from: Matt DeVillier. Anastasia Klimchuk has posted comments on this change by Matt DeVillier. ( https://review.coreboot.org/c/flashrom/+/85159?usp=email ) Change subject: cli_classic.c: Make -r/-w/-v argument optional when using -i ...................................................................... Patch Set 3: (6 comments) File cli_classic.c: https://review.coreboot.org/c/flashrom/+/85159/comment/a6830f11_b430c68b?us… : PS2, Line 691: if (read_buf_from_include_args(get_layout(flash), newcontents)) > I don't think so - we check if there is a filename following -r/-w/-v, and if so read the file into […] Yeah you are right. I think I wrote this comment in the beginning of reviewing, and then looked through the rest of the change. Resolving this. https://review.coreboot.org/c/flashrom/+/85159/comment/0814c2e2_8c30607c?us… : PS2, Line 727: if (read_buf_from_include_args(get_layout(flash), newcontents)) > same as above Done https://review.coreboot.org/c/flashrom/+/85159/comment/74384811_8da42d86?us… : PS2, Line 1351: then: : * - Do partial read for each -i arg, creating a new file for : * each region where a filename is provided (-i region:filename). : * - Create a ROM-sized file with partially filled content. For each : * -i arg, fill the corresponding offset with content from ROM. > no, -r is the exception here. Yes, now I see it! Your new formatting of the long comment works, it is more readable now! https://review.coreboot.org/c/flashrom/+/85159/comment/74d33814_fca299df?us… : PS2, Line 1357: Rules for writing and verifying: > I went to re-write this section based on your comments, but feel that it is actually correct as-is. […] Readability accomplished. I only have two tiny comments left (for this comment) File cli_classic.c: https://review.coreboot.org/c/flashrom/+/85159/comment/c83ea5bd_cf7eca3b?us… : PS3, Line 1344: There are few trailing spaces (shown in gerrit in bright pink) in this comment https://review.coreboot.org/c/flashrom/+/85159/comment/99152410_a78aae67?us… : PS3, Line 1366: * - If file is specified for -w/-v and no files are specified with -i : * args, then the file is to be used for writing/verifying the entire : * ROM. Maybe this one can be promoted to common rules? It does apply to all of them -r/-w/-v ? -- To view, visit https://review.coreboot.org/c/flashrom/+/85159?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I6eba095d478f1a7bdbc3854627a656f93dd9e452 Gerrit-Change-Number: 85159 Gerrit-PatchSet: 3 Gerrit-Owner: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-Comment-Date: Thu, 21 Nov 2024 01:37:41 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Matt DeVillier <matt.devillier(a)gmail.com> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>

1 0

[M] Change in flashrom[main]: cli_classic.c: Make -r/-w/-v argument optional when using -i
by Matt DeVillier (Code Review) 20 Nov '24

20 Nov '24

Attention is currently required from: Anastasia Klimchuk. Matt DeVillier has posted comments on this change by Matt DeVillier. ( https://review.coreboot.org/c/flashrom/+/85159?usp=email ) Change subject: cli_classic.c: Make -r/-w/-v argument optional when using -i ...................................................................... Patch Set 2: (4 comments) File cli_classic.c: https://review.coreboot.org/c/flashrom/+/85159/comment/c7ef2d96_8070bda9?us… : PS2, Line 691: if (read_buf_from_include_args(get_layout(flash), newcontents)) > Should it be `else if`? We can't have both, right? I don't think so - we check if there is a filename following -r/-w/-v, and if so read the file into a buffer. Then we check if there is a filename following the region if -i is used, and if so, read that into the same buffer. So the way it works now, the filename following the region takes precedence, which is consistent with existing behavior and the documentation https://review.coreboot.org/c/flashrom/+/85159/comment/ad117f43_ace3a81c?us… : PS2, Line 727: if (read_buf_from_include_args(get_layout(flash), newcontents)) > Same question, should it be `else if`? same as above https://review.coreboot.org/c/flashrom/+/85159/comment/fc14e833_5823a68d?us… : PS2, Line 1351: then: : * - Do partial read for each -i arg, creating a new file for : * each region where a filename is provided (-i region:filename). : * - Create a ROM-sized file with partially filled content. For each : * -i arg, fill the corresponding offset with content from ROM. > ... then filename specified for `-r` is ignored? […] no, -r is the exception here. https://review.coreboot.org/c/flashrom/+/85159/comment/35f8d3c8_f2c34dca?us… : PS2, Line 1357: Rules for writing and verifying: > I know it wasn't you who wrote these Rules initially, but let's make them more readable (it's hard r […] I went to re-write this section based on your comments, but feel that it is actually correct as-is. I reformatted it slightly for readability. -- To view, visit https://review.coreboot.org/c/flashrom/+/85159?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I6eba095d478f1a7bdbc3854627a656f93dd9e452 Gerrit-Change-Number: 85159 Gerrit-PatchSet: 2 Gerrit-Owner: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Wed, 20 Nov 2024 21:50:42 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>

1 0

[M] Change in flashrom[main]: cli_classic.c: Make -r/-w/-v argument optional when using -i
by Matt DeVillier (Code Review) 20 Nov '24

20 Nov '24

Attention is currently required from: Matt DeVillier. Hello Anastasia Klimchuk, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/85159?usp=email to look at the new patch set (#3). The following approvals got outdated and were removed: Verified+1 by build bot (Jenkins) Change subject: cli_classic.c: Make -r/-w/-v argument optional when using -i ...................................................................... cli_classic.c: Make -r/-w/-v argument optional when using -i Make the filename parameter directly following -r/-w/-v optional, since the -i parameter allows the image to be written to be sourced from multiple files, regions to be read from flash and written to separate image files, and regions to be verified using an image file only containing that region. Since the filename parameter following -r/-w/-v was ignored when a filename was specified following `-i <region>:<filename>`, this patch essentially removes the requirement to provide an unused parameter. Based on https://review.coreboot.org/c/flashrom/+/52362. TEST=run the following commands on a supported board: flashrom -p internal -r /tmp/coreboot.rom flashrom -p internal -r --ifd -i bios:/tmp/coreboot.rom flashrom -p internal -r /tmp/coreboot.rom --ifd -i bios:/tmp/bios.bin flashrom -p internal -w /tmp/coreboot.rom flashrom -p internal -w --ifd -i bios:/tmp/coreboot.rom flashrom -p internal -w /tmp/coreboot.rom --ifd -i bios:/tmp/bios.bin flashrom -p internal -v /tmp/coreboot.rom flashrom -p internal -v --ifd -i bios:/tmp/coreboot.rom flashrom -p internal -v /tmp/coreboot.rom --ifd -i bios:/tmp/bios.bin Change-Id: I6eba095d478f1a7bdbc3854627a656f93dd9e452 Signed-off-by: Matt DeVillier <matt.devillier(a)gmail.com> --- M cli_classic.c M doc/classic_cli_manpage.rst M include/layout.h M layout.c 4 files changed, 123 insertions(+), 23 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/59/85159/3 -- To view, visit https://review.coreboot.org/c/flashrom/+/85159?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: newpatchset Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I6eba095d478f1a7bdbc3854627a656f93dd9e452 Gerrit-Change-Number: 85159 Gerrit-PatchSet: 3 Gerrit-Owner: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Matt DeVillier <matt.devillier(a)gmail.com>

1 0

[S] Change in flashrom[main]: libflashrom: Fix comparison of layout entries
by Anastasia Klimchuk (Code Review) 20 Nov '24

20 Nov '24

Anastasia Klimchuk has posted comments on this change by Anastasia Klimchuk. ( https://review.coreboot.org/c/flashrom/+/85201?usp=email ) Change subject: libflashrom: Fix comparison of layout entries ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: Cherry-picked from 1.3.x, looks like a reasonable fix but was never reviewed. Doesn't build at the moment, needs further work. The code path which is modified is never executed in the current tree (which probably explains why it was not noticed). -- To view, visit https://review.coreboot.org/c/flashrom/+/85201?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I125d3892d9efc68e8fc19eef559c82d46c3bdc94 Gerrit-Change-Number: 85201 Gerrit-PatchSet: 1 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Wed, 20 Nov 2024 02:39:22 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No

1 0

[S] Change in flashrom[main]: libflashrom: Fix comparison of layout entries
by Anastasia Klimchuk (Code Review) 20 Nov '24

20 Nov '24

Anastasia Klimchuk has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/85201?usp=email ) Change subject: libflashrom: Fix comparison of layout entries ...................................................................... libflashrom: Fix comparison of layout entries A `next` pointer was added to `struct romentry` in commit 953c5ad44058 (layout: Use linked list for `struct romentry`). Hence, comparing the whole contents to match entries doesn't work anymore. Solve that by comparing the `start`, `end` and `name` fields individually. The introduced layout_cmp() function could be made a new API, but that's not necessary for the release branch. Change-Id: I125d3892d9efc68e8fc19eef559c82d46c3bdc94 Signed-off-by: Nico Huber <nico.h(a)gmx.de> --- M libflashrom.c 1 file changed, 16 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/01/85201/1 diff --git a/libflashrom.c b/libflashrom.c index dfc86e6..3961e24 100644 --- a/libflashrom.c +++ b/libflashrom.c @@ -306,6 +306,21 @@ } } +static int layout_cmp(const struct romentry *const a, const struct romentry *const b) +{ + int ret; + + ret = (int)a->start - (int)b->start; + if (ret) + return ret; + + ret = (int)a->end - (int)b->end; + if (ret) + return ret; + + return strcmp(a->name, b->name); +} + int flashrom_layout_read_from_ifd(struct flashrom_layout **const layout, struct flashctx *const flashctx, const void *const dump, const size_t len) { @@ -343,7 +358,7 @@ const struct romentry *chip_entry = layout_next(chip_layout, NULL); const struct romentry *dump_entry = layout_next(dump_layout, NULL); - while (chip_entry && dump_entry && !memcmp(chip_entry, dump_entry, sizeof(*chip_entry))) { + while (chip_entry && dump_entry && !layout_cmp(chip_entry, dump_entry)) { chip_entry = layout_next(chip_layout, chip_entry); dump_entry = layout_next(dump_layout, dump_entry); } -- To view, visit https://review.coreboot.org/c/flashrom/+/85201?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: newchange Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I125d3892d9efc68e8fc19eef559c82d46c3bdc94 Gerrit-Change-Number: 85201 Gerrit-PatchSet: 1 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>

1 0

[S] Change in flashrom[main]: cli_classic: Add option to use first detected chip if multiple found
by Anastasia Klimchuk (Code Review) 19 Nov '24

19 Nov '24

Attention is currently required from: David Hendricks, Matt DeVillier. Anastasia Klimchuk has posted comments on this change by Matt DeVillier. ( https://review.coreboot.org/c/flashrom/+/85160?usp=email ) Change subject: cli_classic: Add option to use first detected chip if multiple found ...................................................................... Patch Set 2: (2 comments) Patchset: PS2: > Adding a couple comments which are probably out-of-scope and non-blocking, though you should check w […] Thank you David, this is useful! File cli_classic.c: https://review.coreboot.org/c/flashrom/+/85160/comment/44fb6b68_c05d5b07?us… : PS2, Line 133: " --use-first-chip in cases where multiple chips are detected, use the first one found\n" > here's my problem: if you're flashing internally, you likely have no idea what chip is actually on t […] The context is very useful, thank you. This explains more narrowed-down use case that you want to handle: that was meant for internal only. David made a good point about write-protect feature. Out of several models with the same IDs, some of them won't support WP, some others will. It happened a bunch of times before my eyes that we needed to split "joint" chip definition into two, exactly because newer model supports WP, and people want to use it. And older model, with the same ID, does not support WP. For your use case, are you interested at all in WP feature? If yes, I am not sure it would work with the strategy of choosing first matching model always. If no (you don't care about WP), I just thought about one more thing, a different idea :) Please tell me what you think. If most of chips in this context are new enough to support SFDP, would you consider trying SFDP auto-detection first? Specifically, that's adding `-c "SFDP-capable chip"` to command line, so that instead of going through the full list flashrom only tries to talk to chip as a generic "SFDP-capable chip"? It actually prints the info that is returned in SFDP header (maybe in verbose mode), so maybe some info about chip can be obtained this way. I will check later what exactly is printed. Like you said > But the vast majority of my users (ChromeOS devices owners) have no idea where the flash chip is on their device, how to access it, etc but if the info is printed, then maybe no need to access the chip -- To view, visit https://review.coreboot.org/c/flashrom/+/85160?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I0427b1ef80e4eca16623f0fc9119d79f7dd62551 Gerrit-Change-Number: 85160 Gerrit-PatchSet: 2 Gerrit-Owner: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Attention: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-Attention: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Comment-Date: Tue, 19 Nov 2024 06:28:41 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Matt DeVillier <matt.devillier(a)gmail.com> Comment-In-Reply-To: David Hendricks <david.hendricks(a)gmail.com> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit