Attention is currently required from: Subrata Banik, Nikolai Artemiev.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/69129 )
Change subject: writeprotect,ichspi,spi25: handle register access constraints
......................................................................
Patch Set 3: Code-Review+2
--
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: 3
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-Attention: Subrata Banik <subratabanik(a)google.com>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Comment-Date: Fri, 04 Nov 2022 03:26:06 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Subrata Banik, Edward O'Callaghan.
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 (#3).
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
BRANCH=none
TEST=builds
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, 75 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/29/69129/3
--
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: 3
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-Attention: Subrata Banik <subratabanik(a)google.com>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-MessageType: newpatchset
Attention is currently required from: Nico Huber, Maciej Pijanowski, Angel Pons, Nikolai Artemiev, Sergii Dmytruk.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/66212 )
Change subject: writeprotect_ranges.c: add more range functions
......................................................................
Patch Set 8:
(1 comment)
Patchset:
PS3:
> Thanks for committing those. […]
Okay, I am resolving this comment thread, seems like all done here.
I will look into the discussion you mentioned, will see how we can move it forward
--
To view, visit https://review.coreboot.org/c/flashrom/+/66212
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ied7b27be2ee2426af8f473432e2b01a290de2365
Gerrit-Change-Number: 66212
Gerrit-PatchSet: 8
Gerrit-Owner: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-CC: Maciej Pijanowski <maciej.pijanowski(a)3mdeb.com>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Maciej Pijanowski <maciej.pijanowski(a)3mdeb.com>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Comment-Date: Fri, 04 Nov 2022 01:39:09 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Maciej Pijanowski <maciej.pijanowski(a)3mdeb.com>
Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Comment-In-Reply-To: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-MessageType: comment
Attention is currently required from: Nikolai Artemiev.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/69130 )
Change subject: tests: ensure chip erase operation is executed
......................................................................
Patch Set 3: Code-Review+2
(1 comment)
File include/flash.h:
https://review.coreboot.org/c/flashrom/+/69130/comment/83e009de_c566bf8a
PS1, Line 159: #define UNERASED_VALUE(flash)
> It's a useful counterpart to `ERASED_VALUE()` and it might be helpful for erase optimization code et […]
The thoughts I had: we have ERASED_VALUE which is one value (for a given chip), but technically unerased value is any other value which is not ERASED_VALUE. Like for a real chip, there could be any data written. There is no such operation as "unerase chip".
For unit tests, yes it makes sense to create a mock "unerased chip memory" with just one and the same "unerased value".
If everyone else agrees, I am fine to have the macro here.
--
To view, visit https://review.coreboot.org/c/flashrom/+/69130
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ia00444dcd2ad96c64832a13201efbd064cd7302d
Gerrit-Change-Number: 69130
Gerrit-PatchSet: 3
Gerrit-Owner: Nikolai Artemiev <nartemiev(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: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Comment-Date: Fri, 04 Nov 2022 00:29:01 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: Nikolai Artemiev <nartemiev(a)google.com>
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Anastasia Klimchuk.
Nikolai Artemiev has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/69130 )
Change subject: tests: ensure chip erase operation is executed
......................................................................
Patch Set 3:
(3 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/69130/comment/977d339b_1fa0484f
PS1, Line 18: (hopefully)
> "and is expected to fail"
I have rephrased this sentence a bit now.
https://review.coreboot.org/c/flashrom/+/69130/comment/7b6f389a_3162107a
PS1, Line 18: exectued
> exectued -> executed (typo)
Done
File include/flash.h:
https://review.coreboot.org/c/flashrom/+/69130/comment/545a7087_b748f374
PS1, Line 159: #define UNERASED_VALUE(flash)
> Will this ever be needed outside of tests? […]
It's a useful counterpart to `ERASED_VALUE()` and it might be helpful for erase optimization code etc, so I think it makes sense to leave them both here.
--
To view, visit https://review.coreboot.org/c/flashrom/+/69130
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ia00444dcd2ad96c64832a13201efbd064cd7302d
Gerrit-Change-Number: 69130
Gerrit-PatchSet: 3
Gerrit-Owner: Nikolai Artemiev <nartemiev(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: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Fri, 04 Nov 2022 00:19:49 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Nikolai Artemiev.
Hello build bot (Jenkins), Edward O'Callaghan, Angel Pons, Anastasia Klimchuk,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/69130
to look at the new patch set (#3).
Change subject: tests: ensure chip erase operation is executed
......................................................................
tests: ensure chip erase operation is executed
The `full_chip_erase_with_wp_dummyflasher_test_success` test case
checks that erasing a write-protected region of a dummyflasher chip
fails.
However erase optimization may cause the erase operation to be skipped
if the flash contents are already erased, so the erase operation appears
to succeed and the test case fails.
Writing a non-erased value to the chip ensures that an erase operation
will be executed and write protection will be properly tested.
BUG=b:237620197
BRANCH=none
TEST=ninja test
Change-Id: Ia00444dcd2ad96c64832a13201efbd064cd7302d
Signed-off-by: Nikolai Artemiev <nartemiev(a)google.com>
---
M include/flash.h
M tests/chip_wp.c
2 files changed, 35 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/30/69130/3
--
To view, visit https://review.coreboot.org/c/flashrom/+/69130
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ia00444dcd2ad96c64832a13201efbd064cd7302d
Gerrit-Change-Number: 69130
Gerrit-PatchSet: 3
Gerrit-Owner: Nikolai Artemiev <nartemiev(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: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-MessageType: newpatchset