November 2024 - flashrom-gerrit

[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

[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: (2 comments) Patchset: PS2: Comment to the patch in general: 1) this needs update of the manpage, the source is at doc/classic_cli_manpage.rst This source will generate manpage, and also a webpage https://flashrom.org/classic_cli_manpage.html We have doc how to update docs: https://flashrom.org/how_to_add_docs.html 2) this needs to be added to the doc of Recent Development doc/release_notes/devel.rst Now is the special time we prepare new release, so devel.rst is being modified in CB:85156 Given that you have another patch, which also changed command line options, perhaps you can do a separate patch with both features explained? But keep in mind there is CB:85156 in flight File cli_classic.c: https://review.coreboot.org/c/flashrom/+/85160/comment/09288e46_0b7c71b5?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 family. It could be same IDs for the chips in unrelated families, and even from different vendors. This feature can result in flashrom thinking it is processing an entirely different chip, not the one it actually is. Usually in such situation we want to protect the users from accidentally using the feature. You can never fully protect, but at least to some extent. Basically, we want to only the people who know exactly what they are doing, to use the feature. The approach used in the similar cases in the past was: 1) make default option safe (you did this already) 2) make the option name very scary, so that no one uses it out of curiosity Existing examples of 2) are options such as `allow_brick` and `force_I_want_a_brick` This comment is about naming: we need to come up a name that's scary enough. I will be thinking about it on the background, but maybe you can think too :) If you have ideas, please tell! -- 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:05:00 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit November 2024