- flashrom-gerrit - mail.coreboot.org

[S] Change in flashrom[master]: pcidev: remove pcidev_getdevfn() function
by Edward O'Callaghan (Code Review) 11 Mar '23

11 Mar '23

Attention is currently required from: Thomas Heijligen. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/73570 ) Change subject: pcidev: remove pcidev_getdevfn() function ...................................................................... Patch Set 4: (1 comment) Patchset: PS4: > oh sorry :( what do we do now? PCI access in flashrom is a big mess. Possibly the structure to handle the issues is to have a struct full of pci worker func pointers containing an opaque state machine with the pacc handle inside initialised at `struct bus_cfg` initiation time then passed down into either board_cfg || programmer_cfg or maybe just itself. It is hard to think so many moves ahead as other pre-requirements need to land first like the board_cfg cleanup series. So that could look something akin to; ``` struct flashrom_pcihandle { struct pci_access *pacc; }; struct flashrom_pcibus { struct flashrom_pci handle; int (*find)(..); int (*scan)(..); int (*read)(..); int (*write)(..); }; ``` I don't really like the OOP'ism however the tree has deeply ingrained excessive encapsulation patterns gone rampant and so any procedural state management winds up tripping you up on edge cases. In either case, I would say this commit could be reverted as it seems to be motivated by aesthetic but not fixing a actually issue. The author just didn't notice the `pacc` singleton state machine containment as there was one patch missing that made it truly static. The reason being is that patch got stalled out unfortunately. -- To view, visit https://review.coreboot.org/c/flashrom/+/73570 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Id2983420080f75ae6992edfb032bf5c83b29c803 Gerrit-Change-Number: 73570 Gerrit-PatchSet: 4 Gerrit-Owner: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> 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: Thomas Heijligen <src(a)posteo.de> Gerrit-Comment-Date: Sat, 11 Mar 2023 10:15:20 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: tests: Add wrap for __fstat50 to fix tests for NetBSD
by Anastasia Klimchuk (Code Review) 11 Mar '23

11 Mar '23

Anastasia Klimchuk has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/73649 ) Change subject: tests: Add wrap for __fstat50 to fix tests for NetBSD ...................................................................... tests: Add wrap for __fstat50 to fix tests for NetBSD Tested by running unit tests on NetBSD 9.2 Ubuntu 22.04.1 (still pass) Change-Id: Icb8e453328cb40ab9d628f01ecdc3886a233dad5 Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org> --- M tests/meson.build M tests/tests.c M tests/wraps.h 3 files changed, 22 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/49/73649/1 diff --git a/tests/meson.build b/tests/meson.build index 2de4ee8..df866d7 100644 --- a/tests/meson.build +++ b/tests/meson.build @@ -60,6 +60,7 @@ '-Wl,--wrap=__xstat64', '-Wl,--wrap=fstat', '-Wl,--wrap=fstat64', + '-Wl,--wrap=__fstat50', '-Wl,--wrap=__fxstat', '-Wl,--wrap=__fxstat64', '-Wl,--wrap=fileno', diff --git a/tests/tests.c b/tests/tests.c index 911cf47..d296a98 100644 --- a/tests/tests.c +++ b/tests/tests.c @@ -229,6 +229,12 @@ return 0; } +int __wrap___fstat50(int fd, void *buf) +{ + LOG_ME; + return 0; +} + int __wrap___fxstat(int fd, void *buf) { LOG_ME; diff --git a/tests/wraps.h b/tests/wraps.h index 6cc9567..089d992 100644 --- a/tests/wraps.h +++ b/tests/wraps.h @@ -46,6 +46,7 @@ int __wrap___xstat64(const char *path, void *buf); int __wrap_fstat(int fd, void *buf); int __wrap_fstat64(int fd, void *buf); +int __wrap___fstat50(int fd, void *buf); int __wrap___fxstat(int fd, void *buf); int __wrap___fxstat64(int fd, void *buf); char *__wrap_fgets(char *buf, int len, FILE *fp); -- To view, visit https://review.coreboot.org/c/flashrom/+/73649 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Icb8e453328cb40ab9d628f01ecdc3886a233dad5 Gerrit-Change-Number: 73649 Gerrit-PatchSet: 1 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: newchange

1 0

[S] Change in flashrom[master]: MAINTAINERS: Add Aarya Chaumal for erasure_layout
by Anastasia Klimchuk (Code Review) 11 Mar '23

11 Mar '23

Attention is currently required from: Felix Singer, Thomas Heijligen, Aarya. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/73642 ) Change subject: MAINTAINERS: Add Aarya Chaumal for erasure_layout ...................................................................... Patch Set 2: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/73642 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Id0188add883336f2fb494e4169a668e8b502682f Gerrit-Change-Number: 73642 Gerrit-PatchSet: 2 Gerrit-Owner: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> 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-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Comment-Date: Sat, 11 Mar 2023 06:56:58 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: pcidev: remove pcidev_getdevfn() function
by Anastasia Klimchuk (Code Review) 11 Mar '23

11 Mar '23

Attention is currently required from: Thomas Heijligen. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/73570 ) Change subject: pcidev: remove pcidev_getdevfn() function ...................................................................... Patch Set 4: (1 comment) Patchset: PS4: > Hmm, the intention of `pcidev_getdevfn()` was to try and constrain `pacc` usage in the one place. oh sorry :( what do we do now? -- To view, visit https://review.coreboot.org/c/flashrom/+/73570 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Id2983420080f75ae6992edfb032bf5c83b29c803 Gerrit-Change-Number: 73570 Gerrit-PatchSet: 4 Gerrit-Owner: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> 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: Thomas Heijligen <src(a)posteo.de> Gerrit-Comment-Date: Sat, 11 Mar 2023 06:53:26 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: internal: Move is_laptop into board_cfg
by Edward O'Callaghan (Code Review) 11 Mar '23

11 Mar '23

Attention is currently required from: Thomas Heijligen. Hello build bot (Jenkins), Thomas Heijligen, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/73455 to look at the new patch set (#2). Change subject: internal: Move is_laptop into board_cfg ...................................................................... internal: Move is_laptop into board_cfg Change-Id: I24e38e4457299934acdcd70325d0bf0f4b139e5f Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M board_enable.c M include/programmer.h M internal.c 3 files changed, 18 insertions(+), 9 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/55/73455/2 -- To view, visit https://review.coreboot.org/c/flashrom/+/73455 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I24e38e4457299934acdcd70325d0bf0f4b139e5f Gerrit-Change-Number: 73455 Gerrit-PatchSet: 2 Gerrit-Owner: 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: Thomas Heijligen <src(a)posteo.de> Gerrit-MessageType: newpatchset

1 0

[M] Change in flashrom[master]: internal: Move laptop_ok into board_cfg
by Edward O'Callaghan (Code Review) 11 Mar '23

11 Mar '23

Attention is currently required from: Thomas Heijligen, Angel Pons. Hello build bot (Jenkins), Thomas Heijligen, Angel Pons, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/73456 to look at the new patch set (#3). Change subject: internal: Move laptop_ok into board_cfg ...................................................................... internal: Move laptop_ok into board_cfg Due to how internal is structured around chipset_flash_enable() entry we need to prepare a crafted programmer_cfg that contains a board_enable substructure with data derived from the board_enable subsystem. While this is certainly not perfection, it does make clear the relationships between board_enable into chipset_flash_enable and subsequently the overall internal programmer initialisation in a RAII fashion at the type level over closure upon global state that is impossible to reason about. Also flip predicate in report_nonwl_laptop_detected() and return early with the trivial base-case. TEST=`$ sudo ./flashrom -p internal --flash-name`. Change-Id: I459215253845c2af73262943ce91a36464e9eb06 Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M board_enable.c M chipset_enable.c M flashrom.c M include/programmer.h M internal.c 5 files changed, 69 insertions(+), 44 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/56/73456/3 -- To view, visit https://review.coreboot.org/c/flashrom/+/73456 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I459215253845c2af73262943ce91a36464e9eb06 Gerrit-Change-Number: 73456 Gerrit-PatchSet: 3 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> 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: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: newpatchset

1 0

[M] Change in flashrom[master]: dmi.c: Pass is_laptop by ref into dmi
by Edward O'Callaghan (Code Review) 11 Mar '23

11 Mar '23

Attention is currently required from: Felix Singer, Thomas Heijligen, Angel Pons. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/71622 ) Change subject: dmi.c: Pass is_laptop by ref into dmi ...................................................................... Patch Set 5: (2 comments) File internal.c: https://review.coreboot.org/c/flashrom/+/71622/comment/8985a57a_8a6d7961 PS4, Line 31: laptop_ok > Sorry when my intent was not clear. […] Done https://review.coreboot.org/c/flashrom/+/71622/comment/e600d430_63a37960 PS4, Line 272: > ? Done -- To view, visit https://review.coreboot.org/c/flashrom/+/71622 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I3d5ad6c0623269492d775a99a947fd6fe26c5f91 Gerrit-Change-Number: 71622 Gerrit-PatchSet: 5 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Fri, 10 Mar 2023 23:54:32 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No 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

[S] Change in flashrom[master]: tree/: Rename 'laptop_ok-> g_laptop_ok' to avoid shadowing
by Edward O'Callaghan (Code Review) 11 Mar '23

11 Mar '23

Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/73646 ) Change subject: tree/: Rename 'laptop_ok-> g_laptop_ok' to avoid shadowing ...................................................................... tree/: Rename 'laptop_ok-> g_laptop_ok' to avoid shadowing Avoid global symbol shadowing in local functions. Change-Id: Idfca0b7e46d8051bf680227250cf40483e19dc53 Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M board_enable.c M chipset_enable.c M include/programmer.h M internal.c 4 files changed, 24 insertions(+), 12 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/46/73646/1 diff --git a/board_enable.c b/board_enable.c index 3e6a5a7..4ea53ec 100644 --- a/board_enable.c +++ b/board_enable.c @@ -2294,7 +2294,7 @@ static int p2_whitelist_laptop(void) { is_laptop = 1; - laptop_ok = true; + g_laptop_ok = true; msg_pdbg("Whitelisted laptop detected.\n"); return 0; } diff --git a/chipset_enable.c b/chipset_enable.c index e1dd6a3..37a48fc 100644 --- a/chipset_enable.c +++ b/chipset_enable.c @@ -826,7 +826,7 @@ /* Suppress unknown laptop warning if we booted from SPI. */ if (boot_buses & BUS_SPI) - laptop_ok = true; + g_laptop_ok = true; return 0; } @@ -971,7 +971,7 @@ /* Suppress unknown laptop warning if we booted from SPI. */ if (!ret && (boot_buses & BUS_SPI)) - laptop_ok = true; + g_laptop_ok = true; _freepci_ret: pci_free_dev(spi_dev); @@ -1087,7 +1087,7 @@ /* Suppress unknown laptop warning if we booted from SPI. */ if (boot_buses & BUS_SPI) - laptop_ok = true; + g_laptop_ok = true; return 0; } @@ -1676,7 +1676,7 @@ /* Suppress unknown laptop warning if we booted from SPI. */ if (!ret && want_spi) - laptop_ok = true; + g_laptop_ok = true; return ret; } diff --git a/include/programmer.h b/include/programmer.h index 9728d8d..9da6165 100644 --- a/include/programmer.h +++ b/include/programmer.h @@ -263,7 +263,7 @@ #if CONFIG_INTERNAL == 1 extern int is_laptop; -extern bool laptop_ok; +extern bool g_laptop_ok; extern bool force_boardmismatch; void probe_superio(void); int register_superio(struct superio s); diff --git a/internal.c b/internal.c index c75b472..0c23326 100644 --- a/internal.c +++ b/internal.c @@ -28,7 +28,7 @@ #endif int is_laptop = 0; -bool laptop_ok = false; +bool g_laptop_ok = false; bool force_boardmismatch = false; @@ -109,9 +109,9 @@ } // FIXME: remove '_' suffix from parameters once global shadowing is fixed. -static void report_nonwl_laptop_detected(int is_laptop_, bool laptop_ok_) +static void report_nonwl_laptop_detected(int is_laptop_, bool laptop_ok) { - if (is_laptop_ && !laptop_ok_) { + if (is_laptop_ && !laptop_ok) { msg_pinfo("========================================================================\n"); if (is_laptop_ == 1) { msg_pinfo("You seem to be running flashrom on an unknown laptop. Some\n" @@ -159,7 +159,7 @@ return ret; /* Unconditionally reset global state from previous operation. */ - laptop_ok = false; + g_laptop_ok = false; /* Default to Parallel/LPC/FWH flash devices. If a known host controller * is found, the host controller init routine sets the @@ -229,7 +229,7 @@ * this isn't a laptop. Board-enables may override this, * non-legacy buses (SPI and opaque atm) are probed anyway. */ - if (is_laptop && !(laptop_ok || force_laptop || (not_a_laptop && is_laptop == 2))) + if (is_laptop && !(g_laptop_ok || force_laptop || (not_a_laptop && is_laptop == 2))) internal_buses_supported = BUS_NONE; /* try to enable it. Failure IS an option, since not all motherboards @@ -258,7 +258,7 @@ internal_par_init(internal_buses_supported); /* Report if a non-whitelisted laptop is detected that likely uses a legacy bus. */ - report_nonwl_laptop_detected(is_laptop, laptop_ok); + report_nonwl_laptop_detected(is_laptop, g_laptop_ok); ret = 0; -- To view, visit https://review.coreboot.org/c/flashrom/+/73646 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Idfca0b7e46d8051bf680227250cf40483e19dc53 Gerrit-Change-Number: 73646 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: newchange

1 0

[M] Change in flashrom[master]: dmi.c: Pass is_laptop by ref into dmi
by Edward O'Callaghan (Code Review) 11 Mar '23

11 Mar '23

Attention is currently required from: Felix Singer, Edward O'Callaghan, Angel Pons. Hello Felix Singer, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/71622 to look at the new patch set (#5). Change subject: dmi.c: Pass is_laptop by ref into dmi ...................................................................... dmi.c: Pass is_laptop by ref into dmi Prefix the remaining global cases with `g_` to avoid shadowing issues and for easy greping. Change-Id: I3d5ad6c0623269492d775a99a947fd6fe26c5f91 Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M board_enable.c M dmi.c M include/programmer.h M internal.c 4 files changed, 44 insertions(+), 31 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/22/71622/5 -- To view, visit https://review.coreboot.org/c/flashrom/+/71622 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I3d5ad6c0623269492d775a99a947fd6fe26c5f91 Gerrit-Change-Number: 71622 Gerrit-PatchSet: 5 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: newpatchset

1 0

[S] Change in flashrom[master]: pcidev: remove pcidev_getdevfn() function
by Edward O'Callaghan (Code Review) 11 Mar '23

11 Mar '23

Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/73570 ) Change subject: pcidev: remove pcidev_getdevfn() function ...................................................................... Patch Set 4: (1 comment) Patchset: PS4: Hmm, the intention of `pcidev_getdevfn()` was to try and constrain `pacc` usage in the one place. -- To view, visit https://review.coreboot.org/c/flashrom/+/73570 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Id2983420080f75ae6992edfb032bf5c83b29c803 Gerrit-Change-Number: 73570 Gerrit-PatchSet: 4 Gerrit-Owner: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> 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-Comment-Date: Fri, 10 Mar 2023 23:44:40 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

2025

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit