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

Change in flashrom[master]: tests: Mock file i/o for linux_mtd and linux_spi tests
by Anastasia Klimchuk (Code Review) 20 Aug '21

20 Aug '21

Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/56413 ) Change subject: tests: Mock file i/o for linux_mtd and linux_spi tests ...................................................................... Patch Set 10: (6 comments) File tests/init_shutdown.c: https://review.coreboot.org/c/flashrom/+/56413/comment/bcdbbb06_e473ea02 PS4, Line 219: // > `//` is generally accepted by the syscalls. I think that means the […] I started reading man page for fnmatch() and realised I am not sure what the end goal is. Can we talk a bit more about this? fnmatch asks me for a pattern for pathname+filename. Is the pattern same as now but with choice of / or //? I don't know how to express the idea "accept what Linux accepts". Haven't changed anything, so I mark this comment unresolved. File tests/init_shutdown.c: https://review.coreboot.org/c/flashrom/+/56413/comment/88eba068_4b23b7a0 PS9, Line 206: io_state->fopen_path = pathname; > We should keep a copy (strdup()). There is no guarantee* that the […] Thank you, done (fclose is now freeing memory)! Yes, ideally we test that driver is functional, and also ideally test is changed very very rare, only if some dramatic changes such that mocks need to be adjusted. https://review.coreboot.org/c/flashrom/+/56413/comment/0c4184c8_d154b651 PS9, Line 208: return (void *)0x55AA; /* valid file descriptor */ > I have to say, I only understood the comment by reading the code (and would […] I really like NOT_NULL, I added it here. I was thinking, as next step, merge this with MOCK_HANDLER which is at the moment defined in tests.c, move it to io_mock.h and use everywhere. I like NOT_NULL name more than MOCK_HANDLER. https://review.coreboot.org/c/flashrom/+/56413/comment/b1f6e95a_1178f2aa PS9, Line 233: const struct linux_mtd_fread_mock_entry entry = fread_mock_map[i]; > I think I used a pointer in the original suggestion, but I think I forgot to dereference `fread_mock […] Sorry, that's my fault, I used your code and fixed compile error into the wrong direction :\ Changed to a pointer, and added & https://review.coreboot.org/c/flashrom/+/56413/comment/8b72d44a_f68a59ec PS9, Line 236: + 1 > I don't think the files in sysfs provide a 0-termination? There's a linebreak […] Do I understand you correctly, if there is a linebreak sometimes then we still need +1 to strlen? File tests/io_mock.h: https://review.coreboot.org/c/flashrom/+/56413/comment/ef149e57_6ad2ae89 PS9, Line 86: /* File I/O */ > This is usually referred to as `stdio` or `Standard I/O`. Done -- To view, visit https://review.coreboot.org/c/flashrom/+/56413 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I73f0d6ff2ad5074add7a721ed3416230d3647e3f Gerrit-Change-Number: 56413 Gerrit-PatchSet: 10 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.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-Comment-Date: Fri, 20 Aug 2021 06:29:24 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: tests: Mock file i/o for linux_mtd and linux_spi tests
by Anastasia Klimchuk (Code Review) 20 Aug '21

20 Aug '21

