December 2023 - flashrom-gerrit

[S] Change in flashrom[main]: buspirate_spi: return init error for invalid values of pullups, hiz, ...
by David Reguera Garcia (Code Review) 07 Dec '23

07 Dec '23

David Reguera Garcia has uploaded a new patch set (#2). ( https://review.coreboot.org/c/flashrom/+/79407?usp=email ) Change subject: buspirate_spi: return init error for invalid values of pullups, hiz, psus ...................................................................... buspirate_spi: return init error for invalid values of pullups, hiz, psus Previously, invalid values of the params pullups, hiz, psus were handled as "off" (i.e. disabled). This created a potential human error when users were running flashrom with e.g. pullups=1 and expected pullups to be on, while the value 1 was handled as invalid and off. Change-Id: I8944598b266fd77ee041f0ab03724655714a3344 Signed-off-by: David Reguera Garcia <dreg(a)rootkit.es> --- M buspirate_spi.c 1 file changed, 15 insertions(+), 6 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/07/79407/2 -- To view, visit https://review.coreboot.org/c/flashrom/+/79407?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I8944598b266fd77ee041f0ab03724655714a3344 Gerrit-Change-Number: 79407 Gerrit-PatchSet: 2 Gerrit-Owner: David Reguera Garcia <dreg(a)rootkit.es> Gerrit-MessageType: newpatchset

1 0

[S] Change in flashrom[main]: buspirate_spi: Add support for hiz output with pullups=off
by Anastasia Klimchuk (Code Review) 07 Dec '23

07 Dec '23

Attention is currently required from: David Reguera Garcia, Thomas Heijligen. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/79299?usp=email ) Change subject: buspirate_spi: Add support for hiz output with pullups=off ...................................................................... Patch Set 7: (6 comments) Patchset: PS6: > Therefore, 'pullups=on' inherently works as an open collector. Thus, setting both 'pullups=on' and 'hiz=on' is redundant but not incorrect. My main idea was: if this is a valid combination, it would be great to document it. Is it correct to say that if `pullups=on` then the value of `hiz` doesn't matter? From the user point of view, is there any difference between running with: 1) pullups=on (hiz not set) 2) pullups=on,hiz=on 3) pullups=on,hiz=off I think it would be useful to add one more sentence to the man page, clarifying what happens when various combinations of two params are used. If any of 1,2,3 is invalid combinations, you can catch that and return, as in the other patch. Patchset: PS7: David, thank you so much for your patches! I definitely want them both to get submitted. I went through the diffs in more detail and added a few comments. This is a very normal process of code review, you would typically get a few comments, fix/respond to them, and upload new patchset to the same patch. If all is good, all comments resolved, the patch will be approved. File buspirate_spi.c: https://review.coreboot.org/c/flashrom/+/79299/comment/7a94f6d6_c3364bd1 : PS7, Line 709: (Open-Drain mode) Is this always true? If both params are set, `pullup == true && hiz == true`, from you explanation I understand that will be open collector? I just want to make sure the message is correct. This branch is executed if `pullup || hiz` which includes the cases one of the params true, or they both true. File doc/classic_cli_manpage.rst: https://review.coreboot.org/c/flashrom/+/79299/comment/2ccf5c95_7e04852a : PS7, Line 766: trailing whitespace, but you don't need a new line here, you can make new line after the end of sentence `resistors.`) https://review.coreboot.org/c/flashrom/+/79299/comment/6c7e6899_f5b4116d : PS7, Line 765: : When working with low-voltage chips, the internal 10k pull-ups of the Bus Pirate might be too high. Could you swap this new paragraph about `hiz` with existing about `pullups` param? It seems more logical, first there will be information on `pullups` param, and then further explanation for the case `pullups` param is not enough and you need to use `hiz`. https://review.coreboot.org/c/flashrom/+/79299/comment/d681e339_4ed2da4a : PS7, Line 767: trailing whitespace -- To view, visit https://review.coreboot.org/c/flashrom/+/79299?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I6d9909b23782ef3fcb1428fd127dae0d6b211855 Gerrit-Change-Number: 79299 Gerrit-PatchSet: 7 Gerrit-Owner: David Reguera Garcia <dreg(a)rootkit.es> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: David Reguera Garcia <dreg(a)rootkit.es> 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: David Reguera Garcia <dreg(a)rootkit.es> Gerrit-Comment-Date: Thu, 07 Dec 2023 01:23:00 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: David Reguera Garcia <dreg(a)rootkit.es> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

[S] Change in flashrom[main]: Return init error for invalid values of pullups, hiz, psus
by David Reguera Garcia (Code Review) 07 Dec '23

07 Dec '23

