Attention is currently required from: Felix Singer, Thomas Heijligen, Anastasia Klimchuk.
Hello Felix Singer, build bot (Jenkins), Thomas Heijligen, Angel Pons, Peter Marheine,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/68433
to look at the new patch set (#5).
Change subject: tests: Add prefix to io_mock functions not to clash with macros
......................................................................
tests: Add prefix to io_mock functions not to clash with macros
Flashrom I/O mock functions need to be renamed so that they do not
have name clash with standard I/O, because the latter are allowed
to be macros. Adding a prefix to flashrom mock functions avoids
them being accidentally expanded. Standard I/O functions are
expanded and flashrom mocks stays as they are.
BUG=b:237606255
TEST=ninja test
1) gcc 12.2.0 on Debian
2) clang 15.0 on Chromium OS
Ticket: https://ticket.coreboot.org/issues/411
Change-Id: I7998a8fb1b9e65621e12adbfab5460a245d5606b
Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org>
---
M tests/chip.c
M tests/io_mock.c
M tests/io_mock.h
M tests/linux_mtd.c
M tests/linux_spi.c
M tests/parade_lspcon.c
M tests/realtek_mst_i2c_spi.c
M tests/tests.c
8 files changed, 65 insertions(+), 43 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/33/68433/5
--
To view, visit https://review.coreboot.org/c/flashrom/+/68433
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I7998a8fb1b9e65621e12adbfab5460a245d5606b
Gerrit-Change-Number: 68433
Gerrit-PatchSet: 5
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
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-Attention: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: newpatchset
Attention is currently required from: Felix Singer, Thomas Heijligen, Anastasia Klimchuk.
Hello Felix Singer, build bot (Jenkins), Thomas Heijligen, Angel Pons, Peter Marheine,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/68432
to look at the new patch set (#5).
Change subject: tests: Undefine _FORTIFY_SOURCE for unit tests to avoid _chk variants
......................................................................
tests: Undefine _FORTIFY_SOURCE for unit tests to avoid _chk variants
The option _FORTIFY_SOURCE, when enabled, can result in some functions
being expanded into _chk variants. For example, `fprintf` can get
expanded into `__fprintf_chk`. This makes sense for building a real
binary, but is not needed for unit tests.
In unit test environment all those functions are wrapped. In the
example above, both `fprintf` and `__fprintf_chk` needed to be mocked.
Disabling _FORTIFY_SOURCE avoids expanding functions into _chk
variants, without any loss of testing coverage because that would
be wrapped/mocked anyway.
This patch also removes two existing _chk wraps because they are not
needed anymore.
BUG=b:237606255
TEST=ninja test on
1) gcc 12.2.0 on Debian
2) clang 15.0 on Chromium OS
Ticket: https://ticket.coreboot.org/issues/411
Change-Id: I70cb1cd90d1f377ff4606acad3c1b514120ae4f7
Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org>
---
M tests/meson.build
M tests/tests.c
2 files changed, 31 insertions(+), 18 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/32/68432/5
--
To view, visit https://review.coreboot.org/c/flashrom/+/68432
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I70cb1cd90d1f377ff4606acad3c1b514120ae4f7
Gerrit-Change-Number: 68432
Gerrit-PatchSet: 5
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
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-Attention: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: newpatchset
Attention is currently required from: Felix Singer, Thomas Heijligen, Anastasia Klimchuk.
Hello Felix Singer, build bot (Jenkins), Thomas Heijligen, Angel Pons, Peter Marheine,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/68433
to look at the new patch set (#4).
Change subject: tests: Add prefix to io_mock functions not to clash with macros
......................................................................
tests: Add prefix to io_mock functions not to clash with macros
Flashrom I/O mock functions need to be renamed so that they do not
have name clash with standard I/O, because the latter are allowed
to be macros. Adding a prefix to flashrom mock functions avoids
them being accidentally expanded. Standard I/O functions are
expanded and flashrom mocks stays as they are.
BUG=b:237606255
TEST=ninja test
1) gcc 12.2.0 on Debian
2) clang 15.0 on Chromium OS
Ticket: https://ticket.coreboot.org/issues/411
Change-Id: I7998a8fb1b9e65621e12adbfab5460a245d5606b
Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org>
---
M tests/chip.c
M tests/io_mock.c
M tests/io_mock.h
M tests/linux_mtd.c
M tests/linux_spi.c
M tests/parade_lspcon.c
M tests/realtek_mst_i2c_spi.c
M tests/tests.c
8 files changed, 65 insertions(+), 43 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/33/68433/4
--
To view, visit https://review.coreboot.org/c/flashrom/+/68433
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I7998a8fb1b9e65621e12adbfab5460a245d5606b
Gerrit-Change-Number: 68433
Gerrit-PatchSet: 4
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
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-Attention: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: newpatchset
Attention is currently required from: Felix Singer, Thomas Heijligen, Anastasia Klimchuk.
Hello Felix Singer, build bot (Jenkins), Thomas Heijligen, Angel Pons, Peter Marheine,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/68432
to look at the new patch set (#4).
Change subject: tests: Undefine _FORTIFY_SOURCE for unit tests to avoid _chk variants
......................................................................
tests: Undefine _FORTIFY_SOURCE for unit tests to avoid _chk variants
The option _FORTIFY_SOURCE, when enabled, can result in some functions
being expanded into _chk variants. For example, `fprintf` can get
expanded into `__fprintf_chk`. This makes sence for building a real
binary, but is not needed for unit tests.
In unit test environment all those functions are wrapped. In the
example above, both `fprintf` and `__fprintf_chk` needed to be mocked.
Disabling _FORTIFY_SOURCE avoids expanding functions into _chk
variants, without any loss of testing coverage because that would
be wrapped/mocked anyway.
This patch also removes two existing _chk wraps because they are not
needed anymore.
BUG=b:237606255
TEST=ninja test on
1) gcc 12.2.0 on Debian
2) clang 15.0 on Chromium OS
Ticket: https://ticket.coreboot.org/issues/411
Change-Id: I70cb1cd90d1f377ff4606acad3c1b514120ae4f7
Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org>
---
M tests/meson.build
M tests/tests.c
2 files changed, 31 insertions(+), 18 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/32/68432/4
--
To view, visit https://review.coreboot.org/c/flashrom/+/68432
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I70cb1cd90d1f377ff4606acad3c1b514120ae4f7
Gerrit-Change-Number: 68432
Gerrit-PatchSet: 4
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
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-Attention: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: newpatchset
Attention is currently required from: Felix Singer, Thomas Heijligen, Angel Pons.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/68162 )
Change subject: meson: Move programmer test sources into programmer definition
......................................................................
Patch Set 16: Code-Review+1
(2 comments)
File meson.build:
https://review.coreboot.org/c/flashrom/+/68162/comment/a6448335_ee63435a
PS16, Line 191: 'test_srcs' :
Is it possible to align all `:` ? Maybe it's just me... but it jumps into my eyes that `:`s are not aligned, and same for the loop below :)
File tests/meson.build:
https://review.coreboot.org/c/flashrom/+/68162/comment/08ceadfa_504bbc2b
PS10, Line 32: p_data += {
: 'testsrc' : p_data.get('testsrc', []),
: }
: srcs += p_data.get('testsrc')
> I just noticed I made a typo and that's why it didn't work. oopsie 😄 […]
Good to know it works! And looks even better in the latest patchset!
--
To view, visit https://review.coreboot.org/c/flashrom/+/68162
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I307faaf8a9f7ae3c54bd96e7d871a3abb8aadea3
Gerrit-Change-Number: 68162
Gerrit-PatchSet: 16
Gerrit-Owner: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Comment-Date: Wed, 02 Nov 2022 01:48:24 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: Felix Singer <felixsinger(a)posteo.net>
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Felix Singer, Angel Pons, Anastasia Klimchuk.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67240 )
Change subject: tree/: Convert flashchip erase_block func ptr to enumerate
......................................................................
Patch Set 15:
(1 comment)
Patchset:
PS15:
> Besides the at45db changes, looks OK. […]
I've done what you have asked of me https://review.coreboot.org/c/flashrom/+/69131
Although this change was extensively tested as well as has a breakdown into its constituents (abandon patches) from which it was squashed together.
Personally I feel there is more opportunity to introduce bugs messing around between representations between commits even though your idea in principle is good. In any case I have done what you have asked in CB:69131
--
To view, visit https://review.coreboot.org/c/flashrom/+/67240
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I58415a4f2589812b26d284c96876e88f8d9acf52
Gerrit-Change-Number: 67240
Gerrit-PatchSet: 15
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Wed, 02 Nov 2022 01:47:46 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-MessageType: comment
Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/69131 )
Change subject: tree/: Convert flashchips db to use indirection for erase_block
......................................................................
tree/: Convert flashchips db to use indirection for erase_block
This paves the way to allow for the conversion of flashchip erase_block
func ptr to enumerate values. This change should be a NOP.
Change-Id: I122295ec9add0fe0efd27273c9725e5d64f6dbe2
Signed-off-by: Edward O'Callaghan <quasisec(a)google.com>
---
M at45db.c
M atapromise.c
M flashchips.c
M flashrom.c
M include/flash.h
M sfdp.c
M spi25.c
M tests/chip.c
M tests/chip_wp.c
9 files changed, 2,020 insertions(+), 1,971 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/31/69131/1
--
To view, visit https://review.coreboot.org/c/flashrom/+/69131
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I122295ec9add0fe0efd27273c9725e5d64f6dbe2
Gerrit-Change-Number: 69131
Gerrit-PatchSet: 1
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-MessageType: newchange
Nikolai Artemiev has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/69130 )
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 exectued and (hopefully) fail due to write protection.
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/1
diff --git a/include/flash.h b/include/flash.h
index 2ba235d..f0c6026 100644
--- a/include/flash.h
+++ b/include/flash.h
@@ -156,6 +156,7 @@
#define FEATURE_WRSR3 (1 << 23)
#define ERASED_VALUE(flash) (((flash)->chip->feature_bits & FEATURE_ERASED_ZERO) ? 0x00 : 0xff)
+#define UNERASED_VALUE(flash) (((flash)->chip->feature_bits & FEATURE_ERASED_ZERO) ? 0xff : 0x00)
enum test_state {
OK = 0,
diff --git a/tests/chip_wp.c b/tests/chip_wp.c
index 8f6cb5f..739c4f9 100644
--- a/tests/chip_wp.c
+++ b/tests/chip_wp.c
@@ -63,6 +63,7 @@
static const struct flashchip chip_W25Q128_V = {
.vendor = "aklm&dummyflasher",
.total_size = 16 * 1024,
+ .page_size = 1024,
.tested = TEST_OK_PREW,
.read = SPI_CHIP_READ,
.write = SPI_CHIP_WRITE256,
@@ -268,6 +269,14 @@
this stage WP is not enabled and erase completes successfully. */
assert_int_equal(0, flashrom_flash_erase(&flash));
+ /* Write non-erased value to entire chip so that erase operations cannot
+ * be optimized away. */
+ unsigned long size = flashrom_flash_getsize(&flash);
+ uint8_t *const contents = malloc(size);
+ memset(contents, UNERASED_VALUE(&flash), size);
+ assert_int_equal(0, flashrom_image_write(&flash, contents, size, NULL));
+ free(contents);
+
assert_int_equal(0, flashrom_wp_read_cfg(wp_cfg, &flash));
/* Hardware-protect first 4 KiB. */
--
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: 1
Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-MessageType: newchange
Attention is currently required from: Edward O'Callaghan.
Nikolai Artemiev has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/69129 )
Change subject: writeprotect,ichspi,spi25: handle register access constraints
......................................................................
Patch Set 1:
(1 comment)
File ichspi.c:
https://review.coreboot.org/c/flashrom/+/69129/comment/835348b0_45622de3
PS1, Line 1398: SPI_INVALID_OPCODE
This isn't a SPI master so we should technically return something else, but it makes things more consistent and easier to handle in writeprotect.c.
--
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: 1
Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Comment-Date: Wed, 02 Nov 2022 00:59:35 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment