April 2022 - flashrom-gerrit - mail.coreboot.org

Change in flashrom[master]: Endian conversion: move to platform.h and platform/endian*.c
by Nico Huber (Code Review) 11 Apr '22

11 Apr '22

Attention is currently required from: Thomas Heijligen, Angel Pons, Anastasia Klimchuk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62899 ) Change subject: Endian conversion: move to platform.h and platform/endian*.c ...................................................................... Patch Set 8: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/62899 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I35640282d451960f2a329ae24339ec05dbae6d30 Gerrit-Change-Number: 62899 Gerrit-PatchSet: 8 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: 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: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Mon, 11 Apr 2022 09:32:43 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: hwaccess: replace macros by C code
by Nico Huber (Code Review) 11 Apr '22

11 Apr '22

Attention is currently required from: Thomas Heijligen, Edward O'Callaghan, Angel Pons, Anastasia Klimchuk, Nikolai Artemiev. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62898 ) Change subject: hwaccess: replace macros by C code ...................................................................... Patch Set 8: Code-Review+2 (2 comments) File hwaccess.h: https://review.coreboot.org/c/flashrom/+/62898/comment/d776532d_1036d3bb PS8, Line 27: /* swap bytes */ : static inline uint8_t swap8(const uint8_t value) : { : return (value & (uint8_t)0xffU); : } : : static inline uint16_t swap16(const uint16_t value) : { : return ((value & (uint16_t)0x00ffU) << 8) | : ((value & (uint16_t)0xff00U) >> 8); : } : : static inline uint32_t swap32(const uint32_t value) : { : return ((value & (uint32_t)0x000000ffUL) << 24) | : ((value & (uint32_t)0x0000ff00UL) << 8) | : ((value & (uint32_t)0x00ff0000UL) >> 8) | : ((value & (uint32_t)0xff000000UL) >> 24); : } : : static inline uint64_t swap64(const uint64_t value) : { : return ((value & (uint64_t)0x00000000000000ffULL) << 56) | : ((value & (uint64_t)0x000000000000ff00ULL) << 40) | : ((value & (uint64_t)0x0000000000ff0000ULL) << 24) | : ((value & (uint64_t)0x00000000ff000000ULL) << 8) | : ((value & (uint64_t)0x000000ff00000000ULL) >> 8) | : ((value & (uint64_t)0x0000ff0000000000ULL) >> 24) | : ((value & (uint64_t)0x00ff000000000000ULL) >> 40) | : ((value & (uint64_t)0xff00000000000000ULL) >> 56); : } : In case somebody wants to, we could get rid of the spurious parentheses, casting and L's in a follow-up. File hwaccess.h: https://review.coreboot.org/c/flashrom/+/62898/comment/2e0db042_153529ef PS2, Line 84: #define be_to_cpu8 cpu_to_be8 : #define be_to_cpu16 cpu_to_be16 : #define be_to_cpu32 cpu_to_be32 : #define be_to_cpu64 cpu_to_be64 : #define le_to_cpu8 cpu_to_le8 : #define le_to_cpu16 cpu_to_le16 : #define le_to_cpu32 cpu_to_le32 : #define le_to_cpu64 cpu_to_le64 : > The duplications in the .c files are replaced by macros. […] Ack -- To view, visit https://review.coreboot.org/c/flashrom/+/62898 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I86d38d816b37c283279c485fac8027f8fb94364a Gerrit-Change-Number: 62898 Gerrit-PatchSet: 8 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: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Martin Roth <martinroth(a)google.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Mon, 11 Apr 2022 09:16:55 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Thomas Heijligen <src(a)posteo.de> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: Makefile: build shared usb code only when needed by programmer
by Thomas Heijligen (Code Review) 11 Apr '22

11 Apr '22

Attention is currently required from: Nico Huber, Edward O'Callaghan. Thomas Heijligen has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62832 ) Change subject: Makefile: build shared usb code only when needed by programmer ...................................................................... Patch Set 2: (1 comment) Patchset: PS1: > While I can see the reasoning for this and it makes sense I have a supplementary argument to Nico's […] This patch changes only the reason why the usb_device.c and usbdev.c are build. There are no changes from the compiler or linkers view. The functions in usb_device.c or usbdev.c are part of the programmer and just outsourced for reuse in other programmers. Therefore it should be build as part of the programmer and not as part of the dependency. -- To view, visit https://review.coreboot.org/c/flashrom/+/62832 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I9a91c5801a573e7021686a183a1bd874122e35cc Gerrit-Change-Number: 62832 Gerrit-PatchSet: 2 Gerrit-Owner: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> 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-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Comment-Date: Mon, 11 Apr 2022 08:42:15 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Thomas Heijligen <src(a)posteo.de> Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: internal.c: Fix map_flash() failures with linux_mtd via internal
by Edward O'Callaghan (Code Review) 11 Apr '22

11 Apr '22

Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/63543 ) Change subject: internal.c: Fix map_flash() failures with linux_mtd via internal ...................................................................... internal.c: Fix map_flash() failures with linux_mtd via internal prepare_flash_access() calls into map_flash() however this can fail on some DUT's. The issue is that map_flash() can fail on opaque spi masters such as linux_mtd and even ichspi when they are dispatched into via 'internal'. The issue with 'internal' is that the hook 'map_flash_region = physmap' is used from the internal struct entry-point instance and not the native one for the underlying master. This fails on some board topologies such Volteer. Structually this is not easy to solve with how internal is currently implemented where it partially replaces itself with the underlying master and so this fix only addresses the case of linux_mtd. BUG=b:171093672 TEST=builds Change-Id: Ia32ff1357b73166cdcec8a62ab5c516d1d1527f5 Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M internal.c 1 file changed, 19 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/43/63543/1 diff --git a/internal.c b/internal.c index 4049ef1..57b6ed6 100644 --- a/internal.c +++ b/internal.c @@ -188,6 +188,8 @@ return 0; } +static bool is_phys_master = true; + static int internal_init(void) { int ret = 0; @@ -213,6 +215,7 @@ internal_buses_supported = BUS_NONSPI; if (try_mtd() == 0) { + is_phys_master = false; ret = 0; goto internal_init_exit; } @@ -339,12 +342,26 @@ return ret; } +static void *internal_map_flash_region(const char *descr, uintptr_t phys_addr, size_t len) +{ + if (is_phys_master) + return physmap(descr, phys_addr, len); + return fallback_map(descr, phys_addr, len); +} + +static void internal_unmap_flash_region(void *virt_addr, size_t len) +{ + if (is_phys_master) + return physunmap(virt_addr, len); + return fallback_unmap(virt_addr, len); +} + const struct programmer_entry programmer_internal = { .name = "internal", .type = OTHER, .devs.note = NULL, .init = internal_init, - .map_flash_region = physmap, - .unmap_flash_region = physunmap, + .map_flash_region = internal_map_flash_region, + .unmap_flash_region = internal_unmap_flash_region, .delay = internal_delay, }; -- To view, visit https://review.coreboot.org/c/flashrom/+/63543 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ia32ff1357b73166cdcec8a62ab5c516d1d1527f5 Gerrit-Change-Number: 63543 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: print_buildinfo: remove unreachable print of libpci version
by Edward O'Callaghan (Code Review) 11 Apr '22

11 Apr '22

Attention is currently required from: Felix Singer, Nico Huber, Thomas Heijligen. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62833 ) Change subject: print_buildinfo: remove unreachable print of libpci version ...................................................................... Patch Set 4: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/62833 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I0b5dbf3bd82a2ffe64b73881383e92f7dad4c382 Gerrit-Change-Number: 62833 Gerrit-PatchSet: 4 Gerrit-Owner: Thomas Heijligen <src(a)posteo.de> 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: Felix Singer <felixsinger(a)posteo.net> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Comment-Date: Mon, 11 Apr 2022 04:51:56 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: Drop STANDALONE mode
by Edward O'Callaghan (Code Review) 11 Apr '22

11 Apr '22

Attention is currently required from: Nico Huber, Thomas Heijligen, Angel Pons. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/63469 ) Change subject: Drop STANDALONE mode ...................................................................... Patch Set 2: Code-Review+2 (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/63469/comment/3ff308a7_40ad24e9 PS2, Line 11: For a while now, the code which is covered by STANDALONE has : moved to cli_*.c and is not used for libflashrom Wonderful! -- To view, visit https://review.coreboot.org/c/flashrom/+/63469 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I58fb82270a9884a323d9850176708d230fdc5165 Gerrit-Change-Number: 63469 Gerrit-PatchSet: 2 Gerrit-Owner: Thomas Heijligen <src(a)posteo.de> 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-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Mon, 11 Apr 2022 04:51:10 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: Makefile: build shared usb code only when needed by programmer
by Edward O'Callaghan (Code Review) 11 Apr '22

11 Apr '22

Attention is currently required from: Nico Huber, Thomas Heijligen. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62832 ) Change subject: Makefile: build shared usb code only when needed by programmer ...................................................................... Patch Set 2: (1 comment) Patchset: PS1: > Yes, that is correct. I just want to make sure we are not too pedantic […] While I can see the reasoning for this and it makes sense I have a supplementary argument to Nico's diff reduction maintenance one. I would rather we go the other way as by reducing both build system and pre-processor cyclomatic complexity means that the compiler is more likely to see a more consistent picture of the tree as the moving window of changes sweeps HEAD. Furthermore, it lets the linker in the toolchain do it's job and cull symbols that are not required in the resulting binary rather than trying to pre-optimise at the build system level. The associativity property should rather be done at the build target level to again, let the build system do its job of relationship management best. Hopefully my thinking makes sense here Thomas, however thanks for the different prospective too! -- To view, visit https://review.coreboot.org/c/flashrom/+/62832 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I9a91c5801a573e7021686a183a1bd874122e35cc Gerrit-Change-Number: 62832 Gerrit-PatchSet: 2 Gerrit-Owner: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> 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-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Comment-Date: Mon, 11 Apr 2022 04:49:12 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Thomas Heijligen <src(a)posteo.de> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: NEED_PCI: remove macro
by Edward O'Callaghan (Code Review) 11 Apr '22

11 Apr '22

Attention is currently required from: Nico Huber, Thomas Heijligen. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62834 ) Change subject: NEED_PCI: remove macro ...................................................................... Patch Set 4: Code-Review+2 (1 comment) File programmer.h: https://review.coreboot.org/c/flashrom/+/62834/comment/b4ef6282_bb8a6216 PS4, Line 265: extraneous line change. -- To view, visit https://review.coreboot.org/c/flashrom/+/62834 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I6895b795bc96b3e251700bff4b0054407aac789a Gerrit-Change-Number: 62834 Gerrit-PatchSet: 4 Gerrit-Owner: Thomas Heijligen <src(a)posteo.de> 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-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Comment-Date: Mon, 11 Apr 2022 04:35:08 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: programmer.h: regroup parts for the internal programmer
by Edward O'Callaghan (Code Review) 11 Apr '22

11 Apr '22

Attention is currently required from: Nico Huber, Thomas Heijligen, Angel Pons. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/63468 ) Change subject: programmer.h: regroup parts for the internal programmer ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/63468 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I0dcdb6c7998eec36e619f6e5c82825f60a2435ac Gerrit-Change-Number: 63468 Gerrit-PatchSet: 1 Gerrit-Owner: Thomas Heijligen <src(a)posteo.de> 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-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Mon, 11 Apr 2022 04:33:11 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: flashrom.c: Warn when mapping >=16M flash chips from high mem
by Edward O'Callaghan (Code Review) 11 Apr '22

11 Apr '22

Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/63534 ) Change subject: flashrom.c: Warn when mapping >=16M flash chips from high mem ...................................................................... flashrom.c: Warn when mapping >=16M flash chips from high mem Change-Id: Ibf59c243fb9c6e0db68f17ee87fc1bb354a3f886 Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M flashrom.c 1 file changed, 4 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/34/63534/1 diff --git a/flashrom.c b/flashrom.c index ac61259..ccdf7f2 100644 --- a/flashrom.c +++ b/flashrom.c @@ -694,6 +694,10 @@ if (flash->chip->total_size == 0) return 0; + /* FIXME: The internal master hooks 'map_flash_region = physmap'. */ + if (flash->chip->total_size > 16 * 1024) + msg_cwarn("Warning: maping %d kB sized flash chip. This could cause issue on x86.\n", flash->chip->total_size * 1024); + const chipsize_t size = flash->chip->total_size * 1024; uintptr_t base = flashbase ? flashbase : (0xffffffff - size + 1); void *addr = programmer_map_flash_region(flash->chip->name, base, size); -- To view, visit https://review.coreboot.org/c/flashrom/+/63534 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ibf59c243fb9c6e0db68f17ee87fc1bb354a3f886 Gerrit-Change-Number: 63534 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: newchange

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit April 2022