January 2023 - flashrom-gerrit

[M] Change in flashrom[master]: tests/chip: Add non-aligned write within a region unit-test
by Edward O'Callaghan (Code Review) Jan. 12, 2023

Jan. 12, 2023

Attention is currently required from: Sam McNally, Edward O'Callaghan, Anastasia Klimchuk. Hello Sam McNally, build bot (Jenkins), Evan Benn, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/71659 to look at the new patch set (#5). Change subject: tests/chip: Add non-aligned write within a region unit-test ...................................................................... tests/chip: Add non-aligned write within a region unit-test A written region that is sized below that of the erasure granularity can result in a incorrectly read region that does not include prior content within the region before the write op. This was dealt with in ChromeOS downstream by expanding out the read to match the erase granularity however does not seem to impact upstream. Add a unit-test to avoid regression as this is important behaviour to cover. Change-Id: Id3ce5cd1936f0f348d34a6c77cee15e27a5c353f Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M tests/chip.c M tests/tests.c M tests/tests.h 3 files changed, 110 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/59/71659/5 -- To view, visit https://review.coreboot.org/c/flashrom/+/71659 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Id3ce5cd1936f0f348d34a6c77cee15e27a5c353f Gerrit-Change-Number: 71659 Gerrit-PatchSet: 5 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Evan Benn <evanbenn(a)google.com> Gerrit-Reviewer: Sam McNally <sammc(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Sam McNally <sammc(a)google.com> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

[M] Change in flashrom[master]: tree/: Drop default_spi_probe_opcode for NULL case
by Edward O'Callaghan (Code Review) Jan. 12, 2023

Jan. 12, 2023

Attention is currently required from: Thomas Heijligen, Aarya, Anastasia Klimchuk. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/70264 ) Change subject: tree/: Drop default_spi_probe_opcode for NULL case ...................................................................... Patch Set 5: (2 comments) Patchset: PS1: > I looked into remaining Aarya's patches, and it seems like there won't be conflicts, my understandin […] Correct. Patchset: PS4: > Needs to be rebased, otherwise looks good (hope my other comment is correct). Done. -- To view, visit https://review.coreboot.org/c/flashrom/+/70264 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Id502c5d2596ad1db52faf05723083620e4c52c12 Gerrit-Change-Number: 70264 Gerrit-PatchSet: 5 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Peter Marheine <pmarheine(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-Attention: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Thu, 12 Jan 2023 01:29:17 +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> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: tree/: Drop default_spi_probe_opcode for NULL case
by Edward O'Callaghan (Code Review) Jan. 12, 2023

Jan. 12, 2023

Attention is currently required from: Thomas Heijligen, Edward O'Callaghan, Aarya. Hello Felix Singer, build bot (Jenkins), Thomas Heijligen, Aarya, Anastasia Klimchuk, Peter Marheine, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/70264 to look at the new patch set (#5). Change subject: tree/: Drop default_spi_probe_opcode for NULL case ...................................................................... tree/: Drop default_spi_probe_opcode for NULL case A NULL func pointer is necessary and sufficient for the condition `NULL func pointer => true' as to not need this boilerplate as it implies default behaviour of a supported opcode within the `check_block_eraser()` match supported loop. Ran; ``` $ find . -name '*.[c,h]' -exec sed -i '/.probe_opcode = default_spi_probe_opcode,/d' '{}' \; ``` Change-Id: Id502c5d2596ad1db52faf05723083620e4c52c12 Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M bitbang_spi.c M buspirate_spi.c M ch341a_spi.c M dediprog.c M digilent_spi.c M dirtyjtag_spi.c M flashrom.c M ft2232_spi.c M include/chipdrivers.h M include/programmer.h M it87spi.c M jlink_spi.c M linux_spi.c M mediatek_i2c_spi.c M mstarddc_spi.c M ni845x_spi.c M parade_lspcon.c M pickit2_spi.c M raiden_debug_spi.c M realtek_mst_i2c_spi.c M sb600spi.c M serprog.c M spi.c M spi25_statusreg.c M stlinkv3_spi.c M usbblaster_spi.c M wbsio_spi.c 27 files changed, 30 insertions(+), 32 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/64/70264/5 -- To view, visit https://review.coreboot.org/c/flashrom/+/70264 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Id502c5d2596ad1db52faf05723083620e4c52c12 Gerrit-Change-Number: 70264 Gerrit-PatchSet: 5 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Peter Marheine <pmarheine(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-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Aarya <aarya.chaumal(a)gmail.com> Gerrit-MessageType: newpatchset

1 0

[M] Change in flashrom[master]: tree/: Drop default_spi_probe_opcode for NULL case
by Anastasia Klimchuk (Code Review) Jan. 12, 2023

Jan. 12, 2023

Attention is currently required from: Thomas Heijligen, Edward O'Callaghan, Aarya. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/70264 ) Change subject: tree/: Drop default_spi_probe_opcode for NULL case ...................................................................... Patch Set 4: Code-Review+2 (2 comments) Patchset: PS1: > rebased, it should be fine to merge now? I looked into remaining Aarya's patches, and it seems like there won't be conflicts, my understanding that the ones that had overlap in code has already been merged. Thomas/Edward, please correct me if I am wrong. Patchset: PS4: Needs to be rebased, otherwise looks good (hope my other comment is correct). -- To view, visit https://review.coreboot.org/c/flashrom/+/70264 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Id502c5d2596ad1db52faf05723083620e4c52c12 Gerrit-Change-Number: 70264 Gerrit-PatchSet: 4 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Peter Marheine <pmarheine(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-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Comment-Date: Thu, 12 Jan 2023 01:09:01 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes 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]: tests/chip.c: Set MOCK_CHIP_CONTENT non-ambiguously
by Anastasia Klimchuk (Code Review) Jan. 12, 2023

Jan. 12, 2023

Attention is currently required from: Edward O'Callaghan. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/71826 ) Change subject: tests/chip.c: Set MOCK_CHIP_CONTENT non-ambiguously ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/71826 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I15905180141aee54c166ff1c0275d1a7dfde0a46 Gerrit-Change-Number: 71826 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Comment-Date: Thu, 12 Jan 2023 00:47:31 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: programmer: Add bitbanging programmer driver for Linux libgpiod
by Anastasia Klimchuk (Code Review) Jan. 12, 2023

Jan. 12, 2023

Attention is currently required from: Thomas Heijligen, Angel Pons, Steve Markgraf. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/71801 ) Change subject: programmer: Add bitbanging programmer driver for Linux libgpiod ...................................................................... Patch Set 1: (2 comments) Patchset: PS1: Hello Steve, Thank you so much for your contribution! While this will need to go through all reviews from other people as well, I have a question to you. As you are an original author of the programmer, would you agree to add yourself into MAINTAINERS file for this programmer? This would mean you will be automatically added as reviewer if someone sends a patch which touches code for the programmer. Also since MAINTAINERS file is a bit of documentation, you will be the person who "knows something about this programmer" and people might ask you questions :) It would be great if you agree! As I said, the patch needs to go through all reviews, but I decided to ask a question now so you can think about it. Thank you! File linux_gpiod.c: https://review.coreboot.org/c/flashrom/+/71801/comment/2e5df448_ff6592ac PS1, Line 148: if (data) { : if (gpiod_line_bulk_num_lines(&data->bulk) > 0) : gpiod_line_release_bulk(&data->bulk); : : free(data); : } : : if (chip) : gpiod_chip_close(chip); I have few questions about the error path exit. 1) It looks like it is doing the same as shutdown function (which of course makes sense). Is it possible to try re-write a bit, so that just call shutdown function? If any changes are needed later for shutdown process, only one place would need to be changed. 2) The first two callsites of `goto err_exit` , where there is no data and chip is not opened yet: does anything need to be done there? could that be `return 1` (tell me if I am wrong)? I am thinking that maybe those two first callsites could be `return 1` and then err_exit could just call shutdown function? -- To view, visit https://review.coreboot.org/c/flashrom/+/71801 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Icad3eb7764f28feaea51bda3a7893da724c86d06 Gerrit-Change-Number: 71801 Gerrit-PatchSet: 1 Gerrit-Owner: Steve Markgraf <steve(a)steve-m.de> 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-CC: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Steve Markgraf <steve(a)steve-m.de> Gerrit-Comment-Date: Thu, 12 Jan 2023 00:46:48 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: tests/: Add non-aligned write within a region unit-test
by Evan Benn (Code Review) Jan. 12, 2023

Jan. 12, 2023

Attention is currently required from: Sam McNally, Edward O'Callaghan, Anastasia Klimchuk. Evan Benn has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/71659 ) Change subject: tests/: Add non-aligned write within a region unit-test ...................................................................... Patch Set 4: Code-Review+1 (8 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/71659/comment/e022e2b5_a5c37f5c PS2, Line 7: tests/: Add subregion alignment unit-test > Done What does non-aligned mean? The commit message can match the test name File tests/chip.c: https://review.coreboot.org/c/flashrom/+/71659/comment/866dc56d_59aa0572 PS2, Line 469: * {MOCK_CHIP_SUBREGION_CONTENTS} [..] {MOCK_CHIP_SUBREGION_CONTENTS}. > Done I dont understand this: `{MOCK_CHIP_SUBREGION_CONTENTS} [..] {MOCK_CHIP_SUBREGION_CONTENTS}` File tests/chip.c: https://review.coreboot.org/c/flashrom/+/71659/comment/4bfa7968_2a006efd PS4, Line 443: /* FIXME: MOCK_CHIP_CONTENT is buggy within setup_chip, it should also should this comment be here? if its buggy does the test work? what bug? https://review.coreboot.org/c/flashrom/+/71659/comment/5887cb65_e01c274f PS4, Line 447: #define MOCK_CHIP_SUBREGION_CONTENTS 0xCC CC is now the default setting of the chip, probably doesnt matter https://review.coreboot.org/c/flashrom/+/71659/comment/ee2c82e2_129af283 PS4, Line 457: /* Expect to verify the write completed successfuly within the region. */ is the presence of these flags important for catching the bug? If so, document why. If not, remove them. https://review.coreboot.org/c/flashrom/+/71659/comment/bd3c251f_82049b0f PS4, Line 469: * Create subregion smaller than erase granularity of chip. subregion -> region whole file replace please https://review.coreboot.org/c/flashrom/+/71659/comment/417ed310_671617d6 PS4, Line 498: /* .. */ remove https://review.coreboot.org/c/flashrom/+/71659/comment/aa83260a_d1be3a51 PS4, Line 500: /* Expect a verification pass that content within the region however this doesnt make sense -- To view, visit https://review.coreboot.org/c/flashrom/+/71659 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Id3ce5cd1936f0f348d34a6c77cee15e27a5c353f Gerrit-Change-Number: 71659 Gerrit-PatchSet: 4 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Evan Benn <evanbenn(a)google.com> Gerrit-Reviewer: Sam McNally <sammc(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Sam McNally <sammc(a)google.com> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Thu, 12 Jan 2023 00:40:20 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Comment-In-Reply-To: Evan Benn <evanbenn(a)google.com> Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: flashrom_tester: Remove --output log redirect option
by Peter Marheine (Code Review) Jan. 12, 2023

Jan. 12, 2023

Attention is currently required from: Edward O'Callaghan, Evan Benn. Peter Marheine has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/71773 ) Change subject: flashrom_tester: Remove --output log redirect option ...................................................................... Patch Set 2: Code-Review+1 -- To view, visit https://review.coreboot.org/c/flashrom/+/71773 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I5eab8169644a16ba31b203e8607853c459f92978 Gerrit-Change-Number: 71773 Gerrit-PatchSet: 2 Gerrit-Owner: Evan Benn <evanbenn(a)google.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> 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-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Evan Benn <evanbenn(a)google.com> Gerrit-Comment-Date: Thu, 12 Jan 2023 00:28:29 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: tests/chip.c: Set MOCK_CHIP_CONTENT non-ambiguously
by Edward O'Callaghan (Code Review) Jan. 12, 2023

Jan. 12, 2023

Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/71826 ) Change subject: tests/chip.c: Set MOCK_CHIP_CONTENT non-ambiguously ...................................................................... tests/chip.c: Set MOCK_CHIP_CONTENT non-ambiguously A chip content setup as 0x00 is ambiguous from a zero'ed heap or some erased chips and 0xFF is ambiguous from an erased chip. Change-Id: I15905180141aee54c166ff1c0275d1a7dfde0a46 Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M tests/chip.c 1 file changed, 14 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/26/71826/1 diff --git a/tests/chip.c b/tests/chip.c index 1d9cdc9..580f4ea 100644 --- a/tests/chip.c +++ b/tests/chip.c @@ -37,7 +37,7 @@ #include "programmer.h" #define MOCK_CHIP_SIZE (8*MiB) -#define MOCK_CHIP_CONTENT 0xff +#define MOCK_CHIP_CONTENT 0xCC /* 0x00 is a zeroed heap and 0xFF is an erased chip. */ static struct { unsigned int unlock_calls; /* how many times unlock function was called */ -- To view, visit https://review.coreboot.org/c/flashrom/+/71826 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I15905180141aee54c166ff1c0275d1a7dfde0a46 Gerrit-Change-Number: 71826 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: newchange

1 0

[M] Change in flashrom[master]: flashrom_tester: Remove --output log redirect option
by Nikolai Artemiev (Code Review) Jan. 12, 2023

Jan. 12, 2023

Attention is currently required from: Edward O'Callaghan, Evan Benn, Peter Marheine. Nikolai Artemiev has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/71773 ) Change subject: flashrom_tester: Remove --output log redirect option ...................................................................... Patch Set 2: Code-Review+1 (1 comment) Patchset: PS2: I support removing this, we get weird diffs with the `--output` option. `log()` calls are included in both stdout and the log file, but `println()!` is only included in stdout. Unix style redirect is more reliable for now. -- To view, visit https://review.coreboot.org/c/flashrom/+/71773 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I5eab8169644a16ba31b203e8607853c459f92978 Gerrit-Change-Number: 71773 Gerrit-PatchSet: 2 Gerrit-Owner: Evan Benn <evanbenn(a)google.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> 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-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Evan Benn <evanbenn(a)google.com> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Comment-Date: Thu, 12 Jan 2023 00:11:44 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0