February 2023 - flashrom-gerrit

[S] Change in flashrom[master]: tests: add bus coverage test for dummy
by Anastasia Klimchuk (Code Review) 03 Feb '23

03 Feb '23

Attention is currently required from: Alexander Goncharov. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/72665 ) Change subject: tests: add bus coverage test for dummy ...................................................................... Patch Set 2: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/72665 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Iafe715feb5f5c0b5efd6827cdb2c3a314f542319 Gerrit-Change-Number: 72665 Gerrit-PatchSet: 2 Gerrit-Owner: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Comment-Date: Fri, 03 Feb 2023 07:48:10 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: tests: add bus coverage test for dummy
by Alexander Goncharov (Code Review) 03 Feb '23

03 Feb '23

Attention is currently required from: Anastasia Klimchuk. Alexander Goncharov has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/72665 ) Change subject: tests: add bus coverage test for dummy ...................................................................... Patch Set 2: (1 comment) File tests/dummyflasher.c: https://review.coreboot.org/c/flashrom/+/72665/comment/695bb03c_a71edf47 PS1, Line 126: dummy_cover_buses_test_success > I would rename to `dummy_all_buses_test_success` Done -- To view, visit https://review.coreboot.org/c/flashrom/+/72665 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Iafe715feb5f5c0b5efd6827cdb2c3a314f542319 Gerrit-Change-Number: 72665 Gerrit-PatchSet: 2 Gerrit-Owner: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Fri, 03 Feb 2023 07:39:02 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: tests: add bus coverage test for dummy
by Alexander Goncharov (Code Review) 03 Feb '23

03 Feb '23

Attention is currently required from: Alexander Goncharov. Hello build bot (Jenkins), Anastasia Klimchuk, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/72665 to look at the new patch set (#2). Change subject: tests: add bus coverage test for dummy ...................................................................... tests: add bus coverage test for dummy Dummy programmer has a shared data between *_masters. To make sure the dummy has no memory leakage, we need a test that will covers initialization and shutdown of the programmer with different bus types, i.e. programmer specific, non-SPI and SPI. TEST=ninja test Change-Id: Iafe715feb5f5c0b5efd6827cdb2c3a314f542319 Signed-off-by: Alexander Goncharov <chat(a)joursoir.net> --- M tests/dummyflasher.c M tests/tests.c M tests/tests.h 3 files changed, 38 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/65/72665/2 -- To view, visit https://review.coreboot.org/c/flashrom/+/72665 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Iafe715feb5f5c0b5efd6827cdb2c3a314f542319 Gerrit-Change-Number: 72665 Gerrit-PatchSet: 2 Gerrit-Owner: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Alexander Goncharov <chat(a)joursoir.net> Gerrit-MessageType: newpatchset

1 0

[S] Change in flashrom[master]: jedec.c: Fold up dst into toggle_ready_jedec()
by Anastasia Klimchuk (Code Review) 03 Feb '23

03 Feb '23

Attention is currently required from: Stefan Reinauer, Edward O'Callaghan. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/72691 ) Change subject: jedec.c: Fold up dst into toggle_ready_jedec() ...................................................................... Patch Set 1: (1 comment) File jedec.c: https://review.coreboot.org/c/flashrom/+/72691/comment/0356f8b6_13bae55c PS1, Line 72: const chipaddr dst = flash->virtual_memory; Just wondering, I see that all existing callsites have dst as `flash->virtual_memory` but is this always like that? Can dst be anything else? -- To view, visit https://review.coreboot.org/c/flashrom/+/72691 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ib7a3fdbc6e0a888093dc8da6f5567a7301ec5040 Gerrit-Change-Number: 72691 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Comment-Date: Fri, 03 Feb 2023 06:34:40 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: ni845x_spi: pass global state through func params
by Anastasia Klimchuk (Code Review) 03 Feb '23

03 Feb '23

Attention is currently required from: Miklós Márton, Alexander Goncharov. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/72154 ) Change subject: ni845x_spi: pass global state through func params ...................................................................... Patch Set 2: (4 comments) Patchset: PS2: > This programmer requires a special environment to run, and unfortunately, it only works on Windows. […] Just a small note from me: maybe we can do the code review first (for all the patches in the chain) and then when all comments resolved, do the testing? Also as it's typically happens, testing is done at the end of the chain (not for each individual patch). Let's maybe keep this comment unresolved, and do the rest of code reviews. Miklós thank you so much for help! File ni845x_spi.c: https://review.coreboot.org/c/flashrom/+/72154/comment/d3c279e4_dc4bf5cf PS2, Line 131: static int32 ni845x_spi_open_resource(char *resource_handle, uInt32 *opened_handle, : enum USB845x_type device_pid) I am thinking, this could fit in one line: the hard limit for line length is 112 chars. Of course not all the lines need to be that long, but for the case like this (function arguments list) we can use one line, mainly because it makes it easier to grep. https://review.coreboot.org/c/flashrom/+/72154/comment/396a202a_216c01c0 PS2, Line 152: static int ni845x_spi_open(const char *serial, uInt32 *return_handle, : enum USB845x_type *device_pid) this can be one line too https://review.coreboot.org/c/flashrom/+/72154/comment/d9cbca5d_cd118ba6 PS2, Line 233: set_io_voltage_mV Not sure you meant it: there is `set_io_voltage_mV` which is an existing second argument in the function, and there is also `io_voltage_in_mV` which is global. Why do you replace one with the other? To follow what the patch is doing, you would need to add one more argument to this function, `io_voltage_in_mV` and use it instead of a global var? -- To view, visit https://review.coreboot.org/c/flashrom/+/72154 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I5daeb0839a4cc18b82d38cc06eeba88a619bec61 Gerrit-Change-Number: 72154 Gerrit-PatchSet: 2 Gerrit-Owner: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-Attention: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Comment-Date: Fri, 03 Feb 2023 06:27:18 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Alexander Goncharov <chat(a)joursoir.net> Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: dummyflasher: use new API to register shutdown function
by Anastasia Klimchuk (Code Review) 03 Feb '23

03 Feb '23

Attention is currently required from: Edward O'Callaghan, Alexander Goncharov. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/72408 ) Change subject: dummyflasher: use new API to register shutdown function ...................................................................... Patch Set 2: (3 comments) File dummyflasher.c: https://review.coreboot.org/c/flashrom/+/72408/comment/7529d834_1e795536 PS2, Line 79: uint8_t refs_cnt; Can you add a comment on this: to explain that dummyflasher can register multiple masters and they all share the same emu_data, so keeping track of reference count is needed to do clean up at shutdown time. https://review.coreboot.org/c/flashrom/+/72408/comment/71262ae6_032575af PS2, Line 910: > the last owner of the data must release this data yeah that's a good argument, let's keep it. I mark resolved https://review.coreboot.org/c/flashrom/+/72408/comment/ed9a83ea_04ffbf74 PS2, Line 1416: data->refs_cnt += ((dummy_buses_supported & BUS_PROG) != BUS_NONE); : data->refs_cnt += ((dummy_buses_supported & BUS_NONSPI) != BUS_NONE); : data->refs_cnt += ((dummy_buses_supported & BUS_SPI) != BUS_NONE); : : #if 0 : data->refs_cnt += (dummy_buses_supported & BUS_PROG) ? 1 : 0; : data->refs_cnt += (dummy_buses_supported & BUS_NONSPI) ? 1 : 0; : data->refs_cnt += (dummy_buses_supported & BUS_SPI) ? 1 : 0; : #endif > Which one is more appropriate? I have another idea! :) What if you add `refs_cnt++` into every if below. There are the same three if conditions below, already here. -- To view, visit https://review.coreboot.org/c/flashrom/+/72408 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I0c67c25b0f53cd8c564c4ea0f09f2728e856f6ea Gerrit-Change-Number: 72408 Gerrit-PatchSet: 2 Gerrit-Owner: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Comment-Date: Fri, 03 Feb 2023 05:38:36 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Comment-In-Reply-To: Alexander Goncharov <chat(a)joursoir.net> Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: tests: add bus coverage test for dummy
by Anastasia Klimchuk (Code Review) 03 Feb '23

03 Feb '23

Attention is currently required from: Alexander Goncharov. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/72665 ) Change subject: tests: add bus coverage test for dummy ...................................................................... Patch Set 1: Code-Review+1 (2 comments) Patchset: PS1: just one little comment File tests/dummyflasher.c: https://review.coreboot.org/c/flashrom/+/72665/comment/4ef5f5ca_f1791f3a PS1, Line 126: dummy_cover_buses_test_success I would rename to `dummy_all_buses_test_success` -- To view, visit https://review.coreboot.org/c/flashrom/+/72665 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Iafe715feb5f5c0b5efd6827cdb2c3a314f542319 Gerrit-Change-Number: 72665 Gerrit-PatchSet: 1 Gerrit-Owner: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Comment-Date: Fri, 03 Feb 2023 05:16:41 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: dummyflasher: use new API to register shutdown function
by Alexander Goncharov (Code Review) 02 Feb '23

02 Feb '23

Attention is currently required from: Edward O'Callaghan, Anastasia Klimchuk. Alexander Goncharov has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/72408 ) Change subject: dummyflasher: use new API to register shutdown function ...................................................................... Patch Set 2: (1 comment) File dummyflasher.c: https://review.coreboot.org/c/flashrom/+/72408/comment/cce6a0d1_52e92a26 PS2, Line 1416: data->refs_cnt += ((dummy_buses_supported & BUS_PROG) != BUS_NONE); : data->refs_cnt += ((dummy_buses_supported & BUS_NONSPI) != BUS_NONE); : data->refs_cnt += ((dummy_buses_supported & BUS_SPI) != BUS_NONE); : : #if 0 : data->refs_cnt += (dummy_buses_supported & BUS_PROG) ? 1 : 0; : data->refs_cnt += (dummy_buses_supported & BUS_NONSPI) ? 1 : 0; : data->refs_cnt += (dummy_buses_supported & BUS_SPI) ? 1 : 0; : #endif Which one is more appropriate? -- To view, visit https://review.coreboot.org/c/flashrom/+/72408 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I0c67c25b0f53cd8c564c4ea0f09f2728e856f6ea Gerrit-Change-Number: 72408 Gerrit-PatchSet: 2 Gerrit-Owner: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Thu, 02 Feb 2023 13:38:46 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: tests: add bus coverage test for dummy
by Alexander Goncharov (Code Review) 02 Feb '23

02 Feb '23

Alexander Goncharov has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/72665 ) Change subject: tests: add bus coverage test for dummy ...................................................................... tests: add bus coverage test for dummy Dummy programmer has a shared data between *_masters. To make sure the dummy has no memory leakage, we need a test that will covers initialization and shutdown of the programmer with different bus types, i.e. programmer specific, non-SPI and SPI. TEST=ninja test Change-Id: Iafe715feb5f5c0b5efd6827cdb2c3a314f542319 Signed-off-by: Alexander Goncharov <chat(a)joursoir.net> --- M tests/dummyflasher.c M tests/tests.c M tests/tests.h 3 files changed, 38 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/65/72665/1 diff --git a/tests/dummyflasher.c b/tests/dummyflasher.c index c41f9df..5a85cf3 100644 --- a/tests/dummyflasher.c +++ b/tests/dummyflasher.c @@ -123,6 +123,24 @@ run_basic_lifecycle(state, &dummy_io, &programmer_dummy, NULL); } +void dummy_cover_buses_test_success(void **state) +{ + struct io_mock_fallback_open_state dummy_fallback_open_state = { + .noc = 0, + .paths = { NULL }, + }; + const struct io_mock dummy_io = { + .fallback_open_state = &dummy_fallback_open_state, + }; + + run_basic_lifecycle(state, &dummy_io, &programmer_dummy, "bus=lpc+fwh"); + run_basic_lifecycle(state, &dummy_io, &programmer_dummy, "bus=spi"); + run_basic_lifecycle(state, &dummy_io, &programmer_dummy, "bus=prog"); + run_basic_lifecycle(state, &dummy_io, &programmer_dummy, "bus=parallel+fwh+prog"); + run_basic_lifecycle(state, &dummy_io, &programmer_dummy, "bus=spi+prog"); + run_basic_lifecycle(state, &dummy_io, &programmer_dummy, "bus=parallel+lpc+spi"); +} + #else SKIP_TEST(dummy_basic_lifecycle_test_success) SKIP_TEST(dummy_probe_lifecycle_test_success) @@ -131,4 +149,5 @@ SKIP_TEST(dummy_init_success_invalid_param_test_success) SKIP_TEST(dummy_init_success_unhandled_param_test_success) SKIP_TEST(dummy_null_prog_param_test_success) + SKIP_TEST(dummy_cover_buses_test_success) #endif /* CONFIG_DUMMY */ diff --git a/tests/tests.c b/tests/tests.c index 0912f35..35abb51 100644 --- a/tests/tests.c +++ b/tests/tests.c @@ -447,6 +447,7 @@ cmocka_unit_test(dummy_init_success_invalid_param_test_success), cmocka_unit_test(dummy_init_success_unhandled_param_test_success), cmocka_unit_test(dummy_null_prog_param_test_success), + cmocka_unit_test(dummy_cover_buses_test_success), cmocka_unit_test(nicrealtek_basic_lifecycle_test_success), cmocka_unit_test(raiden_debug_basic_lifecycle_test_success), cmocka_unit_test(dediprog_basic_lifecycle_test_success), diff --git a/tests/tests.h b/tests/tests.h index bdcfdae..59a00f3 100644 --- a/tests/tests.h +++ b/tests/tests.h @@ -51,6 +51,7 @@ void dummy_init_success_invalid_param_test_success(void **state); void dummy_init_success_unhandled_param_test_success(void **state); void dummy_null_prog_param_test_success(void **state); +void dummy_cover_buses_test_success(void **state); void nicrealtek_basic_lifecycle_test_success(void **state); void raiden_debug_basic_lifecycle_test_success(void **state); void dediprog_basic_lifecycle_test_success(void **state); -- To view, visit https://review.coreboot.org/c/flashrom/+/72665 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Iafe715feb5f5c0b5efd6827cdb2c3a314f542319 Gerrit-Change-Number: 72665 Gerrit-PatchSet: 1 Gerrit-Owner: Alexander Goncharov <chat(a)joursoir.net> Gerrit-MessageType: newchange

1 0

[S] Change in flashrom[master]: jedec.c: Trivial code style fix
by Edward O'Callaghan (Code Review) 02 Feb '23

02 Feb '23

Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/72606 ) ( 1 is the latest approved patch-set. No files were changed between the latest approved patch-set and the submitted one. )Change subject: jedec.c: Trivial code style fix ...................................................................... jedec.c: Trivial code style fix Change-Id: I42ebdda07512d0a84a6bd6d0630f96c40f039259 Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> Reviewed-on: https://review.coreboot.org/c/flashrom/+/72606 Reviewed-by: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> --- M jedec.c 1 file changed, 15 insertions(+), 4 deletions(-) Approvals: build bot (Jenkins): Verified Stefan Reinauer: Looks good to me, approved diff --git a/jedec.c b/jedec.c index b44b9c6..e97468e 100644 --- a/jedec.c +++ b/jedec.c @@ -196,8 +196,7 @@ if (probe_timing_enter) programmer_delay(flash, probe_timing_enter); /* Reset chip to a clean slate */ - if ((chip->feature_bits & FEATURE_RESET_MASK) == FEATURE_LONG_RESET) - { + if ((chip->feature_bits & FEATURE_RESET_MASK) == FEATURE_LONG_RESET) { chip_writeb(flash, 0xAA, bios + ((shifted ? 0x2AAA : 0x5555) & mask)); if (probe_timing_exit) programmer_delay(flash, 10); @@ -239,8 +238,7 @@ } /* Issue JEDEC Product ID Exit command */ - if ((chip->feature_bits & FEATURE_RESET_MASK) == FEATURE_LONG_RESET) - { + if ((chip->feature_bits & FEATURE_RESET_MASK) == FEATURE_LONG_RESET) { chip_writeb(flash, 0xAA, bios + ((shifted ? 0x2AAA : 0x5555) & mask)); if (probe_timing_exit) programmer_delay(flash, 10); -- To view, visit https://review.coreboot.org/c/flashrom/+/72606 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I42ebdda07512d0a84a6bd6d0630f96c40f039259 Gerrit-Change-Number: 72606 Gerrit-PatchSet: 3 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit February 2023