- flashrom-gerrit - mail.coreboot.org

Change in flashrom[master]: tests: Set up mock chip memory in consistent and predictable way
by Edward O'Callaghan (Code Review) 14 Feb '22

14 Feb '22

Attention is currently required from: Nico Huber, Paul Menzel, Angel Pons, Anastasia Klimchuk. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59237 ) Change subject: tests: Set up mock chip memory in consistent and predictable way ...................................................................... Patch Set 7: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/59237 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I0d7623a601c207bfc62d54ab89d94cda56d85871 Gerrit-Change-Number: 59237 Gerrit-PatchSet: 7 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: 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: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Mon, 14 Feb 2022 03:15:53 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: tests: add fprintf() mock with redirection to io_mock
by Edward O'Callaghan (Code Review) 13 Feb '22

13 Feb '22

Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/61708 ) Change subject: tests: add fprintf() mock with redirection to io_mock ...................................................................... tests: add fprintf() mock with redirection to io_mock We've seen somewhat obscure test failures where the real fprintf() function was passed a fake file returned by the fopen() mock. Although the code that caused the specific failure was cros-specific, adding an fprintf() mock should help avoid future debugging. TEST=ninja test BRANCH=none BUG=b:217661133 Change-Id: I3f8594ea24d17436a7932732d9d05416b804dc93 Signed-off-by: Nikolai Artemiev <nartemiev(a)google.com> Reviewed-on: https://review.coreboot.org/c/flashrom/+/61708 Reviewed-by: Anastasia Klimchuk <aklm(a)chromium.org> Reviewed-by: Edward O'Callaghan <quasisec(a)chromium.org> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> --- M tests/io_mock.h M tests/meson.build M tests/tests.c 3 files changed, 16 insertions(+), 0 deletions(-) Approvals: build bot (Jenkins): Verified Edward O'Callaghan: Looks good to me, approved Anastasia Klimchuk: Looks good to me, approved diff --git a/tests/io_mock.h b/tests/io_mock.h index 854ddd9..8734774 100644 --- a/tests/io_mock.h +++ b/tests/io_mock.h @@ -102,6 +102,7 @@ FILE* (*fopen)(void *state, const char *pathname, const char *mode); char* (*fgets)(void *state, char *buf, int len, FILE *fp); size_t (*fread)(void *state, void *buf, size_t size, size_t len, FILE *fp); + int (*fprintf)(void *state, FILE *fp, const char *fmt, va_list args); int (*fclose)(void *state, FILE *fp); }; diff --git a/tests/meson.build b/tests/meson.build index 5bb6ac9..2f0fc67 100644 --- a/tests/meson.build +++ b/tests/meson.build @@ -56,6 +56,7 @@ '-Wl,--wrap=fsync', '-Wl,--wrap=fread', '-Wl,--wrap=fgets', + '-Wl,--wrap=fprintf', '-Wl,--wrap=fclose', '-Wl,--wrap=feof', '-Wl,--wrap=ferror', diff --git a/tests/tests.c b/tests/tests.c index a4a312e..06dc12f 100644 --- a/tests/tests.c +++ b/tests/tests.c @@ -236,6 +236,20 @@ return 0; } +int __wrap_fprintf(FILE *fp, const char *fmt, ...) +{ + LOG_ME; + if (get_io() && get_io()->fprintf) { + va_list args; + int out; + va_start(args, fmt); + out = get_io()->fprintf(get_io()->state, fp, fmt, args); + va_end(args); + return out; + } + return 0; +} + int __wrap_fclose(FILE *fp) { LOG_ME; -- To view, visit https://review.coreboot.org/c/flashrom/+/61708 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I3f8594ea24d17436a7932732d9d05416b804dc93 Gerrit-Change-Number: 61708 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-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-MessageType: merged

1 0

Change in flashrom[master]: ichspi.c: Check SPI Cycle In-Progress prior start HW Seq
by Rizwan Qureshi (Code Review) 13 Feb '22

13 Feb '22

Attention is currently required from: Nico Huber, Subrata Banik, Caveh Jalali, Tim Wawrzynczak, Sridhar Siricilla, Angel Pons, Nick Vaccaro, Alex Levin, YH Lin, Boris Mittelberg. Rizwan Qureshi has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/61854 ) Change subject: ichspi.c: Check SPI Cycle In-Progress prior start HW Seq ...................................................................... Patch Set 4: Code-Review+1 (1 comment) Patchset: PS2: > > > > > > This leaves some questions. Is something broken wrt. arbitration? How does […] If there is a Hardware Sequencing Cycle In Progress and an attempt is made to program any of the control, address, or data register the cycle is blocked and the FCERR bit is set. So a check is required in flashrom on H_SCIP, so that multiple instances (on same master) do not cancel each other's requests. -- To view, visit https://review.coreboot.org/c/flashrom/+/61854 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ib9265cc20513fd00f32f8fa22e28c312903ca484 Gerrit-Change-Number: 61854 Gerrit-PatchSet: 4 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Alex Levin <levinale(a)chromium.org> Gerrit-Reviewer: Boris Mittelberg <bmbm(a)google.com> Gerrit-Reviewer: Caveh Jalali <caveh(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: YH Lin <yueherngl(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Caveh Jalali <caveh(a)chromium.org> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Attention: Alex Levin <levinale(a)chromium.org> Gerrit-Attention: YH Lin <yueherngl(a)chromium.org> Gerrit-Attention: Boris Mittelberg <bmbm(a)google.com> Gerrit-Comment-Date: Sun, 13 Feb 2022 14:21:43 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com> Comment-In-Reply-To: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: libflashrom,linux_mtd: add linux_mtd writeprotect support
by Nikolai Artemiev (Code Review) 13 Feb '22

13 Feb '22

Nikolai Artemiev has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/61897 ) Change subject: libflashrom,linux_mtd: add linux_mtd writeprotect support ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: draft version of the patch, will test tomorrow, early comments welcome -- To view, visit https://review.coreboot.org/c/flashrom/+/61897 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I5c86e28cdec44bec49ba1d36f8ab62241b9b01da Gerrit-Change-Number: 61897 Gerrit-PatchSet: 1 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Sun, 13 Feb 2022 11:40:07 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: libflashrom,linux_mtd: add linux_mtd writeprotect support
by Nikolai Artemiev (Code Review) 13 Feb '22

13 Feb '22

Nikolai Artemiev has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/61897 ) Change subject: libflashrom,linux_mtd: add linux_mtd writeprotect support ...................................................................... libflashrom,linux_mtd: add linux_mtd writeprotect support BUG=b:182223106 TEST=make BRANCH=none Change-Id: I5c86e28cdec44bec49ba1d36f8ab62241b9b01da Signed-off-by: Nikolai Artemiev <nartemiev(a)google.com> --- M libflashrom.c M linux_mtd.c M programmer.h 3 files changed, 138 insertions(+), 19 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/97/61897/1 diff --git a/libflashrom.c b/libflashrom.c index eb283c8..1fabab6 100644 --- a/libflashrom.c +++ b/libflashrom.c @@ -711,12 +711,15 @@ */ enum flashrom_wp_result flashrom_wp_write_cfg(struct flashctx *flash, const struct flashrom_wp_cfg *cfg) { - /* - * TODO: Call custom implementation if the programmer is opaque, as - * direct WP operations require SPI access. In particular, linux_mtd - * has its own WP operations we should use instead. - */ - return wp_write_cfg(flash, cfg); + if (flash->mst->buses_supported & BUS_SPI) + return wp_write_cfg(flash, cfg); + + if (flash->mst->buses_supported & BUS_PROG) { + if (flash->mst->opaque.wp_write_cfg) + return flash->mst->opaque.wp_write_cfg(flash, cfg); + } + + return FLASHROM_WP_ERR_OTHER; } /** @@ -730,12 +733,15 @@ */ enum flashrom_wp_result flashrom_wp_read_cfg(struct flashrom_wp_cfg *cfg, struct flashctx *flash) { - /* - * TODO: Call custom implementation if the programmer is opaque, as - * direct WP operations require SPI access. In particular, linux_mtd - * has its own WP operations we should use instead. - */ - return wp_read_cfg(cfg, flash); + if (flash->mst->buses_supported & BUS_SPI) + return wp_read_cfg(cfg, flash); + + if (flash->mst->buses_supported & BUS_PROG) { + if (flash->mst->opaque.wp_read_cfg) + return flash->mst->opaque.wp_read_cfg(cfg, flash); + } + + return FLASHROM_WP_ERR_OTHER; } /** @@ -752,13 +758,15 @@ */ enum flashrom_wp_result flashrom_wp_get_available_ranges(struct flashrom_wp_range_list **list, struct flashrom_flashctx *flash) { - /* - * TODO: Call custom implementation if the programmer is opaque, as - * direct WP operations require SPI access. We actually can't implement - * this in linux_mtd right now, but we should adopt a proper generic - * architechure to match the read and write functions anyway. - */ - return wp_get_available_ranges(list, flash); + if (flash->mst->buses_supported & BUS_SPI) + return wp_get_available_ranges(list, flash); + + if (flash->mst->buses_supported & BUS_PROG) { + if (flash->mst->opaque.wp_get_ranges) + return flash->mst->opaque.wp_get_ranges(list, flash); + } + + return FLASHROM_WP_ERR_OTHER; } /** diff --git a/linux_mtd.c b/linux_mtd.c index 9d80a51..7d394a5 100644 --- a/linux_mtd.c +++ b/linux_mtd.c @@ -308,6 +308,111 @@ return 0; } +static enum flashrom_wp_result linux_mtd_wp_write_cfg(struct flashctx *flash, const struct flashrom_wp_cfg *cfg) +{ + struct linux_mtd_data *data = flash->mst->opaque.data; + + struct erase_info_user entire_chip = { + .start = 0, + .length = data->total_size, + }; + struct erase_info_user desired_range = { + .start = cfg->range.start, + .length = cfg->range.len, + }; + + /* + * Enabling a protection range will enable hardware status register + * protection as well, bail out if the requested configuration is not + * supported by the MTD interface. + */ + if (cfg->range.len > 0 && cfg->mode != FLASHROM_WP_MODE_HARDWARE) { + return FLASHROM_WP_ERR_OTHER; + } + + /* + * MTD handles write-protection additively, so whatever new range is + * specified is added to the range which is currently protected. To + * just protect the requsted range, we need to disable the current + * write protection and then enable it for the desired range. + */ + int ret = ioctl(fileno(data->dev_fp), MEMUNLOCK, &entire_chip); + if (ret < 0) { + msg_perr("%s: Failed to disable write-protection, MEMUNLOCK ioctl " + "retuned %d, error: %s\n", __func__, ret, strerror(errno)); + return FLASHROM_WP_ERR_WRITE_FAILED; + } + + if (cfg->range.len > 0) { + ret = ioctl(fileno(data->dev_fp), MEMLOCK, &desired_range); + if (ret < 0) { + msg_perr("%s: Failed to enable write-protection, " + "MEMLOCK ioctl retuned %d, error: %s\n", + __func__, ret, strerror(errno)); + return FLASHROM_WP_ERR_WRITE_FAILED; + } + } + + return FLASHROM_WP_OK; +} + +static enum flashrom_wp_result linux_mtd_wp_read_cfg(struct flashrom_wp_cfg *cfg, struct flashctx *flash) +{ + struct linux_mtd_data *data = flash->mst->opaque.data; + bool start_found = false; + bool end_found = false; + + cfg->mode = FLASHROM_WP_MODE_DISABLED; + cfg->range.start = 0; + cfg->range.len = 0; + + /* Check protection status of each block */ + for (size_t u = 0; u < data->total_size; u += data->erasesize) { + struct erase_info_user erase_info = { + .start = u, + .length = data->erasesize, + }; + + int ret = ioctl(fileno(data->dev_fp), MEMISLOCKED, &erase_info); + if (ret == 0) { + /* Block is unprotected. */ + + if (start_found) { + end_found = true; + } + } else if (ret == 1) { + /* Block is protected. */ + + if (end_found) { + /* + * We already found the end of another + * protection range, so this is the start of a + * new one. + */ + return FLASHROM_WP_ERR_OTHER; + } + if (!start_found) { + cfg->range.start = erase_info.start; + cfg->mode = FLASHROM_WP_MODE_HARDWARE; + start_found = true; + } + cfg->range.len += data->erasesize; + } else { + msg_perr("%s: ioctl: %s\n", __func__, strerror(errno)); + return FLASHROM_WP_ERR_READ_FAILED; + } + + } + + return FLASHROM_WP_OK; +} + +static enum flashrom_wp_result linux_mtd_wp_get_available_ranges(struct flashrom_wp_range_list **list, struct flashctx *flash) +{ + /* Not supported by MTD interface. */ + return FLASHROM_WP_ERR_RANGE_LIST_UNAVAILABLE; +} + static const struct opaque_master linux_mtd_opaque_master = { /* max_data_{read,write} don't have any effect for this programmer */ .max_data_read = MAX_DATA_UNSPECIFIED, @@ -317,6 +422,9 @@ .write = linux_mtd_write, .erase = linux_mtd_erase, .shutdown = linux_mtd_shutdown, + .wp_read_cfg = linux_mtd_wp_read_cfg, + .wp_write_cfg = linux_mtd_wp_write_cfg, + .wp_get_ranges = linux_mtd_wp_get_available_ranges, }; /* Returns 0 if setup is successful, non-zero to indicate error */ diff --git a/programmer.h b/programmer.h index 08e5e9c..9344685 100644 --- a/programmer.h +++ b/programmer.h @@ -400,6 +400,9 @@ int (*read) (struct flashctx *flash, uint8_t *buf, unsigned int start, unsigned int len); int (*write) (struct flashctx *flash, const uint8_t *buf, unsigned int start, unsigned int len); int (*erase) (struct flashctx *flash, unsigned int blockaddr, unsigned int blocklen); + enum flashrom_wp_result (*wp_write_cfg)(struct flashctx *, const struct flashrom_wp_cfg *); + enum flashrom_wp_result (*wp_read_cfg)(struct flashrom_wp_cfg *, struct flashctx *); + enum flashrom_wp_result (*wp_get_ranges)(struct flashrom_wp_range_list **, struct flashctx *); int (*shutdown)(void *data); void *data; }; -- To view, visit https://review.coreboot.org/c/flashrom/+/61897 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I5c86e28cdec44bec49ba1d36f8ab62241b9b01da Gerrit-Change-Number: 61897 Gerrit-PatchSet: 1 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: cli_classic: add writeprotect CLI
by Nikolai Artemiev (Code Review) 13 Feb '22

13 Feb '22

Attention is currently required from: Nico Huber, Paul Menzel, Edward O'Callaghan, Angel Pons, Nikolai Artemiev, Sergii Dmytruk. Hello build bot (Jenkins), Nico Huber, Paul Menzel, Edward O'Callaghan, Angel Pons, Anastasia Klimchuk, Sergii Dmytruk, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/58738 to look at the new patch set (#41). Change subject: cli_classic: add writeprotect CLI ...................................................................... cli_classic: add writeprotect CLI Tested with: GD25LQ128, GD25Q32, GD25Q256. BUG=b:195381327,b:153800563 TEST=flashrom --wp-{enable,disable,range,list,status} BRANCH=none Change-Id: I499f521781ee8999921996517802c0c0c641d869 Signed-off-by: Nikolai Artemiev <nartemiev(a)google.com> --- M cli_classic.c 1 file changed, 317 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/38/58738/41 -- To view, visit https://review.coreboot.org/c/flashrom/+/58738 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I499f521781ee8999921996517802c0c0c641d869 Gerrit-Change-Number: 58738 Gerrit-PatchSet: 41 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: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Reviewer: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: 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: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: ichspi.c: Check SPI Cycle In-Progress prior start HW Seq
by Subrata Banik (Code Review) 12 Feb '22

12 Feb '22

Attention is currently required from: Nico Huber, Caveh Jalali, Rizwan Qureshi, Tim Wawrzynczak, Sridhar Siricilla, Angel Pons, Nick Vaccaro, Alex Levin, YH Lin, Boris Mittelberg. Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/61854 ) Change subject: ichspi.c: Check SPI Cycle In-Progress prior start HW Seq ...................................................................... Patch Set 4: (1 comment) Patchset: PS2: > > > > > This leaves some questions. Is something broken wrt. arbitration? How does > > > > arbitration work if two masters set FGO at the same time? is that accounted > > > > for in the hardware? > > > > > > > > Yes this is taken care in the HW. The SPI Ctrlr will queue the transaction of one of the masters until the other is served. > > > > > > > Thank you Rizwan. That's how I always understood it. However, Subrata's test > > > result suggest otherwise: > > > > > > My Bad Nico, I'm also saying the same. This SCIP bit is for each master like when u start a flashrom operation that sets the SCIP bit and attempt to run flashrom using other application (where currently flashrom doesn't bother to look at SCIP even on the same master), we are seeing `timeout` issue in transaction. > > > > > > Note: there are other tool that uses the flashrom even on running from host (if we leave CSE for now) and if there is no sync mechanism in operations at host side, we will run into the issue. > > > > As I said there is no way to synchronize this with the SCIP bit alone. > > Can you please share your thoughts about how the sync should have been taken care without a HW sync bit at each master operation? Additionally, what is your thought behind running more than one application that might potentially use flashrom concurrently being same context master? In such case, how those processes can avoid entering into SPI access move using same flashrom utility? > Don't we need simple semaphore logic to prohibit more than instance to trigger the SPI access using flashrom. > > Here is a scenario and please suggest how would you implement such sync: an application example: set_gbb.sh (which might internally use flashrom) would know if some flash operations are in progress using flashrom (example: futility) at the same time? Eventually multiple flashrom utilities would start executing in parallel and without any lock mechanism, we are meant to run into such a timeout error. > > The recommendation is to check H_SCIP bit before starting a transaction. This indicates a transaction by the same master is in progress. > > > Trying to > > do so makes the code look rather ill-considered. Running two programs using these > > registers at once has many more implications, e.g. one process could clear status > > bits that another is waiting for. > > This is what is happening now. You could argue saying, we could use the file locking mechanism to avoid multiple program to use the same flashrom while one operation is in progress. Isn't having HW register (SCIP) is better than SW file locking logic. > > > It seems mind-boggling to continue the program > > when it detects that another one is accessing the registers. > > This is the point, how do you even *detect* this today that *another* one is accessing register without knowing the busy bit register. And SCIP polling is the way to prevent such access and wait till SPI bus is freely available before initiate newer command with FGO. (This is my bad that I overlooked SCIP polling at first place in SKL days during HW sequencing implemented in coreboot and later flashrom just ported coreboot fast spi driver hence, missed this too). > > > > > Please describe your use-case including this other tool in detail and bring it > > to the mailing list <flashrom(a)flashrom.org>. > > I have added Edward to help on this regard. > > > We can try to help finding a solution > > to your problems but not if the problem description stays vague, changes at your > > convenience and earlier test results that suggest that your hardware does something > > wrong are ignored. > > Sorry I didn't get your point. Can you please try this at your side - Run `flashrom -p host -r read.bin` in a loop while running the flashrom write test. we are seeing below error: Transaction error between offset 0x003f5680 and 0x003f56bf (= 0x003f5680 + 63)! Read operation failed! -- To view, visit https://review.coreboot.org/c/flashrom/+/61854 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ib9265cc20513fd00f32f8fa22e28c312903ca484 Gerrit-Change-Number: 61854 Gerrit-PatchSet: 4 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Alex Levin <levinale(a)chromium.org> Gerrit-Reviewer: Boris Mittelberg <bmbm(a)google.com> Gerrit-Reviewer: Caveh Jalali <caveh(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: YH Lin <yueherngl(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Caveh Jalali <caveh(a)chromium.org> Gerrit-Attention: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Attention: Alex Levin <levinale(a)chromium.org> Gerrit-Attention: YH Lin <yueherngl(a)chromium.org> Gerrit-Attention: Boris Mittelberg <bmbm(a)google.com> Gerrit-Comment-Date: Sat, 12 Feb 2022 18:35:42 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com> Comment-In-Reply-To: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ichspi.c: Check SPI Cycle In-Progress prior start HW Seq
by Subrata Banik (Code Review) 12 Feb '22

