Attention is currently required from: Thomas Heijligen.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55118 )
Change subject: programmer_table: print.c use fixed fixed name instead of reference
......................................................................
Patch Set 3:
(1 comment)
Commit Message:
https://review.coreboot.org/c/flashrom/+/55118/comment/fbe3d2ff_15b61317
PS3, Line 7: programmer_table: print.c use fixed fixed name instead of reference
How about `print.c: Use static-string in print`
--
To view, visit https://review.coreboot.org/c/flashrom/+/55118
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: If2cf95c71425efdd864457e213dd34b929fe8805
Gerrit-Change-Number: 55118
Gerrit-PatchSet: 3
Gerrit-Owner: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: Xiang W <wxjstz(a)126.com>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Comment-Date: Mon, 07 Jun 2021 04:18:11 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Thomas Heijligen.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55117 )
Change subject: programmer_table: replace PROGRAMMER_INVALID to programmer_table_size in itterations
......................................................................
Patch Set 3: Code-Review+1
(1 comment)
Commit Message:
https://review.coreboot.org/c/flashrom/+/55117/comment/c62af569_934ef5d4
PS3, Line 7: programmer_table: replace PROGRAMMER_INVALID to programmer_table_size in itterations
> It's just too long, how about […]
could this just be squashed into the previous patch that introduced it earlier in the serires? Seems reasonable to have it introduced and used in the same patch.
--
To view, visit https://review.coreboot.org/c/flashrom/+/55117
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Icaaeefe001de604df9d7fdd06f05a5ed39fdbd84
Gerrit-Change-Number: 55117
Gerrit-PatchSet: 3
Gerrit-Owner: Thomas Heijligen <src(a)posteo.de>
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-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: Xiang W <wxjstz(a)126.com>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Comment-Date: Mon, 07 Jun 2021 04:17:00 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Gerrit-MessageType: comment
Attention is currently required from: Thomas Heijligen.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52946 )
Change subject: programmer_table: move each entry to the associated programmer source
......................................................................
Patch Set 4: Code-Review+1
--
To view, visit https://review.coreboot.org/c/flashrom/+/52946
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I3d02bd789f0299e936eb86819b3b15b5ea2bb921
Gerrit-Change-Number: 52946
Gerrit-PatchSet: 4
Gerrit-Owner: Thomas Heijligen <src(a)posteo.de>
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-CC: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: Xiang W <wxjstz(a)126.com>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Comment-Date: Mon, 07 Jun 2021 04:15:17 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Thomas Heijligen.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52944 )
Change subject: programmer_table: move array content to an own file
......................................................................
Patch Set 3: Code-Review+1
(1 comment)
Patchset:
PS3:
I look forward to this landing. Thanks for all the work Thomas!
--
To view, visit https://review.coreboot.org/c/flashrom/+/52944
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I8e6d704e845ee4152c8676dd19dff0934fff007b
Gerrit-Change-Number: 52944
Gerrit-PatchSet: 3
Gerrit-Owner: Thomas Heijligen <src(a)posteo.de>
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-CC: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: Xiang W <wxjstz(a)126.com>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Comment-Date: Mon, 07 Jun 2021 04:13:20 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/54748 )
Change subject: dummyflasher.c: Fix data leak in params processing error paths
......................................................................
Patch Set 5:
(4 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/54748/comment/f27ff257_c8034a9a
PS4, Line 11: A good side-effect of the change is: 13 error paths of params
: processing are not leaking data anymore. All those error paths
: return 1 back to init function which frees data.
:
: And there was just one more error path in init function were free(data)
: needed to be added.
> These are the actual changes. Everything else is a mere refactoring. […]
You are back!! This is awesome! :)
Done, I updated commit message.
https://review.coreboot.org/c/flashrom/+/54748/comment/01fbda58_c934e882
PS4, Line 20:
> Could add a […]
I haven't seen this tag before, is it used often? I saw people mention in commit message "follow-up something something that was introduced in CB:xxxxx" or similar. Can I do like this instead?
File dummyflasher.c:
https://review.coreboot.org/c/flashrom/+/54748/comment/e16fcb36_8bdb1c01
PS1, Line 26: /* Remove the #define below if you don't want SPI flash chip emulation. */
: #define EMULATE_SPI_CHIP 1
> IMO, both defines should just be dropped. If they were compile-time configs […]
Just to confirm, you mean, let's assume (EMULATE_CHIP && EMULATE_SPI_CHIP) always true and drop the code outside the #if? So that dummy will always be dummy_spi? But then maybe have another one dummy_par? Maybe someone needs those different values.
I don't know what was the initial meaning of !EMULATE_CHIP use case. What dummy is doing if it is not emulating any chip? I see it can emulate spi or par, I can understand, but not emulating anything?
File dummyflasher.c:
https://review.coreboot.org/c/flashrom/+/54748/comment/e8f94f97_caade0bd
PS4, Line 994: dummy_buses_supported
> Makes one wonder why this is not part of `emu_data`...
I was thinking about it, and my guess was, because it is only needed for init time, and not needed later. It would never be used as a part of emu_data.
Yes adding second parameter in init_data felt slightly awkward, but adding into emu_data and then never using it would also be...
--
To view, visit https://review.coreboot.org/c/flashrom/+/54748
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I04f55f77bb4703f1d88b2191c45a22be3c97bf87
Gerrit-Change-Number: 54748
Gerrit-PatchSet: 5
Gerrit-Owner: 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: Namyoon Woo <namyoon(a)google.com>
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: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Comment-Date: Mon, 07 Jun 2021 02:02:28 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Angel Pons, Anastasia Klimchuk.
Hello build bot (Jenkins), Nico Huber, Namyoon Woo, Edward O'Callaghan, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/54748
to look at the new patch set (#5).
Change subject: dummyflasher.c: Fix data leak in params processing error paths
......................................................................
dummyflasher.c: Fix data leak in params processing error paths
This patch extracts params processing into a separate function. Now
all error paths of params processing return 1 back to init function
which frees data.
And there was just one more error path in init function were free(data)
needed to be added.
This is a follow up on CB:45230 which moves global state into
spi_master data.
A good side-effect of the change is: init function becomes easier
to read.
BUG=b:185191942
TEST=ninja test
Change-Id: I04f55f77bb4703f1d88b2191c45a22be3c97bf87
Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org>
---
M dummyflasher.c
1 file changed, 51 insertions(+), 27 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/48/54748/5
--
To view, visit https://review.coreboot.org/c/flashrom/+/54748
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I04f55f77bb4703f1d88b2191c45a22be3c97bf87
Gerrit-Change-Number: 54748
Gerrit-PatchSet: 5
Gerrit-Owner: 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: Namyoon Woo <namyoon(a)google.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: newpatchset
Ivan V has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/54964 )
Change subject: platform: Fix endianness detection for Apple Silicon Macs
......................................................................
Patch Set 3:
(1 comment)
Patchset:
PS3:
> Btw. what is working for you yet on these Macs? if anything...
I was able to successfully (and repeatedly) read and write W25X40 SPI flash with CH341A-based programmer on macOS 11.4. Unfortunately, I don't have anything else to test with.
--
To view, visit https://review.coreboot.org/c/flashrom/+/54964
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ifdb1523ee2c7023e657cfd7b823b091d5deef513
Gerrit-Change-Number: 54964
Gerrit-PatchSet: 3
Gerrit-Owner: Ivan V <root(a)pcm720.me>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Comment-Date: Sun, 06 Jun 2021 17:55:40 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Gerrit-MessageType: comment
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/54964 )
Change subject: platform: Fix endianness detection for Apple Silicon Macs
......................................................................
Patch Set 3:
(1 comment)
Patchset:
PS3:
Btw. what is working for you yet on these Macs? if anything...
--
To view, visit https://review.coreboot.org/c/flashrom/+/54964
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ifdb1523ee2c7023e657cfd7b823b091d5deef513
Gerrit-Change-Number: 54964
Gerrit-PatchSet: 3
Gerrit-Owner: Ivan V <root(a)pcm720.me>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Comment-Date: Sun, 06 Jun 2021 14:49:36 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment