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

Change in flashrom[master]: tests: Do not run a test if its driver is not built
by Nico Huber (Code Review) 25 Jun '21

25 Jun '21

Attention is currently required from: Thomas Heijligen, Edward O'Callaghan, Anastasia Klimchuk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55295 ) Change subject: tests: Do not run a test if its driver is not built ...................................................................... Patch Set 5: (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/55295/comment/7eb1f553_18538c5d PS5, Line 10: however config options can be disabled and in that case test should : not be run. > >>>What was discussed exactly? […] I'd even say it's better to return back to this later. Why switch to another scheme before we have seen how this one turns out? I grasp from the name `init_shutdown.c` that the idea was to have individual files per tested path. Once we try to add another file, we'll probably much easier see what the advantages and disadvan- tages are. As stated earlier, missing dependencies might be a reason to favor per programmer files. For instance if we need some library's header file to compile the test code. But that's a problem that I'd try to solve only once it's clear to exist :) As for weak functions. I'd rather clutter the code with if (CONFIG(MEC1308)) than with weak declarations. In the only project I know that makes heavily use of weak symbols (coreboot) they often turn bad. -- To view, visit https://review.coreboot.org/c/flashrom/+/55295 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ic1c48e41f658045a608f46636071f478ba646f77 Gerrit-Change-Number: 55295 Gerrit-PatchSet: 5 Gerrit-Owner: Anastasia Klimchuk <aklm(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-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Fri, 25 Jun 2021 09:00:09 +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: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: hwaccess_x86_io_unittest: Add dummy iopl to avoid including sys/io.h
by Angel Pons (Code Review) 25 Jun '21

25 Jun '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/+/55741 ) Change subject: hwaccess_x86_io_unittest: Add dummy iopl to avoid including sys/io.h ...................................................................... Patch Set 6: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/55741 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I3f2f0408be7c00f954b899031b52b2b97ef19ca3 Gerrit-Change-Number: 55741 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-Comment-Date: Fri, 25 Jun 2021 07:59:42 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: hwaccess_x86_io_unittest: Add dummy iopl to avoid including sys/io.h
by Angel Pons (Code Review) 25 Jun '21

25 Jun '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/+/55741 ) Change subject: hwaccess_x86_io_unittest: Add dummy iopl to avoid including sys/io.h ...................................................................... Patch Set 6: (1 comment) Patchset: PS5: > It is working!! No more sys/io. […] This is why I suggested trying to run the test on ARM. If we're mocking the arch-specific stuff, we don't need system support 😉 -- To view, visit https://review.coreboot.org/c/flashrom/+/55741 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I3f2f0408be7c00f954b899031b52b2b97ef19ca3 Gerrit-Change-Number: 55741 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-Comment-Date: Fri, 25 Jun 2021 07:59:17 +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

1 0

Change in flashrom[master]: tests: Add unit test to run init/shutdown for mec1308.c, ene_lpc.c
by Anastasia Klimchuk (Code Review) 25 Jun '21

25 Jun '21

Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/51487 ) Change subject: tests: Add unit test to run init/shutdown for mec1308.c, ene_lpc.c ...................................................................... Patch Set 19: (1 comment) File hwaccess_x86_io_unittest.h: https://review.coreboot.org/c/flashrom/+/51487/comment/e9267370_97b99faf PS19, Line 41: #include <sys/io.h> > > I am heavily confused now, how the tests work without it? […] Thank you so much! I used the idea in CB:55741, it is working! -- 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: 19 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: Simon Glass <sjg(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Comment-Date: Fri, 25 Jun 2021 06:43:35 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> 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]: hwaccess_x86_io_unittest: Add dummy iopl to avoid including sys/io.h
by Anastasia Klimchuk (Code Review) 25 Jun '21

25 Jun '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/+/55741 ) Change subject: hwaccess_x86_io_unittest: Add dummy iopl to avoid including sys/io.h ...................................................................... Patch Set 6: (1 comment) Patchset: PS5: > I really like your idea in CB:51487 not to use sys/io. […] It is working!! No more sys/io.h ! :) -- To view, visit https://review.coreboot.org/c/flashrom/+/55741 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I3f2f0408be7c00f954b899031b52b2b97ef19ca3 Gerrit-Change-Number: 55741 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: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Fri, 25 Jun 2021 06:42:14 +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

1 0

Change in flashrom[master]: hwaccess_x86_io_unittest: Add dummy iopl to avoid including sys/io.h
by Anastasia Klimchuk (Code Review) 25 Jun '21

25 Jun '21

Attention is currently required from: Nico Huber, 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/+/55741 to look at the new patch set (#6). Change subject: hwaccess_x86_io_unittest: Add dummy iopl to avoid including sys/io.h ...................................................................... hwaccess_x86_io_unittest: Add dummy iopl to avoid including sys/io.h sys/io.h is platform specific, and also in tests environment we don't need real functions anyway. Adding dummy implementation of iopl is sufficient for tests. The rest of io is not needed because hwaccess_x86_io_unittest.h re-defines macros OUTB/INB/etc and those macros evaluate to test-only functions. This is a follow up on commit 21e22ba8a7750f1cfe5cd3323e3137695ffef0a4 which introduced hwaccess_x86_io_unittest.h BUG=b:181803212 TEST=builds and ninja test on x86 (same as before) Change-Id: I3f2f0408be7c00f954b899031b52b2b97ef19ca3 Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org> --- M tests/hwaccess_x86_io_unittest.h 1 file changed, 11 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/41/55741/6 -- To view, visit https://review.coreboot.org/c/flashrom/+/55741 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I3f2f0408be7c00f954b899031b52b2b97ef19ca3 Gerrit-Change-Number: 55741 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: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: tests: Do not run a test if its driver is not built
by Anastasia Klimchuk (Code Review) 25 Jun '21

25 Jun '21

Attention is currently required from: Nico Huber, Thomas Heijligen, Edward O'Callaghan. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55295 ) Change subject: tests: Do not run a test if its driver is not built ...................................................................... Patch Set 5: (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/55295/comment/a5aa26a7_98c4ccc0 PS5, Line 10: however config options can be disabled and in that case test should : not be run. > > As we discussed today; […] >>>What was discussed exactly? It is all in the first comment, the idea to split the tests and their object files, and more details on how to do this. That was exactly the discussion. "As we discussed" is probably for me to better understand what the comment is about. So you think this should not be a priority? You mean we can return back to this question later, not now? -- To view, visit https://review.coreboot.org/c/flashrom/+/55295 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ic1c48e41f658045a608f46636071f478ba646f77 Gerrit-Change-Number: 55295 Gerrit-PatchSet: 5 Gerrit-Owner: Anastasia Klimchuk <aklm(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-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Comment-Date: Fri, 25 Jun 2021 01:00:28 +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: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: tests: Guard sys/io.h the same as in hwaccess_x86_io.h
by Anastasia Klimchuk (Code Review) 25 Jun '21

25 Jun '21

Attention is currently required from: Nico Huber, Angel Pons. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55741 ) Change subject: tests: Guard sys/io.h the same as in hwaccess_x86_io.h ...................................................................... Patch Set 5: (2 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/55741/comment/72a10f5d_65549313 PS2, Line 19: BUG=b:181803212 > I'd like to know what the bug is? This "bug" is not really a bug, more like a task, and it just says "Unit tests for Flashrom". I need to add this every time so that my work can be tracked. It feels a bit awkward for me, because I know you can't open b:181803212 and it's just some magic number :\ I have only two magic numbers, this one and 185191942 for register_master API. Everything that is relevant I always discuss in gerrit, so that everyone can see. Patchset: PS5: > It seems this is trying to fix problems that don't exist yet? Neither […] I really like your idea in CB:51487 not to use sys/io.h at all, and instead add few more lines that we need. I am happy to do this instead of this patch. Why sys/io.h is here in the first place: I was trying to make tests work and noticed that including this header helps, got cheered up and wasn't looking further. But making tests work without including the header is even better of course. -- To view, visit https://review.coreboot.org/c/flashrom/+/55741 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I3f2f0408be7c00f954b899031b52b2b97ef19ca3 Gerrit-Change-Number: 55741 Gerrit-PatchSet: 5 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: Fri, 25 Jun 2021 00:48:02 +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: Add unit test to run init/shutdown for mec1308.c, ene_lpc.c
by Nico Huber (Code Review) 24 Jun '21

24 Jun '21

Attention is currently required from: Anastasia Klimchuk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/51487 ) Change subject: tests: Add unit test to run init/shutdown for mec1308.c, ene_lpc.c ...................................................................... Patch Set 19: (1 comment) File hwaccess_x86_io_unittest.h: https://review.coreboot.org/c/flashrom/+/51487/comment/9458cffa_824129c7 PS19, Line 41: #include <sys/io.h> > I am heavily confused now, how the tests work without it? Nevermind, rget_io_perms() is already wrapped, that's why it's working :) -- 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: 19 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: Simon Glass <sjg(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Thu, 24 Jun 2021 20:17:06 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> 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: Add unit test to run init/shutdown for mec1308.c, ene_lpc.c
by Nico Huber (Code Review) 24 Jun '21

24 Jun '21

Attention is currently required from: Anastasia Klimchuk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/51487 ) Change subject: tests: Add unit test to run init/shutdown for mec1308.c, ene_lpc.c ...................................................................... Patch Set 19: (1 comment) File hwaccess_x86_io_unittest.h: https://review.coreboot.org/c/flashrom/+/51487/comment/6d93dfd8_cddea923 PS19, Line 41: #include <sys/io.h> > If we're mocking I/O access, we most likely don't need to use system headers. I just tested what is missing to compile on Linux: Just the forward declarations of the i/o functions and iopl(). The former are not even used with `hwaccess_x86_io_unittest.h` and the latter, it seems, we forgot to mock anyway ;) so simply replacing the #include with this might be enough: static inline int iopl(int level) { return 0; } I am heavily confused now, how the tests work without it? -- 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: 19 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: Simon Glass <sjg(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Thu, 24 Jun 2021 19:53:36 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> 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

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit June 2021