December 2021 - flashrom-gerrit

Change in flashrom[master]: Makefile: Make pkg-config mandatory to find libftdi1
by Nico Huber (Code Review) 09 Dec '21

09 Dec '21

Attention is currently required from: Thomas Heijligen. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58623 ) Change subject: Makefile: Make pkg-config mandatory to find libftdi1 ...................................................................... Patch Set 8: (1 comment) File Makefile.include: https://review.coreboot.org/c/flashrom/+/58623/comment/2182cb58_8d80a970 PS5, Line 40: 2>/dev/null > I think the terminal output of the Makefile should be enough in the end. How? I mean it's about unexpected errors, not expected ones. So I don't think we'd want to clutter the Makefile just to provide accurate messages. For instance, if anything goes wrong in `find_dependency` but lets make continue, we'd simply print that the dependency wasn't found. Ok, that's true, but doesn't tell us why it wasn't found. It could still be there. We'd at least have to also check if `pkg-config` is there, and if `pkg-config` can run (e.g. doesn't have trouble with shared libraries; I get that a lot on ArchLinux when installing something new without updating the whole system). Then, if pkg-config knows any error condi- tions, we'd have to relay that too? OTOH, it's not very hard to keep the current behavior, e.g. just call `debug_shell` instead of `shell`? -- To view, visit https://review.coreboot.org/c/flashrom/+/58623 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I41f5186d9f3e063c12c8c6eea888d0b0bf534259 Gerrit-Change-Number: 58623 Gerrit-PatchSet: 8 Gerrit-Owner: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> 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: Thomas Heijligen <src(a)posteo.de> Gerrit-Comment-Date: Thu, 09 Dec 2021 18:38:49 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Thomas Heijligen <src(a)posteo.de> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: pci.h: move include into own wrapper
by Thomas Heijligen (Code Review) 08 Dec '21

08 Dec '21

Attention is currently required from: Nico Huber, Angel Pons. Thomas Heijligen has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58883 ) Change subject: pci.h: move include into own wrapper ...................................................................... Patch Set 3: (1 comment) File hwaccess.h: https://review.coreboot.org/c/flashrom/+/58883/comment/abff5a46_66fc362d PS1, Line 36: #define pci_mmio_writeb mmio_le_writeb : #define pci_mmio_writew mmio_le_writew : #define pci_mmio_writel mmio_le_writel : #define pci_mmio_readb mmio_le_readb : #define pci_mmio_readw mmio_le_readw : #define pci_mmio_readl mmio_le_readl > cf. 1d3a2fef […] Ok, I'll keep the pci_*** names for now. -- To view, visit https://review.coreboot.org/c/flashrom/+/58883 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ibf00356f0ef5cc92e0ec99f8fe5cdda56f47b166 Gerrit-Change-Number: 58883 Gerrit-PatchSet: 3 Gerrit-Owner: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> 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: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Wed, 08 Dec 2021 18:31:32 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Thomas Heijligen <src(a)posteo.de> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: Makefile: Make pkg-config mandatory to find libftdi1
by Thomas Heijligen (Code Review) 08 Dec '21

08 Dec '21

Attention is currently required from: Nico Huber. Thomas Heijligen has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58623 ) Change subject: Makefile: Make pkg-config mandatory to find libftdi1 ...................................................................... Patch Set 8: (1 comment) File Makefile.include: https://review.coreboot.org/c/flashrom/+/58623/comment/348499bd_b79ecfd3 PS5, Line 40: 2>/dev/null > Looking at CB:59230 made me realize that we don't output into […] I think the terminal output of the Makefile should be enough in the end. -- To view, visit https://review.coreboot.org/c/flashrom/+/58623 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I41f5186d9f3e063c12c8c6eea888d0b0bf534259 Gerrit-Change-Number: 58623 Gerrit-PatchSet: 8 Gerrit-Owner: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> 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-Comment-Date: Wed, 08 Dec 2021 15:21:02 +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]: [FIX ME] tests: Add test for extract operation
by Anastasia Klimchuk (Code Review) 08 Dec '21

08 Dec '21

