Attention is currently required from: Daniel Campello, Peter Marheine.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52876 )
Change subject: flashrom_tester: update syntax for --wp-range
......................................................................
Patch Set 1: Code-Review+2
--
To view, visit https://review.coreboot.org/c/flashrom/+/52876
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: If17a40fba1f7d41e09e0163b353d1602c215c8db
Gerrit-Change-Number: 52876
Gerrit-PatchSet: 1
Gerrit-Owner: Daniel Campello <campello(a)chromium.org>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Daniel Campello <campello(a)chromium.org>
Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Comment-Date: Tue, 04 May 2021 03:05:03 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
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/+/52362
to look at the new patch set (#16).
Change subject: cli_classic.c: Make -r/-w/-v argument optional
......................................................................
cli_classic.c: Make -r/-w/-v argument optional
Makes filename optional from -r/-w/-v since the -i parameter allows
building the image to be written from multiple files, and regions to be
read from flash and written to separate image files,
Signed-off-by: Daniel Campello <campello(a)chromium.org>
Change-Id: I15384b92cb987a6ea1bb4369ef415488b9cf7f41
---
M cli_classic.c
M flashrom.c
2 files changed, 86 insertions(+), 18 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/62/52362/16
--
To view, visit https://review.coreboot.org/c/flashrom/+/52362
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I15384b92cb987a6ea1bb4369ef415488b9cf7f41
Gerrit-Change-Number: 52362
Gerrit-PatchSet: 16
Gerrit-Owner: Daniel Campello <campello(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: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-CC: Nikolai Artemiev <nartemiev(a)google.com>
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: Nico Huber, Edward O'Callaghan, Angel Pons, Simon Glass.
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 10:
(2 comments)
File tests/init_shutdown.c:
https://review.coreboot.org/c/flashrom/+/51487/comment/334bd496_cae2d50c
PS10, Line 53: MEC1308_SIO_PORT1
> Well you can move them to a header file
Yes, although that would mean including more header files here ... but maybe this is fine?
In any case, that would be in a separate patch.
What do other people think about it? And if yes, would you prefer do that separate patch before or after this one?
File tests/tests.c:
https://review.coreboot.org/c/flashrom/+/51487/comment/13e1d1b3_8ed2075e
PS10, Line 92: if (current_io && current_io->outb)
> OK, so do you mean to allow current_io to be NULL as well?
Yes! If a test wants io mocking, it sets current_io before executing whatever it is testing, and then at the end cleans up and sets current_io NULL (you can have a look at init_shutdown.c).
So by default when test starts execution, current_io is NULL and if "do nothing" mocking is sufficient then nothing to do, NULL means "do nothing".
--
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: 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-CC: Simon Glass <sjg(a)chromium.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: Simon Glass <sjg(a)chromium.org>
Gerrit-Comment-Date: Tue, 04 May 2021 02:07:36 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Comment-In-Reply-To: Simon Glass <sjg(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Simon Buhrow, Edward O'Callaghan, Samir Ibradžić.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/40477 )
Change subject: ft2232_spi.c: Implement spi_send_multicommand()
......................................................................
Patch Set 32:
(1 comment)
Patchset:
PS32:
> * I´m running flashrom on a virutal machine (don´t think this makes any difference)
This explains it (unless you have passed the whole USB controller
into the VM). Emulated USB controllers cause a high latency for
individual packets. The dediprog driver is also optimized to avoid
waiting on packets (at least for reading) because I use it much with
virtualbox.
> NB: yepp, I noticed that with the erase block size, as well and have it on my todo list. I was thinking about something like comandline parameter to select block size or an extra parameter at flashchips.c to mark the preferred block size. May be we should open a new patch for that to discuss ideas... (it was just because of lack of support for this patch for month why I did not step further)
Giving a hint on the command line might work. The "preferred" block
size depends on the writing pattern, i.e. how big are the changed
chunks compared to current flash contents. So that's nothing to
predict.
--
To view, visit https://review.coreboot.org/c/flashrom/+/40477
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ie4a07499ec5ef0af23818593f45dc427285a9e8a
Gerrit-Change-Number: 40477
Gerrit-PatchSet: 32
Gerrit-Owner: Simon Buhrow
Gerrit-Reviewer: Alan Green <avg(a)google.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: Samir Ibradžić <sibradzic(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Simon Buhrow
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Samir Ibradžić <sibradzic(a)gmail.com>
Gerrit-Comment-Date: Mon, 03 May 2021 16:20:33 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Simon Buhrow
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Gerrit-MessageType: comment
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/+/52774 )
Change subject: pickit2_spi.c: Refactor singleton states into reentrant pattern
......................................................................
Patch Set 2: Code-Review+1
(2 comments)
File pickit2_spi.c:
https://review.coreboot.org/c/flashrom/+/52774/comment/4b51141e_2977d293
PS1, Line 57: };
> Oh I am so happy that you asked! We haven't discussed this aspect in details, which I was slightly w […]
I see what you are getting at.
Still, in theory, a struct with a single member is storage-wise the
same as that member without a struct. If you'd let the framework
allocate a pointer (instead of a struct with only a pointer within)
it wouldn't make a difference. `data` would then be of type
`libusb_device_handle **`. A pointer to a struct is nicer than
that, so struct seems good!
https://review.coreboot.org/c/flashrom/+/52774/comment/2bf8b7dd_2b612051
PS1, Line 343: static struct spi_master spi_master_pickit2 = {
> >>> The per-instance `data` pointer should simply be passed to register_spi_master() […]
Ack
--
To view, visit https://review.coreboot.org/c/flashrom/+/52774
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ibacc4738bee02c371c41583d321e0337128ad18a
Gerrit-Change-Number: 52774
Gerrit-PatchSet: 2
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: Mon, 03 May 2021 16:07:59 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
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
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/+/52773 )
Change subject: pickit2_spi.c: Separate shutdown from failed init cleanup
......................................................................
Patch Set 1:
(1 comment)
File pickit2_spi.c:
https://review.coreboot.org/c/flashrom/+/52773/comment/f84d14d9_bd9f498e
PS1, Line 482: goto init_err_cleanup_exit;
> At the same time, what was before: if failure happened between pickit2_get_firmware_version() and register_spi_master() then pickit2_shutdown() would be called (because at the point when pickit2_get_firmware_version() run, shutdown function has already been registered). This stays the same: all failures before pickit2_get_firmware_version() do cleanup for themselves, all failures after pickit2_get_firmware_version() rely on pickit2_shutdown().
I think you are right wrt. the state of the hardware. But software-wise,
in case register_shutdown() failed, we'd have left dangling resources
(e.g. libusb_exit() wasn't called).
> What do you think? I can change code here if you think I should do that, no problem!
The patch is fine, hence the +2. I only wanted to point our that behavioral
changes usually have priority in commit messages. Generally, this can also
be achieved by splitting them out into separate commits.
Nothing to do here.
--
To view, visit https://review.coreboot.org/c/flashrom/+/52773
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I1b672b33169a7a1b6ceab190ad3f48c2f35c3a1f
Gerrit-Change-Number: 52773
Gerrit-PatchSet: 1
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: Miklós Márton <martonmiklosqdev(a)gmail.com>
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: Mon, 03 May 2021 15:56:06 +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.
Simon Glass 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 10:
(2 comments)
File tests/init_shutdown.c:
https://review.coreboot.org/c/flashrom/+/51487/comment/cb2965ae_12d7618c
PS10, Line 53: MEC1308_SIO_PORT1
> These are not enums, these are #define values in mec1308. […]
Well you can move them to a header file
File tests/tests.c:
https://review.coreboot.org/c/flashrom/+/51487/comment/12490f1d_179d94bb
PS10, Line 92: if (current_io && current_io->outb)
> There are drivers where "doing nothing" mocking strategy is sufficient. […]
OK, so do you mean to allow current_io to be NULL as well?
--
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: 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-CC: Simon Glass <sjg(a)chromium.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: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Mon, 03 May 2021 15:00:51 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Comment-In-Reply-To: Simon Glass <sjg(a)chromium.org>
Gerrit-MessageType: comment