12 Feb '22

Attention is currently required from: Nico Huber, Caveh Jalali, Rizwan Qureshi, Tim Wawrzynczak, Sridhar Siricilla, Angel Pons, Nick Vaccaro, Alex Levin, YH Lin, Boris Mittelberg. Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/61854 ) Change subject: ichspi.c: Check SPI Cycle In-Progress prior start HW Seq ...................................................................... Patch Set 4: (1 comment) Patchset: PS2: > > > > This leaves some questions. Is something broken wrt. arbitration? How does > > > arbitration work if two masters set FGO at the same time? is that accounted > > > for in the hardware? > > > > > > Yes this is taken care in the HW. The SPI Ctrlr will queue the transaction of one of the masters until the other is served. > > > > > Thank you Rizwan. That's how I always understood it. However, Subrata's test > > result suggest otherwise: > > > > My Bad Nico, I'm also saying the same. This SCIP bit is for each master like when u start a flashrom operation that sets the SCIP bit and attempt to run flashrom using other application (where currently flashrom doesn't bother to look at SCIP even on the same master), we are seeing `timeout` issue in transaction. > > > > Note: there are other tool that uses the flashrom even on running from host (if we leave CSE for now) and if there is no sync mechanism in operations at host side, we will run into the issue. > > As I said there is no way to synchronize this with the SCIP bit alone. Can you please share your thoughts about how the sync should have been taken care without a HW sync bit at each master operation? Additionally, what is your thought behind running more than one application that might potentially use flashrom concurrently being same context master? In such case, how those processes can avoid entering into SPI access move using same flashrom utility? Don't we need simple semaphore logic to prohibit more than instance to trigger the SPI access using flashrom. Here is a scenario and please suggest how would you implement such sync: an application example: set_gbb.sh (which might internally use flashrom) would know if some flash operations are in progress using flashrom (example: futility) at the same time? Eventually multiple flashrom utilities would start executing in parallel and without any lock mechanism, we are meant to run into such a timeout error. The recommendation is to check H_SCIP bit before starting a transaction. This indicates a transaction by the same master is in progress. > Trying to > do so makes the code look rather ill-considered. Running two programs using these > registers at once has many more implications, e.g. one process could clear status > bits that another is waiting for. This is what is happening now. You could argue saying, we could use the file locking mechanism to avoid multiple program to use the same flashrom while one operation is in progress. Isn't having HW register (SCIP) is better than SW file locking logic. > It seems mind-boggling to continue the program > when it detects that another one is accessing the registers. This is the point, how do you even *detect* this today that *another* one is accessing register without knowing the busy bit register. And SCIP polling is the way to prevent such access and wait till SPI bus is freely available before initiate newer command with FGO. (This is my bad that I overlooked SCIP polling at first place in SKL days during HW sequencing implemented in coreboot and later flashrom just ported coreboot fast spi driver hence, missed this too). > > Please describe your use-case including this other tool in detail and bring it > to the mailing list <flashrom(a)flashrom.org>. I have added Edward to help on this regard. > We can try to help finding a solution > to your problems but not if the problem description stays vague, changes at your > convenience and earlier test results that suggest that your hardware does something > wrong are ignored. Sorry I didn't get your point. -- To view, visit https://review.coreboot.org/c/flashrom/+/61854 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ib9265cc20513fd00f32f8fa22e28c312903ca484 Gerrit-Change-Number: 61854 Gerrit-PatchSet: 4 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Alex Levin <levinale(a)chromium.org> Gerrit-Reviewer: Boris Mittelberg <bmbm(a)google.com> Gerrit-Reviewer: Caveh Jalali <caveh(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: YH Lin <yueherngl(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Caveh Jalali <caveh(a)chromium.org> Gerrit-Attention: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Attention: Alex Levin <levinale(a)chromium.org> Gerrit-Attention: YH Lin <yueherngl(a)chromium.org> Gerrit-Attention: Boris Mittelberg <bmbm(a)google.com> Gerrit-Comment-Date: Sat, 12 Feb 2022 17:55:51 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com> Comment-In-Reply-To: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ichspi.c: Check SPI Cycle In-Progress prior start HW Seq
by Subrata Banik (Code Review) 12 Feb '22

