Attention is currently required from: Edward O'Callaghan, Angel Pons, Anastasia Klimchuk.
Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/70006 )
Change subject: layout: Check return values for strdup in register_include_arg
......................................................................
Patch Set 2:
(1 comment)
File layout.c:
https://review.coreboot.org/c/flashrom/+/70006/comment/55ca34b7_43c58438
PS2, Line 139: || file == NULL
> This is wrong. `file` is allowed to be NULL since it's optional (see line 130-135). […]
Just rethinking.. I would still merge this with lines 132-135 but I would leave lines 132-135 where they are. So I would just expand lines 132-135. The check for the colon and the second parameter should happen after line 131.
--
To view, visit https://review.coreboot.org/c/flashrom/+/70006
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I6c22196be6847a8c9704f1de936604a51b4b8a28
Gerrit-Change-Number: 70006
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: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.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: Fri, 25 Nov 2022 22:04:09 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Felix Singer <felixsinger(a)posteo.net>
Gerrit-MessageType: comment
Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/69789 )
(
2 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
)Change subject: ichspi: Fix number of bytes for HW seq operations
......................................................................
ichspi: Fix number of bytes for HW seq operations
This patch fixes a potential issue where the SPI controller register
HSFC.FDBC (bits 24-29) value gets incorrectly calculated while passing
the `len` as `0` instead of `1`.
As per Intel EDS, `0b` in the FDBC represents 1 byte while `0x3f`
represents 64-bytes to be transferred. The number of bytes
transferred is the value of this field plus 1.
If we would like to transfer 1 byte then we need to set `0b` in
FDBC for operations like read, write, flash id as to account for
the `set byte count` hence, the `len` argument should be `1`.
Additionally, as per EDS, the FDBC field is ignored for any block
erase command.
BUG=b:258280679
TEST=Able to build flashrom and perform below operations on Google,
Rex and Google, Kano/Taeko.
During `--wp-disable` HW seq operation that requires 1 byte data
transfer.
HSFC.FDBC value while passing `len` as `0` = 0x3f (represents 64-byte)
HSFC.FDBC value while passing `len` as `1` = 0x0 (represents 1-byte)
Signed-off-by: Subrata Banik <subratabanik(a)google.com>
Change-Id: I5b911655649c693e576497520687d7810bbd3c54
Reviewed-on: https://review.coreboot.org/c/flashrom/+/69789
Reviewed-by: Angel Pons <th3fanbus(a)gmail.com>
Reviewed-by: Edward O'Callaghan <quasisec(a)chromium.org>
Reviewed-by: Nikolai Artemiev <nartemiev(a)google.com>
Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org>
---
M ichspi.c
1 file changed, 50 insertions(+), 3 deletions(-)
Approvals:
build bot (Jenkins): Verified
Angel Pons: Looks good to me, but someone else must approve
Edward O'Callaghan: Looks good to me, approved
Nikolai Artemiev: Looks good to me, but someone else must approve
diff --git a/ichspi.c b/ichspi.c
index 0e2a15b..62d1799 100644
--- a/ichspi.c
+++ b/ichspi.c
@@ -1360,6 +1360,12 @@
/* Set up transaction parameters. */
hsfc |= hsfc_cycle;
+ /*
+ * The number of bytes transferred is the value of `FDBC` plus 1, hence,
+ * subtracted 1 from the length field.
+ * As per Intel EDS, `0b` in the FDBC represents 1 byte while `0x3f`
+ * represents 64-bytes to be transferred.
+ */
hsfc |= HSFC_FDBC_VAL(len - 1);
hsfc |= HSFC_FGO; /* start */
prettyprint_ich9_reg_hsfc(hsfc, ich_generation);
@@ -1399,7 +1405,7 @@
}
msg_pdbg("Reading Status register\n");
- if (ich_exec_sync_hwseq_xfer(flash, HSFC_CYCLE_RD_STATUS, 0, len, ich_generation,
+ if (ich_exec_sync_hwseq_xfer(flash, HSFC_CYCLE_RD_STATUS, 1, len, ich_generation,
hwseq_data->addr_mask)) {
msg_perr("Reading Status register failed\n!!");
return -1;
@@ -1422,7 +1428,7 @@
ich_fill_data(&value, len, ICH9_REG_FDATA0);
- if (ich_exec_sync_hwseq_xfer(flash, HSFC_CYCLE_WR_STATUS, 0, len, ich_generation,
+ if (ich_exec_sync_hwseq_xfer(flash, HSFC_CYCLE_WR_STATUS, 1, len, ich_generation,
hwseq_data->addr_mask)) {
msg_perr("Writing Status register failed\n!!");
return -1;
@@ -1518,7 +1524,7 @@
msg_pdbg("Erasing %d bytes starting at 0x%06x.\n", len, addr);
- if (ich_exec_sync_hwseq_xfer(flash, HSFC_CYCLE_BLOCK_ERASE, addr, 0, ich_generation,
+ if (ich_exec_sync_hwseq_xfer(flash, HSFC_CYCLE_BLOCK_ERASE, addr, 1, ich_generation,
hwseq_data->addr_mask))
return -1;
return 0;
--
To view, visit https://review.coreboot.org/c/flashrom/+/69789
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I5b911655649c693e576497520687d7810bbd3c54
Gerrit-Change-Number: 69789
Gerrit-PatchSet: 4
Gerrit-Owner: Subrata Banik <subratabanik(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: Nikolai Artemiev <nartemiev(a)chromium.org>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-MessageType: merged
Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/69788 )
(
2 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
)Change subject: ichspi: Clear Fast SPI HSFC register before HW seq operation
......................................................................
ichspi: Clear Fast SPI HSFC register before HW seq operation
This patch fixes a regression introduced with
commit 7ed1337309d3fe74f5af09520970f0f1d417399a (ichspi: Factor out
common hwseq_xfer logic into helpers).
The reason for the regression is ignoring the fact that the Fast SPI
controller MMIO register HSFC (0x06) might not hold the default zero
value before initiating the HW sequencing operation.
Having a `1b` value in the HSFC.FDBC (bits 24-29) field would represent
a byte that needs to be transfered.
While debugging the regression, we have observed that the default value
in the FDBC (prior to initiate any operation) is 0x3f (instead of
zero) which represents 64-byte transfer.
localhost ~ # iotools mmio_read32 0x92d16006
0x3f00
<Fast SPI MMIO BAR: 0x92d16000 and HSFC offset: 0x06>
FDBC offset during `--wp-disable` operation represents higher numbers of
bytes than the actual and eventually results in the error.
Additionally, dropped unused variable (struct hwseq_data *hwseq_data).
BUG=b:258280679
TEST=Able to build flashrom and perform below operations on Google, Rex
and Google, Kano/Taeko.
Without this patch:
HSFC register value inside ich_start_hwseq_xfer() before initiating
the HW seq operations: 0x3f00
HSFC register value inside ich_start_hwseq_xfer() during the HW seq
operations (Read Status): 0x3f11
With this patch:
HSFC register value inside ich_start_hwseq_xfer() before initiating
the HW seq operations: 0x0
HSFC register value inside ich_start_hwseq_xfer() during the HW seq
operations (Read Status): 0x11
Additionally, verified other HW sequencing operations (like read, write,
erase, read status, write status, read ID) working fine without any
error.
Signed-off-by: Subrata Banik <subratabanik(a)google.com>
Change-Id: I4cc3f24f880d1d621f1f48a6e6b276449fa46f98
Reviewed-on: https://review.coreboot.org/c/flashrom/+/69788
Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org>
Reviewed-by: Edward O'Callaghan <quasisec(a)chromium.org>
Reviewed-by: Nikolai Artemiev <nartemiev(a)google.com>
---
M ichspi.c
1 file changed, 63 insertions(+), 4 deletions(-)
Approvals:
build bot (Jenkins): Verified
Edward O'Callaghan: Looks good to me, approved
Nikolai Artemiev: Looks good to me, but someone else must approve
diff --git a/ichspi.c b/ichspi.c
index cd330e3..0e2a15b 100644
--- a/ichspi.c
+++ b/ichspi.c
@@ -1349,8 +1349,8 @@
uint32_t hsfc_cycle, uint32_t flash_addr, size_t len,
uint32_t addr_mask)
{
- uint16_t hsfc;
- struct hwseq_data *hwseq_data = get_hwseq_data_from_context(flash);
+ /* make sure HSFC register is cleared before initiate any operation */
+ uint16_t hsfc = 0;
/* Sets flash_addr in FADDR */
ich_hwseq_set_addr(flash_addr, addr_mask);
@@ -1359,8 +1359,6 @@
REGWRITE16(ICH9_REG_HSFS, REGREAD16(ICH9_REG_HSFS));
/* Set up transaction parameters. */
- hsfc = REGREAD16(ICH9_REG_HSFC);
- hsfc &= ~hwseq_data->hsfc_fcycle; /* clear operation */
hsfc |= hsfc_cycle;
hsfc |= HSFC_FDBC_VAL(len - 1);
hsfc |= HSFC_FGO; /* start */
--
To view, visit https://review.coreboot.org/c/flashrom/+/69788
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I4cc3f24f880d1d621f1f48a6e6b276449fa46f98
Gerrit-Change-Number: 69788
Gerrit-PatchSet: 4
Gerrit-Owner: Subrata Banik <subratabanik(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: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)chromium.org>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-MessageType: merged
Attention is currently required from: Edward O'Callaghan, Angel Pons, Anastasia Klimchuk.
Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/70006 )
Change subject: layout: Check return values for strdup in register_include_arg
......................................................................
Patch Set 2:
(2 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/70006/comment/bf7f8ba8_4310e870
PS2, Line 11:
Explain the issues and the new code changes.
File layout.c:
https://review.coreboot.org/c/flashrom/+/70006/comment/23b45f39_49c20fb8
PS2, Line 139: || file == NULL
This is wrong. `file` is allowed to be NULL since it's optional (see line 130-135). That's why it's set to NULL in line 137 when no colon was found.
Line 137 needs to be written differently so that we can differentiate between NULL returned by strdup and NULL that is allowed. We need an extra if for that and check for allocated memory separately.
1. Set `file` in line 123 to NULL as default value.
2. Move this if-statement above line 137 and remove `file` from the condition.
```
name = colon ? strndup(arg, colon - arg) : strdup(arg);
if (!name) {
msg_gerr("Could not allocate memory.\n");
goto error;
}
```
3. Rewrite line 137, and I also would merge it with lines 132-135 so that we don't check for `colon` twice.
```
if (colon) {
if (!colon[1]) {
msg_gerr("Missing filename parameter in %s\n", arg);
return 1;
}
file = strdup(colon + 1);
if (!file) {
msg_gerr("Could not allocate memory.\n");
goto error;
}
}
```
--
To view, visit https://review.coreboot.org/c/flashrom/+/70006
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I6c22196be6847a8c9704f1de936604a51b4b8a28
Gerrit-Change-Number: 70006
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: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.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: Fri, 25 Nov 2022 21:43:41 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Nikolai Artemiev.
Felix Singer has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/69984 )
Change subject: linux_mtd: Mark Opaque chip as tested for WP
......................................................................
linux_mtd: Mark Opaque chip as tested for WP
Since linux_mtd supports write-protect, its probe function needs
to mark Opaque chip as tested for WP. Programmers which are
opaque masters are responsible for populating flashchip#tested
struct in probe function.
Without the patch, any operation running via linux_mtd displays
a message "This flash part has status UNTESTED for operations: WP".
With the patch, the message is not displayed anymore.
BUG=b:258755442
BRANCH=none
TEST=flashrom -p host on ARM dut
Found Programmer flash chip "Opaque flash chip"
(8192 kB, Programmer-specific) on host.
No operations were specified.
Original-Change-Id: Icc0521c28555a93f26ce66bdbeaa68590f10c358
Original-Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org>
Original-Reviewed-on: https://review.coreboot.org/c/flashrom/+/69518
Original-Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org>
Original-Reviewed-by: Edward O'Callaghan <quasisec(a)chromium.org>
Original-Reviewed-by: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Change-Id: I2740e75c974538ddae9f5b17dc4fba9a3101cbb3
Signed-off-by: Felix Singer <felixsinger(a)posteo.net>
---
M linux_mtd.c
1 file changed, 33 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/84/69984/1
diff --git a/linux_mtd.c b/linux_mtd.c
index 5a1360e..c49b14a 100644
--- a/linux_mtd.c
+++ b/linux_mtd.c
@@ -179,7 +179,7 @@
if (data->no_erase)
flash->chip->feature_bits |= FEATURE_NO_ERASE;
- flash->chip->tested = TEST_OK_PREW;
+ flash->chip->tested = TEST_OK_PREWB;
flash->chip->total_size = data->total_size / 1024; /* bytes -> kB */
flash->chip->block_erasers[0].eraseblocks[0].size = data->erasesize;
flash->chip->block_erasers[0].eraseblocks[0].count =
--
To view, visit https://review.coreboot.org/c/flashrom/+/69984
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: 1.3.x
Gerrit-Change-Id: I2740e75c974538ddae9f5b17dc4fba9a3101cbb3
Gerrit-Change-Number: 69984
Gerrit-PatchSet: 1
Gerrit-Owner: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-MessageType: newchange
Felix Singer has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/69983 )
Change subject: cli_classic.c: Be consistent with pointer types
......................................................................
cli_classic.c: Be consistent with pointer types
With `i586-pc-msdosdjgpp-gcc (GCC) 12.2.0`, `uint32_t` is defined as
`long unsigned int`, which is not the same as `unsigned int`. As the
`flashrom_layout_get_region_range()` function is part of libflashrom
API, adjust `cli_classic.c` instead to avoid type mismatches.
Original-Change-Id: Ie8f5bc0d9296f7c6b8f8a351b53052f5fe86b09d
Original-Signed-off-by: Angel Pons <th3fanbus(a)gmail.com>
Original-Reviewed-on: https://review.coreboot.org/c/flashrom/+/69451
Original-Reviewed-by: Arthur Heymans <arthur(a)aheymans.xyz>
Original-Reviewed-by: Evan Benn <evanbenn(a)google.com>
Original-Reviewed-by: Nikolai Artemiev <nartemiev(a)google.com>
Original-Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org>
Change-Id: I05dbda3923f1cd262bdad62e58f9c0ae7d7ffe6f
Signed-off-by: Felix Singer <felixsinger(a)posteo.net>
---
M cli_classic.c
1 file changed, 24 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/83/69983/1
diff --git a/cli_classic.c b/cli_classic.c
index a77422e..4649c73 100644
--- a/cli_classic.c
+++ b/cli_classic.c
@@ -126,7 +126,7 @@
return true;
}
-static int parse_wp_range(uint32_t *start, uint32_t *len)
+static int parse_wp_range(unsigned int *start, unsigned int *len)
{
char *endptr = NULL, *token = NULL;
@@ -568,7 +568,7 @@
int namelen, opt, i, j;
int startchip = -1, chipcount = 0, option_index = 0;
int operation_specified = 0;
- uint32_t wp_start = 0, wp_len = 0;
+ unsigned int wp_start = 0, wp_len = 0;
bool force = false, ifd = false, fmap = false;
#if CONFIG_PRINT_WIKI == 1
bool list_supported_wiki = false;
--
To view, visit https://review.coreboot.org/c/flashrom/+/69983
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: 1.3.x
Gerrit-Change-Id: I05dbda3923f1cd262bdad62e58f9c0ae7d7ffe6f
Gerrit-Change-Number: 69983
Gerrit-PatchSet: 1
Gerrit-Owner: Felix Singer <felixsinger(a)posteo.net>
Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-MessageType: newchange