March 2021 - flashrom-gerrit - mail.coreboot.org

Change in flashrom[master]: linux_mtd: Switch fopen() to open() for MTD devices
by Hung-Te Lin (Code Review) March 16, 2021

March 16, 2021

Attention is currently required from: Nico Huber, Douglas Anderson, Edward O'Callaghan, Angel Pons, Patrick Rudolph. Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/50206 ) Change subject: linux_mtd: Switch fopen() to open() for MTD devices ...................................................................... Patch Set 2: (1 comment) Patchset: PS2: > You only included half the comment, though! Yes I saw the fixme and considered "not all MTD drivers handle arbitrary large reads well." as the unexpected behavior. > I guess it was the chunking that was important not the alignment but the two were conflated in the comment. I looked at the history and found things interesting. - The implementation was using read/write in the beginning, and was changed by https://chromium-review.googlesource.com/2310521 (b/153598951) - to sync with upstream. - The chunk was implemented on https://chromium-review.googlesource.com/505409/ (b/35573113) and yes, from there alignment is not important. So yes, staying with libc is probably the right way. -- To view, visit https://review.coreboot.org/c/flashrom/+/50206 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ifeb00b049ba5aa0bc8fdc591ac1f9861ad5d428d Gerrit-Change-Number: 50206 Gerrit-PatchSet: 2 Gerrit-Owner: Douglas Anderson <dianders(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Hung-Te Lin <hungte(a)chromium.org> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Douglas Anderson <dianders(a)chromium.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Comment-Date: Mon, 15 Mar 2021 23:19:31 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Hung-Te Lin <hungte(a)chromium.org> Comment-In-Reply-To: Douglas Anderson <dianders(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: linux_mtd: Switch fopen() to open() for MTD devices
by Douglas Anderson (Code Review) March 15, 2021

March 15, 2021

Attention is currently required from: Hung-Te Lin, Nico Huber, Edward O'Callaghan, Angel Pons, Patrick Rudolph. Douglas Anderson has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/50206 ) Change subject: linux_mtd: Switch fopen() to open() for MTD devices ...................................................................... Patch Set 2: (1 comment) Patchset: PS2: > The comment said "Try to align reads to eraseblock size.". > > So, if you use fread, the stdio may do unaligned reads internally. If we call read directly, at least we can look at return values, decide if we should go back to previous aligned offset and read again. You only included half the comment, though! The full comment also has: ``` * FIXME: Shouldn't actually be necessary, but not all MTD * drivers handle arbitrary large reads well. ``` Not only does that imply that the alignment shouldn't really be necessary but also that the problem being worked around was reads that were too large. I guess it was the chunking that was important not the alignment but the two were conflated in the comment. -- To view, visit https://review.coreboot.org/c/flashrom/+/50206 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ifeb00b049ba5aa0bc8fdc591ac1f9861ad5d428d Gerrit-Change-Number: 50206 Gerrit-PatchSet: 2 Gerrit-Owner: Douglas Anderson <dianders(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Hung-Te Lin <hungte(a)chromium.org> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Hung-Te Lin <hungte(a)chromium.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: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Comment-Date: Mon, 15 Mar 2021 17:07:21 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Hung-Te Lin <hungte(a)chromium.org> Comment-In-Reply-To: Douglas Anderson <dianders(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: linux_mtd: Switch fopen() to open() for MTD devices
by Hung-Te Lin (Code Review) March 15, 2021

March 15, 2021

Attention is currently required from: Nico Huber, Douglas Anderson, Edward O'Callaghan, Angel Pons, Patrick Rudolph. Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/50206 ) Change subject: linux_mtd: Switch fopen() to open() for MTD devices ...................................................................... Patch Set 2: (1 comment) Patchset: PS2: > I'm fine abandoning this change, but I'm also not really convinced if fread is better. […] The comment said "Try to align reads to eraseblock size.". So, if you use fread, the stdio may do unaligned reads internally. If we call read directly, at least we can look at return values, decide if we should go back to previous aligned offset and read again. -- To view, visit https://review.coreboot.org/c/flashrom/+/50206 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ifeb00b049ba5aa0bc8fdc591ac1f9861ad5d428d Gerrit-Change-Number: 50206 Gerrit-PatchSet: 2 Gerrit-Owner: Douglas Anderson <dianders(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Hung-Te Lin <hungte(a)chromium.org> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Douglas Anderson <dianders(a)chromium.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Comment-Date: Mon, 15 Mar 2021 16:00:51 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Hung-Te Lin <hungte(a)chromium.org> Comment-In-Reply-To: Douglas Anderson <dianders(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: linux_mtd: Switch fopen() to open() for MTD devices
by Hung-Te Lin (Code Review) March 15, 2021

March 15, 2021

Attention is currently required from: Nico Huber, Douglas Anderson, Edward O'Callaghan, Angel Pons, Patrick Rudolph. Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/50206 ) Change subject: linux_mtd: Switch fopen() to open() for MTD devices ...................................................................... Patch Set 2: (1 comment) Patchset: PS2: > I think Nico's point about read() being able to return a shorter read is a good one and I'm thinking […] I'm fine abandoning this change, but I'm also not really convinced if fread is better. It's true fread/fwrite have some internal retry loops so reading or writing to a real file should work properly. However for device files, I wonder if the retry would cause unexpected behavior in underlying driver / hardware. IMHO if we are accessing a driver via device file, any interrupt should be considered as failure, not silently retrying... Having said that, if the fread/fwrite version is known to work and we don't see problems, I do agree there's no need to change to read/write. -- To view, visit https://review.coreboot.org/c/flashrom/+/50206 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ifeb00b049ba5aa0bc8fdc591ac1f9861ad5d428d Gerrit-Change-Number: 50206 Gerrit-PatchSet: 2 Gerrit-Owner: Douglas Anderson <dianders(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Hung-Te Lin <hungte(a)chromium.org> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Douglas Anderson <dianders(a)chromium.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Comment-Date: Mon, 15 Mar 2021 15:56:29 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Douglas Anderson <dianders(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: flashrom.c: automatic generated programmer_enum.h
by Nico Huber (Code Review) March 15, 2021

March 15, 2021

Attention is currently required from: Sam McNally, Xiang Wang, Stefan Reinauer. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/49599 ) Change subject: flashrom.c: automatic generated programmer_enum.h ...................................................................... Patch Set 4: (3 comments) Patchset: PS4: Thanks for starting the effort. Many people stumbled over the odd #if mess in this area, lately. While the generated .h file works somehow, I don't think it's necessary. `enum programmer` seems pretty much unused and could be removed instead? Also, Thomas mentioned the thought to move the actual pro- grammer structs into the respective .c files. Then we'd only need a global array with pointers. File Makefile: https://review.coreboot.org/c/flashrom/+/49599/comment/7bab6ad5_9fec7ad2 PS4, Line 681: $(shell bash ./util/generator_programmer_enum.sh) This should be run as a recipe for the file. Currently it would be re-written on every `make` run, right? I'm not sure how our depen- dency tracking copes with that. File flashrom.c: https://review.coreboot.org/c/flashrom/+/49599/comment/81bf9082_bb672d28 PS4, Line 43: #include "programmer_table.c" .c includes are generally discouraged. -- To view, visit https://review.coreboot.org/c/flashrom/+/49599 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I3f4370ccae2b64da3c4178243b192700d3d205d2 Gerrit-Change-Number: 49599 Gerrit-PatchSet: 4 Gerrit-Owner: Xiang Wang <merle(a)hardenedlinux.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Sam McNally <sammc(a)google.com> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-CC: Idwer Vollering <vidwer(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-CC: Shawn C <citypw(a)hardenedlinux.org> Gerrit-Attention: Sam McNally <sammc(a)google.com> Gerrit-Attention: Xiang Wang <merle(a)hardenedlinux.org> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Comment-Date: Mon, 15 Mar 2021 15:42:15 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: linux_mtd: Switch fopen() to open() for MTD devices
by Douglas Anderson (Code Review) March 15, 2021

March 15, 2021

Attention is currently required from: Hung-Te Lin, Nico Huber, Edward O'Callaghan, Angel Pons, Patrick Rudolph. Douglas Anderson has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/50206 ) Change subject: linux_mtd: Switch fopen() to open() for MTD devices ...................................................................... Patch Set 2: (3 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/50206/comment/c9847dcc_d0d14070 PS2, Line 26: overhead / translation that we just don't need. > Proof? I mean, the OS exposes the open/close/read/write calls and so the stdio functions _must_ be implemented atop them. It can't be done any other way. Thus the sdio functions _must_ be an extra layer? What am I missing? ...or are you saying to prove the "we just don't need". Aside from your comment about short reads, I would argue that the new implementation works as well as the old and isn't any more complicated. In any case, probably not relevant since I'm proposing abandoning this change in response to your point about short reads. Patchset: PS2: I think Nico's point about read() being able to return a shorter read is a good one and I'm thinking that maybe we should just abandon this change? I mostly implemented it in response to Hung-Te's feedback in <https://review.coreboot.org/c/flashrom/+/50155> and I have no objection to leaving things using stdio. Hung-Te: what do you think? File linux_mtd.c: https://review.coreboot.org/c/flashrom/+/50206/comment/ce81c346_9cf3b813 PS2, Line 206: if (read(dev_fileno, buf + i, step) != step) { > AFAIR, read() can be interrupted and return a short count even if […] Huh, this is a good point and I didn't think about it. I think you're right and this could be a real problem. From my understanding the fread() abstracts this out from us? That would actually be a good reason to use fread(). -- To view, visit https://review.coreboot.org/c/flashrom/+/50206 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ifeb00b049ba5aa0bc8fdc591ac1f9861ad5d428d Gerrit-Change-Number: 50206 Gerrit-PatchSet: 2 Gerrit-Owner: Douglas Anderson <dianders(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Hung-Te Lin <hungte(a)chromium.org> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Hung-Te Lin <hungte(a)chromium.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: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Comment-Date: Mon, 15 Mar 2021 15:35:06 +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]: linux_mtd: Switch fopen() to open() for MTD devices
by Nico Huber (Code Review) March 15, 2021

March 15, 2021

Attention is currently required from: Douglas Anderson, Edward O'Callaghan, Angel Pons, Patrick Rudolph. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/50206 ) Change subject: linux_mtd: Switch fopen() to open() for MTD devices ...................................................................... Patch Set 2: (2 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/50206/comment/a4a43916_8361f83e PS2, Line 26: overhead / translation that we just don't need. Proof? File linux_mtd.c: https://review.coreboot.org/c/flashrom/+/50206/comment/f59485ad_1a049631 PS2, Line 206: if (read(dev_fileno, buf + i, step) != step) { AFAIR, read() can be interrupted and return a short count even if everything is alright. Is that not true for MTD device files? If it can't be interrupted, is that specified somewhere or implementation specific? -- To view, visit https://review.coreboot.org/c/flashrom/+/50206 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ifeb00b049ba5aa0bc8fdc591ac1f9861ad5d428d Gerrit-Change-Number: 50206 Gerrit-PatchSet: 2 Gerrit-Owner: Douglas Anderson <dianders(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Hung-Te Lin <hungte(a)chromium.org> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Douglas Anderson <dianders(a)chromium.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Comment-Date: Mon, 15 Mar 2021 15:20:35 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: Enable dynamic memory allocation checks for cmocka unit tests.
by Angel Pons (Code Review) March 15, 2021

March 15, 2021

Attention is currently required from: Anastasia Klimchuk. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/51243 ) Change subject: Enable dynamic memory allocation checks for cmocka unit tests. ...................................................................... Patch Set 1: Code-Review+1 (2 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/51243/comment/e78b5ebc_8f8fbacd PS1, Line 7: . nit: no trailing period in the commit summary File tests/flashrom.c: https://review.coreboot.org/c/flashrom/+/51243/comment/6feefcf8_34efa83e PS1, Line 31: free(text); I'd define a helper macro to avoid repeating this pattern: #define assert_string_equal_with_free(text, expected) \ do { \ assert_string_equal(text, expected); \ free(text); \ while (0) Two things to note: - I've intentionally used a macro instead of a function. Because `assert_string_equal` is a macro that uses `__FILE__` and `__LINE__`, using a helper function would result in the wrong line number being printed. - The do-while construct in the macro is intentional: https://gcc.gnu.org/onlinedocs/cpp/Swallowing-the-Semicolon.html -- To view, visit https://review.coreboot.org/c/flashrom/+/51243 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I0c6b6b8dc17aaee28640e3fca3d1fc9f7feabf5f Gerrit-Change-Number: 51243 Gerrit-PatchSet: 1 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Daniel Kurtz <djkurtz(a)google.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Daniel Kurtz <djkurtz(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Mon, 15 Mar 2021 09:50:24 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: Add unit test to run init/shutdown for enabled drivers
by Anastasia Klimchuk (Code Review) March 15, 2021

March 15, 2021

Anastasia Klimchuk has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/51487 ) Change subject: Add unit test to run init/shutdown for enabled drivers ...................................................................... Add unit test to run init/shutdown for enabled drivers I want to do this for all drivers in programmer table, this patch has 3 of them as a proof of concept. There is no hardware in unit test environment, all operations with hardware need to be mocked/redefined. Calls to __wrap functions are now logged to stdout, makes it easier to understand what’s happening and I find it really helps when writing tests (and can be useful when debugging tests). Extract from meson-logs/testlog.txt for new test: [ RUN ] linux_spi_init_and_shutdown_test_success Testing programmer_init for programmer=25 ... __wrap_open64 is called __wrap_ioctl is called __wrap_ioctl is called __wrap_ioctl is called __wrap_fopen64 is called ... programmer_init for programmer=25 successful Testing programmer_shutdown for programmer=25 ... ... programmer_shutdown for programmer=25 successful [ OK ] linux_spi_init_and_shutdown_test_success BUG=b:181803212 TEST=builds and ninja test Change-Id: I3af612defe1af3850dfc1626a208d873e3a3eddc Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org> --- M mec1308.c A tests/init_shutdown.c M tests/meson.build M tests/tests.c M tests/tests.h 5 files changed, 139 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/87/51487/1 diff --git a/mec1308.c b/mec1308.c index c4b34f5..0d81a60 100644 --- a/mec1308.c +++ b/mec1308.c @@ -100,6 +100,20 @@ #define MEC1308_CMD_PASSTHRU_SEND 0xf2 /* send byte from data0 */ #define MEC1308_CMD_PASSTHRU_READ 0xf3 /* read byte, place in data0 */ +#ifdef UNIT_TEST_ENV +/* No hardware in unit test environment. + * So outb and inb need to be redefined, to pretend they work. */ +static uint8_t outb_val = 0; +#undef OUTB +#undef INB +#define OUTB(x, y) outb_val = x; +#define INB(x) (((x) == MEC1308_SIO_PORT1 || (x) == MEC1308_SIO_PORT2) \ + ? MEC1308_DEVICE_ID_REG \ + : ((outb_val == MEC1308_MBX_DATA_START) \ + ? MEC1308_CMD_PASSTHRU_SUCCESS \ + : 0)) +#endif /* UNIT_TEST_ENV */ + typedef struct { unsigned int in_sio_cfgmode; diff --git a/tests/init_shutdown.c b/tests/init_shutdown.c new file mode 100644 index 0000000..e057f64 --- /dev/null +++ b/tests/init_shutdown.c @@ -0,0 +1,48 @@ +/* + * This file is part of the flashrom project. + * + * Copyright 2021 Google LLC + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <include/test.h> +#include <string.h> + +#include "programmer.h" + +static void run_lifecycle(void **state, enum programmer prog, const char *param) +{ + (void) state; /* unused */ + + printf("Testing programmer_init for programmer=%u ...\n", prog); + assert_int_equal(0, programmer_init(prog, strdup(param))); + printf("... programmer_init for programmer=%u successful\n", prog); + + printf("Testing programmer_shutdown for programmer=%u ...\n", prog); + assert_int_equal(0, programmer_shutdown()); + printf("... programmer_shutdown for programmer=%u successful\n", prog); +} + +void dummy_init_and_shutdown_test_success(void **state) +{ + run_lifecycle(state, PROGRAMMER_DUMMY, ""); +} + +void mec1308_init_and_shutdown_test_success(void **state) +{ + will_return_always(__wrap_sio_read, 0x4d); /* MEC1308_DEVICE_ID_VAL */ + run_lifecycle(state, PROGRAMMER_MEC1308, ""); +} + +void linux_spi_init_and_shutdown_test_success(void **state) +{ + run_lifecycle(state, PROGRAMMER_LINUX_SPI, "dev=/dev/null"); +} diff --git a/tests/meson.build b/tests/meson.build index f0cb76d..2b21602 100644 --- a/tests/meson.build +++ b/tests/meson.build @@ -18,12 +18,21 @@ 'helpers.c', 'flashrom.c', 'spi25.c', + 'init_shutdown.c', ] mocks = [ '-Wl,--wrap=physunmap', '-Wl,--wrap=physmap', '-Wl,--wrap=spi_send_command', + '-Wl,--wrap=sio_write', + '-Wl,--wrap=sio_read', + '-Wl,--wrap=open', + '-Wl,--wrap=open64', + '-Wl,--wrap=ioctl', + '-Wl,--wrap=fopen', + '-Wl,--wrap=fopen64', + '-Wl,--wrap=rget_io_perms', '-Wl,--gc-sections', ] @@ -35,6 +44,7 @@ '-ffunction-sections', '-fdata-sections', # '-DSTANDALONE', + '-DUNIT_TEST_ENV', '-DCONFIG_DEFAULT_PROGRAMMER=PROGRAMMER_DUMMY', '-DCONFIG_DEFAULT_PROGRAMMER_ARGS=""', ], diff --git a/tests/tests.c b/tests/tests.c index 3124607..86e5959 100644 --- a/tests/tests.c +++ b/tests/tests.c @@ -17,22 +17,75 @@ #include "tests.h" #include <stdio.h> +#include <stdint.h> /* redefinitions/wrapping */ +#define LOG_ME printf("%s is called\n", __func__) + void __wrap_physunmap(void *virt_addr, size_t len) { - fprintf(stderr, "%s\n", __func__); + LOG_ME; } + void *__wrap_physmap(const char *descr, uintptr_t phys_addr, size_t len) { - fprintf(stderr, "%s\n", __func__); + LOG_ME; return NULL; } +void __wrap_sio_write(uint16_t port, uint8_t reg, uint8_t data) +{ + LOG_ME; +} + +uint8_t __wrap_sio_read(uint16_t port, uint8_t reg) +{ + LOG_ME; + return (uint8_t)mock(); +} + +int __wrap_open(const char *pathname, int flags) +{ + LOG_ME; + return 2021; +} + +int __wrap_open64(const char *pathname, int flags) +{ + LOG_ME; + return 2021; +} + +int __wrap_ioctl(int fd, unsigned long int request, ...) +{ + LOG_ME; + return 2021; +} + +FILE *__wrap_fopen(const char *pathname, const char *mode) +{ + LOG_ME; + return NULL; +} + +FILE *__wrap_fopen64(const char *pathname, const char *mode) +{ + LOG_ME; + return NULL; +} + +int __wrap_rget_io_perms(void) +{ + LOG_ME; + return 0; +} + int main(void) { int ret = 0; + cmocka_set_message_output(CM_OUTPUT_STDOUT); + const struct CMUnitTest helpers_tests[] = { cmocka_unit_test(address_to_bits_test_success), cmocka_unit_test(bitcount_test_success), @@ -63,5 +116,12 @@ }; ret |= cmocka_run_group_tests_name("spi25.c tests", spi25_tests, NULL, NULL); + const struct CMUnitTest init_shutdown_tests[] = { + cmocka_unit_test(dummy_init_and_shutdown_test_success), + cmocka_unit_test(linux_spi_init_and_shutdown_test_success), + cmocka_unit_test(mec1308_init_and_shutdown_test_success), + }; + ret |= cmocka_run_group_tests_name("init_shutdown.c tests", init_shutdown_tests, NULL, NULL); + return ret; } diff --git a/tests/tests.h b/tests/tests.h index cb905fd..97cae7c 100644 --- a/tests/tests.h +++ b/tests/tests.h @@ -40,4 +40,9 @@ void probe_spi_at25f_test_success(void **state); void probe_spi_st95_test_success(void **state); /* spi95.c */ +/* init_shutdown.c */ +void dummy_init_and_shutdown_test_success(void **state); +void linux_spi_init_and_shutdown_test_success(void **state); +void mec1308_init_and_shutdown_test_success(void **state); + #endif /* TESTS_H */ -- To view, visit https://review.coreboot.org/c/flashrom/+/51487 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I3af612defe1af3850dfc1626a208d873e3a3eddc Gerrit-Change-Number: 51487 Gerrit-PatchSet: 1 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: Add Manibuilder
by Angel Pons (Code Review) March 14, 2021

March 14, 2021

Attention is currently required from: Nico Huber, David Hendricks. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/23005 ) Change subject: Add Manibuilder ...................................................................... Patch Set 11: (1 comment) File util/manibuilder/README.md: https://review.coreboot.org/c/flashrom/+/23005/comment/23e2bf4c_3ad5842a PS3, Line 4: hold > held uh, just realized this comment is still unresolved -- To view, visit https://review.coreboot.org/c/flashrom/+/23005 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I60863a5c7d70dde71486fccb66cb59b30ba4d982 Gerrit-Change-Number: 23005 Gerrit-PatchSet: 11 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Comment-Date: Sun, 14 Mar 2021 15:19:49 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0