12 Feb '22

Attention is currently required from: Subrata Banik, Caveh Jalali, Rizwan Qureshi, Tim Wawrzynczak, Sridhar Siricilla, Angel Pons, Nick Vaccaro, Alex Levin, YH Lin, Boris Mittelberg. Hello build bot (Jenkins), Nico Huber, Caveh Jalali, Rizwan Qureshi, Tim Wawrzynczak, Edward O'Callaghan, Sridhar Siricilla, Nick Vaccaro, Alex Levin, YH Lin, Boris Mittelberg, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/61854 to look at the new patch set (#4). Change subject: ichspi.c: Check SPI Cycle In-Progress prior start HW Seq ...................................................................... ichspi.c: Check SPI Cycle In-Progress prior start HW Seq As per EDS, SPI controller sets the HSFSTS.bit5 (SCIP) when software sets the Flash Cycle Go (FGO) bit in the Hardware Sequencing Flash Control register. This bit remains set until the cycle completes on the SPI interface. Hardware automatically sets and clears this bit so that software can determine when read data is valid and/or when it is safe to begin programming the next command. Software must initiate the next SPI transaction when this bit is 0. Without this synchronisation being implemented, flashrom is running into below error: Erasing and writing flash chip... Timeout error between offset 0x0061c000 and 0x0061c03f (= 0x0061c000 + 63)! FAILED! Uh oh. Erase/write failed. Checking if anything has changed. Added blocking mechanism without timeout to ensure previous SPI transaction is complete before initiating newer command. Current debug data suggests that concurrent access to flashrom by user space utility might run into issues where some operations are getting timed out without synchronisation. BUG=b:215255210 TEST=Concurrent flashrom access is not throwing timeout. Signed-off-by: Subrata Banik <subratabanik(a)google.com> Change-Id: Ib9265cc20513fd00f32f8fa22e28c312903ca484 --- M ichspi.c 1 file changed, 38 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/54/61854/4 -- To view, visit https://review.coreboot.org/c/flashrom/+/61854 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ib9265cc20513fd00f32f8fa22e28c312903ca484 Gerrit-Change-Number: 61854 Gerrit-PatchSet: 4 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Alex Levin <levinale(a)chromium.org> Gerrit-Reviewer: Boris Mittelberg <bmbm(a)google.com> Gerrit-Reviewer: Caveh Jalali <caveh(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: YH Lin <yueherngl(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Caveh Jalali <caveh(a)chromium.org> Gerrit-Attention: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Attention: Alex Levin <levinale(a)chromium.org> Gerrit-Attention: YH Lin <yueherngl(a)chromium.org> Gerrit-Attention: Boris Mittelberg <bmbm(a)google.com> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: ichspi.c: Check SPI Cycle In-Progress prior start HW Seq
by Nico Huber (Code Review) 12 Feb '22

