November 2021 - flashrom-gerrit

Change in flashrom[master]: [RFC] writeprotect: implement wp_get_ranges()
by Sergii Dmytruk (Code Review) 01 Nov '21

01 Nov '21

Attention is currently required from: Nikolai Artemiev. Sergii Dmytruk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58481 ) Change subject: [RFC] writeprotect: implement wp_get_ranges() ...................................................................... Patch Set 6: (1 comment) File writeprotect.h: https://review.coreboot.org/c/flashrom/+/58481/comment/fb6f127c_0105c6f5 PS6, Line 57: wp_get_ranges > I previously had wp_get_available_ranges() but thought it was a bit too long. […] "get_range_list" is better than "get_ranges", but still might suggest multiple ranges rather than a list of possible values. Other suggestions: wp_get_range_values(), wp_allowed_ranges(). -- To view, visit https://review.coreboot.org/c/flashrom/+/58481 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Id51f038f03305c8536d80313e52f77d27835f34d Gerrit-Change-Number: 58481 Gerrit-PatchSet: 6 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Mon, 01 Nov 2021 13:28:52 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nikolai Artemiev <nartemiev(a)google.com> Comment-In-Reply-To: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: [RFC] writeprotect: implement wp_{get,set}_range()
by Nico Huber (Code Review) 01 Nov '21

01 Nov '21

Attention is currently required from: Nikolai Artemiev. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58482 ) Change subject: [RFC] writeprotect: implement wp_{get,set}_range() ...................................................................... Patch Set 6: (1 comment) Patchset: PS6: > > I guess a separate tool that handles only block protection (or write > protection or flash configuration in general) would be much easier to maintain. > > I think you mean building a separate binary from common sources right? Right, nothing else makes sense, IMO :) > I'm fine with building a separate binary, though I think we would need to be careful about how we partition the functionality to avoid feature duplication. I think the first sentence of flashrom's manpage provides a reasonable scope for one binary: "flashrom is a utility for detecting, reading, writing, verifying and erasing flash chips." I guess one could even argue that this is already too much. But that's what it always was, and it seems convenient enough. ;) For future binaries, I would just use the smallest set of functions that overlap somehow. For instance in flashrom, `writing` overlaps with all the other functions. We could start a new binary for the configuration of write protection, and then only add there what overlaps with write protection (ATM, nothing comes to my mind). > > > I would much prefer to only call libflashrom functions from any CLI code. > > I agree, I definitely want to move the writeprotect interface into libflashrom. I thought it would be easier to call the functions directly for now, but it shouldn't take too much work to squash writeprotect.h into libflashrom.h. Sounds like a plan. However, for libflashrom, we should make sure to get the abstraction right from the beginning. The API is not considered stable, but I still wouldn't like to change too much later on. -- To view, visit https://review.coreboot.org/c/flashrom/+/58482 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I7d26f43fb05c5828b9839bb57a28fa1088dcd9a0 Gerrit-Change-Number: 58482 Gerrit-PatchSet: 6 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> 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: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Mon, 01 Nov 2021 12:48:08 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: [TESTME]sb600spi/yangtze+: Allow and account for 4BA commands
by Angel Pons (Code Review) 01 Nov '21

01 Nov '21

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58777 ) Change subject: [TESTME]sb600spi/yangtze+: Allow and account for 4BA commands ...................................................................... Patch Set 1: Code-Review+1 -- To view, visit https://review.coreboot.org/c/flashrom/+/58777 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: If67c04e6b262401159ac8ad12c8cb3d482e08ec7 Gerrit-Change-Number: 58777 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-CC: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Comment-Date: Mon, 01 Nov 2021 10:43:43 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: [TESTME]sb600spi: Remove memory-mapped read workaround
by Angel Pons (Code Review) 01 Nov '21

01 Nov '21

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58776 ) Change subject: [TESTME]sb600spi: Remove memory-mapped read workaround ...................................................................... Patch Set 1: Code-Review+1 -- To view, visit https://review.coreboot.org/c/flashrom/+/58776 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I67f9ca0cd88de35953d988ab51b4b5f485085f49 Gerrit-Change-Number: 58776 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-CC: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Comment-Date: Mon, 01 Nov 2021 10:43:35 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: chipset_enable.c: Mark Intel Q67 as DEP
by Angel Pons (Code Review) 01 Nov '21

01 Nov '21

Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/47797 ) Change subject: chipset_enable.c: Mark Intel Q67 as DEP ...................................................................... chipset_enable.c: Mark Intel Q67 as DEP Tested reading, writing and erasing the internal flash chip using an HP Elite 8200 mainboard with an Intel Q67 PCH. However, since ME-enabled chipsets are marked as DEP instead of OK, this one shall also be. Change-Id: I2bd431c5c72824654b6b5b840f9af55dfe9d3554 Signed-off-by: Angel Pons <th3fanbus(a)gmail.com> --- M chipset_enable.c 1 file changed, 1 insertion(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/97/47797/1 diff --git a/chipset_enable.c b/chipset_enable.c index 04ff9d8..5e4a547 100644 --- a/chipset_enable.c +++ b/chipset_enable.c @@ -1829,7 +1829,7 @@ {0x8086, 0x1c4b, B_FS, NT, "Intel", "HM67", enable_flash_pch6}, {0x8086, 0x1c4c, B_FS, NT, "Intel", "Q65", enable_flash_pch6}, {0x8086, 0x1c4d, B_FS, NT, "Intel", "QS67", enable_flash_pch6}, - {0x8086, 0x1c4e, B_FS, NT, "Intel", "Q67", enable_flash_pch6}, + {0x8086, 0x1c4e, B_FS, DEP, "Intel", "Q67", enable_flash_pch6}, {0x8086, 0x1c4f, B_FS, DEP, "Intel", "QM67", enable_flash_pch6}, {0x8086, 0x1c50, B_FS, NT, "Intel", "B65", enable_flash_pch6}, {0x8086, 0x1c52, B_FS, NT, "Intel", "C202", enable_flash_pch6}, -- To view, visit https://review.coreboot.org/c/flashrom/+/47797 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I2bd431c5c72824654b6b5b840f9af55dfe9d3554 Gerrit-Change-Number: 47797 Gerrit-PatchSet: 1 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: newchange

4 5

Change in flashrom[master]: [RFC] writeprotect: implement wp_{get,set}_range()
by Nikolai Artemiev (Code Review) 01 Nov '21

01 Nov '21

Attention is currently required from: Nico Huber. Nikolai Artemiev has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58482 ) Change subject: [RFC] writeprotect: implement wp_{get,set}_range() ...................................................................... Patch Set 6: (1 comment) Patchset: PS6: > I guess a separate tool that handles only block protection (or write > protection or flash configuration in general) would be much easier to maintain. I think you mean building a separate binary from common sources right? Splitting off an entirely separate project would be a lot of extra work, especially when read/write/erase operations need some of the same functionality (i.e. disabling block protection). I'm fine with building a separate binary, though I think we would need to be careful about how we partition the functionality to avoid feature duplication. > I would much prefer to only call libflashrom functions from any CLI code. I agree, I definitely want to move the writeprotect interface into libflashrom. I thought it would be easier to call the functions directly for now, but it shouldn't take too much work to squash writeprotect.h into libflashrom.h. -- To view, visit https://review.coreboot.org/c/flashrom/+/58482 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I7d26f43fb05c5828b9839bb57a28fa1088dcd9a0 Gerrit-Change-Number: 58482 Gerrit-PatchSet: 6 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> 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: Nico Huber <nico.h(a)gmx.de> Gerrit-Comment-Date: Mon, 01 Nov 2021 10:24:17 +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]: chipset_enable.c: Mark Intel Z390 as DEP
by Angel Pons (Code Review) 01 Nov '21

01 Nov '21

