February 2021 - flashrom-gerrit

Change in flashrom[master]: programmer.h: Convert anon union to anon struct
by Angel Pons (Code Review) 03 Feb '21

03 Feb '21

Attention is currently required from: Stefan Reinauer, Edward O'Callaghan. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/50246 ) Change subject: programmer.h: Convert anon union to anon struct ...................................................................... Patch Set 2: Code-Review+1 (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/50246/comment/02fc5f79_4efbf4cf PS2, Line 29: printf("%d\n", *foo.b); In C, dereferencing a null pointer is undefined behavior. I'm not sure what the purpose of this code snippet is. If it's to show the problems of non-null but invalid pointers, I'd use pointers to functions with different signatures. Still, I believe this change is a good idea: it's better to have null pointers than non-null but invalid pointers. -- To view, visit https://review.coreboot.org/c/flashrom/+/50246 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I576967a8599b923c902e39f177f39146291cc242 Gerrit-Change-Number: 50246 Gerrit-PatchSet: 2 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Alan Green <avg(a)google.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Sam McNally <sammc(a)google.com> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Comment-Date: Wed, 03 Feb 2021 11:20:50 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: realtek_mst_i2c_spi.c: Move gpio 88 toggle outside write function
by Edward O'Callaghan (Code Review) 03 Feb '21

03 Feb '21

Attention is currently required from: Shiyu Sun, Angel Pons. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/49785 ) Change subject: realtek_mst_i2c_spi.c: Move gpio 88 toggle outside write function ...................................................................... Patch Set 2: Code-Review+1 (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/49785/comment/064e8abb_9a5fe7a8 PS2, Line 9: Gpio 88 toggle is used as write protection disable/enable now : and we need that to happen at the initialization of programmer. Happy to +2 this however I still would like to see a better explanation as to why. -- To view, visit https://review.coreboot.org/c/flashrom/+/49785 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I237bf9f8aa0fcbb904e7f0c09c74fd179e8c70c1 Gerrit-Change-Number: 49785 Gerrit-PatchSet: 2 Gerrit-Owner: Shiyu Sun <sshiyu(a)google.com> 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: Shiyu Sun <sshiyu(a)google.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Wed, 03 Feb 2021 02:53:03 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: flashrom.c: Fix 4BA for non SPI masters
by Nick Vaccaro (Code Review) 03 Feb '21

03 Feb '21

Attention is currently required from: Edward O'Callaghan. Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/50245 ) Change subject: flashrom.c: Fix 4BA for non SPI masters ...................................................................... Patch Set 1: (1 comment) This change is ready for review. Patchset: PS1: > thanks for the review however this was untested see TEST= line ;) hence no reviewers added. […] Understood, I was asked to review this CL in the bug. Please add me as a reviewer when ready. Thanks ! -- To view, visit https://review.coreboot.org/c/flashrom/+/50245 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ieb59d9d69d5b4f5c1e5aac424898e94c9fe3790b Gerrit-Change-Number: 50245 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Comment-Date: Wed, 03 Feb 2021 02:20:08 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Comment-In-Reply-To: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: flashrom.c: Fix 4BA for non SPI masters
by Nick Vaccaro (Code Review) 03 Feb '21

03 Feb '21

Attention is currently required from: Edward O'Callaghan. Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/50245 ) Change subject: flashrom.c: Fix 4BA for non SPI masters ...................................................................... Patch Set 1: Code-Review-1 (1 comment) Patchset: PS1: I still see failure with this CL. -- To view, visit https://review.coreboot.org/c/flashrom/+/50245 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ieb59d9d69d5b4f5c1e5aac424898e94c9fe3790b Gerrit-Change-Number: 50245 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Comment-Date: Wed, 03 Feb 2021 01:05:54 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: programmer.h: Convert anon union to anon struct
by Sam McNally (Code Review) 03 Feb '21

03 Feb '21

Attention is currently required from: Stefan Reinauer, Edward O'Callaghan, Angel Pons. Sam McNally has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/50246 ) Change subject: programmer.h: Convert anon union to anon struct ...................................................................... Patch Set 2: (1 comment) File programmer.h: https://review.coreboot.org/c/flashrom/+/50246/comment/82d9cab0_df91fec7 PS2, Line 752: struct { This will need corresponding updates to ensure unused masters are zeroed out. -- To view, visit https://review.coreboot.org/c/flashrom/+/50246 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I576967a8599b923c902e39f177f39146291cc242 Gerrit-Change-Number: 50246 Gerrit-PatchSet: 2 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Alan Green <avg(a)google.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Sam McNally <sammc(a)google.com> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Wed, 03 Feb 2021 01:03:55 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: programmer.h: Convert anon union to anon struct
by Alan Green (Code Review) 03 Feb '21

03 Feb '21

Attention is currently required from: Sam McNally, Stefan Reinauer, Edward O'Callaghan, Angel Pons. Alan Green has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/50246 ) Change subject: programmer.h: Convert anon union to anon struct ...................................................................... Patch Set 2: Code-Review+1 -- To view, visit https://review.coreboot.org/c/flashrom/+/50246 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I576967a8599b923c902e39f177f39146291cc242 Gerrit-Change-Number: 50246 Gerrit-PatchSet: 2 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Alan Green <avg(a)google.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Sam McNally <sammc(a)google.com> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Sam McNally <sammc(a)google.com> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Wed, 03 Feb 2021 00:36:05 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: programmer.h: Convert anon union to anon struct
by Edward O'Callaghan (Code Review) 03 Feb '21

03 Feb '21

Hello build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/50246 to look at the new patch set (#2). Change subject: programmer.h: Convert anon union to anon struct ...................................................................... programmer.h: Convert anon union to anon struct Convert the anon union of registered masters in the mst field of the flashctx to a anon struct. If we are going to dereference a pointer there in an undefined way we should crash and not plow ahead with invalid memory. i.e., As a minimal example, ``` struct idk { struct { int *a; int *b; }; }; int main() { struct idk foo = {0}; int i = 12; foo.a = &i; printf("%d\n", *foo.b); return 0; } ``` The user of the registered_masters type is therefore responsible for querying the buses_supported field before attempting to dereference a ptr field in the anon struct. BUG=? TEST=<needed> Change-Id: I576967a8599b923c902e39f177f39146291cc242 Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M programmer.h 1 file changed, 1 insertion(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/46/50246/2 -- To view, visit https://review.coreboot.org/c/flashrom/+/50246 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I576967a8599b923c902e39f177f39146291cc242 Gerrit-Change-Number: 50246 Gerrit-PatchSet: 2 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: programmer.h: Convert anon union to anon struct
by Edward O'Callaghan (Code Review) 03 Feb '21

03 Feb '21

Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/50246 ) Change subject: programmer.h: Convert anon union to anon struct ...................................................................... programmer.h: Convert anon union to anon struct Convert the anon union of registered masters in the mst field of the flashctx to a anon struct. If we are going to dereference a pointer there in an undefined way we should crash and not plow ahead with invalid memory. BUG=? TEST=<needed> Change-Id: I576967a8599b923c902e39f177f39146291cc242 Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M programmer.h 1 file changed, 1 insertion(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/46/50246/1 diff --git a/programmer.h b/programmer.h index b575a72..8cb8690 100644 --- a/programmer.h +++ b/programmer.h @@ -749,7 +749,7 @@ int register_par_master(const struct par_master *mst, const enum chipbustype buses); struct registered_master { enum chipbustype buses_supported; - union { + struct { struct par_master par; struct spi_master spi; struct opaque_master opaque; -- To view, visit https://review.coreboot.org/c/flashrom/+/50246 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I576967a8599b923c902e39f177f39146291cc242 Gerrit-Change-Number: 50246 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: flashrom.c: Fix 4BA for non SPI masters
by Edward O'Callaghan (Code Review) 03 Feb '21

03 Feb '21

Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/50245 ) Change subject: flashrom.c: Fix 4BA for non SPI masters ...................................................................... flashrom.c: Fix 4BA for non SPI masters Chips that indicate they support 4BA in their feature flags break horribly as the current 4BA support assumes a SPI master bus in spi_master_no_4ba_modes() by dereferencing flash->mst.spi in the mst union type. BUG=b:178419441,b:177252170 TEST=<needed> Change-Id: Ieb59d9d69d5b4f5c1e5aac424898e94c9fe3790b Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M flashrom.c M programmer.h 2 files changed, 13 insertions(+), 8 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/45/50245/1 diff --git a/flashrom.c b/flashrom.c index c89abad..c1438c9 100644 --- a/flashrom.c +++ b/flashrom.c @@ -2287,12 +2287,14 @@ flash->chip_restore_fn_count = 0; /* Be careful about 4BA chips and broken masters */ - if (flash->chip->total_size > 16 * 1024 && spi_master_no_4ba_modes(flash)) { - /* If we can't use native instructions, bail out */ - if ((flash->chip->feature_bits & FEATURE_4BA_NATIVE) != FEATURE_4BA_NATIVE - || !spi_master_4ba(flash)) { - msg_cerr("Programmer doesn't support this chip. Aborting.\n"); - return 1; + if (flash->chip->total_size > 16 * 1024 && spi_bus_no_4ba_modes(flash)) { + if (spi_master_no_4ba_modes(flash)) { + /* If we can't use native instructions, bail out */ + if ((flash->chip->feature_bits & FEATURE_4BA_NATIVE) != FEATURE_4BA_NATIVE + || !spi_master_4ba(flash)) { + msg_cerr("Programmer doesn't support this chip. Aborting.\n"); + return 1; + } } } diff --git a/programmer.h b/programmer.h index 29a100b..b575a72 100644 --- a/programmer.h +++ b/programmer.h @@ -820,8 +820,11 @@ } static inline bool spi_master_no_4ba_modes(const struct flashctx *const flash) { - return flash->mst->buses_supported & BUS_SPI && - flash->mst->spi.features & SPI_MASTER_NO_4BA_MODES; + return flash->mst->spi.features & SPI_MASTER_NO_4BA_MODES; +} +static inline bool spi_bus_no_4ba_modes(const struct flashctx *const flash) +{ + return flash->mst->buses_supported & BUS_SPI; } /* usbdev.c */ -- To view, visit https://review.coreboot.org/c/flashrom/+/50245 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ieb59d9d69d5b4f5c1e5aac424898e94c9fe3790b Gerrit-Change-Number: 50245 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: linux_mtd: Switch fopen() to open() for MTD devices
by Hung-Te Lin (Code Review) 02 Feb '21

02 Feb '21

Attention is currently required from: Douglas Anderson, Edward O'Callaghan, Angel Pons, Patrick Rudolph. Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/50206 ) Change subject: linux_mtd: Switch fopen() to open() for MTD devices ...................................................................... Patch Set 1: Code-Review+1 -- To view, visit https://review.coreboot.org/c/flashrom/+/50206 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ifeb00b049ba5aa0bc8fdc591ac1f9861ad5d428d Gerrit-Change-Number: 50206 Gerrit-PatchSet: 1 Gerrit-Owner: Douglas Anderson <dianders(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Hung-Te Lin <hungte(a)chromium.org> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Douglas Anderson <dianders(a)chromium.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Comment-Date: Mon, 01 Feb 2021 23:46:32 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit February 2021