David Reguera Garcia has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/79407?usp=email ) Change subject: Return init error for invalid values of pullups, hiz, psus ...................................................................... Return init error for invalid values of pullups, hiz, psus Previously, invalid values of the params pullups, hiz, psus were handled as "off" (i.e. disabled). This created a potential human error when users were running flashrom with e.g. pullups=1 and expected pullups to be on, while the value 1 was handled as invalid and off. Change-Id: I8944598b266fd77ee041f0ab03724655714a3344 Signed-off-by: David Reguera Garcia <dreg(a)rootkit.es> --- M buspirate_spi.c 1 file changed, 15 insertions(+), 6 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/07/79407/1 diff --git a/buspirate_spi.c b/buspirate_spi.c index 7436d80..bcc2964 100644 --- a/buspirate_spi.c +++ b/buspirate_spi.c @@ -375,8 +375,11 @@ hiz = true; else if (strcasecmp("off", tmp) == 0) ; // ignore - else - msg_perr("Invalid hiz state, not using them.\n"); + else { + msg_perr("Invalid hiz state. Use on/off.\n"); + free(tmp); + return 1; + } } free(tmp); @@ -386,8 +389,11 @@ pullup = true; else if (strcasecmp("off", tmp) == 0) ; // ignore - else - msg_perr("Invalid pullups state, not using them.\n"); + else { + msg_perr("Invalid pullups state. Use on/off.\n"); + free(tmp); + return 1; + } } free(tmp); @@ -397,8 +403,11 @@ psu = true; else if (strcasecmp("off", tmp) == 0) ; // ignore - else - msg_perr("Invalid psus state, not enabling.\n"); + else { + msg_perr("Invalid psus state. Use on/off.\n"); + free(tmp); + return 1; + } } free(tmp); -- To view, visit https://review.coreboot.org/c/flashrom/+/79407?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I8944598b266fd77ee041f0ab03724655714a3344 Gerrit-Change-Number: 79407 Gerrit-PatchSet: 1 Gerrit-Owner: David Reguera Garcia <dreg(a)rootkit.es> Gerrit-MessageType: newchange

1 0

[S] Change in flashrom[main]: buspirate_spi: Stop execution when invalid params
by Anastasia Klimchuk (Code Review) 07 Dec '23

07 Dec '23

Attention is currently required from: David Reguera Garcia. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/79405?usp=email ) Change subject: buspirate_spi: Stop execution when invalid params ...................................................................... Patch Set 1: (3 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/79405/comment/3ba96206_92280869 : PS1, Line 7: Stop execution when invalid params Return init error for invalid values of pullups, hiz, psus https://review.coreboot.org/c/flashrom/+/79405/comment/ff7636da_68ef2884 : PS1, Line 8: Maybe you can add a little clarifying paragraph here in commit message, for example: > Previously, invalid values of the params pullups, hiz, psus were handled as "off" (i.e. disabled). This created a potential human error when users were running flashrom with e.g. `pullups=1` and expected pullups to be on, while the value `1` was handled as invalid and off. File buspirate_spi.c: https://review.coreboot.org/c/flashrom/+/79405/comment/e7e1a03a_cc598b9d : PS1, Line 380: return You need to `free(tmp)` before returning (and same for two other places below) -- To view, visit https://review.coreboot.org/c/flashrom/+/79405?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: Icce40fa841bb3289db4a4895785bb19dfb625e5f Gerrit-Change-Number: 79405 Gerrit-PatchSet: 1 Gerrit-Owner: David Reguera Garcia <dreg(a)rootkit.es> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: David Reguera Garcia <dreg(a)rootkit.es> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: David Reguera Garcia <dreg(a)rootkit.es> Gerrit-Comment-Date: Thu, 07 Dec 2023 00:52:25 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[S] Change in flashrom[main]: buspirate_spi: Stop execution when invalid params
by David Reguera Garcia (Code Review) 06 Dec '23

06 Dec '23

Attention is currently required from: Anastasia Klimchuk. David Reguera Garcia has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/79405?usp=email ) Change subject: buspirate_spi: Stop execution when invalid params ...................................................................... Patch Set 1: Code-Review+1 -- To view, visit https://review.coreboot.org/c/flashrom/+/79405?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: Icce40fa841bb3289db4a4895785bb19dfb625e5f Gerrit-Change-Number: 79405 Gerrit-PatchSet: 1 Gerrit-Owner: David Reguera Garcia <dreg(a)rootkit.es> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: David Reguera Garcia <dreg(a)rootkit.es> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Wed, 06 Dec 2023 19:30:00 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in flashrom[main]: buspirate_spi: Stop execution when invalid params
by David Reguera Garcia (Code Review) 06 Dec '23

06 Dec '23

David Reguera Garcia has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/79405?usp=email ) Change subject: buspirate_spi: Stop execution when invalid params ...................................................................... buspirate_spi: Stop execution when invalid params Change-Id: Icce40fa841bb3289db4a4895785bb19dfb625e5f Signed-off-by: David Reguera Garcia <dreg(a)rootkit.es> --- M buspirate_spi.c 1 file changed, 12 insertions(+), 6 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/05/79405/1 diff --git a/buspirate_spi.c b/buspirate_spi.c index 7436d80..e09cea5 100644 --- a/buspirate_spi.c +++ b/buspirate_spi.c @@ -375,8 +375,10 @@ hiz = true; else if (strcasecmp("off", tmp) == 0) ; // ignore - else - msg_perr("Invalid hiz state, not using them.\n"); + else { + msg_perr("Invalid hiz state. Use on/off.\n"); + return 1; + } } free(tmp); @@ -386,8 +388,10 @@ pullup = true; else if (strcasecmp("off", tmp) == 0) ; // ignore - else - msg_perr("Invalid pullups state, not using them.\n"); + else { + msg_perr("Invalid pullups state. Use on/off.\n"); + return 1; + } } free(tmp); @@ -397,8 +401,10 @@ psu = true; else if (strcasecmp("off", tmp) == 0) ; // ignore - else - msg_perr("Invalid psus state, not enabling.\n"); + else { + msg_perr("Invalid psus state. Use on/off.\n"); + return 1; + } } free(tmp); -- To view, visit https://review.coreboot.org/c/flashrom/+/79405?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: Icce40fa841bb3289db4a4895785bb19dfb625e5f Gerrit-Change-Number: 79405 Gerrit-PatchSet: 1 Gerrit-Owner: David Reguera Garcia <dreg(a)rootkit.es> Gerrit-MessageType: newchange

1 0

[S] Change in flashrom[main]: buspirate_spi: Add support for hiz output with pullups=off
by Dreg (Code Review) 06 Dec '23

06 Dec '23

Attention is currently required from: Anastasia Klimchuk, Thomas Heijligen. Dreg has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/79299?usp=email ) Change subject: buspirate_spi: Add support for hiz output with pullups=off ...................................................................... Patch Set 7: Code-Review+1 (1 comment) Patchset: PS6: > Hello! good points, […] I've prepared another code to make the use of psus, pullups and hi-z mode safer. Can you merge this and I'll open another PR with the changes? To me, they are different issues. Thx Anastasia -- To view, visit https://review.coreboot.org/c/flashrom/+/79299?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I6d9909b23782ef3fcb1428fd127dae0d6b211855 Gerrit-Change-Number: 79299 Gerrit-PatchSet: 7 Gerrit-Owner: Dreg <dreg(a)rootkit.es> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Dreg <dreg(a)rootkit.es> 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: Wed, 06 Dec 2023 19:10:01 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Dreg <dreg(a)rootkit.es> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

[XS] Change in flashrom[main]: fmap: Update major/minor version check
by Paul Menzel (Code Review) 06 Dec '23

06 Dec '23

Attention is currently required from: Arthur Heymans, Brian Norris, Edward O'Callaghan. Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/79060?usp=email ) Change subject: fmap: Update major/minor version check ...................................................................... Patch Set 2: (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/79060/comment/38b4d504_90285e03 : PS2, Line 7: Update Maybe more specific: > Drop minor version check in version check -- To view, visit https://review.coreboot.org/c/flashrom/+/79060?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I984835579d3b257a2462906f1f5091b179891bd0 Gerrit-Change-Number: 79060 Gerrit-PatchSet: 2 Gerrit-Owner: Brian Norris <briannorris(a)chromium.org> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Hsuan-ting Chen <roccochen(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Hsuan Ting Chen <roccochen(a)chromium.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Brian Norris <briannorris(a)chromium.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Comment-Date: Wed, 06 Dec 2023 11:08:23 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[XS] Change in flashrom[main]: fmap: Update major/minor version check
by Hsuan-ting Chen (Code Review) 06 Dec '23

06 Dec '23

Attention is currently required from: Arthur Heymans, Brian Norris, Edward O'Callaghan. Hsuan-ting Chen has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/79060?usp=email ) Change subject: fmap: Update major/minor version check ...................................................................... Patch Set 2: Code-Review+1 -- To view, visit https://review.coreboot.org/c/flashrom/+/79060?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I984835579d3b257a2462906f1f5091b179891bd0 Gerrit-Change-Number: 79060 Gerrit-PatchSet: 2 Gerrit-Owner: Brian Norris <briannorris(a)chromium.org> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Hsuan-ting Chen <roccochen(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Hsuan Ting Chen <roccochen(a)chromium.org> Gerrit-Attention: Brian Norris <briannorris(a)chromium.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Comment-Date: Wed, 06 Dec 2023 10:33:52 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[XS] Change in flashrom[main]: MAINTAINERS: Add Hsuan Ting Chen for flashrom_tester
by Anastasia Klimchuk (Code Review) 05 Dec '23

05 Dec '23

Attention is currently required from: Hsuan-ting Chen. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/79319?usp=email ) Change subject: MAINTAINERS: Add Hsuan Ting Chen for flashrom_tester ...................................................................... Patch Set 2: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/79319?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I71139401b5f2c7c1b4e76ba67ff774e030a343aa Gerrit-Change-Number: 79319 Gerrit-PatchSet: 2 Gerrit-Owner: Hsuan-ting Chen <roccochen(a)google.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Evan Benn <evanbenn(a)gmail.com> Gerrit-Attention: Hsuan-ting Chen <roccochen(a)google.com> Gerrit-Comment-Date: Tue, 05 Dec 2023 06:13:19 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit December 2023