12 Feb '22

Attention is currently required from: Subrata Banik, Caveh Jalali, Rizwan Qureshi, Tim Wawrzynczak, Sridhar Siricilla, Angel Pons, Nick Vaccaro, Alex Levin, YH Lin, Boris Mittelberg. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/61854 ) Change subject: ichspi.c: Check SPI Cycle In-Progress prior start HW Seq ...................................................................... Patch Set 3: Code-Review-1 (1 comment) Patchset: PS2: > > > This leaves some questions. Is something broken wrt. arbitration? How does > > arbitration work if two masters set FGO at the same time? is that accounted > > for in the hardware? > > > > Yes this is taken care in the HW. The SPI Ctrlr will queue the transaction of one of the masters until the other is served. > > > Thank you Rizwan. That's how I always understood it. However, Subrata's test > result suggest otherwise: > > My Bad Nico, I'm also saying the same. This SCIP bit is for each master like when u start a flashrom operation that sets the SCIP bit and attempt to run flashrom using other application (where currently flashrom doesn't bother to look at SCIP even on the same master), we are seeing `timeout` issue in transaction. > > Note: there are other tool that uses the flashrom even on running from host (if we leave CSE for now) and if there is no sync mechanism in operations at host side, we will run into the issue. As I said there is no way to synchronize this with the SCIP bit alone. Trying to do so makes the code look rather ill-considered. Running two programs using these registers at once has many more implications, e.g. one process could clear status bits that another is waiting for. It seems mind-boggling to continue the program when it detects that another one is accessing the registers. Please describe your use-case including this other tool in detail and bring it to the mailing list <flashrom(a)flashrom.org>. We can try to help finding a solution to your problems but not if the problem description stays vague, changes at your convenience and earlier test results that suggest that your hardware does something wrong are ignored. -- To view, visit https://review.coreboot.org/c/flashrom/+/61854 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ib9265cc20513fd00f32f8fa22e28c312903ca484 Gerrit-Change-Number: 61854 Gerrit-PatchSet: 3 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Alex Levin <levinale(a)chromium.org> Gerrit-Reviewer: Boris Mittelberg <bmbm(a)google.com> Gerrit-Reviewer: Caveh Jalali <caveh(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: YH Lin <yueherngl(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Caveh Jalali <caveh(a)chromium.org> Gerrit-Attention: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Attention: Alex Levin <levinale(a)chromium.org> Gerrit-Attention: YH Lin <yueherngl(a)chromium.org> Gerrit-Attention: Boris Mittelberg <bmbm(a)google.com> Gerrit-Comment-Date: Sat, 12 Feb 2022 12:41:38 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com> Comment-In-Reply-To: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-MessageType: comment

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit