Attention is currently required from: Angel Pons, Daniel Campello.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/51930 )
Change subject: Makefile,meson.build: Fix dependency issues with raiden_debug_spi
......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1:
> Thanks! Having a last look, I've noticed another potential […]
No worries, luckily I saw the email in my uncontrollable corp inbox.
Do you mean line `1090: NEED_LIBUSB1 += CONFIG_DEDIPROG` isn't it already added or did I miss something?
--
To view, visit https://review.coreboot.org/c/flashrom/+/51930
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I2d6a8c33a2228abf006a9b278bcb7133765c7074
Gerrit-Change-Number: 51930
Gerrit-PatchSet: 1
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Daniel Campello <campello(a)chromium.org>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Daniel Campello <campello(a)chromium.org>
Gerrit-Comment-Date: Wed, 31 Mar 2021 23:41:28 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Angel Pons.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/51487 )
Change subject: Add unit test to run init/shutdown for enabled drivers
......................................................................
Patch Set 1:
(1 comment)
File mec1308.c:
https://review.coreboot.org/c/flashrom/+/51487/comment/004d3454_73f426ba
PS1, Line 115: #endif /* UNIT_TEST_ENV */
> I know it won't be as easy, but I'd much prefer such mocks in […]
I was thinking about separate compilation units for a while, how exactly to do this. Maybe you have some concrete ideas in mind?
All that inb/outb functionality is in hwaccess.h. Can we have a hwaccess_unittest.h for test environment? Although it does not solve the question because for every driver needs its own mocking. For example mec1308 in this patch expects specific value from inb, not just any value.
--
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-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Comment-Date: Wed, 31 Mar 2021 22:27:33 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Gerrit-MessageType: comment
Attention is currently required from: David Hendricks, Edward O'Callaghan, 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 7: Code-Review+2
--
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: 7
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: David Hendricks <david.hendricks(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: Daniel Kurtz <djkurtz(a)chromium.org>
Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-CC: Thomas Heijligen <src(a)posteo.de>
Gerrit-CC: Victor Ding <victording(a)chromium.org>
Gerrit-Attention: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Wed, 31 Mar 2021 22:24:45 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Thomas Heijligen, Paul Menzel.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/51979 )
Change subject: test_build.sh: use -C option of ninja to specify the build directory
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit https://review.coreboot.org/c/flashrom/+/51979
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I04a0fdf9b5126b9f4006e8229c3926ceb1013456
Gerrit-Change-Number: 51979
Gerrit-PatchSet: 3
Gerrit-Owner: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Comment-Date: Wed, 31 Mar 2021 22:24:35 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: David Hendricks, Edward O'Callaghan, Angel Pons, Anastasia Klimchuk.
Nico Huber 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 7: Code-Review+2
(1 comment)
File meson.build:
https://review.coreboot.org/c/flashrom/+/51243/comment/27bad3fb_f29ac524
PS6, Line 477: -include
> I tried space after -include and it does not work: […]
Ah, sorry. I'm still very used to the comfort and pain of having
everything in a shell command line.
I suppose every string is always passed as one argument, so one
would never have to do any (shell) quoting. Then the separated
alternative would be something like:
'-include', 'stdlib.h',
'-include', 'unittest_env.h',
But I'm not even sure if I'd prefer that. Your call.
--
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: 7
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: David Hendricks <david.hendricks(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: Daniel Kurtz <djkurtz(a)chromium.org>
Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-CC: Thomas Heijligen <src(a)posteo.de>
Gerrit-CC: Victor Ding <victording(a)chromium.org>
Gerrit-Attention: David Hendricks <david.hendricks(a)gmail.com>
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: Wed, 31 Mar 2021 22:04:51 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
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: Thomas Heijligen, Paul Menzel, Angel Pons.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/51979 )
Change subject: test_build.sh: use -C option of ninja to specify the build directory
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit https://review.coreboot.org/c/flashrom/+/51979
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I04a0fdf9b5126b9f4006e8229c3926ceb1013456
Gerrit-Change-Number: 51979
Gerrit-PatchSet: 3
Gerrit-Owner: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Comment-Date: Wed, 31 Mar 2021 22:00:21 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Thomas Heijligen.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/51925 )
Change subject: [WIP] replace programmer enum with a pointer to programmer_entry
......................................................................
Patch Set 4:
(5 comments)
This change is ready for review.
Patchset:
PS4:
Looks quite good already. I think the DEFAULT_PROGRAMMER issue
(and also the INTERNAL_PROGRAMMER checks) would solve itself
if you would move the programmer_entry structs to the individual
source files, first. e.g.
* first commit: move programmer_entry, turn programmer_table into an array of pointers (leaving the enum stuff as is for now), and s/programmer_table[.*]\./programmer_table[.*]->/ everywhere.
* second commit: introduce a default_programmer pointer. (const, set at compile time,
replacing CONFIG_DEFAULT_PROGRAMMER).
* third commit: this change, using `default_programmer` and `&internal_programmer` where needed.
File cli_classic.c:
https://review.coreboot.org/c/flashrom/+/51925/comment/de8b970a_e4e10051
PS4, Line 379: size_t _i
Common flashrom style is to avoid such `for` local declarations. What I usually
say here: the style should be discussed on the mailing list (and I really wouldn't
mind).
Anyway, you can just use `i` here.
https://review.coreboot.org/c/flashrom/+/51925/comment/5e7672d3_809b9a64
PS4, Line 383: prog = &programmer_table[_i];
This needs to be moved to the positive cases below (':' and '\0'). Otherwise we
would turn this into a prefix match by accident. Or, when we hit the `continue;`
below, it should be reset to `NULL`.
File flashrom.c:
https://review.coreboot.org/c/flashrom/+/51925/comment/f6a76ee5_276acf11
PS4, Line 2452: cb_check_image(newcontents, flash_size) < 0) {
Should be indented with either 4 spaces or 2 tabs (shouldn't match with the
body). Or don't break the line: our limit is 112 chars[1].
[1] https://flashrom.org/Development_Guidelines#Coding_style
File libflashrom.c:
https://review.coreboot.org/c/flashrom/+/51925/comment/5f85e505_4c9930cc
PS4, Line 128: }
This is quite weird. It leaves the last two pointers uninitialized.
The API seems also meaningless, there is no way to know how many
items are returned.
--
To view, visit https://review.coreboot.org/c/flashrom/+/51925
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I7435ee2bb661e4f139034bb289b84ebd41e1e9e9
Gerrit-Change-Number: 51925
Gerrit-PatchSet: 4
Gerrit-Owner: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-CC: Xiang Wang <merle(a)hardenedlinux.org>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Comment-Date: Wed, 31 Mar 2021 14:33:30 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Paul Menzel, Angel Pons.
Thomas Heijligen has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/51979 )
Change subject: test_build.sh: use -C option of ninja to specify the build directory
......................................................................
Patch Set 3:
(1 comment)
Commit Message:
https://review.coreboot.org/c/flashrom/+/51979/comment/4412a0ec_685d30ba
PS2, Line 7: test_build.sh: use -C option of ninja
> … to specify the build directory
Done
--
To view, visit https://review.coreboot.org/c/flashrom/+/51979
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I04a0fdf9b5126b9f4006e8229c3926ceb1013456
Gerrit-Change-Number: 51979
Gerrit-PatchSet: 3
Gerrit-Owner: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Comment-Date: Wed, 31 Mar 2021 13:24:52 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Thomas Heijligen, Angel Pons.
Hello build bot (Jenkins), Nico Huber, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/51979
to look at the new patch set (#3).
Change subject: test_build.sh: use -C option of ninja to specify the build directory
......................................................................
test_build.sh: use -C option of ninja to specify the build directory
Change-Id: I04a0fdf9b5126b9f4006e8229c3926ceb1013456
Signed-off-by: Thomas Heijligen <thomas.heijligen(a)secunet.de>
---
M test_build.sh
1 file changed, 5 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/79/51979/3
--
To view, visit https://review.coreboot.org/c/flashrom/+/51979
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I04a0fdf9b5126b9f4006e8229c3926ceb1013456
Gerrit-Change-Number: 51979
Gerrit-PatchSet: 3
Gerrit-Owner: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-MessageType: newpatchset
Attention is currently required from: Nico Huber, Paul Menzel, Thomas Heijligen.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/51978 )
Change subject: test_build.sh: use sh from env
......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1:
> I think there are pros and cons for this pattern.
Which ones? I'm curious.
If I were an English teacher, I'd ask you to write a for-and-against essay. 😜 But since I'm not, a brief explanation is more than enough 😊
--
To view, visit https://review.coreboot.org/c/flashrom/+/51978
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I3897b8d980425ecbb89b238d4a766f628cf9d3e6
Gerrit-Change-Number: 51978
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
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: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Comment-Date: Wed, 31 Mar 2021 12:44:07 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-MessageType: comment