Attention is currently required from: Nico Huber, Edward O'Callaghan, Anastasia Klimchuk. Hello build bot (Jenkins), Nico Huber, Edward O'Callaghan, Angel Pons, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/56413 to look at the new patch set (#10). Change subject: tests: Mock file i/o for linux_mtd and linux_spi tests ...................................................................... tests: Mock file i/o for linux_mtd and linux_spi tests This patch adds an init-shutdown test for linux_mtd. Since linux_mtd is using file i/o operations, those are added to the framework and mocked. Another driver linux_spi which is also using file i/o, got an upgrade in this patch, and it is now reading max buffer size from sysfs (using mocked file i/o). A good side-effect is that linux_mtd is the first test for opaque masters, which is great to have in preparation for a change like CB:56103 but for opaque masters. BUG=b:181803212 TEST=builds and ninja test Change-Id: I73f0d6ff2ad5074add7a721ed3416230d3647e3f Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org> --- M tests/init_shutdown.c M tests/io_mock.h M tests/meson.build M tests/tests.c M tests/tests.h 5 files changed, 180 insertions(+), 4 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/13/56413/10 -- To view, visit https://review.coreboot.org/c/flashrom/+/56413 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I73f0d6ff2ad5074add7a721ed3416230d3647e3f Gerrit-Change-Number: 56413 Gerrit-PatchSet: 10 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: ni845x_spi: add missing const specifier to the spi_transmit function
by Anastasia Klimchuk (Code Review) 20 Aug '21

20 Aug '21

Attention is currently required from: Nico Huber, Miklós Márton, Edward O'Callaghan. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/57003 ) Change subject: ni845x_spi: add missing const specifier to the spi_transmit function ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: > Ah, I should fix my pidgin to being able connect to the flashrom channel when I am online, (broken s […] sounds good! -- To view, visit https://review.coreboot.org/c/flashrom/+/57003 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I2c14361283b2da3725d9ba2cae0e1007e6be3d66 Gerrit-Change-Number: 57003 Gerrit-PatchSet: 1 Gerrit-Owner: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Comment-Date: Fri, 20 Aug 2021 00:28:05 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Miklós Márton <martonmiklosqdev(a)gmail.com> Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: tests: Mock file i/o for linux_mtd and linux_spi tests
by Angel Pons (Code Review) 19 Aug '21

19 Aug '21

Attention is currently required from: Nico Huber, Edward O'Callaghan, Anastasia Klimchuk. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/56413 ) Change subject: tests: Mock file i/o for linux_mtd and linux_spi tests ...................................................................... Patch Set 9: (1 comment) File tests/init_shutdown.c: https://review.coreboot.org/c/flashrom/+/56413/comment/63b7d275_3c954da5 PS9, Line 233: const struct linux_mtd_fread_mock_entry entry = fread_mock_map[i]; > This creates a copy of the struct, looks like a pointer would do as well. I think I used a pointer in the original suggestion, but I think I forgot to dereference `fread_mock_map[i]` with an `&`. -- To view, visit https://review.coreboot.org/c/flashrom/+/56413 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I73f0d6ff2ad5074add7a721ed3416230d3647e3f Gerrit-Change-Number: 56413 Gerrit-PatchSet: 9 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Thu, 19 Aug 2021 16:49:56 +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]: tests: Mock file i/o for linux_mtd and linux_spi tests
by Nico Huber (Code Review) 19 Aug '21

19 Aug '21

Attention is currently required from: Edward O'Callaghan, Angel Pons, Anastasia Klimchuk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/56413 ) Change subject: tests: Mock file i/o for linux_mtd and linux_spi tests ...................................................................... Patch Set 9: (6 comments) File tests/init_shutdown.c: https://review.coreboot.org/c/flashrom/+/56413/comment/a9ea6a39_6fefa236 PS4, Line 219: // > "//" is what's in the actual path which is constructed by linux_mtd. […] `//` is generally accepted by the syscalls. I think that means the tests should accept it as well. But actually they should accept what Linux accepts, and not just what the driver happens to produce right now. Maybe fnmatch() could be used? File tests/init_shutdown.c: https://review.coreboot.org/c/flashrom/+/56413/comment/14895fee_8141c827 PS9, Line 206: io_state->fopen_path = pathname; We should keep a copy (strdup()). There is no guarantee* that the pointer stays valid. (* assuming we want to test that the driver is functional, and not that the driver code always stays the same) https://review.coreboot.org/c/flashrom/+/56413/comment/d8639a62_3b120d56 PS9, Line 208: return (void *)0x55AA; /* valid file descriptor */ I have to say, I only understood the comment by reading the code (and would have understood that without the comment too). Technically, the comment is wrong. It's a pointer (could refer to it as handle), not a file descriptor. The file descriptor is the `int` that is handled by the kernel and returned by open(). Which we obviously don't have here. Also, we don't need a *valid* pointer, we need one that is not `NULL`, actually no matter if it's valid. Commonly, code is better than comments, we could introduce something like this #define NOT_NULL ((void *)0xf000baaa) That would move the discussion and any potential confusion to a single place and make it obvious what is going on every time the macro is used. https://review.coreboot.org/c/flashrom/+/56413/comment/1c9a3928_37d5e058 PS9, Line 233: const struct linux_mtd_fread_mock_entry entry = fread_mock_map[i]; This creates a copy of the struct, looks like a pointer would do as well. https://review.coreboot.org/c/flashrom/+/56413/comment/0c26de91_ea412139 PS9, Line 236: + 1 I don't think the files in sysfs provide a 0-termination? There's a linebreak sometimes, though. File tests/io_mock.h: https://review.coreboot.org/c/flashrom/+/56413/comment/7890fca3_a3c5e1ca PS9, Line 86: /* File I/O */ This is usually referred to as `stdio` or `Standard I/O`. -- To view, visit https://review.coreboot.org/c/flashrom/+/56413 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I73f0d6ff2ad5074add7a721ed3416230d3647e3f Gerrit-Change-Number: 56413 Gerrit-PatchSet: 9 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Thu, 19 Aug 2021 15:28:16 +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

Change in flashrom[master]: ni845x_spi: add missing const specifier to the spi_transmit function
by Miklós Márton (Code Review) 19 Aug '21

19 Aug '21

Attention is currently required from: Nico Huber, Edward O'Callaghan. Miklós Márton has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/57003 ) Change subject: ni845x_spi: add missing const specifier to the spi_transmit function ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: > I realised my comment may seem odd on the top of Angel's comment. […] Ah, I should fix my pidgin to being able connect to the flashrom channel when I am online, (broken since the freenode migration) to be able to discuss these things there easier. -- To view, visit https://review.coreboot.org/c/flashrom/+/57003 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I2c14361283b2da3725d9ba2cae0e1007e6be3d66 Gerrit-Change-Number: 57003 Gerrit-PatchSet: 1 Gerrit-Owner: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Comment-Date: Thu, 19 Aug 2021 09:20:32 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Miklós Márton <martonmiklosqdev(a)gmail.com> Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: tests: Mock file i/o for linux_mtd and linux_spi tests
by Anastasia Klimchuk (Code Review) 19 Aug '21

19 Aug '21

Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/56413 ) Change subject: tests: Mock file i/o for linux_mtd and linux_spi tests ...................................................................... Patch Set 9: (2 comments) Patchset: PS9: Rebased to merge with other submitted tests. File tests/init_shutdown.c: https://review.coreboot.org/c/flashrom/+/56413/comment/757f0072_62b275ac PS6, Line 237: return strlen(entry.data); > I added `strlen(entry.data) + 1`. […] Forgot to mark this resolved, this is done in the latest patchset -- To view, visit https://review.coreboot.org/c/flashrom/+/56413 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I73f0d6ff2ad5074add7a721ed3416230d3647e3f Gerrit-Change-Number: 56413 Gerrit-PatchSet: 9 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.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-Comment-Date: Thu, 19 Aug 2021 05:21:39 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: tests: Mock file i/o for linux_mtd and linux_spi tests
by Anastasia Klimchuk (Code Review) 19 Aug '21

19 Aug '21

Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons. Hello build bot (Jenkins), Nico Huber, Edward O'Callaghan, Angel Pons, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/56413 to look at the new patch set (#9). Change subject: tests: Mock file i/o for linux_mtd and linux_spi tests ...................................................................... tests: Mock file i/o for linux_mtd and linux_spi tests This patch adds an init-shutdown test for linux_mtd. Since linux_mtd is using file i/o operations, those are added to the framework and mocked. Another driver linux_spi which is also using file i/o, got an upgrade in this patch, and it is now reading max buffer size from sysfs (using mocked file i/o). A good side-effect is that linux_mtd is the first test for opaque masters, which is great to have in preparation for a change like CB:56103 but for opaque masters. BUG=b:181803212 TEST=builds and ninja test Change-Id: I73f0d6ff2ad5074add7a721ed3416230d3647e3f Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org> --- M tests/init_shutdown.c M tests/io_mock.h M tests/meson.build M tests/tests.c M tests/tests.h 5 files changed, 165 insertions(+), 4 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/13/56413/9 -- To view, visit https://review.coreboot.org/c/flashrom/+/56413 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I73f0d6ff2ad5074add7a721ed3416230d3647e3f Gerrit-Change-Number: 56413 Gerrit-PatchSet: 9 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.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-MessageType: newpatchset

1 0

Change in flashrom[master]: tests: Add tests to erase a chip
by Edward O'Callaghan (Code Review) 19 Aug '21

19 Aug '21

Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/56501 ) Change subject: tests: Add tests to erase a chip ...................................................................... tests: Add tests to erase a chip Two tests cover the code which performs do_erase operation. First one works with fake chip and dummy programmer. Fake chip has all operations defined, and a buffer to emulate chip memory. Second one uses the chip which is closer to the real one, because read/write/unlock/erase operations are real. The tests takes the advantage of dummyflasher's capability of emulating a W25Q128.V chip. BUG=b:181803212 TEST=builds and ninja test Change-Id: I6f74bfe4e02244d24d6c837cc3d551251e7b4898 Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org> Reviewed-on: https://review.coreboot.org/c/flashrom/+/56501 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Edward O'Callaghan <quasisec(a)chromium.org> --- A tests/chip.c M tests/meson.build M tests/tests.c M tests/tests.h 4 files changed, 237 insertions(+), 0 deletions(-) Approvals: build bot (Jenkins): Verified Edward O'Callaghan: Looks good to me, approved diff --git a/tests/chip.c b/tests/chip.c new file mode 100644 index 0000000..0cb05ad --- /dev/null +++ b/tests/chip.c @@ -0,0 +1,226 @@ +/* + * 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 <stdio.h> +#include <string.h> + +#include "chipdrivers.h" +#include "flash.h" +#include "programmer.h" + +#define CHIP_TOTAL_SIZE 8192 + +static struct { + unsigned int unlock_calls; /* how many times unlock function was called */ + uint8_t buf[CHIP_TOTAL_SIZE]; /* buffer of total size of chip, to emulate a chip */ +} g_chip_state = { + .unlock_calls = 0, + .buf = { 0 }, +}; + +int read_chip(struct flashctx *flash, uint8_t *buf, unsigned int start, unsigned int len) +{ + printf("Read chip called with start=0x%x, len=0x%x\n", start, len); + if (!g_chip_state.unlock_calls) { + printf("Error while reading chip: unlock was not called.\n"); + return 1; + } + + assert_in_range(start + len, 0, CHIP_TOTAL_SIZE); + + memcpy(buf, &g_chip_state.buf[start], len); + return 0; +} + +int write_chip(struct flashctx *flash, const uint8_t *buf, unsigned int start, unsigned int len) +{ + printf("Write chip called with start=0x%x, len=0x%x\n", start, len); + if (!g_chip_state.unlock_calls) { + printf("Error while writing chip: unlock was not called.\n"); + return 1; + } + + assert_in_range(start + len, 0, CHIP_TOTAL_SIZE); + + memcpy(&g_chip_state.buf[start], buf, len); + return 0; +} + +int unlock_chip(struct flashctx *flash) +{ + printf("Unlock chip called\n"); + g_chip_state.unlock_calls++; + + if (g_chip_state.unlock_calls > 1) { + printf("Error: Unlock called twice\n"); + return -1; + } + + return 0; +} + +int block_erase_chip(struct flashctx *flash, unsigned int blockaddr, unsigned int blocklen) +{ + printf("Block erase called with blockaddr=0x%x, blocklen=0x%x\n", blockaddr, blocklen); + if (!g_chip_state.unlock_calls) { + printf("Error while erasing chip: unlock was not called.\n"); + return 1; + } + + assert_in_range(blockaddr + blocklen, 0, CHIP_TOTAL_SIZE); + + memset(&g_chip_state.buf[blockaddr], 0xff, blocklen); + return 0; +} + +void erase_chip_test_success(void **state) +{ + (void) state; /* unused */ + + struct flashchip chip = { + .vendor = "aklm", + /* + * Total size less than 16 to skip some steps + * in flashrom.c#prepare_flash_access. + */ + .total_size = 8, + .tested = TEST_OK_PREW, + .read = read_chip, + .write = write_chip, + .unlock = unlock_chip, + .block_erasers = + {{ + /* All blocks within total size of the chip. */ + .eraseblocks = { {2 * 1024, 4} }, + .block_erase = block_erase_chip, + }}, + }; + struct flashrom_flashctx flash = { + .chip = &chip, + }; + + struct flashrom_layout *layout; + + printf("Creating layout with one included region... "); + assert_int_equal(0, flashrom_layout_new(&layout)); + /* One region which covers total size of chip. */ + assert_int_equal(0, flashrom_layout_add_region(layout, 0, chip.total_size * 1024 - 1, "region")); + assert_int_equal(0, flashrom_layout_include_region(layout, "region")); + + flashrom_layout_set(&flash, layout); + printf("done\n"); + + /* + * We need some programmer (any), and dummy is a very good one, + * because it doesn't need any mocking. So no extra complexity + * from a programmer side, and test can focus on the code which + * erases the chip. + */ + const char *param = ""; /* default values for all params */ + printf("Dummyflasher initialising... "); + assert_int_equal(0, programmer_init(&programmer_dummy, param)); + printf("done\n"); + + printf("Erase chip operation started.\n"); + assert_int_equal(0, do_erase(&flash)); + printf("Erase chip operation done.\n"); + + printf("Dummyflasher shutdown... "); + assert_int_equal(0, programmer_shutdown()); + printf("done\n"); + + printf("Releasing layout... "); + flashrom_layout_release(layout); + printf("done\n"); +} + +void erase_chip_with_dummyflasher_test_success(void **state) +{ + (void) state; /* unused */ + + struct flashchip chip = { + .vendor = "aklm&dummyflasher", + /* + * Setup the values for W25Q128.V because we ask dummyflasher + * to emulate this chip. All operations: read/write/unlock/erase + * are real, not mocks, and they are expected to be handled by + * dummyflasher. + */ + .total_size = 16 * 1024, + .tested = TEST_OK_PREW, + .read = spi_chip_read, + .write = spi_chip_write_256, + .unlock = spi_disable_blockprotect, + .block_erasers = + { + { + .eraseblocks = { {4 * 1024, 4096} }, + .block_erase = spi_block_erase_20, + }, { + .eraseblocks = { {32 * 1024, 512} }, + .block_erase = spi_block_erase_52, + }, { + .eraseblocks = { {64 * 1024, 256} }, + .block_erase = spi_block_erase_d8, + }, { + .eraseblocks = { {16 * 1024 * 1024, 1} }, + .block_erase = spi_block_erase_60, + }, { + .eraseblocks = { {16 * 1024 * 1024, 1} }, + .block_erase = spi_block_erase_c7, + } + }, + }; + struct flashrom_flashctx flash = { + .chip = &chip, + }; + + struct flashrom_layout *layout; + + printf("Creating layout with one included region... "); + assert_int_equal(0, flashrom_layout_new(&layout)); + /* One region which covers total size of chip. */ + assert_int_equal(0, flashrom_layout_add_region(layout, 0, chip.total_size * 1024 - 1, "region")); + assert_int_equal(0, flashrom_layout_include_region(layout, "region")); + + flashrom_layout_set(&flash, layout); + printf("done\n"); + + /* + * Dummyflasher is capable to emulate a chip, so we ask it to do this. + * Nothing to mock, dummy is taking care of this already. + */ + char *param_dup = strdup("bus=spi,emulate=W25Q128FV"); + printf("Dummyflasher initialising to emulate W25Q128FV... "); + assert_int_equal(0, programmer_init(&programmer_dummy, param_dup)); + /* Assignment below normally happens while probing, but this test is not probing. */ + flash.mst = &registered_masters[0]; + printf("done\n"); + + printf("Erase chip operation started.\n"); + assert_int_equal(0, do_erase(&flash)); + printf("Erase chip operation done.\n"); + + printf("Dummyflasher shutdown... "); + assert_int_equal(0, programmer_shutdown()); + printf("done\n"); + + printf("Releasing layout... "); + flashrom_layout_release(layout); + printf("done\n"); + + free(param_dup); +} diff --git a/tests/meson.build b/tests/meson.build index 2bde3f0..63fec5a 100644 --- a/tests/meson.build +++ b/tests/meson.build @@ -20,6 +20,7 @@ 'spi25.c', 'init_shutdown.c', 'layout.c', + 'chip.c', ] mocks = [ diff --git a/tests/tests.c b/tests/tests.c index 1368632..bfc0e53 100644 --- a/tests/tests.c +++ b/tests/tests.c @@ -292,5 +292,11 @@ }; ret |= cmocka_run_group_tests_name("layout.c tests", layout_tests, NULL, NULL); + const struct CMUnitTest chip_tests[] = { + cmocka_unit_test(erase_chip_test_success), + cmocka_unit_test(erase_chip_with_dummyflasher_test_success), + }; + ret |= cmocka_run_group_tests_name("chip.c tests", chip_tests, NULL, NULL); + return ret; } diff --git a/tests/tests.h b/tests/tests.h index 2b03458..512b72d 100644 --- a/tests/tests.h +++ b/tests/tests.h @@ -57,4 +57,8 @@ void layout_region_invalid_address_test_success(void **state); void layout_region_invalid_range_test_success(void **state); +/* chip.c */ +void erase_chip_test_success(void **state); +void erase_chip_with_dummyflasher_test_success(void **state); + #endif /* TESTS_H */ -- To view, visit https://review.coreboot.org/c/flashrom/+/56501 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I6f74bfe4e02244d24d6c837cc3d551251e7b4898 Gerrit-Change-Number: 56501 Gerrit-PatchSet: 12 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-MessageType: merged

1 0

Change in flashrom[master]: tests: Use real spi_send_command for all tests except spi25.c
by Edward O'Callaghan (Code Review) 19 Aug '21

19 Aug '21

Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/56753 ) Change subject: tests: Use real spi_send_command for all tests except spi25.c ...................................................................... tests: Use real spi_send_command for all tests except spi25.c At the moment one test (spi25.c) uses wrap of spi_send_command, and all other existing tests don't care about this function (don't call it at all). However in the next patch a new test in introduced, which needs a real spi_send_command. Following the approach "don't mock unless it is strictly necessary", all tests that don't care go into real function bucket. A declaration for __real_spi_send_command is needed for visibility, this way wrap function can redirect to real function for all other tests except spi25.c. Additionally, wrap function moves below mock_chip, so that mock_chip is visible inside wrap. BUG=b:181803212 TEST=builds and ninja test Change-Id: I22945cce3d0f36adaa8032167a3ef4e54eccb611 Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org> Reviewed-on: https://review.coreboot.org/c/flashrom/+/56753 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Edward O'Callaghan <quasisec(a)chromium.org> --- M tests/spi25.c 1 file changed, 32 insertions(+), 16 deletions(-) Approvals: build bot (Jenkins): Verified Edward O'Callaghan: Looks good to me, approved diff --git a/tests/spi25.c b/tests/spi25.c index 942fe6e..251c98c 100644 --- a/tests/spi25.c +++ b/tests/spi25.c @@ -20,22 +20,6 @@ #include "chipdrivers.h" #include "spi.h" -int __wrap_spi_send_command(const struct flashctx *flash, - unsigned int writecnt, unsigned int readcnt, - const unsigned char *writearr, unsigned char *readarr) -{ - check_expected_ptr(flash); - assert_int_equal(writecnt, mock_type(int)); - assert_int_equal(writearr[0], mock_type(int)); - - int rcnt = mock_type(int); - assert_int_equal(readcnt, rcnt); - for (int i = 0; i < rcnt; i++) - readarr[i] = i; - - return 0; -} - struct flashchip mock_chip = { .vendor = "Generic", .name = "unknown SPI chip (RDID)", @@ -49,6 +33,38 @@ .write = NULL, }; +/* + * This declaration is needed for visibility, so that wrap below could + * redirect to real function. + */ +int __real_spi_send_command(const struct flashctx *flash, + unsigned int writecnt, unsigned int readcnt, + const unsigned char *writearr, unsigned char *readarr); + +int __wrap_spi_send_command(const struct flashctx *flash, + unsigned int writecnt, unsigned int readcnt, + const unsigned char *writearr, unsigned char *readarr) +{ + if (flash->chip != &mock_chip) + /* + * Caller is some other test, redirecting to real function. + * This test is the only one which uses wrap of spi_send_command, + * all other tests use real function. + */ + return __real_spi_send_command(flash, writecnt, readcnt, writearr, readarr); + + check_expected_ptr(flash); + assert_int_equal(writecnt, mock_type(int)); + assert_int_equal(writearr[0], mock_type(int)); + + int rcnt = mock_type(int); + assert_int_equal(readcnt, rcnt); + for (int i = 0; i < rcnt; i++) + readarr[i] = i; + + return 0; +} + void spi_write_enable_test_success(void **state) { (void) state; /* unused */ -- To view, visit https://review.coreboot.org/c/flashrom/+/56753 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I22945cce3d0f36adaa8032167a3ef4e54eccb611 Gerrit-Change-Number: 56753 Gerrit-PatchSet: 6 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-MessageType: merged

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit August 2021