Attention is currently required from: Edward O'Callaghan, Daniel Campello, Peter Marheine. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59532 ) Change subject: [FIX ME] tests: Add test for extract operation ...................................................................... Patch Set 2: (1 comment) Patchset: PS1: > I disagree... […] Just a note that fix is under review and discussion continues in CB:59921 -- To view, visit https://review.coreboot.org/c/flashrom/+/59532 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I13107c0e095d53184e32a41fa72227cf7dc1d449 Gerrit-Change-Number: 59532 Gerrit-PatchSet: 2 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Daniel Campello <campello(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Daniel Campello <campello(a)chromium.org> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Comment-Date: Wed, 08 Dec 2021 03:04:37 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Daniel Campello <campello(a)google.com> Comment-In-Reply-To: Daniel Campello <campello(a)chromium.org> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Comment-In-Reply-To: Peter Marheine <pmarheine(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: flashrom.c: extract operation only uses layout files
by Anastasia Klimchuk (Code Review) 08 Dec '21

08 Dec '21

Attention is currently required from: Daniel Campello, Angel Pons. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59921 ) Change subject: flashrom.c: extract operation only uses layout files ...................................................................... Patch Set 4: (1 comment) Patchset: PS4: > I don't understand the concern that goes away when "filename check would move from read_flash_to_file into do_read". In both cases (with and without CB:59291) the filename in do_read should be optional I don't think filename in do_read *should* be optional, but at least I can agree it *can* be optional in do_read. This patch, however, makes it optional in read_flash_to_file, which is one level lower and IMO creates potential source of errors for other use cases. > Since this CL is solving a bug I believe it should go on first Yes this patch is solving a bug, but ... the bug was introduced 7 months ago, and no one noticed. This is ofc not an excuse to have broken code, but I think we can afford to spend few extra days to do the right thing. CB:59291 is pretty much ready, and tested, so there is not much risk in rebasing on top of it. > Otherwise I'm not strongly opposed with rebasing. This is awesome! Thank you Daniel! Let's rebase :) -- To view, visit https://review.coreboot.org/c/flashrom/+/59921 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ibc9a4e2966385863345f06662521d6d0e4685121 Gerrit-Change-Number: 59921 Gerrit-PatchSet: 4 Gerrit-Owner: Daniel Campello <campello(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Daniel Campello <campello(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Wed, 08 Dec 2021 02:57:48 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Daniel Campello <campello(a)chromium.org> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: flashrom: Convert do_read() into a libflashrom user
by Daniel Campello (Code Review) 08 Dec '21

08 Dec '21

Attention is currently required from: Edward O'Callaghan, Angel Pons, Anastasia Klimchuk. Daniel Campello has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59291 ) Change subject: flashrom: Convert do_read() into a libflashrom user ...................................................................... Patch Set 4: (1 comment) File flashrom.c: https://review.coreboot.org/c/flashrom/+/59291/comment/bc930a87_0aef02a0 PS4, Line 2165: int do_read(struct flashctx *const flash, const char *const filename) It may make sense to move all of do_read() (and friends) to cli_classic.c if the objective is to make the CLI look like any other client that uses libflashrom -- To view, visit https://review.coreboot.org/c/flashrom/+/59291 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I4b690b688acf9d5deb46e8642a252a2132ea8c73 Gerrit-Change-Number: 59291 Gerrit-PatchSet: 4 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Daniel Campello <campello(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-CC: 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: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Wed, 08 Dec 2021 02:23:50 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: flashrom.c: extract operation only uses layout files
by Daniel Campello (Code Review) 08 Dec '21

08 Dec '21

Attention is currently required from: Angel Pons, Anastasia Klimchuk. Daniel Campello has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59921 ) Change subject: flashrom.c: extract operation only uses layout files ...................................................................... Patch Set 4: (1 comment) Patchset: PS4: > Sorry for delay, just looked into this patch. […] I don't understand the concern that goes away when "filename check would move from read_flash_to_file into do_read". In both cases (with and without CB:59291) the filename in do_read should be optional (i.e., required and enforced by cli_classic.c for option -r and missing for option -x) As stated in the other CL, I believe both CLs should be merged but they are solving orthogonal issues. Since this CL is solving a bug I believe it should go on first. Otherwise I'm not strongly opposed with rebasing. -- To view, visit https://review.coreboot.org/c/flashrom/+/59921 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ibc9a4e2966385863345f06662521d6d0e4685121 Gerrit-Change-Number: 59921 Gerrit-PatchSet: 4 Gerrit-Owner: Daniel Campello <campello(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Wed, 08 Dec 2021 02:22:21 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: flashrom: Convert do_read() into a libflashrom user
by Daniel Campello (Code Review) 08 Dec '21

08 Dec '21

Attention is currently required from: Edward O'Callaghan, Angel Pons, Anastasia Klimchuk. Daniel Campello has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59291 ) Change subject: flashrom: Convert do_read() into a libflashrom user ...................................................................... Patch Set 4: (1 comment) File flashrom.c: https://review.coreboot.org/c/flashrom/+/59291/comment/989cf90e_50758c43 PS4, Line 2185: ret = write_buf_to_file(buf, size, filename); > As I already mentioned in CB:59921 the best thing would be to rebase it on the top of this one. […] These two patches are orthogonal. As you see here the filename in do_read will also have to be optionally NULL. Since the other patch is fixing a bug should go first. -- To view, visit https://review.coreboot.org/c/flashrom/+/59291 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I4b690b688acf9d5deb46e8642a252a2132ea8c73 Gerrit-Change-Number: 59291 Gerrit-PatchSet: 4 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Daniel Campello <campello(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-CC: 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: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Wed, 08 Dec 2021 02:18:22 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Daniel Campello <campello(a)chromium.org> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: flashrom: Convert do_read() into a libflashrom user
by Anastasia Klimchuk (Code Review) 08 Dec '21

08 Dec '21

Attention is currently required from: Edward O'Callaghan, Daniel Campello, Angel Pons. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59291 ) Change subject: flashrom: Convert do_read() into a libflashrom user ...................................................................... Patch Set 4: (1 comment) File flashrom.c: https://review.coreboot.org/c/flashrom/+/59291/comment/47e64d5f_4e5e3355 PS4, Line 2185: ret = write_buf_to_file(buf, size, filename); > Just add "if (filename)" as in CB:59921 As I already mentioned in CB:59921 the best thing would be to rebase it on the top of this one. If you can do it, would be great! I would strongly prefer to keep two patches (do not squash into one) because there are two different goals: 1) convert an operation to libflashrom 2) fix a bug. -- To view, visit https://review.coreboot.org/c/flashrom/+/59291 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I4b690b688acf9d5deb46e8642a252a2132ea8c73 Gerrit-Change-Number: 59291 Gerrit-PatchSet: 4 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Daniel Campello <campello(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Daniel Campello <campello(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Wed, 08 Dec 2021 00:01:08 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Daniel Campello <campello(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: flashrom.c: extract operation only uses layout files
by Anastasia Klimchuk (Code Review) 07 Dec '21

07 Dec '21

Attention is currently required from: Daniel Campello, Angel Pons. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59921 ) Change subject: flashrom.c: extract operation only uses layout files ...................................................................... Patch Set 4: (1 comment) Patchset: PS4: Sorry for delay, just looked into this patch. I was initially worried about such a change, because it looked like a hack to me. read_flash_to_file is used in many flows (not only extract operation). Allowing empty filename for all of them would be error-prone, like removing protection on a very low level. However! There is another patch which makes a difference, CB:59291 (convert do_read() into a libflashrom user). If this fix would be based on the top of CB:59291 , it would look more like a solution and not a hack. It would not be a "removing protection on a very low level" that I am concerned about, since filename check would move from read_flash_to_file into do_read. Daniel, if you can rebase this on the top of CB:59291 that would be great, thanks! You already reviewed CB:59291 and added a comment :) -- To view, visit https://review.coreboot.org/c/flashrom/+/59921 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ibc9a4e2966385863345f06662521d6d0e4685121 Gerrit-Change-Number: 59921 Gerrit-PatchSet: 4 Gerrit-Owner: Daniel Campello <campello(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Daniel Campello <campello(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Tue, 07 Dec 2021 23:52:37 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit December 2021