Attention is currently required from: melvyn2. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58774 ) Change subject: chipset_enable.c: Mark Intel Z390 as DEP ...................................................................... Patch Set 3: Code-Review+1 (3 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/58774/comment/e0771009_10192c29 PS3, Line 9: me_cleaner Minor: Our guidelines state that commit message lines wrap at 72 characters [1]. `me_cleaner` goes past the limit, so it should go in a new line. [1]: https://flashrom.org/Development_Guidelines#Commit_message https://review.coreboot.org/c/flashrom/+/58774/comment/9611dc8c_d84dcd52 PS3, Line 12: melvyn2 As per our Sign-off procedure [1], I'm afraid you have to use your real name here. [1]: https://flashrom.org/Development_Guidelines#Sign-off_Procedure Patchset: PS3: Welcome! The change itself looks good, but the sign-off needs to be addressed. -- To view, visit https://review.coreboot.org/c/flashrom/+/58774 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: If14d45c144bb32a1d1046185d4476ea29e4d0912 Gerrit-Change-Number: 58774 Gerrit-PatchSet: 3 Gerrit-Owner: melvyn2 <melvyn2(a)brcok.tk> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: melvyn2 <melvyn2(a)brcok.tk> Gerrit-Comment-Date: Mon, 01 Nov 2021 09:27:05 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: tests: Add wraps for __xstat variants of stat
by Anastasia Klimchuk (Code Review) 01 Nov '21

01 Nov '21

Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons. Hello build bot (Jenkins), Nico Huber, Edward O'Callaghan, Angel Pons, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/58103 to look at the new patch set (#4). Change subject: tests: Add wraps for __xstat variants of stat ...................................................................... tests: Add wraps for __xstat variants of stat __xstat variants of stat are invoked under chromium chroot. For all existing tests it is sufficient for stat to "do nothing, return 0", so new wraps do just that. Without this patch linux_mtd test fails under chromium chroot. BUG=b:181803212 TEST=running tests on three different environments, 1) stat64 (ninja tests in upstream tree) 2) stat64 (ninja tests in chromium tree) 2) __xstat64 (emerge with tests in chromium tree) Change-Id: I4c5c243acde09dc5bb6b2a14042fcd23a49707db Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org> --- M tests/meson.build M tests/tests.c 2 files changed, 14 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/03/58103/4 -- To view, visit https://review.coreboot.org/c/flashrom/+/58103 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I4c5c243acde09dc5bb6b2a14042fcd23a49707db Gerrit-Change-Number: 58103 Gerrit-PatchSet: 4 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: 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: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: tests: Add wraps for all variants of stat
by Anastasia Klimchuk (Code Review) 01 Nov '21

01 Nov '21

Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58103 ) Change subject: tests: Add wraps for all variants of stat ...................................................................... Patch Set 3: (3 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/58103/comment/a917cdb8_c9b64ed7 PS2, Line 9: all > Yes you are right, I will do this. […] Environment this applies to: __xstat64 wrap is needed to run unit tests with emerge under chromium chroot. When I run same tests, in the same chromium tree, with meson, then stat64 is called. There was, as I understand, some change in chromium chroot recently, which caused the switch from stat64 to __xstat64, however I couldn't find what exactly has changed. Good news: "some flashrom developer" won't have a failing test locally, because this only affects people who are running tests with emerge under chromium chroot. Bad news: this affects me :( Partially, because I can work on upstream tree as usual. I will re-write commit message later today, I agree it is too vague. Did I answer your question about environment? Let me know if not! https://review.coreboot.org/c/flashrom/+/58103/comment/7e03e1f7_6d2987c4 PS2, Line 17: TEST=running tests on two different environments, : with 1) stat64 and 2) __xstat64 invoked > Yes sure, I will do this. Let's keep this comment unresolved. I looked into all potentially relevant minijail policies, could not find any recent policy change that would look like it caused switching from stat64 to __xstat64. Maybe it's not minijail. > before being sure we need this wrap Currently, anyone who wants to run unit tests with emerge under chroot, needs this wrap. I think we need it... Patchset: PS3: Sorry for delay, I was gathering info. I am back. -- To view, visit https://review.coreboot.org/c/flashrom/+/58103 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I4c5c243acde09dc5bb6b2a14042fcd23a49707db Gerrit-Change-Number: 58103 Gerrit-PatchSet: 3 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: 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: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Mon, 01 Nov 2021 06:16:35 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: [RFC] writeprotect: implement wp_get_ranges()
by Nikolai Artemiev (Code Review) 01 Nov '21

01 Nov '21

Attention is currently required from: Sergii Dmytruk. Nikolai Artemiev has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58481 ) Change subject: [RFC] writeprotect: implement wp_get_ranges() ...................................................................... Patch Set 6: (1 comment) File writeprotect.h: https://review.coreboot.org/c/flashrom/+/58481/comment/4e9ef98b_36c4fa28 PS6, Line 57: wp_get_ranges > How about renaming it to, say, `wp_list_ranges` to make it less confusing? This will separate it fro […] I previously had wp_get_available_ranges() but thought it was a bit too long. Maybe we should call it wp_get_range_list() since it's returning the list rather than printing it. -- To view, visit https://review.coreboot.org/c/flashrom/+/58481 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Id51f038f03305c8536d80313e52f77d27835f34d Gerrit-Change-Number: 58481 Gerrit-PatchSet: 6 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Mon, 01 Nov 2021 04:44:09 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-MessageType: comment

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit November 2021