- flashrom-gerrit - mail.coreboot.org

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

19 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 2: (13 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/85159/comment/8ab82ee6_0022e36e?us… : PS1, Line 7: Make -r/-w/-v argument optional > Ack. […] Done Commit Message: https://review.coreboot.org/c/flashrom/+/85159/comment/9f4b04e4_0a86b02c?us… : PS2, Line 16: this patch : essentially removes the requirement to provide an unused parameter Oh I really like this part. And thank you for updating existing examples in the manpage (which were only mentioning write, but now you fixed it). https://review.coreboot.org/c/flashrom/+/85159/comment/9931eee1_5648c8f5?us… : PS2, Line 21: TEST=verify read/write/verify operations successful when specifying : the filename only after the region. As a note, you can just copy-paste the list of commands that you ran to test, might be easier than writing a text about what you have done. But more importantly, if you could ran few tests without `-i` option, just a standard read, write, erase: - correct examples - and also with missing file parameter, this should fail to validate the command line syntax (fail before starting the operation) Running with dummyflasher is totally fine File cli_classic.c: https://review.coreboot.org/c/flashrom/+/85159/comment/d141bd1e_5d43c02f?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? https://review.coreboot.org/c/flashrom/+/85159/comment/a56b13a4_7c4665f5?us… : PS2, Line 727: if (read_buf_from_include_args(get_layout(flash), newcontents)) Same question, should it be `else if`? https://review.coreboot.org/c/flashrom/+/85159/comment/1de64943_4e02cc8a?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? The rest should be explained in preceding paragraph already, right? https://review.coreboot.org/c/flashrom/+/85159/comment/c0ced021_396befe0?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 right now). Specific points: 1) Rules for reading, writing , verifying should have the same structure (currently not) 2) Make it as a list of all possible combinations: - no filename specified -> error - filename is specified for -r/-w/-v only - filename is specified for -i only -> explain what happens (which is already written here I think, just scattered) - filename is specified for both -r/-w/-v and -i -> -i takes priority, filename in -r/-w/-v is ignored. It is likely that after writing this, you will copy-paste to manpage (in addition to what's there). So it's very useful to write clear text, and then manpage will be a self-service for users! File doc/classic_cli_manpage.rst: https://review.coreboot.org/c/flashrom/+/85159/comment/36e547a3_de1cb468?us… : PS2, Line 57: file Let's do the same formatting as in the first sentence, `**<file>**` https://review.coreboot.org/c/flashrom/+/85159/comment/7f979c7e_767aa92b?us… : PS2, Line 70: file Let's do the same formatting as in the first sentence, `**<file>**` https://review.coreboot.org/c/flashrom/+/85159/comment/77fa166a_f9dccbf1?us… : PS2, Line 100: file Let's do the same formatting as in the first sentence, `**<file>**` https://review.coreboot.org/c/flashrom/+/85159/comment/fc9a6889_efed68d3?us… : PS2, Line 206: <file> same here, same formatting `**<file>**` File include/layout.h: https://review.coreboot.org/c/flashrom/+/85159/comment/ba4ff03c_3f0bc5d5?us… : PS2, Line 70: int register_include_arg(struct layout_include_args **, const char *arg); : int process_include_args(struct flashrom_layout *, const struct layout_include_args *); : int check_include_args_filename(const struct layout_include_args *); : void cleanup_include_args(struct layout_include_args **); If you are in the good mood, maybe you would consider adding few tests for these functions? We have some tests already for functions in layout.c, in tests/layout.c, it would be great to cover some of include args logic too. I thought you might be interested in that processing of include args has tests and protected from breakage ;) Doesn't need to be in this patch, can be a separate patch, any time later. Either way I would be so so happy to get few more tests! Thank you! File layout.c: https://review.coreboot.org/c/flashrom/+/85159/comment/c385b3f4_fee26836?us… : PS2, Line 296: Error: No region file specified Maybe we can give more details, so that there no confusion for the users where the error comes from: > Error: No region file specified for the --include/-i command. -- 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: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-Comment-Date: Tue, 19 Nov 2024 05:49:12 +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

[S] Change in flashrom[main]: Extract cli_output declarations to a separate header.
by Anastasia Klimchuk (Code Review) 18 Nov '24

18 Nov '24

Attention is currently required from: Antonio Vázquez Blanco. Anastasia Klimchuk 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: (1 comment) File include/cli_output.h: https://review.coreboot.org/c/flashrom/+/85134/comment/a85830fa_cc420984?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 this new file (add you copyright line of course), for this patch? You can create a separate commit on top of this (if you want), just for SPDX lines. Then this patch can go faster and not need to wait for license questions. Thank you! -- 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: Antonio Vázquez Blanco <antoniovazquezblanco(a)gmail.com> Gerrit-Comment-Date: Mon, 18 Nov 2024 09:32:36 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No

1 0

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

18 Nov '24

Attention is currently required from: Anastasia Klimchuk, Matt DeVillier. David Hendricks 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: (1 comment) Patchset: PS2: Adding a couple comments which are probably out-of-scope and non-blocking, though you should check with flashrom's current maintainers: - I've run into issues where write-protection capabilities are different with evil twins. So you might want to bail out if a write-protect command is used or add logic to compare write-protect capabilities with chips sharing the same ID. - Some chips share an ID but use a different voltage. This is probably fine with internal programmers since the programmer's IO pins and and chip ought to match in this regard, but may pose an issue with external programmers such as Dediprog SF100/SF600/etc. - Read, write, and erase opcodes are probably the same unless there's something weird going on with QE or 4BA enable/exit for a particular family of chips. I'm not sure if this is a real issue now, though it might be good to check those modes or feature bits. -- 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: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Mon, 18 Nov 2024 08:34:35 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No

1 0

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

18 Nov '24

Attention is currently required from: Anastasia Klimchuk. 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 (#2). 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=verify read/write/verify operations successful when specifying the filename only after the region. 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, 115 insertions(+), 23 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/59/85159/2 -- 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: 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>

1 0

[L] Change in flashrom[main]: doc: Release notes for v1.5.0
by Anastasia Klimchuk (Code Review) 17 Nov '24

17 Nov '24

Anastasia Klimchuk has posted comments on this change by Anastasia Klimchuk. ( https://review.coreboot.org/c/flashrom/+/85156?usp=email ) The change is no longer submittable: All-Comments-Resolved is unsatisfied now. Change subject: doc: Release notes for v1.5.0 ...................................................................... Patch Set 2: (1 comment) Patchset: PS2: This will be submitted right before changing VERSION to v1.5.0 (ETA 6th Dec) -- To view, visit https://review.coreboot.org/c/flashrom/+/85156?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: I0663779020e84cd6d89d33f23a7b5514f8efa2f4 Gerrit-Change-Number: 85156 Gerrit-PatchSet: 2 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Sun, 17 Nov 2024 23:55:14 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No

1 0

[L] Change in flashrom[main]: doc: Release notes for v1.5.0
by Peter Marheine (Code Review) 17 Nov '24

17 Nov '24

Attention is currently required from: Anastasia Klimchuk. Peter Marheine has posted comments on this change by Anastasia Klimchuk. ( https://review.coreboot.org/c/flashrom/+/85156?usp=email ) Change subject: doc: Release notes for v1.5.0 ...................................................................... Patch Set 2: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/85156?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: I0663779020e84cd6d89d33f23a7b5514f8efa2f4 Gerrit-Change-Number: 85156 Gerrit-PatchSet: 2 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Sun, 17 Nov 2024 22:33:35 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes

1 0

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

16 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 ...................................................................... Patch Set 1: (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/85159/comment/7798b7fe_625a6023?us… : PS1, Line 7: Make -r/-w/-v argument optional > It seems they are not fully optional, they are only optional when `-i` parameter is provided? Maybe […] Ack. Will address these in the next patch set -- 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: 1 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: Sat, 16 Nov 2024 19:38:33 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: 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 Matt DeVillier (Code Review) 16 Nov '24

16 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/+/85160?usp=email ) Change subject: cli_classic: Add option to use first detected chip if multiple found ...................................................................... Patch Set 2: (1 comment) File cli_classic.c: https://review.coreboot.org/c/flashrom/+/85160/comment/5fbbc702_f1882400?us… : PS2, Line 133: " --use-first-chip in cases where multiple chips are detected, use the first one found\n" > This is potentially dangerous option: because same IDs can be not only between the chips in the same […] here's my problem: if you're flashing internally, you likely have no idea what chip is actually on the board. So when presented with 2 or more options and told to choose, how are you supposed to make that determination? With external flashing you can easily inspect the chip, so not a problem. 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 and I don't have a good way to help them resolve the question. I don't know that we need to make the name scary. And if anything, I'd argue those overrides you used as examples are poorly named since they don't make it obvious what function they serve. -- 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-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Sat, 16 Nov 2024 19:37:52 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: 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) 16 Nov '24

16 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/+/85160?usp=email ) Change subject: cli_classic: Add option to use first detected chip if multiple found ...................................................................... Patch Set 2: (1 comment) Patchset: PS2: Forgot to say: on the topic of multiple chips with the same ID, I hope to be able to do more next year to have SFDP auto-detection for most chips. With that, multiple IDs won't be such a pressing issue anymore. But, this is just a plan at the moment. Also old chips might not support the protocol. -- 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-Attention: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-Comment-Date: Sat, 16 Nov 2024 12:45:05 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No

1 0

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

16 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 ...................................................................... Patch Set 1: (2 comments) Patchset: PS1: Matt, thank you so much for your work! I added one comment already, but I will do another round of review later. Commit Message: https://review.coreboot.org/c/flashrom/+/85159/comment/553b68b4_b5913e22?us… : PS1, Line 7: Make -r/-w/-v argument optional It seems they are not fully optional, they are only optional when `-i` parameter is provided? Maybe it's better to say "optional with -i parameter provided", something like that. However, the critical important place where it should be explained is not the commit message, but the manpage, you need to update doc/classic_cli_manpage.rst to fully explain the usage. In this case, also very useful would be to give examples (full command line examples) of a valid usages with this feature. -- 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: 1 Gerrit-Owner: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-Comment-Date: Sat, 16 Nov 2024 12:16:12 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit