- flashrom-gerrit - mail.coreboot.org

Change in flashrom[master]: spi25_statusreg: inline spi_write_register_flag()
by Nikolai Artemiev (Code Review) 11 Jan '22

11 Jan '22

Attention is currently required from: Edward O'Callaghan, Angel Pons, Nikolai Artemiev. Hello build bot (Jenkins), Nico Huber, Edward O'Callaghan, Angel Pons, Anastasia Klimchuk, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/59528 to look at the new patch set (#20). Change subject: spi25_statusreg: inline spi_write_register_flag() ...................................................................... spi25_statusreg: inline spi_write_register_flag() Creating the entire SPI command that should be sent to the chip in spi_write_register() is simpler than splitting it across two functions that have to pass multiple parameters between them. Additionally, having separate spi_write_register_flag() function provided little benefit, as it was only ever called from spi_write_register(). BUG=b:195381327,b:153800563 TEST=builds BRANCH=none Change-Id: I4996b0848d0ed09032bad2ab13ab1f40bbfc0304 Signed-off-by: Nikolai Artemiev <nartemiev(a)google.com> --- M spi25_statusreg.c 1 file changed, 46 insertions(+), 53 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/28/59528/20 -- To view, visit https://review.coreboot.org/c/flashrom/+/59528 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I4996b0848d0ed09032bad2ab13ab1f40bbfc0304 Gerrit-Change-Number: 59528 Gerrit-PatchSet: 20 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: 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-MessageType: newpatchset

1 0

Change in flashrom[master]: writeprotect: print chip range truth table for debugging
by Nikolai Artemiev (Code Review) 11 Jan '22

11 Jan '22

Attention is currently required from: Nikolai Artemiev. Hello build bot (Jenkins), Nico Huber, Anastasia Klimchuk, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/59909 to look at the new patch set (#12). Change subject: writeprotect: print chip range truth table for debugging ...................................................................... writeprotect: print chip range truth table for debugging Make it easier to add new chips and debug range decoding functions by printing a complete truth table of config bits / ranges. BUG=b:195381327,b:153800563 TEST=flashrom --wp-list -V BRANCH=none Change-Id: I2cda441229ffdcabff27906f7804efba82baa6bc Signed-off-by: Nikolai Artemiev <nartemiev(a)google.com> --- M writeprotect.c 1 file changed, 15 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/09/59909/12 -- To view, visit https://review.coreboot.org/c/flashrom/+/59909 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I2cda441229ffdcabff27906f7804efba82baa6bc Gerrit-Change-Number: 59909 Gerrit-PatchSet: 12 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: writeprotect: add {get,set}_wp_mode()
by Nikolai Artemiev (Code Review) 11 Jan '22

11 Jan '22

Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons, Sergii Dmytruk. Nikolai Artemiev has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58483 ) Change subject: writeprotect: add {get,set}_wp_mode() ...................................................................... Patch Set 31: (5 comments) File libflashrom.h: https://review.coreboot.org/c/flashrom/+/58483/comment/dc3b2b1b_b98a1819 PS25, Line 148: const > In a forward declaration, `const` as a qualifier of a parameter (i.e. […] Done File writeprotect.c: https://review.coreboot.org/c/flashrom/+/58483/comment/2cecaf9e_f7e419d9 PS25, Line 374: > Spurious line break? Done https://review.coreboot.org/c/flashrom/+/58483/comment/85d5c77e_4be0c46c PS25, Line 410: return 1; > Wouldn't it be a success if `mode == WP_MODE_DISABLE`? Done https://review.coreboot.org/c/flashrom/+/58483/comment/42fc2e33_f1d280c3 PS25, Line 412: f( > Missing space. Could be a `switch/case` though. Done https://review.coreboot.org/c/flashrom/+/58483/comment/f3e0aede_eb32225b PS25, Line 420: f( > Missing space. Done -- To view, visit https://review.coreboot.org/c/flashrom/+/58483 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I7b68e940f0e1359281806c98e1da119b4caf8405 Gerrit-Change-Number: 58483 Gerrit-PatchSet: 31 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: 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-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Tue, 11 Jan 2022 07:05:33 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: libflashrom,writeprotect: add flashrom_wp_get_ranges()
by Nikolai Artemiev (Code Review) 11 Jan '22

11 Jan '22

Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons, Sergii Dmytruk. Nikolai Artemiev has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58481 ) Change subject: libflashrom,writeprotect: add flashrom_wp_get_ranges() ...................................................................... Patch Set 29: (10 comments) File libflashrom.h: https://review.coreboot.org/c/flashrom/+/58481/comment/6abe25c9_fa070678 PS23, Line 137: #define FLASHROM_WP_MAX_RANGES 128 > Should we use dynamic allocation instead? Done https://review.coreboot.org/c/flashrom/+/58481/comment/ea09f326_7547bf65 PS23, Line 143: size_t > Technically `size_t` is supposed to specify a byte count. It's often […] It's nice that size_t is guaranteed to be large enough to represent the number of elements in any array though. Although size_t's intended use is for representing the size of an object in bytes, I don't think that should keep us from using it for somewhere else where it's well suited. File writeprotect.c: https://review.coreboot.org/c/flashrom/+/58481/comment/07e8e40a_724fb596 PS23, Line 187: preferred > What is preferred and why? For choosing one range encoding over another it's pretty much arbitrary. Usually the only ranges that have multiple encodings either span the entire flash or are completely empty. E.g. TB and SEC have no affect on them. In that case TB/SEC are just set to 0. I've removed preferred from the description since it was misleading. https://review.coreboot.org/c/flashrom/+/58481/comment/9a67f579_b5c10f3c PS23, Line 233: TB in a OTP register > Please mention the chip where you have seen something like this. Sometimes […] Done https://review.coreboot.org/c/flashrom/+/58481/comment/54d9ee3d_f5cc0e35 PS23, Line 234: perserved > p*re*served Done https://review.coreboot.org/c/flashrom/+/58481/comment/d5ac52ce_86fee205 PS23, Line 237: 20 > ARRAY_SIZE(bits->bp) + 1 /* TB */ + 1 /* SEC */ + 1 /* CMP */ ? Done https://review.coreboot.org/c/flashrom/+/58481/comment/88d3b7dc_b1ce3d8e PS23, Line 240: can_write_bit(bits->bp[i]) > This would stop at the first not writable bit and skip further writable […] Done https://review.coreboot.org/c/flashrom/+/58481/comment/bf63877d_b27d14d5 PS23, Line 255: for (uint32_t range_index = 0; range_index < *count; range_index++) { > Please check that the destination arrays don't overflow (wouldn't […] Done https://review.coreboot.org/c/flashrom/+/58481/comment/fbb2de56_a1fff68c PS23, Line 255: 32 > Why 32? Changed to size_t. https://review.coreboot.org/c/flashrom/+/58481/comment/d7afba92_d3026f76 PS23, Line 308: struct wp_range_cfg_pair range_cfg_pairs[FLASHROM_WP_MAX_RANGES]; > Not on the stack, please. Flashrom sometimes runs in embedded environments. Done -- To view, visit https://review.coreboot.org/c/flashrom/+/58481 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Id51f038f03305c8536d80313e52f77d27835f34d Gerrit-Change-Number: 58481 Gerrit-PatchSet: 29 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: 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-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Tue, 11 Jan 2022 07:05:12 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: libflashrom,writeprotect: add flashrom_wp_get_ranges()
by Nikolai Artemiev (Code Review) 11 Jan '22

11 Jan '22

Attention is currently required from: Edward O'Callaghan, Angel Pons, Nikolai Artemiev, Sergii Dmytruk. Hello build bot (Jenkins), Nico Huber, Edward O'Callaghan, Angel Pons, Anastasia Klimchuk, Sergii Dmytruk, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/58481 to look at the new patch set (#29). Change subject: libflashrom,writeprotect: add flashrom_wp_get_ranges() ...................................................................... libflashrom,writeprotect: add flashrom_wp_get_ranges() Generate list of available ranges by enumerating all possible values that range bits (BPx, TB, ...) can take and using the chip's range decoding function to get the range that is selected by each one. BUG=b:195381327,b:153800563 TEST=flashrom --wp-list BRANCH=none Change-Id: Id51f038f03305c8536d80313e52f77d27835f34d Signed-off-by: Nikolai Artemiev <nartemiev(a)google.com> --- M libflashrom.h M writeprotect.c 2 files changed, 194 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/81/58481/29 -- To view, visit https://review.coreboot.org/c/flashrom/+/58481 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Id51f038f03305c8536d80313e52f77d27835f34d Gerrit-Change-Number: 58481 Gerrit-PatchSet: 29 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-MessageType: newpatchset

1 0

Change in flashrom[master]: writeprotect: add get_wp_range() for decoding ranges
by Nikolai Artemiev (Code Review) 11 Jan '22

11 Jan '22

Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons, Sergii Dmytruk. Nikolai Artemiev has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59183 ) Change subject: writeprotect: add get_wp_range() for decoding ranges ...................................................................... Patch Set 19: (1 comment) File writeprotect.c: https://review.coreboot.org/c/flashrom/+/59183/comment/4ec4a49f_2dc71dc4 PS15, Line 189: decode_range > We should check here if the pointer is set. Done -- 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: 19 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: 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-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Tue, 11 Jan 2022 06:30:31 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: writeprotect: add get_wp_range() for decoding ranges
by Nikolai Artemiev (Code Review) 11 Jan '22

11 Jan '22

Attention is currently required from: Edward O'Callaghan, Angel Pons, Nikolai Artemiev, Sergii Dmytruk. Hello build bot (Jenkins), Nico Huber, Edward O'Callaghan, Angel Pons, Anastasia Klimchuk, Sergii Dmytruk, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/59183 to look at the new patch set (#19). Change subject: writeprotect: add get_wp_range() for decoding ranges ...................................................................... writeprotect: add get_wp_range() for decoding ranges BUG=b:195381327,b:153800563 TEST=flashrom --wp-{status,range} BRANCH=none Change-Id: I5a1dfcf384166b1bac319d286306747e1dcaa000 Signed-off-by: Nikolai Artemiev <nartemiev(a)google.com> --- M writeprotect.c 1 file changed, 14 insertions(+), 3 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/83/59183/19 -- 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: 19 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-MessageType: newpatchset

1 0

Change in flashrom[master]: flashchips,writeprotect_ranges: add range decoding function
by Nikolai Artemiev (Code Review) 11 Jan '22

11 Jan '22

Attention is currently required from: Nico Huber, Angel Pons, Sergii Dmytruk. Nikolai Artemiev has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58480 ) Change subject: flashchips,writeprotect_ranges: add range decoding function ...................................................................... Patch Set 26: (9 comments) File flash.h: https://review.coreboot.org/c/flashrom/+/58480/comment/0db226a5_e7521602 PS23, Line 203: uint32_t > Please don't hard-code fixed-width types. `unsigned int` should work as […] Done https://review.coreboot.org/c/flashrom/+/58480/comment/783737c0_70615ed5 PS23, Line 203: typedef > Why have a typedef for it? All the other function pointers in `struct […] Done File libflashrom.h: https://review.coreboot.org/c/flashrom/+/58480/comment/02f29652_11970ecd PS23, Line 134: }; > flashrom_flash_getsize() uses `size_t`? Should this follow? Done File writeprotect_ranges.c: https://review.coreboot.org/c/flashrom/+/58480/comment/75f87846_bbf340d6 PS23, Line 19: > Line break, please. Done https://review.coreboot.org/c/flashrom/+/58480/comment/0c61a626_01893240 PS23, Line 27: 32 > Why 32? I've changed everything to size_t now, I just defaulted to 32 when I was writing it. https://review.coreboot.org/c/flashrom/+/58480/comment/a6768340_494a5430 PS23, Line 29: r( > Missing space. Done https://review.coreboot.org/c/flashrom/+/58480/comment/545a9ed5_1323e9f7 PS23, Line 34: cfg->bp > Isn't this an implicit pointer? Looks like GCC doesn't warn if compared […] Amazing, I have no idea this managed to work at all. Thanks for catching it :) https://review.coreboot.org/c/flashrom/+/58480/comment/12037e4e_5f2e3a69 PS23, Line 50: 4 * 1024 > 4*KiB Done https://review.coreboot.org/c/flashrom/+/58480/comment/d2a36e61_71f780a9 PS23, Line 51: 64 * 1024 > 64*KiB Done -- 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: 26 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: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Tue, 11 Jan 2022 06:28:36 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: libflashrom,writeprotect: add functions for reading/writing WP settings
by Nikolai Artemiev (Code Review) 11 Jan '22

11 Jan '22

Attention is currently required from: Nico Huber, Edward O'Callaghan, Anastasia Klimchuk, Sergii Dmytruk. Nikolai Artemiev has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58479 ) Change subject: libflashrom,writeprotect: add functions for reading/writing WP settings ...................................................................... Patch Set 25: (4 comments) File libflashrom.h: https://review.coreboot.org/c/flashrom/+/58479/comment/d472e6ce_9db44993 PS22, Line 129: }; > Please move to the commit that makes use of it. Done https://review.coreboot.org/c/flashrom/+/58479/comment/8c828b1e_eea17327 PS22, Line 131: const struct flashrom_flashctx *, struct flashrom_wp_chip_config * > Something about APIs in general. We are used to have the destination […] Done - here and in other files/commits. I think I got all occurrences, let me know if I missed something :) File writeprotect.c: https://review.coreboot.org/c/flashrom/+/58479/comment/fa897570_4f3894a9 PS22, Line 48: ** > Library functions should be in a respective documentation group, e.g. @defgroup or @addtogroup. Done https://review.coreboot.org/c/flashrom/+/58479/comment/abdb9bf5_109adc23 PS22, Line 122: INVALID_REG > IMO it is a bit confusing when loop starts with "invalid" value. […] Done -- 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: 25 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: Felix Singer <felixsinger(a)posteo.net> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Tue, 11 Jan 2022 06:26:21 +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> Comment-In-Reply-To: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: writeprotect.h: add structure to represent chip wp configuration bits
by Nikolai Artemiev (Code Review) 11 Jan '22

11 Jan '22

Attention is currently required from: Nico Huber, Angel Pons, Sergii Dmytruk. Nikolai Artemiev has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58478 ) Change subject: writeprotect.h: add structure to represent chip wp configuration bits ...................................................................... Patch Set 24: (4 comments) File flash.h: https://review.coreboot.org/c/flashrom/+/58478/comment/36674104_0ab70722 PS22, Line 201: Complete > `Complete` seems a bit exaggerated. Given the many different […] The intention is that it will be kept complete, i.e. bits will be added as necessary to support new chips. The count/present fields allow unused bits to be ignored for chips that don't need them. You're right that we'll need to add WPS for some chips. https://review.coreboot.org/c/flashrom/+/58478/comment/e01aaec4_46ce0bd9 PS22, Line 202: chip > Isn't everything about chips? Could we just call it `flashrom_wp_config`? I've changed it to `wp_bits`. The name is probably less important now that it is just used internally by writeprotect code, but I'm happy to change it to something else. https://review.coreboot.org/c/flashrom/+/58478/comment/f9e6439a_d1e232e0 PS22, Line 203: srp > Please add comments what all these acronyms mean. Done File flash.h: https://review.coreboot.org/c/flashrom/+/58478/comment/ee60f3a0_39f9647b PS21, Line 201: /* Complete description of a chip's write protection configuration */ : struct flashrom_wp_chip_config { : size_t srp_bit_count; : uint8_t srp[MAX_SRP_BITS]; : : bool cmp_bit_present; : uint8_t cmp; : : bool sec_bit_present; : uint8_t sec; : : bool tb_bit_present; : uint8_t tb; : : size_t bp_bit_count; : uint8_t bp[MAX_BP_BITS]; : }; > Well, not by nature, probably by coincidence. I don't see why one couldn't […] I've though about this more and I agree that it would be best to keep it private to writeprotect/wp_ranges code. I will move it to writeprotect.h. -- To view, visit https://review.coreboot.org/c/flashrom/+/58478 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I17dee630248ce7b51e624a6e46d7097d5d0de809 Gerrit-Change-Number: 58478 Gerrit-PatchSet: 24 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: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Tue, 11 Jan 2022 06:25:58 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit