Attention is currently required from: Subrata Banik, Angel Pons, Nikolai Artemiev.
Hello build bot (Jenkins), Subrata Banik, Edward O'Callaghan,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/69129
to look at the new patch set (#15).
Change subject: writeprotect,ichspi,spi25: handle register access constraints
......................................................................
writeprotect,ichspi,spi25: handle register access constraints
Make the spi25 register read/write functions return SPI_INVALID_OPCODE
if the programmer blocks the read/write opcode for the register.
Likewise, make ichspi read/write register functions return
SPI_INVALID_OPCODE for registers >SR1 as they cannot be accessd.
Make writeprotect ignore SPI_INVALID_OPCODE unless it is trying to
read/write SR1, which should always be supported.
BUG=b:253715389,b:253713774,b:240229722
BRANCH=none
TEST=flashrom --wp-{enable,disable,range,list,status} on dedede
Change-Id: I2145749dcc51f4556550650dab5aa1049f879c45
Signed-off-by: Nikolai Artemiev <nartemiev(a)google.com>
---
M ichspi.c
M spi25_statusreg.c
M writeprotect.c
3 files changed, 71 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/29/69129/15
--
To view, visit https://review.coreboot.org/c/flashrom/+/69129
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I2145749dcc51f4556550650dab5aa1049f879c45
Gerrit-Change-Number: 69129
Gerrit-PatchSet: 15
Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Subrata Banik <subratabanik(a)google.com>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-MessageType: newpatchset
Attention is currently required from: Felix Singer, Nico Huber, Thomas Heijligen, Angel Pons, Anastasia Klimchuk, Alexander Goncharov.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67393 )
Change subject: tree/: Move programmer_delay() out of programmer state machine
......................................................................
Patch Set 9:
(1 comment)
Commit Message:
https://review.coreboot.org/c/flashrom/+/67393/comment/0d255663_7e6f54fa
PS4, Line 13:
> If the only remaining question is about scoping global variables used within the two custom programm […]
I modified the signature as above and updated the commit message to explain that programmer specific delay functions can now access programmer specific opaque data in the usual way i.e., `(const struct myprog_data *)flash->mst->spi.data;`
Resolved?
--
To view, visit https://review.coreboot.org/c/flashrom/+/67393
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Id059abb58b31a066a408009073912da2b224d40c
Gerrit-Change-Number: 67393
Gerrit-PatchSet: 9
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Alexander Goncharov <chat(a)joursoir.net>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Felix Singer <felixsinger(a)posteo.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-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: Alexander Goncharov <chat(a)joursoir.net>
Gerrit-Comment-Date: Tue, 06 Dec 2022 02:09:02 +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: Angel Pons <th3fanbus(a)gmail.com>
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Felix Singer, Nico Huber, Thomas Heijligen, Edward O'Callaghan, Anastasia Klimchuk, Alexander Goncharov.
Hello Felix Singer, build bot (Jenkins), Thomas Heijligen, Angel Pons, Peter Marheine, Alexander Goncharov,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/67393
to look at the new patch set (#9).
Change subject: tree/: Move programmer_delay() out of programmer state machine
......................................................................
tree/: Move programmer_delay() out of programmer state machine
Handle the special cases of both serprog and ch341a_spi.
Also rewrite programmer_delay() to handle the two base
cases of zero time and no valid flashctx yet before
handling per master branching.
Additionally, modify the custom delay function pointer
signature to allow closure over the flashctx. This allows
driver specific delay implementations to recover programmer
specific opaque data within their delay implementations.
Therefore programmer specific delay functions can avoid
programmer specific globals.
Change-Id: Id059abb58b31a066a408009073912da2b224d40c
Signed-off-by: Edward O'Callaghan <quasisec(a)google.com>
---
M ch341a_spi.c
M flashrom.c
M include/programmer.h
M serprog.c
4 files changed, 65 insertions(+), 32 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/93/67393/9
--
To view, visit https://review.coreboot.org/c/flashrom/+/67393
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Id059abb58b31a066a408009073912da2b224d40c
Gerrit-Change-Number: 67393
Gerrit-PatchSet: 9
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Alexander Goncharov <chat(a)joursoir.net>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net>
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-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: Alexander Goncharov <chat(a)joursoir.net>
Gerrit-MessageType: newpatchset
Attention is currently required from: Felix Singer, Nico Huber, Thomas Heijligen, Edward O'Callaghan, Anastasia Klimchuk, Alexander Goncharov.
Hello Felix Singer, build bot (Jenkins), Thomas Heijligen, Angel Pons, Peter Marheine, Alexander Goncharov,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/67393
to look at the new patch set (#8).
Change subject: tree/: Move programmer_delay() out of programmer state machine
......................................................................
tree/: Move programmer_delay() out of programmer state machine
Handle the special cases of both serprog and ch341a_spi.
Also rewrite programmer_delay() to handle the two base
cases of zero time and no valid flashctx yet before
handling per master branching.
Change-Id: Id059abb58b31a066a408009073912da2b224d40c
Signed-off-by: Edward O'Callaghan <quasisec(a)google.com>
---
M ch341a_spi.c
M flashrom.c
M include/programmer.h
M serprog.c
4 files changed, 57 insertions(+), 31 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/93/67393/8
--
To view, visit https://review.coreboot.org/c/flashrom/+/67393
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Id059abb58b31a066a408009073912da2b224d40c
Gerrit-Change-Number: 67393
Gerrit-PatchSet: 8
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Alexander Goncharov <chat(a)joursoir.net>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net>
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-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: Alexander Goncharov <chat(a)joursoir.net>
Gerrit-MessageType: newpatchset
Attention is currently required from: Nikolai Artemiev.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/70349 )
Change subject: tree/: Change chip restore data type from uint8_t to void ptr
......................................................................
Patch Set 4: Code-Review+2
(1 comment)
Commit Message:
https://review.coreboot.org/c/flashrom/+/70349/comment/5c5c856c_0bd9d81f
PS4, Line 21: TEST=todo
test.
--
To view, visit https://review.coreboot.org/c/flashrom/+/70349
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I311b468a4b0349f4da9584c12b36af6ec2394527
Gerrit-Change-Number: 70349
Gerrit-PatchSet: 4
Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Comment-Date: Mon, 05 Dec 2022 23:53:27 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Nikolai Artemiev.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/69517 )
Change subject: flashrom.c: Supplement `chip->unlock()` calls with wp unlocking
......................................................................
Patch Set 7: Code-Review+1
(4 comments)
File flashrom.c:
https://review.coreboot.org/c/flashrom/+/69517/comment/bc4a9eb2_5e70485c
PS7, Line 1687: if (ret != FLASHROM_WP_OK)
: return -1;
:
: return 0;
ternary operator seems more concise here.
https://review.coreboot.org/c/flashrom/+/69517/comment/c7e4c8a3_9ca3c80f
PS7, Line 1695: struct flashrom_wp_cfg *original_wp_cfg = NULL;
: struct flashrom_wp_cfg *unlocked_cfg = NULL;
Do these need to be initialised, if done unnecessarily it can obscure static analysis.
https://review.coreboot.org/c/flashrom/+/69517/comment/4a56e72b_9bdf3531
PS7, Line 1696: unlocked_cfg
be consistent `unlocked_wp_cfg`
https://review.coreboot.org/c/flashrom/+/69517/comment/311ffee1_8cb40d3c
PS7, Line 1708: return -1;
are you leaking `original_wp_cfg` on the return here?
This function could perhaps do with some goto's to clean up the error path resource management complexity.
--
To view, visit https://review.coreboot.org/c/flashrom/+/69517
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I74b3f5d3a17749ea60485b916b2d87467a5d8b2f
Gerrit-Change-Number: 69517
Gerrit-PatchSet: 7
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Comment-Date: Mon, 05 Dec 2022 23:16:52 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Thomas Heijligen, Edward O'Callaghan, Peter Marheine.
Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/70264 )
Change subject: tree/: Drop default_spi_probe_opcode for NULL case
......................................................................
Patch Set 1:
(1 comment)
Commit Message:
https://review.coreboot.org/c/flashrom/+/70264/comment/bcf75c78_4c453bb6
PS1, Line 9: A NULL func pointer is necessary and sufficient for the
> It is also necessary as well as sufficient. That phrasing is less exact.
Done
--
To view, visit https://review.coreboot.org/c/flashrom/+/70264
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Id502c5d2596ad1db52faf05723083620e4c52c12
Gerrit-Change-Number: 70264
Gerrit-PatchSet: 1
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Comment-Date: Mon, 05 Dec 2022 22:35:43 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Felix Singer <felixsinger(a)posteo.net>
Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-MessageType: comment