April 2021 - flashrom-gerrit - mail.coreboot.org

Change in flashrom[master]: ft2232_spi.c: Pack WREN and op in one ftdi_write_data() call
by Nico Huber (Code Review) 18 Apr '21

18 Apr '21

Attention is currently required from: Simon Buhrow, Alan Green, Edward O'Callaghan, Anastasia Klimchuk, Samir Ibradžić. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/40477 ) Change subject: ft2232_spi.c: Pack WREN and op in one ftdi_write_data() call ...................................................................... Patch Set 25: (13 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/40477/comment/f392e74b_1cb2807d PS25, Line 7: ft2232_spi.c: Pack WREN and op in one ftdi_write_data() call This would be more accurate now: ft2232_spi: Implement spi_send_multicommand() https://review.coreboot.org/c/flashrom/+/40477/comment/b17dbb66_4a474a3c PS25, Line 10: seems to take always 2-3ms to respond. Please no line breaks after a sentence. Either it's one paragraph (then no line break) or it's two (then they should be separated by an empty line). https://review.coreboot.org/c/flashrom/+/40477/comment/4896555b_ecc3d883 PS25, Line 20: I am using ftdi-2232H chip. That is why I put it at this place. : If anyone has a good overview about all programmers: : This could be implemented in spi_write_cmd() in case that it is : compatible to all programmers : or this principle could be transfered to other programmers which act : in a similar way. It's already implemented, spi_send_multicommand() is called where possible, so compatible programmers just need to implement it. Patchset: PS25: Hi Simon, thanks for the work and sorry for not responding for that long. Also sorry that people keep merging conflicting changes. I guess they just didn't know your work. You'll need at least one more rebase with fixups. The implementation looks quite nice. Just a few details for edge cases left, I think. Don't get alarmed by the number of comments, half of them are merely about style nits. In a follow-up, we can remove the old ft2232_spi_send_command() implementation, and replace the pointer with `default_spi_send_ command`. File ft2232_spi.c: https://review.coreboot.org/c/flashrom/+/40477/comment/a45125c7_cf903df9 PS25, Line 278: struct spi_command *cmds) This is very oddly indented. Tabs and spaces but still not aligned to anything. You can also remove the line break. Our limit is officially 112 chars. https://review.coreboot.org/c/flashrom/+/40477/comment/216dcc54_dfd97ae2 PS25, Line 283: int i = 0,ret = 0, failed = 0; missing space after first comma. https://review.coreboot.org/c/flashrom/+/40477/comment/52adbddd_487d0cad PS25, Line 294: /* Overflow check for buf Should start with a /* on a line of its own. https://review.coreboot.org/c/flashrom/+/40477/comment/c2cdd203_870097c7 PS25, Line 295: 9 This is 12 now becaues the deassertion of CS# is also packed, AFAICS. https://review.coreboot.org/c/flashrom/+/40477/comment/ca9164c4_4e2ceab4 PS25, Line 304: 0 & ~pinlvl; The respective expression in ft2232_spi_send_command() got a fix in the meantime, this should be buf[i++] = ~ 0x08 & pinlvl; /* assert CS (3rd) bit only */ now. https://review.coreboot.org/c/flashrom/+/40477/comment/ac45471e_0d9841e2 PS25, Line 317: if (!cmds->readcnt && ((cmds + 1)->writecnt || (cmds + 1)->readcnt)){ I think we should check here also if the next command still fits. Might be cleaner by adding a function: static bool ft2232_spi_command_fits(const struct spi_command *cmd, size_t buffer_size) { return 12 + cmd->writecnt + cmd->readcnt <= buffer_size; } And call it with `(cmds + 1, ARRAY_SIZE(buf) - i)` here and with `(cmds, ARRAY_SIZE(buf) - i)` above. If we don't check it here, we might run into a situation where spi_send_multicommand() fails while individual spi_send_command() calls would work. also, missing space before { https://review.coreboot.org/c/flashrom/+/40477/comment/70877e02_c659906d PS25, Line 326: optionally Either /* An optional read command */ or /* Optionally, a read command */ https://review.coreboot.org/c/flashrom/+/40477/comment/4e560b59_49b80d2a PS25, Line 343: msg_perr("send_buf failed: %i\n", ret); If `ret` is non-0, we'd skip the next block, and the loop condition would be false. So we can as well `break;` here, and wouldn't have to check `ret` below and also not in the loop condition. Moreover, as sending the deassertion of CS# is packed into the first command now, there is no way for `failed` to differ from `ret`, so we can just drop `failed` and check `ret` below at the `return`. https://review.coreboot.org/c/flashrom/+/40477/comment/f3d5a483_956983eb PS25, Line 348: * FIXME: This is unreliable. There's no guarantee that : * we read the response directly after sending the read : * command. We may be scheduled out etc. Looks like you lost a space before each asterisk. (I'm not sure if the comment is valid actually.) -- To view, visit https://review.coreboot.org/c/flashrom/+/40477 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie4a07499ec5ef0af23818593f45dc427285a9e8a Gerrit-Change-Number: 40477 Gerrit-PatchSet: 25 Gerrit-Owner: Simon Buhrow Gerrit-Reviewer: Alan Green <avg(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: Samir Ibradžić <sibradzic(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-CC: Patrick Georgi <pgeorgi(a)google.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Simon Buhrow Gerrit-Attention: Alan Green <avg(a)google.com> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Samir Ibradžić <sibradzic(a)gmail.com> Gerrit-Comment-Date: Sun, 18 Apr 2021 15:13:44 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: Revert "Makefile: Explicitly set '-std=c99'"
by Angel Pons (Code Review) 18 Apr '21

18 Apr '21

Attention is currently required from: Sam McNally, Nico Huber, Stefan Reinauer, Edward O'Callaghan, Anastasia Klimchuk, Nikolai Artemiev. Hello Sam McNally, build bot (Jenkins), Nico Huber, Stefan Reinauer, Edward O'Callaghan, Anastasia Klimchuk, Nikolai Artemiev, I'd like you to do a code review. Please visit https://review.coreboot.org/c/flashrom/+/52425 to review the following change. Change subject: Revert "Makefile: Explicitly set '-std=c99'" ...................................................................... Revert "Makefile: Explicitly set '-std=c99'" This reverts commit 5e5c3f15efe262ff23642dd543faf6a9dbb3bbdb. Reason for revert: Breaks building on PowerPC, NetBSD, DJGPP, and it is possible that SPARC fails to build as well. Change-Id: I57b5125207de3fd156dface67cba605da893d6aa Signed-off-by: Angel Pons <th3fanbus(a)gmail.com> --- M Makefile M dmi.c 2 files changed, 1 insertion(+), 3 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/25/52425/1 diff --git a/Makefile b/Makefile index 6d37d55..09b2154 100644 --- a/Makefile +++ b/Makefile @@ -30,7 +30,7 @@ DIFF = diff PREFIX ?= /usr/local MANDIR ?= $(PREFIX)/share/man -CFLAGS ?= -std=c99 -D_DEFAULT_SOURCE -D_POSIX_C_SOURCE=200809L -D_BSD_SOURCE -Os -Wall -Wextra -Wno-unused-parameter -Wshadow -Wmissing-prototypes -Wwrite-strings +CFLAGS ?= -Os -Wall -Wextra -Wno-unused-parameter -Wshadow -Wmissing-prototypes -Wwrite-strings EXPORTDIR ?= . RANLIB ?= ranlib PKG_CONFIG ?= pkg-config diff --git a/dmi.c b/dmi.c index 3b717cd..c44221c 100644 --- a/dmi.c +++ b/dmi.c @@ -19,9 +19,7 @@ /* strnlen is in POSIX but was a GNU extension up to glibc 2.10 */ #if (__GLIBC__ == 2 && __GLIBC_MINOR__ < 10) || __GLIBC__ < 2 -#ifndef _GNU_SOURCE #define _GNU_SOURCE -#endif /* !GNU_SOURCE */ #else #define _POSIX_C_SOURCE 200809L #endif -- To view, visit https://review.coreboot.org/c/flashrom/+/52425 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I57b5125207de3fd156dface67cba605da893d6aa Gerrit-Change-Number: 52425 Gerrit-PatchSet: 1 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.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-CC: Idwer Vollering <vidwer(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Sam McNally <sammc(a)google.com> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: hwaccess.c: Fix PowerPC memory barrier
by Angel Pons (Code Review) 18 Apr '21

18 Apr '21

Attention is currently required from: Nico Huber, Arthur Heymans. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52470 ) Change subject: hwaccess.c: Fix PowerPC memory barrier ...................................................................... Patch Set 1: (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/52470/comment/e6112ed5_467c1bd6 PS1, Line 10: rotten link. > We should try to document what change triggered the […] My best guess: CB:47908 -- To view, visit https://review.coreboot.org/c/flashrom/+/52470 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ia84fd1362a1d39aca6269eef22ac994dee68ba4b Gerrit-Change-Number: 52470 Gerrit-PatchSet: 1 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Comment-Date: Sun, 18 Apr 2021 12:57:19 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: manibuilder/anita: Fix ccache image for non-x86
by Nico Huber (Code Review) 18 Apr '21

18 Apr '21

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52482 ) Change subject: manibuilder/anita: Fix ccache image for non-x86 ...................................................................... Patch Set 2: (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/52482/comment/fac4e753_e3422b3f PS1, Line 9: Spark64 > Spar*c* ? Ooops. -- To view, visit https://review.coreboot.org/c/flashrom/+/52482 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ib298d9cbf5d49ff38a898f4ce3ad54bb6af98d86 Gerrit-Change-Number: 52482 Gerrit-PatchSet: 2 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Sun, 18 Apr 2021 12:52:31 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: manibuilder/anita: Fix ccache image for non-x86
by Nico Huber (Code Review) 18 Apr '21

18 Apr '21

Attention is currently required from: Nico Huber. Hello build bot (Jenkins), Angel Pons, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/52482 to look at the new patch set (#2). Change subject: manibuilder/anita: Fix ccache image for non-x86 ...................................................................... manibuilder/anita: Fix ccache image for non-x86 There is no disk label `a` on non-x86 (at least not on Sparc64). Instead, we use the whole disk which is `d` on x86 and `c` else- where. `newfs` and `fsck` needs a little help in this scenario. Change-Id: Ib298d9cbf5d49ff38a898f4ce3ad54bb6af98d86 Signed-off-by: Nico Huber <nico.h(a)gmx.de> --- M util/manibuilder/Dockerfile.anita 1 file changed, 9 insertions(+), 5 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/82/52482/2 -- To view, visit https://review.coreboot.org/c/flashrom/+/52482 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ib298d9cbf5d49ff38a898f4ce3ad54bb6af98d86 Gerrit-Change-Number: 52482 Gerrit-PatchSet: 2 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: manibuilder: Add more NetBSD targets
by Angel Pons (Code Review) 18 Apr '21

18 Apr '21

Attention is currently required from: Nico Huber. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52485 ) Change subject: manibuilder: Add more NetBSD targets ...................................................................... Patch Set 1: Code-Review+1 -- To view, visit https://review.coreboot.org/c/flashrom/+/52485 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ic323ced43132921d9f6f2c0d5fcf9c581afec0c7 Gerrit-Change-Number: 52485 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Comment-Date: Sun, 18 Apr 2021 12:41:58 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: manibuilder/anita: Always provide reasonable --memory-size
by Angel Pons (Code Review) 18 Apr '21

18 Apr '21

Attention is currently required from: Nico Huber. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52484 ) Change subject: manibuilder/anita: Always provide reasonable --memory-size ...................................................................... Patch Set 1: Code-Review+1 -- To view, visit https://review.coreboot.org/c/flashrom/+/52484 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I255c41aeb92cda29ed23a236017472982e839530 Gerrit-Change-Number: 52484 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Comment-Date: Sun, 18 Apr 2021 12:41:21 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: linux_spi.c: Extract get_max_kernel_buf_size() as a function
by Angel Pons (Code Review) 18 Apr '21

18 Apr '21

Attention is currently required from: Anastasia Klimchuk. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52283 ) Change subject: linux_spi.c: Extract get_max_kernel_buf_size() as a function ...................................................................... Patch Set 2: (1 comment) File linux_spi.c: https://review.coreboot.org/c/flashrom/+/52283/comment/830cd58c_11558754 PS2, Line 123: { nit: opening braces for functions go to the next line -- To view, visit https://review.coreboot.org/c/flashrom/+/52283 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I4b8c5775fb8f4b0dff702fcc0fb258221254c659 Gerrit-Change-Number: 52283 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: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-CC: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-CC: Sam McNally <sammc(a)google.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Sun, 18 Apr 2021 12:40:51 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: manibuilder/anita: Factor things out into Makefile.anita
by Angel Pons (Code Review) 18 Apr '21

18 Apr '21

Attention is currently required from: Nico Huber. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52483 ) Change subject: manibuilder/anita: Factor things out into Makefile.anita ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/52483 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I0a3ca751b42f1ca8c05d93eb9740bb0ee5cc6d09 Gerrit-Change-Number: 52483 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Comment-Date: Sun, 18 Apr 2021 12:33:53 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: manibuilder/anita: Fix ccache image for non-x86
by Angel Pons (Code Review) 18 Apr '21

18 Apr '21

Attention is currently required from: Nico Huber. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52482 ) Change subject: manibuilder/anita: Fix ccache image for non-x86 ...................................................................... Patch Set 1: Code-Review+2 (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/52482/comment/515349c6_bc5d77db PS1, Line 9: Spark64 Spar*c* ? -- To view, visit https://review.coreboot.org/c/flashrom/+/52482 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ib298d9cbf5d49ff38a898f4ce3ad54bb6af98d86 Gerrit-Change-Number: 52482 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Comment-Date: Sun, 18 Apr 2021 12:33:24 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit April 2021