Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons, Peter Marheine.
Hello build bot (Jenkins), Nico Huber, Edward O'Callaghan, Angel Pons, Anastasia Klimchuk,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/56911
to look at the new patch set (#3).
Change subject: tests: add init_shutdown test for realtek_mst_i2c_spi
......................................................................
tests: add init_shutdown test for realtek_mst_i2c_spi
This can catch regressions like the earlier one in this programmer that
caused initialization to always fail. Requires support for mocking POSIX
file I/O functions because the programmer does ioctls on the opened
file.
TEST=ninja test
Signed-off-by: Peter Marheine <pmarheine(a)chromium.org>
Change-Id: I5a5c617d1ec35d2a3bbe622e5add82a65eb396f0
---
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, 110 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/11/56911/3
--
To view, visit https://review.coreboot.org/c/flashrom/+/56911
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I5a5c617d1ec35d2a3bbe622e5add82a65eb396f0
Gerrit-Change-Number: 56911
Gerrit-PatchSet: 3
Gerrit-Owner: Peter Marheine <pmarheine(a)chromium.org>
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-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-MessageType: newpatchset
Attention is currently required from: Nico Huber, Edward O'Callaghan, Anastasia Klimchuk, Peter Marheine.
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 (#6).
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, 163 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/13/56413/6
--
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: 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-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-Attention: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-MessageType: newpatchset
Attention is currently required from: Peter Marheine.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/56911 )
Change subject: tests: add init_shutdown test for realtek_mst_i2c_spi
......................................................................
Patch Set 2:
(5 comments)
File tests/init_shutdown.c:
https://review.coreboot.org/c/flashrom/+/56911/comment/6e8a138b_0bd1ace7
PS2, Line 307: 0x10ec
This number has some meaning, also I see it is used in other mocks, so surely it is important. Could you please add a short comment here on what this is? Does it have to be exactly this number, or any number, but the same in all mocks? Maybe it can be #define REALTEK_MOCK_FD ?
https://review.coreboot.org/c/flashrom/+/56911/comment/c6e004be_1b7d5a11
PS2, Line 319: printf("ioctl(0x10ec, I2C_SLAVE, %#lx)\n", addr);
This looks like a debugging statement, not sure we need it. Wrap invocations log themselves already (that first line LOG_ME does it), which should be sufficient for a normal situation, when tests pass.
Same for the rest of printf, let's remove. I use them a lot when writing a test, but once test is ready that would just make log larger.
(and when tests fail that would be a more targeted debugging anyway).
For future, if I discover a way to tune verbosity level in cmocka, we may add debug-only messages.
https://review.coreboot.org/c/flashrom/+/56911/comment/2a7af034_ea6cbf70
PS2, Line 320: 0x4a
This number also needs a comment
https://review.coreboot.org/c/flashrom/+/56911/comment/d916089d_b9c5cc24
PS2, Line 341: printf("i2c set register address %#x\n", bytes_buf[0]);
: return 1;
If you remove printf statements, this looks like one error path which returns -1 and one normal path which returns sz?
https://review.coreboot.org/c/flashrom/+/56911/comment/46a57ea7_61298014
PS2, Line 365: T
typo
--
To view, visit https://review.coreboot.org/c/flashrom/+/56911
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I5a5c617d1ec35d2a3bbe622e5add82a65eb396f0
Gerrit-Change-Number: 56911
Gerrit-PatchSet: 2
Gerrit-Owner: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Comment-Date: Thu, 12 Aug 2021 03:50:00 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Anastasia Klimchuk.
Peter Marheine has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/56911 )
Change subject: tests: add init_shutdown test for realtek_mst_i2c_spi
......................................................................
Patch Set 2:
(7 comments)
File tests/init_shutdown.c:
https://review.coreboot.org/c/flashrom/+/56911/comment/db94d22d_08b304de
PS1, Line 302: #include <fcntl.h>
: #include <linux/i2c-dev.h>
> Tests should be as lightweight and independent as possible, they only thing to check is CONFIG_REALT […]
Done
https://review.coreboot.org/c/flashrom/+/56911/comment/bb3ee746_c289f31c
PS1, Line 364: io_mock_register(&realtek_mst_io);
> You need to unregister at the end, […]
Done
File tests/tests.c:
https://review.coreboot.org/c/flashrom/+/56911/comment/373fcab8_07027324
PS1, Line 93: if (current_io->open64)
: return current_io->open64(current_io->state, pathname, flags);
: if (current_io->open)
: return current_io->open(current_io->state, pathname, flags);
> Why do you put both open and open64 into one wrap? There is a dedicated wrap for open, so […]
The intent is to have open64 delegate to open because I expect users mostly won't care about the difference, but neglected to implement the latter. Moved the open() part to __wrap_open and made __wrap_open64 delegate to it.
https://review.coreboot.org/c/flashrom/+/56911/comment/e27d85d2_55315429
PS1, Line 120: return -1;
> Default behaviour for our wraps is "do nothing, pretend everything is successful". […]
Done
https://review.coreboot.org/c/flashrom/+/56911/comment/ff623a2b_3ab106d1
PS1, Line 127: return current_io->read(current_io->state, fd, buf, sz);
> We are following Linux kernel coding style https://git.kernel. […]
Neglected to reformat this file but did the other one. Fixed.
https://review.coreboot.org/c/flashrom/+/56911/comment/3a534342_e08bd9c4
PS1, Line 128: return -1;
> Same here, you need to return value which means success.
Done
https://review.coreboot.org/c/flashrom/+/56911/comment/8dd198a5_8635858c
PS1, Line 345: cmocka_unit_test
> Same thing, indentations should be 8 chars
Done
--
To view, visit https://review.coreboot.org/c/flashrom/+/56911
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I5a5c617d1ec35d2a3bbe622e5add82a65eb396f0
Gerrit-Change-Number: 56911
Gerrit-PatchSet: 2
Gerrit-Owner: Peter Marheine <pmarheine(a)chromium.org>
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: Thu, 12 Aug 2021 02:40:45 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Peter Marheine.
Hello build bot (Jenkins), Anastasia Klimchuk,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/56911
to look at the new patch set (#2).
Change subject: tests: add init_shutdown test for realtek_mst_i2c_spi
......................................................................
tests: add init_shutdown test for realtek_mst_i2c_spi
This can catch regressions like the earlier one in this programmer that
caused initialization to always fail. Requires support for mocking POSIX
file I/O functions because the programmer does ioctls on the opened
file.
TEST=ninja test
Signed-off-by: Peter Marheine <pmarheine(a)chromium.org>
Change-Id: I5a5c617d1ec35d2a3bbe622e5add82a65eb396f0
---
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, 117 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/11/56911/2
--
To view, visit https://review.coreboot.org/c/flashrom/+/56911
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I5a5c617d1ec35d2a3bbe622e5add82a65eb396f0
Gerrit-Change-Number: 56911
Gerrit-PatchSet: 2
Gerrit-Owner: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-MessageType: newpatchset
Attention is currently required from: Nico Huber, Angel Pons.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/56501 )
Change subject: tests: Add tests to erase a chip
......................................................................
Patch Set 9:
(1 comment)
Patchset:
PS2:
> Some more explanations (sorry for spam :D) […]
I made it work, so that wrap redirects into real function for all tests except spi25.c, uploaded into #1 in this chain. This one doesn't need any changes!
--
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: 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: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Comment-Date: Thu, 12 Aug 2021 01:26:32 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: comment
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/+/56753 )
Change subject: tests: Use real spi_send_command for all tests except spi25.c
......................................................................
Patch Set 3:
(1 comment)
Patchset:
PS3:
I have good news! I was able to make it work without splitting into two executables, I am so happy!
So this patch is completely re-done (and it's much smaller now). The next one in the chain, with erase test, stays the same.
--
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: 3
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, 12 Aug 2021 01:24:29 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
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/+/56753
to look at the new patch set (#3).
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.
BUG=b:181803212
TEST=builds and ninja test
Change-Id: I22945cce3d0f36adaa8032167a3ef4e54eccb611
Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org>
---
M tests/spi25.c
1 file changed, 28 insertions(+), 16 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/53/56753/3
--
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: 3
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
Attention is currently required from: Peter Marheine.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/56911 )
Change subject: tests: add init_shutdown test for realtek_mst_i2c_spi
......................................................................
Patch Set 1:
(8 comments)
Patchset:
PS1:
Thanks for writing a test!
File tests/init_shutdown.c:
https://review.coreboot.org/c/flashrom/+/56911/comment/8df145ce_db617b87
PS1, Line 302: #include <fcntl.h>
: #include <linux/i2c-dev.h>
Tests should be as lightweight and independent as possible, they only thing to check is CONFIG_REALTEK_MST_I2C_SPI == 1, and that's inside test method.
Detect exactly what is the minimum set of symbols you need from these libraries (is it O_RDWR and I2C_SLAVE?) and define them in io_mock.h (there are some examples of that already).
Remove these includes, remove check for IS_LINUX.
Mock implementations (open,ioctl,read,write) should compile as is, you only check CONFIG_REALTEK_MST_I2C_SPI == 1 inside the test method.
Follow the example of all other tests.
https://review.coreboot.org/c/flashrom/+/56911/comment/4f925a5e_348e9737
PS1, Line 364: io_mock_register(&realtek_mst_io);
You need to unregister at the end,
`io_mock_register(NULL);`
(you can look at the other tests)
File tests/tests.c:
https://review.coreboot.org/c/flashrom/+/56911/comment/0bdaa2c5_1fdc3cc9
PS1, Line 93: if (current_io->open64)
: return current_io->open64(current_io->state, pathname, flags);
: if (current_io->open)
: return current_io->open(current_io->state, pathname, flags);
Why do you put both open and open64 into one wrap? There is a dedicated wrap for open, so
`current_io->open`
should go there.
https://review.coreboot.org/c/flashrom/+/56911/comment/7c4aa9ad_0703901f
PS1, Line 120: return -1;
Default behaviour for our wraps is "do nothing, pretend everything is successful". You need to return a value which emulates success. Is it sz for this write operation?
https://review.coreboot.org/c/flashrom/+/56911/comment/27a21a61_69f228be
PS1, Line 127: return current_io->read(current_io->state, fd, buf, sz);
We are following Linux kernel coding style https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Doc… which says that indentations are 8 chars.
https://review.coreboot.org/c/flashrom/+/56911/comment/01ad558a_8a5fc193
PS1, Line 128: return -1;
Same here, you need to return value which means success.
https://review.coreboot.org/c/flashrom/+/56911/comment/cb85340b_14ff6424
PS1, Line 345: cmocka_unit_test
Same thing, indentations should be 8 chars
--
To view, visit https://review.coreboot.org/c/flashrom/+/56911
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I5a5c617d1ec35d2a3bbe622e5add82a65eb396f0
Gerrit-Change-Number: 56911
Gerrit-PatchSet: 1
Gerrit-Owner: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Comment-Date: Wed, 11 Aug 2021 23:30:56 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment