October 2023 - flashrom-gerrit

[XS] Change in flashrom[main]: ich_descriptors: Fix debug print
by Anastasia Klimchuk (Code Review) 23 Oct '23

23 Oct '23

Attention is currently required from: Patrick Rudolph. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/78300?usp=email ) Change subject: ich_descriptors: Fix debug print ...................................................................... Patch Set 2: Code-Review+1 -- To view, visit https://review.coreboot.org/c/flashrom/+/78300?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: Ib9276a6c29952487db6e60fb583942c0f24cd6ef Gerrit-Change-Number: 78300 Gerrit-PatchSet: 2 Gerrit-Owner: Patrick Rudolph <rudolphpatrick05(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Attention: Patrick Rudolph <rudolphpatrick05(a)gmail.com> Gerrit-Comment-Date: Mon, 23 Oct 2023 10:39:27 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[XS] Change in flashrom[main]: ich_descriptors: Fix debug print
by Anastasia Klimchuk (Code Review) 23 Oct '23

23 Oct '23

Attention is currently required from: Patrick Rudolph. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/78300?usp=email ) Change subject: ich_descriptors: Fix debug print ...................................................................... Patch Set 2: Code-Review+2 (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/78300/comment/1ac28927_6519e499 : PS2, Line 9: Allow nm, the number of flash masters, to be equal to : ARRAY_SIZE(master_names). The previous logic was probably overlocked when : ich_number_of_masters() was introduced. The loop below makes sure that it : doesn't access the master_names array out of bounds. You need to wrap commit message at 72 char max. You can visually notice the current version is wider and long lines are broken. -- To view, visit https://review.coreboot.org/c/flashrom/+/78300?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: Ib9276a6c29952487db6e60fb583942c0f24cd6ef Gerrit-Change-Number: 78300 Gerrit-PatchSet: 2 Gerrit-Owner: Patrick Rudolph <rudolphpatrick05(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Attention: Patrick Rudolph <rudolphpatrick05(a)gmail.com> Gerrit-Comment-Date: Mon, 23 Oct 2023 10:39:19 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[XS] Change in flashrom[main]: erasure_layout: Add region boundaries to printed info message
by Anastasia Klimchuk (Code Review) 21 Oct '23

21 Oct '23

Attention is currently required from: Aarya, Alexander Goncharov. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/78492?usp=email ) Change subject: erasure_layout: Add region boundaries to printed info message ...................................................................... Patch Set 2: (1 comment) File erasure_layout.c: https://review.coreboot.org/c/flashrom/+/78492/comment/31b0a7ca_a5018705 : PS1, Line 177: msg_cinfo("Region start 0x%08x not sector aligned! Extending start boundaries by 0x%08x.\n", > maybe write "Extending start boundaries by 0x%08x bytes. […] I added "bytes" thank you, good idea. This patch came up in process of debugging https://ticket.coreboot.org/issues/494 , you are very welcome to have a look and say what you think about it! If you look my last comment, with 1-line change in existing test, the issue can repro (test fails). So you don't even need images or any hardware. At the end I will add a proper test to cover this case, but for debugging phase, changing 1 line of code should be fine :) -- To view, visit https://review.coreboot.org/c/flashrom/+/78492?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I511a79754cff15e7dba26f5313d7015830780f60 Gerrit-Change-Number: 78492 Gerrit-PatchSet: 2 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Attention: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Comment-Date: Sat, 21 Oct 2023 12:14:37 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Aarya <aarya.chaumal(a)gmail.com> Gerrit-MessageType: comment

1 0

[XS] Change in flashrom[main]: erasure_layout: Add region boundaries to printed info message
by Anastasia Klimchuk (Code Review) 21 Oct '23

21 Oct '23

Attention is currently required from: Aarya, Anastasia Klimchuk. Hello Aarya, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/78492?usp=email to look at the new patch set (#2). The following approvals got outdated and were removed: Code-Review+2 by Aarya, Verified+1 by build bot (Jenkins) The change is no longer submittable: Code-Review and Verified are unsatisfied now. Change subject: erasure_layout: Add region boundaries to printed info message ...................................................................... erasure_layout: Add region boundaries to printed info message Change-Id: I511a79754cff15e7dba26f5313d7015830780f60 Signed-off-by: Anastasia Klimchuk <aklm(a)flashrom.org> --- M erasure_layout.c 1 file changed, 4 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/92/78492/2 -- To view, visit https://review.coreboot.org/c/flashrom/+/78492?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I511a79754cff15e7dba26f5313d7015830780f60 Gerrit-Change-Number: 78492 Gerrit-PatchSet: 2 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

[XS] Change in flashrom[main]: erasure_layout: Add region boundaries to printed info message
by Aarya (Code Review) 21 Oct '23

21 Oct '23

Attention is currently required from: Anastasia Klimchuk. Aarya has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/78492?usp=email ) Change subject: erasure_layout: Add region boundaries to printed info message ...................................................................... Patch Set 1: (1 comment) File erasure_layout.c: https://review.coreboot.org/c/flashrom/+/78492/comment/2540d9ad_f78ddeba : PS1, Line 177: msg_cinfo("Region start 0x%08x not sector aligned! Extending start boundaries by 0x%08x.\n", maybe write "Extending start boundaries by 0x%08x bytes.\n" -- To view, visit https://review.coreboot.org/c/flashrom/+/78492?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I511a79754cff15e7dba26f5313d7015830780f60 Gerrit-Change-Number: 78492 Gerrit-PatchSet: 1 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Sat, 21 Oct 2023 10:54:54 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[XS] Change in flashrom[main]: erasure_layout: Add region boundaries to printed info message
by Aarya (Code Review) 21 Oct '23

21 Oct '23

Attention is currently required from: Anastasia Klimchuk. Aarya has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/78492?usp=email ) Change subject: erasure_layout: Add region boundaries to printed info message ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/78492?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I511a79754cff15e7dba26f5313d7015830780f60 Gerrit-Change-Number: 78492 Gerrit-PatchSet: 1 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Sat, 21 Oct 2023 10:52:25 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[XS] Change in flashrom[main]: erasure_layout: Add region boundaries to printed info message
by Anastasia Klimchuk (Code Review) 21 Oct '23

21 Oct '23

Anastasia Klimchuk has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/78492?usp=email ) Change subject: erasure_layout: Add region boundaries to printed info message ...................................................................... erasure_layout: Add region boundaries to printed info message Change-Id: I511a79754cff15e7dba26f5313d7015830780f60 Signed-off-by: Anastasia Klimchuk <aklm(a)flashrom.org> --- M erasure_layout.c 1 file changed, 4 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/92/78492/1 diff --git a/erasure_layout.c b/erasure_layout.c index f360653..d166d50 100644 --- a/erasure_layout.c +++ b/erasure_layout.c @@ -174,11 +174,13 @@ } if (start_diff) { - msg_cinfo("Region start not sector aligned! Extending start boundaries...\n"); + msg_cinfo("Region start 0x%08x not sector aligned! Extending start boundaries by 0x%08x.\n", + *region_start, start_diff); *region_start = *region_start - start_diff; } if (end_diff) { - msg_cinfo("Region end not sector aligned! Extending end boundaries...\n"); + msg_cinfo("Region end 0x%08x not sector aligned! Extending end boundaries by 0x%08x.\n", + *region_end, end_diff); *region_end = *region_end + end_diff; } } -- To view, visit https://review.coreboot.org/c/flashrom/+/78492?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I511a79754cff15e7dba26f5313d7015830780f60 Gerrit-Change-Number: 78492 Gerrit-PatchSet: 1 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: newchange

1 0

[XS] Change in flashrom[main]: flashchips: Mark FM25F01 as tested for read/write/probe/erase
by Anastasia Klimchuk (Code Review) 20 Oct '23

20 Oct '23

Anastasia Klimchuk has submitted this change. ( https://review.coreboot.org/c/flashrom/+/78397?usp=email ) Change subject: flashchips: Mark FM25F01 as tested for read/write/probe/erase ...................................................................... flashchips: Mark FM25F01 as tested for read/write/probe/erase As reported on the mailing list: https://mail.coreboot.org/hyperkitty/list/flashrom@flashrom.org/thread/JSCQ… Change-Id: Ibb65a4cb3345eb67c049aa4d8bfd3260f4bf96db Signed-off-by: Anastasia Klimchuk <aklm(a)flashrom.org> Reviewed-on: https://review.coreboot.org/c/flashrom/+/78397 Reviewed-by: Edward O'Callaghan <quasisec(a)chromium.org> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> --- M flashchips.c 1 file changed, 1 insertion(+), 1 deletion(-) Approvals: Edward O'Callaghan: Looks good to me, approved build bot (Jenkins): Verified diff --git a/flashchips.c b/flashchips.c index e156ffb..9210de2 100644 --- a/flashchips.c +++ b/flashchips.c @@ -6042,7 +6042,7 @@ .page_size = 256, /* OTP: 256B total; enter 0x3A */ .feature_bits = FEATURE_WRSR_WREN | FEATURE_OTP, - .tested = TEST_UNTESTED, + .tested = TEST_OK_PREW, .probe = PROBE_SPI_RDID, .probe_timing = TIMING_ZERO, .block_erasers = { -- To view, visit https://review.coreboot.org/c/flashrom/+/78397?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: Ibb65a4cb3345eb67c049aa4d8bfd3260f4bf96db Gerrit-Change-Number: 78397 Gerrit-PatchSet: 2 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

[S] Change in flashrom[main]: erasure_layout: Fix double invocation of erasers
by Peter Marheine (Code Review) 20 Oct '23

20 Oct '23

Attention is currently required from: Aarya, Alexander Goncharov, Anastasia Klimchuk, Sam McNally. Peter Marheine has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/77747?usp=email ) Change subject: erasure_layout: Fix double invocation of erasers ...................................................................... Patch Set 4: (1 comment) Patchset: PS2: > I did some more investigation and traced when this double-erase started: […] I agree that the new condition seems unnecessary, since it duplicates the loop's termination condition and we know the loop terminates. The block at line 245 should simply be deleted. -- To view, visit https://review.coreboot.org/c/flashrom/+/77747?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I92351eba0fd29114ce98b4a839358e92d176af28 Gerrit-Change-Number: 77747 Gerrit-PatchSet: 4 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Sam McNally <sammc(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Peter Marheine <pmarheine(a)chromium.org> Gerrit-CC: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Sam McNally <sammc(a)google.com> Gerrit-Attention: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Comment-Date: Fri, 20 Oct 2023 04:59:10 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Aarya <aarya.chaumal(a)gmail.com> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

[L] Change in flashrom[main]: tests: Unit tests for erase function selection algo
by Peter Marheine (Code Review) 20 Oct '23

20 Oct '23

Attention is currently required from: Aarya, Anastasia Klimchuk, Edward O'Callaghan, Nikolai Artemiev, Simon Buhrow, Thomas Heijligen. Peter Marheine has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67535?usp=email ) Change subject: tests: Unit tests for erase function selection algo ...................................................................... Patch Set 14: (2 comments) File tests/erase_func_algo.c: https://review.coreboot.org/c/flashrom/+/67535/comment/ae68dff3_9bb37321 : PS3, Line 238: void **state > I found something in cmocka docs, around this page https://api.cmocka.org/group__cmocka__exec. […] I think it's worth doing that, but you don't need to expose much detail. If you call `_cmocka_run_group_tests`, then you need to be given a list of test cases which can be allocated in any way desired. It would also be useful to make each test case in `g_state` its own actual test case so it's easier to read the results rather than lumping a bunch of test cases into one test. To that end, I imagine providing a function that returns a bunch of test cases: ``` static const struct test_case { // ... } test_cases[] = { // ... }; struct test_state { uint8_t chip_contents[1024]; const struct test_case *test_case; }; static int setup(void **state) { const struct test_case *test_case = *state; *state = malloc(sizeof(struct test_state)); state->test_case = test_case; // Initialize other test state as desired return 0; } static int teardown(void **state) { free(*state); return 0; } const CMUnitTest *get_erase_func_algo_tests(size_t *num_tests) { CMUnitTest *out = calloc(ARRAY_SIZE(test_cases), sizeof(*test_cases)); for (size_t i = 0; i < ARRAY_SIZE(test_cases); i++) { out[i] = (CMUnitTest){ .setup_func = setup, .teardown_func = teardown, .initial_state = &test_cases[i], .test_func = run_erase_and_verify, }; } *num_tests = ARRAY_SIZE(test_cases); return out; } ``` Then in main run those test cases: ``` size_t n_erase_tests; const CMUnitTest *erase_tests = get_erase_func_algo_tests(&n_erase_tests); _cmocka_run_group_tests("erase stuff", erase_tests, n_erase_tests, NULL, NULL); ``` File tests/erase_func_algo.c: https://review.coreboot.org/c/flashrom/+/67535/comment/6afc7c8c_9753c252 : PS14, Line 61: struct test_case test_cases[TEST_CASES_NUM]; /* Data for running tests. */ I'd prefer to make this a `const struct test_case *` just to capture that the test case data is immutable, but then it seems even be better to replace `ind` with a pointer to a const `test_case` so functions that receive test state don't have access to irrelevant data (and the only place the table of test cases needs to be wrangled is in the functions that iterate over them to generate each test case). -- To view, visit https://review.coreboot.org/c/flashrom/+/67535?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I8f3fdefb76e71f6f8dc295d9dead5f38642aace7 Gerrit-Change-Number: 67535 Gerrit-PatchSet: 14 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Simon Buhrow Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Alexander Goncharov <chat(a)joursoir.net> Gerrit-CC: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-CC: Peter Marheine <pmarheine(a)chromium.org> Gerrit-CC: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Simon Buhrow 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-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Fri, 20 Oct 2023 02:55:25 +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

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit October 2023