September 2022 - flashrom-gerrit

[S] Change in flashrom[master]: it87spi.c: Allow passing programmer_cfg directly
by Anastasia Klimchuk (Code Review) 05 Sep '22

05 Sep '22

Attention is currently required from: Edward O'Callaghan. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/66662 ) Change subject: it87spi.c: Allow passing programmer_cfg directly ...................................................................... Patch Set 8: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/66662 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I598b1811c9734f41eee205d5a2b51ad8ac79e3ab Gerrit-Change-Number: 66662 Gerrit-PatchSet: 8 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Comment-Date: Mon, 05 Sep 2022 01:07:15 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: tree: Allow passing programmer_cfg directly to programmer
by Anastasia Klimchuk (Code Review) 05 Sep '22

05 Sep '22

Attention is currently required from: Edward O'Callaghan, Nikolai Artemiev. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/66655 ) Change subject: tree: Allow passing programmer_cfg directly to programmer ...................................................................... Patch Set 8: Code-Review+2 (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/66655/comment/de041396_a4b0297b PS8, Line 14: $ find -name '*.c' -exec sed -i 's/_init(void)/_init(const char *prog_param)/g' '{}' \; : $ find -name '*.c' -exec sed -i 's/get_params(/get_params(const char *prog_param, /g' '{}' \; : $ find -name '*.c' -exec sed -i 's/const char \*prog_param)/const struct programmer_cfg *cfg)/g' '{}' \; : $ find -name '*.c' -exec sed -i 's/const char \*prog_param,/const struct programmer_cfg *cfg,/g' '{}' \; Ditto -- To view, visit https://review.coreboot.org/c/flashrom/+/66655 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I8bab51a635b9d3a43e1619a7a32b334f4ce2cdd2 Gerrit-Change-Number: 66655 Gerrit-PatchSet: 8 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> 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: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Mon, 05 Sep 2022 01:06:59 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: tree: Change signature of extract_programmer_param_str()
by Anastasia Klimchuk (Code Review) 05 Sep '22

05 Sep '22

Attention is currently required from: Nico Huber, Edward O'Callaghan, Nikolai Artemiev. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/66654 ) Change subject: tree: Change signature of extract_programmer_param_str() ...................................................................... Patch Set 5: Code-Review+2 (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/66654/comment/24d01466_76115de6 PS5, Line 11: $ find -name '*.c' -exec sed -i 's/extract_programmer_param_str(/extract_programmer_param_str(NULL, /g' '{}' \; This seems wider than 72 chars, needs to be wrapped. I understand this is a command, maybe you can insert a line break in the middle to fit into 72 char width? -- To view, visit https://review.coreboot.org/c/flashrom/+/66654 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I781a328fa280e0a9601050dd99a75af72c39c899 Gerrit-Change-Number: 66654 Gerrit-PatchSet: 5 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> 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-CC: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Mon, 05 Sep 2022 01:06:30 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: fix scan build make issues
by Felix Singer (Code Review) 05 Sep '22

05 Sep '22

Attention is currently required from: Dhiren Serai. Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67331 ) Change subject: fix scan build make issues ...................................................................... Patch Set 1: (4 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/67331/comment/55e5aff6_c203cbc0 PS1, Line 7: fix scan build make issues Please don't just refer to scan-build. The issues found by scan-build might be actual issues, but they also could be false-positives. Rather add a short note in the summary below that this was found by scan-build. However, please use a meaningful commit title so that people get an idea what this commit is about. https://review.coreboot.org/c/flashrom/+/67331/comment/d9c30c3b_734da1cd PS1, Line 9: scan build make was showing issues with variables using malloc for uninitialized/garbage values. : For fixing that I used calloc which sets allocated memory to zero as default so it will not be uninitialized/garbage Please wrap the summary at 72 characters. The subject line should be limited to 50 characters. In some cases it can be expanded to 72 characters if there is no other way around it. I recommend reading this about commit messages in general https://cbea.ms/git-commit. File sfdp.c: https://review.coreboot.org/c/flashrom/+/67331/comment/e6ae4c2f_86a7402f PS1, Line 298: hbuf = calloc(8,(nph + 1) * 8); The buffer gets overridden in spi_sfdp_read_sfdp() anyway, so it doesn't matter much if malloc is used. But I also don't have a strong opinion aginst calloc. Definitely not bad to have zeroed memory 😊 Though, it should be `calloc(8, nph + 1)`. https://review.coreboot.org/c/flashrom/+/67331/comment/2a9e7f3a_e0e5e640 PS1, Line 334: len Add a space before `len`. -- To view, visit https://review.coreboot.org/c/flashrom/+/67331 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Id3390a40033408d827d89c3af4cd7b799e254271 Gerrit-Change-Number: 67331 Gerrit-PatchSet: 1 Gerrit-Owner: Dhiren Serai <dhirensr(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Dhiren Serai <dhirensr(a)gmail.com> Gerrit-Comment-Date: Mon, 05 Sep 2022 00:56:42 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: tests: Add workaround to allow tests mock fileno on FreeBSD
by Peter Marheine (Code Review) 05 Sep '22

05 Sep '22

Attention is currently required from: Thomas Heijligen, Anastasia Klimchuk. Peter Marheine has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67312 ) Change subject: tests: Add workaround to allow tests mock fileno on FreeBSD ...................................................................... Patch Set 2: (1 comment) File tests/chip.c: https://review.coreboot.org/c/flashrom/+/67312/comment/d8f51eba_088822c7 PS2, Line 161: __isthreaded = 0; I don't think we should do this, since it's possible at this point that another thread has been created. `__isthreaded == 0` is just a performance optimization so it's fine to leave it once we've set it and clearing it again might be unsafe. -- To view, visit https://review.coreboot.org/c/flashrom/+/67312 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I3789ea9107a4cf8033cf59bb96d3c82aa58de026 Gerrit-Change-Number: 67312 Gerrit-PatchSet: 2 Gerrit-Owner: 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-CC: Idwer Vollering <vidwer(a)gmail.com> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Mon, 05 Sep 2022 00:22:13 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: fix scan build make issues
by Dhiren Serai (Code Review) 04 Sep '22

04 Sep '22

Dhiren Serai has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/67331 ) Change subject: fix scan build make issues ...................................................................... fix scan build make issues scan build make was showing issues with variables using malloc for uninitialized/garbage values. For fixing that I used calloc which sets allocated memory to zero as default so it will not be uninitialized/garbage. Change-Id: Id3390a40033408d827d89c3af4cd7b799e254271 Signed-off-by: dhirensr <dhirensr(a)gmail.com> --- M sfdp.c 1 file changed, 15 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/31/67331/1 diff --git a/sfdp.c b/sfdp.c index b549417..91e0fc2 100644 --- a/sfdp.c +++ b/sfdp.c @@ -295,7 +295,7 @@ nph + 1, nph); /* Fetch all parameter headers, even if we don't use them all (yet). */ - hbuf = malloc((nph + 1) * 8); + hbuf = calloc(8,(nph + 1) * 8); hdrs = malloc((nph + 1) * sizeof(*hdrs)); if (hbuf == NULL || hdrs == NULL ) { msg_gerr("Out of memory!\n"); @@ -331,7 +331,7 @@ continue; } - tbuf = malloc(len); + tbuf = calloc(1,len); if (tbuf == NULL) { msg_gerr("Out of memory!\n"); goto cleanup_hdrs; -- To view, visit https://review.coreboot.org/c/flashrom/+/67331 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Id3390a40033408d827d89c3af4cd7b799e254271 Gerrit-Change-Number: 67331 Gerrit-PatchSet: 1 Gerrit-Owner: Dhiren Serai <dhirensr(a)gmail.com> Gerrit-MessageType: newchange

1 0

[M] Change in flashrom[master]: tests: Add tests to cover unhandled programmer params paths
by Anastasia Klimchuk (Code Review) 04 Sep '22

04 Sep '22

Anastasia Klimchuk has submitted this change. ( https://review.coreboot.org/c/flashrom/+/67199 ) ( 1 is the latest approved patch-set. No files were changed between the latest approved patch-set and the submitted one. )Change subject: tests: Add tests to cover unhandled programmer params paths ...................................................................... tests: Add tests to cover unhandled programmer params paths Unhandled programmer params are considered as an error for initialisation procedure, adding tests to run those scenarios. BUG=b:181803212 TEST=ninja test Change-Id: Ia64f6362f46a029e168bfdb3bdb903328fd1f9c7 Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org> Reviewed-on: https://review.coreboot.org/c/flashrom/+/67199 Reviewed-by: Edward O'Callaghan <quasisec(a)chromium.org> Reviewed-by: Felix Singer <felixsinger(a)posteo.net> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> --- M tests/dummyflasher.c M tests/tests.c M tests/tests.h 3 files changed, 84 insertions(+), 0 deletions(-) Approvals: build bot (Jenkins): Verified Felix Singer: Looks good to me, approved Edward O'Callaghan: Looks good to me, approved diff --git a/tests/dummyflasher.c b/tests/dummyflasher.c index ddaf0cc..b52a542 100644 --- a/tests/dummyflasher.c +++ b/tests/dummyflasher.c @@ -55,8 +55,66 @@ run_probe_lifecycle(state, &dummy_io, &programmer_dummy, "size=8388608,emulate=VARIABLE_SIZE", "Opaque flash chip"); } +void dummy_init_fails_unhandled_param_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, + }; + + /* + * Programmer init should fail due to `dummy_init` failure caused by + * invalid value of `emulate` param. There is unhandled param left + * at the end of param string. + */ + run_init_error_path(state, &dummy_io, &programmer_dummy, "bus=spi,emulate=INVALID,unhandled=value", 1); +} + +void dummy_init_success_invalid_param_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, + }; + + /* + * Programmer init should fail despite of the fact that `dummy_init` + * is successful, due to invalid param at the end of param string. + */ + run_init_error_path(state, &dummy_io, &programmer_dummy, + "bus=spi,emulate=W25Q128FV,invalid=value", ERROR_FATAL); +} + +void dummy_init_success_unhandled_param_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, + }; + + /* + * Programmer init should fail despite of the fact that `dummy_init` + * is successful, due to unhandled param at the end of param string. + * Unhandled param `voltage` is not used for dummyflasher. + */ + run_init_error_path(state, &dummy_io, &programmer_dummy, + "bus=spi,emulate=W25Q128FV,voltage=3.5V", ERROR_FATAL); +} + #else SKIP_TEST(dummy_basic_lifecycle_test_success) SKIP_TEST(dummy_probe_lifecycle_test_success) SKIP_TEST(dummy_probe_variable_size_test_success) + SKIP_TEST(dummy_init_fails_unhandled_param_test_success) + SKIP_TEST(dummy_init_success_invalid_param_test_success) + SKIP_TEST(dummy_init_success_unhandled_param_test_success) #endif /* CONFIG_DUMMY */ diff --git a/tests/tests.c b/tests/tests.c index 2994c02..11348b5 100644 --- a/tests/tests.c +++ b/tests/tests.c @@ -410,6 +410,9 @@ cmocka_unit_test(dummy_basic_lifecycle_test_success), cmocka_unit_test(dummy_probe_lifecycle_test_success), cmocka_unit_test(dummy_probe_variable_size_test_success), + cmocka_unit_test(dummy_init_fails_unhandled_param_test_success), + cmocka_unit_test(dummy_init_success_invalid_param_test_success), + cmocka_unit_test(dummy_init_success_unhandled_param_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 4808b58..cdbf01c 100644 --- a/tests/tests.h +++ b/tests/tests.h @@ -45,6 +45,9 @@ void dummy_basic_lifecycle_test_success(void **state); void dummy_probe_lifecycle_test_success(void **state); void dummy_probe_variable_size_test_success(void **state); +void dummy_init_fails_unhandled_param_test_success(void **state); +void dummy_init_success_invalid_param_test_success(void **state); +void dummy_init_success_unhandled_param_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/+/67199 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ia64f6362f46a029e168bfdb3bdb903328fd1f9c7 Gerrit-Change-Number: 67199 Gerrit-PatchSet: 4 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: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

[S] Change in flashrom[master]: tests/lifecycle: Run shutdown for init error paths
by Anastasia Klimchuk (Code Review) 04 Sep '22

04 Sep '22

Anastasia Klimchuk has submitted this change. ( https://review.coreboot.org/c/flashrom/+/67198 ) ( 1 is the latest approved patch-set. No files were changed between the latest approved patch-set and the submitted one. )Change subject: tests/lifecycle: Run shutdown for init error paths ...................................................................... tests/lifecycle: Run shutdown for init error paths `run_init_error_path` tests the scenario when init function of a programmer fails. Init can fail at different phases and depending on a specific test scenario it could be before or after shutdown function has been registered. If shutdown function has already been registered, it needs to run because it cleans up the resources allocated during init. This patch prevents memory leaks. BUG=b:181803212 TEST=ninja test Change-Id: I604edff18e35b7c044b73d3a8adfa8c800eddfd2 Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org> Reviewed-on: https://review.coreboot.org/c/flashrom/+/67198 Reviewed-by: Edward O'Callaghan <quasisec(a)chromium.org> Reviewed-by: Felix Singer <felixsinger(a)posteo.net> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> --- M tests/lifecycle.c 1 file changed, 37 insertions(+), 0 deletions(-) Approvals: build bot (Jenkins): Verified Felix Singer: Looks good to me, approved Edward O'Callaghan: Looks good to me, approved diff --git a/tests/lifecycle.c b/tests/lifecycle.c index 11884f8..568b179 100644 --- a/tests/lifecycle.c +++ b/tests/lifecycle.c @@ -87,6 +87,17 @@ assert_int_equal(error_code, flashrom_programmer_init(&flashprog, prog->name, param_dup)); printf("... init failed with error code %i as expected\n", error_code); + /* + * `flashrom_programmer_shutdown` runs only registered shutdown functions, which means + * if nothing has been registered then nothing runs. + * Since this is testing error path on initialisation and error can happen at different + * phases of init, we don't know whether shutdown function has already been registered + * or not yet. Running `flashrom_programmer_shutdown` covers both situations. + */ + printf("Running programmer shutdown in case anything got registered...\n"); + assert_int_equal(0, flashrom_programmer_shutdown(flashprog)); + printf("... completed\n"); + free(param_dup); io_mock_register(NULL); -- To view, visit https://review.coreboot.org/c/flashrom/+/67198 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I604edff18e35b7c044b73d3a8adfa8c800eddfd2 Gerrit-Change-Number: 67198 Gerrit-PatchSet: 4 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: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

[M] Change in flashrom[master]: tests/parade_lspcon.c: Add emulation for ioctl, read, write operations
by Anastasia Klimchuk (Code Review) 04 Sep '22

04 Sep '22

Anastasia Klimchuk has submitted this change. ( https://review.coreboot.org/c/flashrom/+/67161 ) Change subject: tests/parade_lspcon.c: Add emulation for ioctl, read, write operations ...................................................................... tests/parade_lspcon.c: Add emulation for ioctl, read, write operations Previously, parade lspcon unit test had no custom emulation and was running basic lifecycle with default mocks (default mocks do nothing and return success). I have discovered that it does not work in all environments. Specifically, in functions `parade_lspcon_wait_command_done` and `parade_lspcon_wait_rom_free` there is a local variable `uint8_t val` which is declared and not explicitly initialised. It can get different initial values depending on the environment/toolchain (for example gcc vs clang). Later during the code execution, this variable is used as a holder for writing/reading data from register(s). For unit test, reading and writing data from registers is emulated. With default mocks, initial value of variable was propagated further as if it was "read" from a register. So the unit test could pass or fail depending on the initial value of local variable, which in turn depends on environment and toolchain. If initial value was 0 the test reliably passed (this is the case with upstream build environment, locally I have gcc 11.3.0 on x86_64). If it was 1 the test reliably failed (this is the case of chromium flashrom tree, this is clang 15.0 under chroot, tested on x86_64 and arm boards). If it was any other value then it could be anything. This patch adds custom mocks for ioctl/read/write operations and emulates a successful scenario of running a lifecycle. Local variable is initialised by reading from the register, same as it happens in the real (non-test) run for this programmer. BUG=b:242816982 TEST=ninja test Change-Id: I98f52507a0ddbbfbeb390038d14192cacc2be683 Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org> Reviewed-on: https://review.coreboot.org/c/flashrom/+/67161 Reviewed-by: Peter Marheine <pmarheine(a)chromium.org> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Felix Singer <felixsinger(a)posteo.net> --- M tests/parade_lspcon.c 1 file changed, 133 insertions(+), 0 deletions(-) Approvals: build bot (Jenkins): Verified Felix Singer: Looks good to me, approved Peter Marheine: Looks good to me, but someone else must approve diff --git a/tests/parade_lspcon.c b/tests/parade_lspcon.c index 478e296..a1c9f72 100644 --- a/tests/parade_lspcon.c +++ b/tests/parade_lspcon.c @@ -17,14 +17,100 @@ #if CONFIG_PARADE_LSPCON == 1 +/* Same macros as in parade_lspcon.c programmer. */ +/* FIXME(aklm): should driver register maps be defined in `include/drivers/` for sharing with tests? */ +#define REGISTER_ADDRESS 0x4a +#define SPISTATUS 0x9e +#define SPISTATUS_SECTOR_ERASE_FINISHED 0 +#define SWSPICTL 0x93 +#define SWSPICTL_ENABLE_READBACK 0x8 +#define SWSPI_RDATA 0x91 +/* Macros for test run. */ +#define DATA_TO_READ 0 +#define MAX_REG_BUF_LEN 2 + +struct parade_lspcon_io_state { + unsigned long addr; /* Address to read and write */ + uint8_t reg_buf[MAX_REG_BUF_LEN]; /* Last value written to the register address */ +}; + +static int parade_lspcon_ioctl(void *state, int fd, unsigned long request, va_list args) +{ + struct parade_lspcon_io_state *io_state = state; + if (request == I2C_SLAVE) + /* Addr is the next (and the only) argument in the parameters list for this ioctl call. */ + io_state->addr = va_arg(args, unsigned long); + + return 0; +} + +static int parade_lspcon_read(void *state, int fd, void *buf, size_t sz) +{ + struct parade_lspcon_io_state *io_state = state; + + /* + * Parade_lspcon programmer has operations over register address and + * page address. In the current emulation for basic lifecycle we need + * to emulate operations over register address. Page address can do + * nothing for now, and just return successful return value. + * + * For future, if this unit test is upgraded to run probing lifecycle, + * page address operations might need to be fully emulated. + */ + if (io_state->addr != REGISTER_ADDRESS) + return sz; + + assert_int_equal(sz, 1); + + switch (io_state->reg_buf[0]) { + case SPISTATUS: + memset(buf, SPISTATUS_SECTOR_ERASE_FINISHED, sz); + break; + case SWSPICTL: + memset(buf, SWSPICTL_ENABLE_READBACK, sz); + break; + case SWSPI_RDATA: + memset(buf, DATA_TO_READ, sz); + break; + default: + memset(buf, 0, sz); + break; + } + + return sz; +} + +static int parade_lspcon_write(void *state, int fd, const void *buf, size_t sz) +{ + struct parade_lspcon_io_state *io_state = state; + + /* + * Only register address operations are needed to be emulated for basic lifecycle. + * See also comment in `parade_lspcon_read`. + */ + if (io_state->addr != REGISTER_ADDRESS) + return sz; + + assert_true(sz <= MAX_REG_BUF_LEN); + + memcpy(io_state->reg_buf, buf, sz); + + return sz; +} + void parade_lspcon_basic_lifecycle_test_success(void **state) { + struct parade_lspcon_io_state parade_lspcon_io_state = { 0 }; struct io_mock_fallback_open_state parade_lspcon_fallback_open_state = { .noc = 0, .paths = { "/dev/i2c-254", NULL }, .flags = { O_RDWR }, }; const struct io_mock parade_lspcon_io = { + .state = &parade_lspcon_io_state, + .ioctl = parade_lspcon_ioctl, + .read = parade_lspcon_read, + .write = parade_lspcon_write, .fallback_open_state = &parade_lspcon_fallback_open_state, }; -- To view, visit https://review.coreboot.org/c/flashrom/+/67161 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I98f52507a0ddbbfbeb390038d14192cacc2be683 Gerrit-Change-Number: 67161 Gerrit-PatchSet: 6 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Alexander Goncharov <chat(a)joursoir.net> 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-CC: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-MessageType: merged

1 0

[S] Change in flashrom[master]: tests: Add workaround to allow tests mock fileno on FreeBSD
by Anastasia Klimchuk (Code Review) 04 Sep '22

04 Sep '22

Attention is currently required from: Thomas Heijligen, Anastasia Klimchuk, Peter Marheine. Hello Felix Singer, build bot (Jenkins), Thomas Heijligen, Peter Marheine, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/67312 to look at the new patch set (#2). Change subject: tests: Add workaround to allow tests mock fileno on FreeBSD ...................................................................... tests: Add workaround to allow tests mock fileno on FreeBSD fileno can be a function and a macro in FreeBSD, depending on whether the environment in multi-threaded or single-threaded. For single thread, macro is used. Macro is expanded into a inline code which accesses private field of file descriptor. This is impossible to mock in tests. Pretending that environment is multi-threaded makes fileno function to be called instead of a macro. Function can be mocked for tests. BUG=b:237606255 TEST=ninja test on two environments: 1) FreeBSD 13.1-RELEASE-p2 GENERIC amd64 2) Debian 5.17.11 x86_64 GNU/Linux Change-Id: I3789ea9107a4cf8033cf59bb96d3c82aa58de026 Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org> TICKET: https://ticket.coreboot.org/issues/411 --- M tests/chip.c 1 file changed, 42 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/12/67312/2 -- To view, visit https://review.coreboot.org/c/flashrom/+/67312 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I3789ea9107a4cf8033cf59bb96d3c82aa58de026 Gerrit-Change-Number: 67312 Gerrit-PatchSet: 2 Gerrit-Owner: 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-CC: Idwer Vollering <vidwer(a)gmail.com> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit September 2022