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

Change in flashrom[master]: ich_descriptors.c: Ensure unsigned types >=0 on to prevent underflow
by Anastasia Klimchuk (Code Review) 05 Apr '22

05 Apr '22

Attention is currently required from: Edward O'Callaghan, Angel Pons, Light. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62764 ) Change subject: ich_descriptors.c: Ensure unsigned types >=0 on to prevent underflow ...................................................................... Patch Set 16: (1 comment) File ich_descriptors.c: https://review.coreboot.org/c/flashrom/+/62764/comment/1757fcc7_dfb935eb PS14, Line 501: for (j = 0; j < (size_t)min(num_regions, 12); j++) > > > Not sure why you don't believe it is the max bounds in this case? […] Thanks so much for explanation! This is a tough case for naming :) Something that was supposed to be a maximum at the time of introduction, but now it is not a maximum anymore. Alright my idea: EXT_REGION_START_INDEX how about this? -- To view, visit https://review.coreboot.org/c/flashrom/+/62764 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I5921cc571f3dca5188ca1973dba6ececbcbe2f39 Gerrit-Change-Number: 62764 Gerrit-PatchSet: 16 Gerrit-Owner: Light <aarya.chaumal(a)gmail.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: build bot (Jenkins) <no-reply(a)coreboot.org> 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: Light <aarya.chaumal(a)gmail.com> Gerrit-Comment-Date: Tue, 05 Apr 2022 10:43:08 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Comment-In-Reply-To: Light <aarya.chaumal(a)gmail.com> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: flashchips: enable write-protection for W25Q{64,128}.V
by Nico Huber (Code Review) 05 Apr '22

05 Apr '22

Attention is currently required from: Angel Pons, Sergii Dmytruk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59071 ) Change subject: flashchips: enable write-protection for W25Q{64,128}.V ...................................................................... Patch Set 25: (2 comments) Patchset: PS15: > This seems resolved, just checking, does everyone agree this is resolved? Ack File flashchips.c: https://review.coreboot.org/c/flashrom/+/59071/comment/fa52acaa_9312ed39 PS25, Line 18017: W25Q64JV We already have the same name in the entry below (with a different .model_id). As I understand the datasheet, this one should be "W25Q64JV-.Q" and the other one "W25Q64JV-.M". -- To view, visit https://review.coreboot.org/c/flashrom/+/59071 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Iccb69a8d3a0dd2192e2c938caddaf07b1889ed35 Gerrit-Change-Number: 59071 Gerrit-PatchSet: 25 Gerrit-Owner: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.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: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Maciej Pijanowski <maciej.pijanowski(a)3mdeb.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Tue, 05 Apr 2022 10:42:28 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Comment-In-Reply-To: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: spi25_statusreg.c: add SR3 read/write support
by Nico Huber (Code Review) 05 Apr '22

05 Apr '22

Attention is currently required from: Sergii Dmytruk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/60230 ) Change subject: spi25_statusreg.c: add SR3 read/write support ...................................................................... Patch Set 9: (1 comment) File flash.h: https://review.coreboot.org/c/flashrom/+/60230/comment/d4df43d2_fe7f1d2e PS2, Line 148: FEATURE_SR3 > Thank for explaining, I understand why there is one flag. […] As mentioned above SR2 is a different case no matter what we do, as different chips have different commands to access it. The question is what we want to achieve with the FEATURE_SR3 flag. In the current PS, it's used to confirm that SR3 is supported before we actually send a related command. But, what does it mean if this check fails? It would be a bug in flashrom or the chip database. If we know that for sure, we should tell the user to tell us. Also, if we consider the database fields for write-protection bits could be wrong what does that say about possibly wrong FEATURE_SR3 flags? :) One might even say due to the redundancy there is more that can go wrong. (Avoiding redundancy was part of my first data- base lessons.) Now, if it's worth the hassle is hard to tell. We already have other mechanisms like the test state to keep track of the sanity of data- base entries. (just noticed, WP lacks a field in `struct tested`) -- To view, visit https://review.coreboot.org/c/flashrom/+/60230 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Id987c544c02da2b956e6ad2c525265cac8f15be1 Gerrit-Change-Number: 60230 Gerrit-PatchSet: 9 Gerrit-Owner: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.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: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Maciej Pijanowski <maciej.pijanowski(a)3mdeb.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Tue, 05 Apr 2022 10:20:26 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Comment-In-Reply-To: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ichspi: Unify timeouts across all SPI operations to 30s
by Nico Huber (Code Review) 05 Apr '22

05 Apr '22

Attention is currently required from: Subrata Banik, Paul Menzel, Edward O'Callaghan, Anastasia Klimchuk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62867 ) Change subject: ichspi: Unify timeouts across all SPI operations to 30s ...................................................................... Patch Set 8: (1 comment) File ichspi.c: https://review.coreboot.org/c/flashrom/+/62867/comment/aafaba51_b7e36294 PS8, Line 68: */ > Just to close the loop here Nico, it was me that asked Subrata to write a very detailed comment sinc […] It's the right idea! However, I think Documentation/ or the commit message is a better place. Everyone can look it up and it doesn't have to clutter the code. -- To view, visit https://review.coreboot.org/c/flashrom/+/62867 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ifa910dea794175d8ee2ad277549e5a0d69cba45b Gerrit-Change-Number: 62867 Gerrit-PatchSet: 8 Gerrit-Owner: Subrata Banik <subratabanik(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: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Subrata Banik <subi.banik(a)gmail.com> Gerrit-Attention: Subrata Banik <subi.banik(a)gmail.com> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Tue, 05 Apr 2022 09:57:52 +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: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: dmi.c: Ensure g_has_dmi_support is default on shutdown
by Angel Pons (Code Review) 05 Apr '22

05 Apr '22

Attention is currently required from: Edward O'Callaghan. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62996 ) Change subject: dmi.c: Ensure g_has_dmi_support is default on shutdown ...................................................................... Patch Set 1: Code-Review+1 (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/62996/comment/c01daaf3_f2da4453 PS1, Line 9: the g_has_dmi_support nit: the g_has_dmi_support *variable* -- To view, visit https://review.coreboot.org/c/flashrom/+/62996 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I0674950304736e53d014117d287682a4f6349879 Gerrit-Change-Number: 62996 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Comment-Date: Tue, 05 Apr 2022 07:01:51 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: dmi.c: Hide has_dmi_support global behind method
by Angel Pons (Code Review) 05 Apr '22

05 Apr '22

Attention is currently required from: Edward O'Callaghan. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59283 ) Change subject: dmi.c: Hide has_dmi_support global behind method ...................................................................... Patch Set 4: Code-Review+1 -- To view, visit https://review.coreboot.org/c/flashrom/+/59283 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ibded9714998ea6f2e5d4e0512fa7c6b105f9638a Gerrit-Change-Number: 59283 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: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Sam McNally <sammc(a)google.com> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Comment-Date: Tue, 05 Apr 2022 07:00:21 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: tests: assert pathname and flags when calling open()
by Anastasia Klimchuk (Code Review) 05 Apr '22

05 Apr '22

Attention is currently required from: Sam McNally, Daniel Campello. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/63227 ) Change subject: tests: assert pathname and flags when calling open() ...................................................................... Patch Set 9: (5 comments) Patchset: PS9: Few small things, this is hopefully the last iteration :) File tests/io_mock.h: https://review.coreboot.org/c/flashrom/+/63227/comment/0392953b_a1bfa89e PS9, Line 118: /* An alternative to custom open mock. A test can either register its : * own mock open function or fallback_open_state. */ The text of the comment is alright, but formatting needs to be adjusted. We have coding style: https://www.flashrom.org/Development_Guidelines#Coding_style which is linux coding style except of line length limits. So multi-line comment format is: /* * An alternative to custom open mock. A test can either register its * own mock open function or fallback_open_state. */ Please check there are no trailing whitespaces, they often sneak in, especially at the end of first line /* https://review.coreboot.org/c/flashrom/+/63227/comment/05999e8d_f13ea114 PS9, Line 120: struct io_mock_fallback_open_state *fallback_open_state; Just to double-check, did you choose longer names deliberately? I was suggesting struct fallback_open_state *fallback_open I was trying to come up with not-so-long names for struct type and field. You will be changing comments format... so maybe you can rename to shorter names? Unless you know there is a reason not to? Once again, my suggestion: struct fallback_open_state *fallback_open File tests/io_mock.c: https://review.coreboot.org/c/flashrom/+/63227/comment/c6ba4a7c_1746187a PS9, Line 24: /* A test can either register its own mock open function or : * fallback_open_state. */ This can be one-line comment. But if you prefer to keep it multi-line, please also align with the coding style (see comment above). https://review.coreboot.org/c/flashrom/+/63227/comment/be2c93f2_71edd47d PS9, Line 26: assert_true(io == NULL || io->open == NULL || : io->fallback_open_state == NULL); This can be one line, as it was before, please return back :) Coding style allows longer line than 80 chars if it's not too much longer and helps readability. In the case I think it is. -- To view, visit https://review.coreboot.org/c/flashrom/+/63227 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ib46ca5b854c8453ec02ae09f3151cd4d25f988eb Gerrit-Change-Number: 63227 Gerrit-PatchSet: 9 Gerrit-Owner: Daniel Campello <campello(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Sam McNally <sammc(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Sam McNally <sammc(a)google.com> Gerrit-Attention: Daniel Campello <campello(a)chromium.org> Gerrit-Comment-Date: Tue, 05 Apr 2022 06:50:07 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: .gitignore: Avoid committing libflashrom.a
by Edward O'Callaghan (Code Review) 05 Apr '22

05 Apr '22

Edward O'Callaghan has abandoned this change. ( https://review.coreboot.org/c/flashrom/+/63320 ) Change subject: .gitignore: Avoid committing libflashrom.a ...................................................................... Abandoned -- To view, visit https://review.coreboot.org/c/flashrom/+/63320 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie9ca7c6ab846d0b07c16798cdd3ff5aa0a748a9b Gerrit-Change-Number: 63320 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: abandon

1 0

Change in flashrom[master]: gitignore: add libflashrom.a
by Edward O'Callaghan (Code Review) 05 Apr '22

05 Apr '22

Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/62826 ) Change subject: gitignore: add libflashrom.a ...................................................................... gitignore: add libflashrom.a Change-Id: I9258a1710780df5c6a7c864de246ebbe850d60fe Signed-off-by: Thomas Heijligen <thomas.heijligen(a)secunet.com> Reviewed-on: https://review.coreboot.org/c/flashrom/+/62826 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Felix Singer <felixsinger(a)posteo.net> Reviewed-by: Angel Pons <th3fanbus(a)gmail.com> Reviewed-by: Edward O'Callaghan <quasisec(a)chromium.org> --- M .gitignore 1 file changed, 1 insertion(+), 0 deletions(-) Approvals: build bot (Jenkins): Verified Felix Singer: Looks good to me, approved Angel Pons: Looks good to me, approved Edward O'Callaghan: Looks good to me, approved diff --git a/.gitignore b/.gitignore index 7c45aba..d47cb9e 100644 --- a/.gitignore +++ b/.gitignore @@ -9,6 +9,7 @@ /flashrom-* /flashrom.exe /flashrom.8 +/libflashrom.a /libflashrom-doc/ /util/ich_descriptors_tool/ich_descriptors_tool -- To view, visit https://review.coreboot.org/c/flashrom/+/62826 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I9258a1710780df5c6a7c864de246ebbe850d60fe Gerrit-Change-Number: 62826 Gerrit-PatchSet: 2 Gerrit-Owner: Thomas Heijligen <src(a)posteo.de> 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-MessageType: merged

1 0

Change in flashrom[master]: gitignore: add libflashrom.a
by Edward O'Callaghan (Code Review) 05 Apr '22

05 Apr '22

Attention is currently required from: Nico Huber, Thomas Heijligen. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62826 ) Change subject: gitignore: add libflashrom.a ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/62826 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I9258a1710780df5c6a7c864de246ebbe850d60fe Gerrit-Change-Number: 62826 Gerrit-PatchSet: 1 Gerrit-Owner: Thomas Heijligen <src(a)posteo.de> 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-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Comment-Date: Tue, 05 Apr 2022 06:16:05 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

2025

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit April 2022