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
Nico Huber has submitted this change. ( https://review.coreboot.org/c/flashrom/+/54964 )
Change subject: platform: Fix endianness detection for Apple Silicon Macs
......................................................................
platform: Fix endianness detection for Apple Silicon Macs
Building flashrom on Apple Silicon Macs fails with
"Unable to determine endianness" error. It seems that current
endianness detection fails on macOS due to a combination of
three issues:
1. On macOS, neither GCC nor Clang have __ARMEL__ macros used
by architecture-specific detection;
2. Generic detection fails because Apple uses LITTLE_ENDIAN,
BIG_ENDIAN and BYTE_ORDER macros instead of __BYTE_ORDER and
__LITTLE_ENDIAN;
3. In platform.h, __LITTLE_ENDIAN__ and __BIG_ENDIAN__ macros
are checked only for PowerPC architecture.
This error can be fixed by appending __LITTLE_ENDIAN__ and
__BIG_ENDIAN__ to conditions in IS_ARM branch. I've considered
multiple approaches, but this one seems the cleanest to me.
Signed-off-by: Ivan V <root(a)pcm720.me>
Change-Id: Ifdb1523ee2c7023e657cfd7b823b091d5deef513
Reviewed-on: https://review.coreboot.org/c/flashrom/+/54964
Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org>
Reviewed-by: Angel Pons <th3fanbus(a)gmail.com>
Reviewed-by: Nico Huber <nico.h(a)gmx.de>
---
M platform.h
1 file changed, 2 insertions(+), 2 deletions(-)
Approvals:
build bot (Jenkins): Verified
Nico Huber: Looks good to me, approved
Angel Pons: Looks good to me, but someone else must approve
diff --git a/platform.h b/platform.h
index 751957c..04f1ab1 100644
--- a/platform.h
+++ b/platform.h
@@ -116,9 +116,9 @@
#elif IS_ARM
/* ARM can be either endian. */
-#if defined (__ARMEB__)
+#if defined (__ARMEB__) || defined (__BIG_ENDIAN__)
#define __FLASHROM_BIG_ENDIAN__ 1
-#elif defined (__ARMEL__)
+#elif defined (__ARMEL__) || defined (__LITTLE_ENDIAN__)
#define __FLASHROM_LITTLE_ENDIAN__ 1
#endif
--
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-MessageType: merged
Attention is currently required from: Thomas Heijligen, Edward O'Callaghan, Anastasia Klimchuk.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55120 )
Change subject: enum programmer: replace by size_t
......................................................................
Patch Set 4: Code-Review+1
(2 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/55120/comment/5ac54838_6f166d7d
PS4, Line 7: by
nit: replace <old> *with* <new>
https://review.coreboot.org/c/flashrom/+/55120/comment/c02473e3_f924feb1
PS4, Line 7: enum programmer
I see CB:55124 gets rid of it completely.
--
To view, visit https://review.coreboot.org/c/flashrom/+/55120
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Id2ad9630fbc41e98d182768b553788e069fa5095
Gerrit-Change-Number: 55120
Gerrit-PatchSet: 4
Gerrit-Owner: Thomas Heijligen <src(a)posteo.de>
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: 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: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Sun, 06 Jun 2021 14:38:47 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Thomas Heijligen, Edward O'Callaghan, Anastasia Klimchuk.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55119 )
Change subject: flashrom.c: replace enum programmer by struct programmer_entry*
......................................................................
Patch Set 3: Code-Review+2
(2 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/55119/comment/c3f7309d_6a78699e
PS3, Line 7: by
nit: replace <old> *with* <new>
File flashrom.c:
https://review.coreboot.org/c/flashrom/+/55119/comment/e0421671_c7710885
PS3, Line 1570: chat.freenode.net
Reminds me we should update these. Or even drop direct references to specific communication channels, and link to a wiki page instead. But not in this change.
--
To view, visit https://review.coreboot.org/c/flashrom/+/55119
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I478c56354d5f482010bfe1560489700bc889717a
Gerrit-Change-Number: 55119
Gerrit-PatchSet: 3
Gerrit-Owner: Thomas Heijligen <src(a)posteo.de>
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: 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: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Sun, 06 Jun 2021 14:36:47 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Thomas Heijligen.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55123 )
Change subject: default programmer: use programmer name instead of enum for default programmer
......................................................................
Patch Set 5:
(3 comments)
File cli_classic.c:
https://review.coreboot.org/c/flashrom/+/55123/comment/e49e56a8_ee17cafb
PS5, Line 546: #ifdef CONFIG_DEFAULT_PROGRAMMER_NAME
: #define PASTER(x) &programmer_ ## x
: #define EVALUATOR(x) PASTER(x)
: static const struct programmer_entry *default_programmer = EVALUATOR(CONFIG_DEFAULT_PROGRAMMER_NAME);
: #else
: static const struct programmer_entry *default_programmer = NULL;
: #endif
:
This should be much easier to handle outside of C (i.e. Makefile / meson.build). Just prepend "&programmer_" when passing it to `-D`?
https://review.coreboot.org/c/flashrom/+/55123/comment/172c5a90_1d3a658e
PS5, Line 554: #ifndef CONFIG_DEFAULT_PROGRAMMER_ARGS
: #define CONFIG_DEFAULT_PROGRAMMER_ARGS ""
: #endif
This too should be covered outside C. How did it work before?
File util/manibuilder/README.md:
https://review.coreboot.org/c/flashrom/+/55123/comment/13643d6d_04a76007
PS5, Line 66: cc -MMD -Os -Wall -Wshadow -Werror -I/usr/include/libusb-1.0 -D'CONFIG_SERPROG=1' -D'CONFIG_PONY_SPI=1' -D'CONFIG_BITBANG_SPI=1' -D'CONFIG_GFXNVIDIA=1' -D'CONFIG_SATASII=1' -D'CONFIG_ATAVIA=1' -D'CONFIG_IT8212=1' -D'CONFIG_FT2232_SPI=1' -D'CONFIG_USBBLASTER_SPI=1' -D'CONFIG_PICKIT2_SPI=1' -D'HAVE_FT232H=1' -D'CONFIG_DUMMY=1' -D'CONFIG_DRKAISER=1' -D'CONFIG_NICINTEL=1' -D'CONFIG_NICINTEL_SPI=1' -D'CONFIG_NICINTEL_EEPROM=1' -D'CONFIG_OGP_SPI=1' -D'CONFIG_BUSPIRATE_SPI=1' -D'CONFIG_DEDIPROG=1' -D'CONFIG_DEVELOPERBOX_SPI=1' -D'CONFIG_LINUX_MTD=1' -D'CONFIG_LINUX_SPI=1' -D'CONFIG_CH341A_SPI=1' -D'CONFIG_DIGILENT_SPI=1' -D'NEED_PCI=1' -D'NEED_RAW_ACCESS=1' -D'NEED_LIBUSB0=1' -D'NEED_LIBUSB1=1' -D'HAVE_UTSNAME=1' -D'HAVE_CLOCK_GETTIME=1' -D'FLASHROM_VERSION="p1.0-141-g9cecc7e"' -o libflashrom.o -c libflashrom.c
It's just an example and should be kept as is. The list of programmers
is also not kept up-to-date ;)
--
To view, visit https://review.coreboot.org/c/flashrom/+/55123
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I976447787c6f6bfbdc0145d80d61e1ddcf97ac33
Gerrit-Change-Number: 55123
Gerrit-PatchSet: 5
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: 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: Sun, 06 Jun 2021 12:38:02 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment