Michał Żygowski has abandoned this change. ( https://review.coreboot.org/c/flashrom/+/55577 )
Change subject: ich_descriptors.c: Fix PCH detection for Tiger Lake
......................................................................
Abandoned
ICH descriptor probing clean up in separate patch series
--
To view, visit https://review.coreboot.org/c/flashrom/+/55577
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ib61d432ab4eaf00aa4eef50d2844940e73b5cad6
Gerrit-Change-Number: 55577
Gerrit-PatchSet: 1
Gerrit-Owner: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
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: abandon
Attention is currently required from: Nico Huber, Angel Pons.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55934 )
Change subject: dediprog: Init-shutdown test for dediprog
......................................................................
Patch Set 3:
(5 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/55934/comment/032ff14a_606a4d45
PS3, Line 9: This patch adds mocks for libusb functions. To avoid dependency on
> I expect that some of the code could be reused to test other libusb programmers. […]
Yes, hopefully all __wrap functions will be reused for other programmers, because programmer-specific behaviour goes into current_io. Actually, the wraps here are not expected to have much code, if something needs code that code would probably go into current_io and live in the test itself.
Moving mocks into separate compilation unit would need a bit of refactoring, because at the moment there is current_io in this file, and also io_mock_register.
Ideally, I would add one or two more tests which use libusb wraps, and then maybe it will be more obvious [to me] how to move it / where to move it?
File tests/init_shutdown.c:
https://review.coreboot.org/c/flashrom/+/55934/comment/88644d17_af5ef6ad
PS3, Line 98: /* dediprog_cmds CMD_READ_PROG_INFO */
> Hmmm, how about making a patch that moves the dediprog definitions the tests need into a header?
This question came up in the review process of commit 21e22ba8a7750f1cfe5cd3323e3137695ffef0a4, discussion started at Patchset 10 May 01 01:59 init_shutdown.c#53 and the decision was not to move definitions into the header.
But things could have changed, if everyone agrees it's a good idea I can do it.
File tests/io_mock.h:
https://review.coreboot.org/c/flashrom/+/55934/comment/f5519b3b_0d77aa24
PS3, Line 34: /* Define libusb symbols to avoid dependency on libusb.h */
> Now that I realized that we don't mock `libusb.h` for the code under […]
I wrote this tests few weeks ago, but was waiting for other patches for tests/ to be done. So initially I included libusb.h without much thinking.
But then commit 20ff3d464ba20b385b84aa0eb132eeff3345accc and especially commit 4abb62c4ac1f25e40f2569535322342b9c939558 have happened, and I became more aware on what can be a consequence of including a header. Returned back to tests, and first thing I saw was libusb.h with may not be supported.
In an environment where libusb.h is not supported, I can't include it right? This would mean adding pre-processor guards, which I understand better to avoid?
Then I thought the only reason I included the header is for the libusb_device_handle type. But real handles are not needed for tests, so I replaced with void* and boom, everything works and header not needed.
Individual tests are guarded by CONFIG_XXX anyway, so test would be skipped if driver is not built? and driver not built is libusb not supported.
File tests/io_mock.h:
https://review.coreboot.org/c/flashrom/+/55934/comment/ebc6961a_0765c50b
PS2, Line 52: */
> Oh, I missed earlier that you avoid the libusb dependency here while […]
WOW thank you for explanation... So many opportunities to hack things!
File tests/tests.c:
https://review.coreboot.org/c/flashrom/+/55934/comment/a03f0286_26c59f57
PS3, Line 140: 2021
> Is this magic number the current year? Or does it have another meaning?
Yeah that's the current year. I needed something to pretend it's a valid file descriptor, and then something as a valid device handle, and there will be more of that I am sure. The number itself doesn't matter.
Is this an acceptable approach, or maybe I should create a VALID_POINTER macro instead of inline magic number?
--
To view, visit https://review.coreboot.org/c/flashrom/+/55934
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I38508dfb6d7c24d42522f22fcae0c5e410c5f7ea
Gerrit-Change-Number: 55934
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-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Comment-Date: Thu, 01 Jul 2021 00:48:07 +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