December 2021 - flashrom-gerrit

Change in flashrom[master]: hwaccess: move x86 port I/O related code into own files
by Anastasia Klimchuk (Code Review) 17 Dec '21

17 Dec '21

Attention is currently required from: Nico Huber, Thomas Heijligen, Angel Pons. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/60110 ) Change subject: hwaccess: move x86 port I/O related code into own files ...................................................................... Patch Set 5: Code-Review+1 (1 comment) File Makefile: https://review.coreboot.org/c/flashrom/+/60110/comment/606202e6_116f9fdd PS4, Line 783: ifneq ($(NEED_RAW_ACCESS), ) : # Raw memory, MSR or PCI port I/O access. : FEATURE_CFLAGS += -D'NEED_RAW_ACCESS=1' -D'__FLASHROM_HAVE_OUTB__=1' : PROGRAMMER_OBJS += physmap.o hwaccess.o hwaccess_x86_io.o > `make CONFIG_ENABLE_LIBPCI_PROGRAMMERS=no CONFIG_RAYER_SPI=no` build successful on x86_64 Linux and […] Thank you for explanation! -- To view, visit https://review.coreboot.org/c/flashrom/+/60110 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I372b4a409f036da766c42bc406b596bc41b0f75a Gerrit-Change-Number: 60110 Gerrit-PatchSet: 5 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-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: Fri, 17 Dec 2021 10:14:58 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Thomas Heijligen <src(a)posteo.de> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: hwaccess: move x86 port I/O related code into own files
by Thomas Heijligen (Code Review) 17 Dec '21

17 Dec '21

Attention is currently required from: Nico Huber, Angel Pons, Anastasia Klimchuk. Thomas Heijligen has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/60110 ) Change subject: hwaccess: move x86 port I/O related code into own files ...................................................................... Patch Set 5: (1 comment) File Makefile: https://review.coreboot.org/c/flashrom/+/60110/comment/6e467f4f_742e6516 PS4, Line 783: ifneq ($(NEED_RAW_ACCESS), ) : # Raw memory, MSR or PCI port I/O access. : FEATURE_CFLAGS += -D'NEED_RAW_ACCESS=1' -D'__FLASHROM_HAVE_OUTB__=1' : PROGRAMMER_OBJS += physmap.o hwaccess.o hwaccess_x86_io.o > > Yes, currently it's disabling all libpci programmer and rayer_spi. […] `make CONFIG_ENABLE_LIBPCI_PROGRAMMERS=no CONFIG_RAYER_SPI=no` build successful on x86_64 Linux and hwaccess* files are not build as expected. NEED_RAW_ACCESS is an internal make variable. This should not be overwritten by the user. So `make NEED_RAW_ACCESS=` is not supported. The user should select the programmers to build and the Makefile selects all necessary features for them or disable the programmer when it's not supported on the platform. -- To view, visit https://review.coreboot.org/c/flashrom/+/60110 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I372b4a409f036da766c42bc406b596bc41b0f75a Gerrit-Change-Number: 60110 Gerrit-PatchSet: 5 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-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Fri, 17 Dec 2021 09:33:06 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Thomas Heijligen <src(a)posteo.de> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: libflashrom,writeprotect: add flashrom_wp_{read,write}_chip_config()
by Nico Huber (Code Review) 17 Dec '21

17 Dec '21

Attention is currently required from: Edward O'Callaghan, Angel Pons, Nikolai Artemiev, Sergii Dmytruk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58479 ) Change subject: libflashrom,writeprotect: add flashrom_wp_{read,write}_chip_config() ...................................................................... Patch Set 22: (1 comment) File libflashrom.h: https://review.coreboot.org/c/flashrom/+/58479/comment/a9b4adb1_f81b57e0 PS22, Line 131: const struct flashrom_flashctx *, struct flashrom_wp_chip_config * Something about APIs in general. We are used to have the destination argument first. Just like in an assignment where the destination is on the left-hand side. This also applies to functions added later. Also to internal functions but we can make exceptions if there is a good reason. -- To view, visit https://review.coreboot.org/c/flashrom/+/58479 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I3ad25708c3321b8fb0216c3eaf6ffc07616537ad Gerrit-Change-Number: 58479 Gerrit-PatchSet: 22 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> 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: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Fri, 17 Dec 2021 01:02:24 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: hwaccess: move x86 port I/O related code into own files
by Anastasia Klimchuk (Code Review) 17 Dec '21

17 Dec '21

Attention is currently required from: Nico Huber, Thomas Heijligen, Angel Pons. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/60110 ) Change subject: hwaccess: move x86 port I/O related code into own files ...................................................................... Patch Set 5: (1 comment) File Makefile: https://review.coreboot.org/c/flashrom/+/60110/comment/1565d87a_103a872c PS4, Line 783: ifneq ($(NEED_RAW_ACCESS), ) : # Raw memory, MSR or PCI port I/O access. : FEATURE_CFLAGS += -D'NEED_RAW_ACCESS=1' -D'__FLASHROM_HAVE_OUTB__=1' : PROGRAMMER_OBJS += physmap.o hwaccess.o hwaccess_x86_io.o > Yes, currently it's disabling all libpci programmer and rayer_spi. is it realistic to check that such a case builds? does it matter? or maybe you did it already > Building with `make NEED_RAW_ACCESS=` fails during linking. Maybe I am missing something, why this is fine? was it always like this, building `make NEED_RAW_ACCESS=` never intent to work? -- To view, visit https://review.coreboot.org/c/flashrom/+/60110 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I372b4a409f036da766c42bc406b596bc41b0f75a Gerrit-Change-Number: 60110 Gerrit-PatchSet: 5 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-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: Fri, 17 Dec 2021 01:01:16 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Thomas Heijligen <src(a)posteo.de> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: libflashrom,writeprotect: add flashrom_wp_{read,write}_chip_config()
by Nico Huber (Code Review) 17 Dec '21

17 Dec '21

Attention is currently required from: Edward O'Callaghan, Angel Pons, Nikolai Artemiev, Sergii Dmytruk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58479 ) Change subject: libflashrom,writeprotect: add flashrom_wp_{read,write}_chip_config() ...................................................................... Patch Set 22: (1 comment) File writeprotect.c: https://review.coreboot.org/c/flashrom/+/58479/comment/6957c4a1_1c2f2ed3 PS22, Line 56: struct wp_chip_config *cfg Only realized this now: this requires the caller to know the size of `struct wp_chip_config` which is currently not given by `libflashrom.h`. IMO, we should use dynamic allocation instead, like in the rest of the API. It will require yet another de-allocation function, but having a clean, consistent API seems worth it. -- To view, visit https://review.coreboot.org/c/flashrom/+/58479 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I3ad25708c3321b8fb0216c3eaf6ffc07616537ad Gerrit-Change-Number: 58479 Gerrit-PatchSet: 22 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> 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: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Fri, 17 Dec 2021 00:56:07 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: libflashrom,writeprotect: add flashrom_wp_get_range()
by Nico Huber (Code Review) 17 Dec '21

17 Dec '21

Attention is currently required from: Edward O'Callaghan, Angel Pons, Nikolai Artemiev, Sergii Dmytruk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59183 ) Change subject: libflashrom,writeprotect: add flashrom_wp_get_range() ...................................................................... Patch Set 15: (1 comment) File writeprotect.c: https://review.coreboot.org/c/flashrom/+/59183/comment/7fa716a4_c684d1ae PS15, Line 189: decode_range We should check here if the pointer is set. -- To view, visit https://review.coreboot.org/c/flashrom/+/59183 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I5a1dfcf384166b1bac319d286306747e1dcaa000 Gerrit-Change-Number: 59183 Gerrit-PatchSet: 15 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> 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: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Fri, 17 Dec 2021 00:49:13 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: hwaccess: move x86 port I/O related code into own files
by Thomas Heijligen (Code Review) 16 Dec '21

16 Dec '21

Attention is currently required from: Nico Huber, Angel Pons, Anastasia Klimchuk. Thomas Heijligen has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/60110 ) Change subject: hwaccess: move x86 port I/O related code into own files ...................................................................... Patch Set 5: (2 comments) File Makefile: https://review.coreboot.org/c/flashrom/+/60110/comment/c071fe9e_9a2c1f8f PS4, Line 783: ifneq ($(NEED_RAW_ACCESS), ) : # Raw memory, MSR or PCI port I/O access. : FEATURE_CFLAGS += -D'NEED_RAW_ACCESS=1' -D'__FLASHROM_HAVE_OUTB__=1' : PROGRAMMER_OBJS += physmap.o hwaccess.o hwaccess_x86_io.o > Mostly curiosity: what is the way to build with NEED_RAW_ACCESS empty? Would that be disabling all t […] Yes, currently it's disabling all libpci programmer and rayer_spi. Building with `make NEED_RAW_ACCESS=` fails during linking. On x86 Linux NEED_RAW_ACCESS is enabled by CONFIG_RAYER_SPI which builds by default and all libcpi programmer when libpci is found File tests/hwaccess_x86_io_unittest.h: https://review.coreboot.org/c/flashrom/+/60110/comment/f790396a_9fbacab0 PS4, Line 33: int rget_io_perms(void); > If you could move it below all defines and includes (right above iopl)? thanks! Done -- To view, visit https://review.coreboot.org/c/flashrom/+/60110 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I372b4a409f036da766c42bc406b596bc41b0f75a Gerrit-Change-Number: 60110 Gerrit-PatchSet: 5 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-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Thu, 16 Dec 2021 20:39:27 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: hwaccess: move x86 port I/O related code into own files
by Thomas Heijligen (Code Review) 16 Dec '21

16 Dec '21

Attention is currently required from: Nico Huber, Thomas Heijligen, Angel Pons. Hello build bot (Jenkins), Nico Huber, Angel Pons, Anastasia Klimchuk, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/60110 to look at the new patch set (#5). Change subject: hwaccess: move x86 port I/O related code into own files ...................................................................... hwaccess: move x86 port I/O related code into own files Allow port I/O related code to be compiled independent from memory mapping functionality. This enables for a better selection of needed hardware access types. Change-Id: I372b4a409f036da766c42bc406b596bc41b0f75a Signed-off-by: Thomas Heijligen <thomas.heijligen(a)secunet.com> --- M Makefile M amd_imc.c M atahpt.c M atapromise.c M atavia.c M board_enable.c M chipset_enable.c M drkaiser.c M gfxnvidia.c M hwaccess.c M hwaccess.h A hwaccess_x86_io.c M hwaccess_x86_io.h M internal.c M it8212.c M it85spi.c M it87spi.c M meson.build M nic3com.c M nicintel.c M nicintel_eeprom.c M nicintel_spi.c M nicnatsemi.c M nicrealtek.c M ogp_spi.c M programmer.h M rayer_spi.c M satamv.c M satasii.c M tests/hwaccess_x86_io_unittest.h M wbsio_spi.c 31 files changed, 118 insertions(+), 69 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/10/60110/5 -- To view, visit https://review.coreboot.org/c/flashrom/+/60110 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I372b4a409f036da766c42bc406b596bc41b0f75a Gerrit-Change-Number: 60110 Gerrit-PatchSet: 5 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-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-MessageType: newpatchset

1 0

Change in flashrom[master]: flashchips,writeprotect_ranges: add range decoding function
by Nico Huber (Code Review) 16 Dec '21

16 Dec '21

Attention is currently required from: Angel Pons, Nikolai Artemiev, Sergii Dmytruk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58480 ) Change subject: flashchips,writeprotect_ranges: add range decoding function ...................................................................... Patch Set 23: (14 comments) File flash.h: https://review.coreboot.org/c/flashrom/+/58480/comment/1bea2530_6e62729d PS23, Line 203: range Says less than the type, can be dropped. https://review.coreboot.org/c/flashrom/+/58480/comment/b92e456e_f036e317 PS23, Line 203: const struct flashrom_wp_chip_config *cfg, uint32_t chip_len It seems this is exactly the input we need for the `w25` implementation. But will it suit all future implementations? Should we pass the full flash definition or even context instead? https://review.coreboot.org/c/flashrom/+/58480/comment/8ebd4319_a8d1f009 PS23, Line 203: typedef Why have a typedef for it? All the other function pointers in `struct flashchip` don't have/need one? https://review.coreboot.org/c/flashrom/+/58480/comment/959f09d8_7739a02e PS23, Line 203: uint32_t Please don't hard-code fixed-width types. `unsigned int` should work as well, `size_t` too, and for most flexibility there is `chipsize_t` in `layout.h`. I don't promote the latter much because I'm not sure if it's really necessary. Maybe it would become handy if we'd ever encounter >4GiB chips (doesn't seem that far away anymore ^^). https://review.coreboot.org/c/flashrom/+/58480/comment/39774cfd_1376078a PS23, Line 203: cfg Says less than the type already does, can be dropped. File libflashrom.h: https://review.coreboot.org/c/flashrom/+/58480/comment/2962edc7_3661c590 PS23, Line 134: }; flashrom_flash_getsize() uses `size_t`? Should this follow? File writeprotect_ranges.c: https://review.coreboot.org/c/flashrom/+/58480/comment/0d66a75e_b3d9880c PS23, Line 19: Line break, please. https://review.coreboot.org/c/flashrom/+/58480/comment/3a440c7f_d7844f7e PS23, Line 21: TODO Not sure if it's worth a TODO. FWIW, this kind of TODO will just stick. https://review.coreboot.org/c/flashrom/+/58480/comment/9d461a5a_95010486 PS23, Line 27: 32 Why 32? https://review.coreboot.org/c/flashrom/+/58480/comment/c663f78d_4834b28b PS23, Line 29: r( Missing space. https://review.coreboot.org/c/flashrom/+/58480/comment/26fd2df0_486e8c71 PS23, Line 34: cfg->bp Isn't this an implicit pointer? Looks like GCC doesn't warn if compared to 0. Why does it work? I guess `coeff` overflows and is 0 ;) https://review.coreboot.org/c/flashrom/+/58480/comment/a5d85173_90597189 PS23, Line 50: 4 * 1024 4*KiB https://review.coreboot.org/c/flashrom/+/58480/comment/b28bd977_107e19e3 PS23, Line 51: 64 * 1024 64*KiB https://review.coreboot.org/c/flashrom/+/58480/comment/ff061593_9ade5f1f PS23, Line 85: && range->len > 0 Does it matter? i.e. where's the start of an empty range? ^^ -- To view, visit https://review.coreboot.org/c/flashrom/+/58480 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Id163ed80938a946a502ed116e48e8236e36eb203 Gerrit-Change-Number: 58480 Gerrit-PatchSet: 23 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> 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: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Thu, 16 Dec 2021 17:43:06 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: libflashrom,writeprotect: add flashrom_wp_{read,write}_chip_config()
by Nico Huber (Code Review) 16 Dec '21

16 Dec '21

Attention is currently required from: Edward O'Callaghan, Angel Pons, Nikolai Artemiev, Sergii Dmytruk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58479 ) Change subject: libflashrom,writeprotect: add flashrom_wp_{read,write}_chip_config() ...................................................................... Patch Set 22: (24 comments) File libflashrom.h: https://review.coreboot.org/c/flashrom/+/58479/comment/b432f1a5_ced8847a PS22, Line 125: software If it's software protection, why not name it like that? https://review.coreboot.org/c/flashrom/+/58479/comment/ab178333_98a78d51 PS22, Line 129: }; Please move to the commit that makes use of it. https://review.coreboot.org/c/flashrom/+/58479/comment/be8aff87_0a6396e6 PS22, Line 131: const We should be careful with `const` here. Flash access goes very deep down into the code. How could we say for instance, that we won't have to change some state in the future, e.g. to keep track of the last used address, or an error indication. So, we shouldn't use `const` on the flash context, unless the implementation is so simple that we can be sure. File writeprotect.c: PS22: A general note about coding style: You don't make much use of the `const` keyword. It can make the code much easier to read because the reader can always be sure that the variable holds its initial value. So I prefer to use it for all variables and parameters when possible, except for very small functions where one sees immediately that no assignments happen. Modern languages like Rust made immutable variables the default \o/ https://review.coreboot.org/c/flashrom/+/58479/comment/17396b9e_51773c1b PS22, Line 33: struct reg_bit_info bit This would copy the whole struct on every call. Just seems uncommon. https://review.coreboot.org/c/flashrom/+/58479/comment/1462f9ac_c87c3a56 PS22, Line 35: 0 Will this ever be consumed? https://review.coreboot.org/c/flashrom/+/58479/comment/15f53ae5_f9edcffa PS22, Line 48: ** Library functions should be in a respective documentation group, e.g. @defgroup or @addtogroup. https://review.coreboot.org/c/flashrom/+/58479/comment/7ce73e0d_aae350c5 PS22, Line 59: * No dangling asterisk, please. https://review.coreboot.org/c/flashrom/+/58479/comment/ef9287b7_8c24454e PS22, Line 59: intrepret int*er*pret https://review.coreboot.org/c/flashrom/+/58479/comment/82921c8e_3b2c7e48 PS22, Line 61: tmp rather `unused` or `ignored`? https://review.coreboot.org/c/flashrom/+/58479/comment/3e392492_6f9f4378 PS22, Line 63: *cfg = (struct wp_chip_config) {0}; Won't this be overwritten where it matters? iow. will any 0 ever be consumed? https://review.coreboot.org/c/flashrom/+/58479/comment/141764ef_406209cb PS22, Line 66: |= Does it make sense to continue if reading failed? https://review.coreboot.org/c/flashrom/+/58479/comment/dd43046a_30ce303d PS22, Line 70: r( Missing space. https://review.coreboot.org/c/flashrom/+/58479/comment/906a5714_a66631ee PS22, Line 70: for(size_t i = 0; bits->bp[i].reg != INVALID_REG; i++) { Generally, it's preferred to loop over the target array. This way you avoid overflows without further costs. e.g. for (i = 0; i < ARRAY_SIZE(cfg->bp); ++i) { if (bits->bp[i].reg == INVALID_REG) break; ... or just put both in the for condition for (i = 0; i < ARRAY_SIZE(cfg->bp) && bits->bp[i].reg != INVALID_REG; ++i) { https://review.coreboot.org/c/flashrom/+/58479/comment/2266d827_a1249332 PS22, Line 70: size_t Declarations inside a `for` made trouble in some environment. Please avoid it for now. https://review.coreboot.org/c/flashrom/+/58479/comment/5be0d602_8cef9385 PS22, Line 75: r( Missing space. https://review.coreboot.org/c/flashrom/+/58479/comment/6489c05b_13f62a8e PS22, Line 78: } You could also put the loop into a function, to avoid repeating it, e.g. read_reg_bits(). https://review.coreboot.org/c/flashrom/+/58479/comment/6bdb81c5_b7ff2273 PS22, Line 84: * No dangling asterisk, please. Or maybe just use the long comment style with line breaks at the beginning/end. I think that's more common at the top level. https://review.coreboot.org/c/flashrom/+/58479/comment/bce72fa7_4d726135 PS22, Line 86: uint8_t *reg_values, uint8_t *write_masks You can also write these as `uint8_t reg_values[MAX_REGISTERS]` to make the intention clear. https://review.coreboot.org/c/flashrom/+/58479/comment/786dddc8_434c7b8b PS22, Line 122: INVALID_REG This is very confusing. I see it's skipped because the mask would be 0, but having to check that makes the code harder to read. https://review.coreboot.org/c/flashrom/+/58479/comment/89fdf11a_41470051 PS22, Line 126: = 0 Why initialize? https://review.coreboot.org/c/flashrom/+/58479/comment/e846f72a_57f51a0f PS22, Line 131: & write_masks[reg] This is not necessary as reg_values[] was initialized to 0. https://review.coreboot.org/c/flashrom/+/58479/comment/24b7e0e9_0947d96c PS22, Line 144: configuraitons configura*ti*ons https://review.coreboot.org/c/flashrom/+/58479/comment/a085366a_7c6f07a7 PS22, Line 145: which of two configurations should be used if they both select the same : * protection range Does this really work? How can we say, for instance, that SRP1 being unset is preferred? -- To view, visit https://review.coreboot.org/c/flashrom/+/58479 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I3ad25708c3321b8fb0216c3eaf6ffc07616537ad Gerrit-Change-Number: 58479 Gerrit-PatchSet: 22 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> 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: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Thu, 16 Dec 2021 13:05:55 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit December 2021