September 2022 - flashrom-gerrit

[S] Change in flashrom[master]: internal.c: Retype appropriate variables with bool
by Angel Pons (Code Review) 07 Sep '22

07 Sep '22

Attention is currently required from: Felix Singer. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/66870 ) Change subject: internal.c: Retype appropriate variables with bool ...................................................................... Patch Set 13: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/66870 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I159d789112d7a778744b59b45133df3928b8445e Gerrit-Change-Number: 66870 Gerrit-PatchSet: 13 Gerrit-Owner: Felix Singer <felixsinger(a)posteo.net> 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: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Comment-Date: Wed, 07 Sep 2022 10:11:34 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: buspirate_spi.c: Retype appropriate variables with bool
by Angel Pons (Code Review) 07 Sep '22

07 Sep '22

Attention is currently required from: Felix Singer. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/66868 ) Change subject: buspirate_spi.c: Retype appropriate variables with bool ...................................................................... Patch Set 11: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/66868 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I245616168796f2c7fe99388688b0f606bd3405bf Gerrit-Change-Number: 66868 Gerrit-PatchSet: 11 Gerrit-Owner: Felix Singer <felixsinger(a)posteo.net> 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: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Comment-Date: Wed, 07 Sep 2022 10:10:48 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: ichspi.c: Retype appropriate variables with bool
by Nico Huber (Code Review) 07 Sep '22

07 Sep '22

Attention is currently required from: Felix Singer, Angel Pons. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/66892 ) Change subject: ichspi.c: Retype appropriate variables with bool ...................................................................... Patch Set 13: (1 comment) File ichspi.c: https://review.coreboot.org/c/flashrom/+/66892/comment/2234ca13_49ef27e5 PS7, Line 872: write_cmd = true; > I like the idea, but this doesn't block the submission of the patches right? I assume if anyone wanted to block anything, they'd have removed the resolved tag. -- To view, visit https://review.coreboot.org/c/flashrom/+/66892 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: If7eeacc44921f52aa593ab1302f17a5c5190f830 Gerrit-Change-Number: 66892 Gerrit-PatchSet: 13 Gerrit-Owner: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Wed, 07 Sep 2022 10:03:50 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Felix Singer <felixsinger(a)posteo.net> 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

[S] Change in flashrom[master]: ichspi.c: Retype appropriate variables with bool
by Felix Singer (Code Review) 07 Sep '22

07 Sep '22

Attention is currently required from: Nico Huber, Angel Pons. Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/66892 ) Change subject: ichspi.c: Retype appropriate variables with bool ...................................................................... Patch Set 13: (1 comment) File ichspi.c: https://review.coreboot.org/c/flashrom/+/66892/comment/21531ba2_b81624a8 PS7, Line 872: write_cmd = true; > OK, I have an idea: I'll try to review the retyping patches and I'll comment on variables that shoul […] I like the idea, but this doesn't block the submission of the patches right? -- To view, visit https://review.coreboot.org/c/flashrom/+/66892 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: If7eeacc44921f52aa593ab1302f17a5c5190f830 Gerrit-Change-Number: 66892 Gerrit-PatchSet: 13 Gerrit-Owner: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Wed, 07 Sep 2022 09:16:48 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Felix Singer <felixsinger(a)posteo.net> 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

[S] Change in flashrom[master]: ichspi.c: Retype appropriate variables with bool
by Angel Pons (Code Review) 07 Sep '22

07 Sep '22

Attention is currently required from: Felix Singer, Nico Huber. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/66892 ) Change subject: ichspi.c: Retype appropriate variables with bool ...................................................................... Patch Set 13: (1 comment) File ichspi.c: https://review.coreboot.org/c/flashrom/+/66892/comment/ace6cef8_697255ee PS7, Line 872: write_cmd = true; > As a reviewer who went through the whole chain, I can say IMO: […] OK, I have an idea: I'll try to review the retyping patches and I'll comment on variables that should be renamed. Then, you can reply to my comments with a CB: number linking to the change that does the rename. -- To view, visit https://review.coreboot.org/c/flashrom/+/66892 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: If7eeacc44921f52aa593ab1302f17a5c5190f830 Gerrit-Change-Number: 66892 Gerrit-PatchSet: 13 Gerrit-Owner: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Comment-Date: Wed, 07 Sep 2022 08:06:29 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Felix Singer <felixsinger(a)posteo.net> 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

[S] Change in flashrom[master]: ichspi.c: Retype appropriate variables with bool
by Anastasia Klimchuk (Code Review) 07 Sep '22

07 Sep '22

Attention is currently required from: Felix Singer, Nico Huber, Angel Pons. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/66892 ) Change subject: ichspi.c: Retype appropriate variables with bool ...................................................................... Patch Set 13: Code-Review+2 (1 comment) File ichspi.c: https://review.coreboot.org/c/flashrom/+/66892/comment/584edf2a_648f7087 PS7, Line 872: write_cmd = true; > Thanks Angel for your comment! You summarized the situation very good. […] As a reviewer who went through the whole chain, I can say IMO: 1) It was easier to review as a chain of smaller patches (as it is now) VS one gigantic patch. I appreciate patch owner did that work. Thank you! 2) My understanding that changing the type int -> bool changes the binary, so commits are not reproducible, doesn't matter whether changes are split or combined. 3) Renaming only comes up for this one file (two variables in here), not for every patch in the chain. I read this comment on my first round of review and payed attention to variables names. I didn't notice anything obvious to rename. Which means, in this specific situation, renaming in a separate patch does not mean twice more patches, it means one more patch. 4) In addition to 1), focusing the chain on one thing, "retyping to bool" makes it easier to review. 5) Changing this one patch now to do two things (retype and rename) unfortunately will create more work for patch owner and reviewers, because this version is already rebased, reviewed and tested. The best thing: everyone has the same end goal in mind: "bool variables with meaningful names" ! :) As I said earlier, I agree with renaming `is_write_command`, it's a good idea. -- To view, visit https://review.coreboot.org/c/flashrom/+/66892 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: If7eeacc44921f52aa593ab1302f17a5c5190f830 Gerrit-Change-Number: 66892 Gerrit-PatchSet: 13 Gerrit-Owner: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Wed, 07 Sep 2022 05:19:47 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Felix Singer <felixsinger(a)posteo.net> 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

[S] Change in flashrom[master]: libflashrom: Use size_t for all range types
by Evan Benn (Code Review) 07 Sep '22

07 Sep '22

Evan Benn has abandoned this change. ( https://review.coreboot.org/c/flashrom/+/65944 ) Change subject: libflashrom: Use size_t for all range types ...................................................................... Abandoned https://review.coreboot.org/c/flashrom/+/67004 -- To view, visit https://review.coreboot.org/c/flashrom/+/65944 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I8593b7a47f084cde3d57111c744deb70606597f4 Gerrit-Change-Number: 65944 Gerrit-PatchSet: 1 Gerrit-Owner: Evan Benn <evanbenn(a)google.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: abandon

1 0

[S] Change in flashrom[master]: libflashrom: Unify wp_range size types
by Evan Benn (Code Review) 07 Sep '22

07 Sep '22

Evan Benn has abandoned this change. ( https://review.coreboot.org/c/flashrom/+/64996 ) Change subject: libflashrom: Unify wp_range size types ...................................................................... Abandoned https://review.coreboot.org/c/flashrom/+/67004 -- To view, visit https://review.coreboot.org/c/flashrom/+/64996 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I22dcab538f196b0ea77afd4a246279f924af6193 Gerrit-Change-Number: 64996 Gerrit-PatchSet: 2 Gerrit-Owner: Evan Benn <evanbenn(a)google.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-MessageType: abandon

1 0

[M] Change in flashrom[master]: Use chipsize_t (uint32_t) for all range types
by Evan Benn (Code Review) 07 Sep '22

07 Sep '22

Attention is currently required from: Nico Huber, Edward O'Callaghan, Anastasia Klimchuk. Evan Benn has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67004 ) Change subject: Use chipsize_t (uint32_t) for all range types ...................................................................... Patch Set 3: (1 comment) File flashrom.c: https://review.coreboot.org/c/flashrom/+/67004/comment/d5cb7453_8aabb6bd PS3, Line 852: const chipsize_t region_start = entry->start; > I removed chipoff_t during this migration, as I did not understand the use case for having two types […] I should say I dont mind adding it back or keeping chipoff_t instead of chipsize_t, but I would want to understand what the purpose is before doing it. -- To view, visit https://review.coreboot.org/c/flashrom/+/67004 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I31348806b7ea92a6ea8391a735f722a63aa7e488 Gerrit-Change-Number: 67004 Gerrit-PatchSet: 3 Gerrit-Owner: Evan Benn <evanbenn(a)google.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Wed, 07 Sep 2022 05:14:48 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Evan Benn <evanbenn(a)google.com> Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: Use chipsize_t (uint32_t) for all range types
by Evan Benn (Code Review) 07 Sep '22

07 Sep '22

Attention is currently required from: Nico Huber, Edward O'Callaghan, Anastasia Klimchuk. Hello build bot (Jenkins), Nico Huber, Edward O'Callaghan, Anastasia Klimchuk, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/67004 to look at the new patch set (#4). Change subject: Use chipsize_t (uint32_t) for all range types ...................................................................... Use chipsize_t (uint32_t) for all range types Change the API of libflashrom and the internal implementation of layout and write protect to use chipsize_t (uint32_t) for all values representing offsets into or ranges in the flash. BUG=b:245453390 BRANCH=None TEST=flashrom_tester on arm32 and amd64 TEST=no new int truncation / size warnings Change-Id: I31348806b7ea92a6ea8391a735f722a63aa7e488 Signed-off-by: Evan Benn <evanbenn(a)chromium.org> --- M bindings/rust/libflashrom/src/lib.rs M cli_classic.c M flashrom.c M ich_descriptors.c M include/layout.h M include/libflashrom.h M layout.c M libflashrom.c M tests/chip_wp.c M util/flashrom_tester/flashrom/src/flashromlib.rs 10 files changed, 122 insertions(+), 91 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/04/67004/4 -- To view, visit https://review.coreboot.org/c/flashrom/+/67004 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I31348806b7ea92a6ea8391a735f722a63aa7e488 Gerrit-Change-Number: 67004 Gerrit-PatchSet: 4 Gerrit-Owner: Evan Benn <evanbenn(a)google.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit September 2022