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

Change in flashrom[master]: spi_master: Use new API to register shutdown function
by Miklós Márton (Code Review) 28 Jul '21

28 Jul '21

Attention is currently required from: Nico Huber, Thomas Heijligen, Angel Pons, Anastasia Klimchuk. Miklós Márton has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/56103 ) Change subject: spi_master: Use new API to register shutdown function ...................................................................... Patch Set 4: (1 comment) Patchset: PS1: > Miklós thanks heaps!! I assume since you +2 that means testing successful? (so I resolve this commen […] Yes I managed to build and test the ni845x_spi with this patch (with compiling with WARNERROR=no due to the signed unsigned comaprison warnings in the ni845x_spi code but thats an another story). I have no objections on the merge from the ni845x_spi side. -- To view, visit https://review.coreboot.org/c/flashrom/+/56103 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ib60300f9ddb295a255d5ef3f8da0e07064207140 Gerrit-Change-Number: 56103 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: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> 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: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Wed, 28 Jul 2021 08:43:20 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Miklós Márton <martonmiklosqdev(a)gmail.com> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: spi_master: Use new API to register shutdown function
by Anastasia Klimchuk (Code Review) 28 Jul '21

28 Jul '21

Attention is currently required from: Nico Huber, Thomas Heijligen, Angel Pons. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/56103 ) Change subject: spi_master: Use new API to register shutdown function ...................................................................... Patch Set 4: (2 comments) Patchset: PS1: > Hello Anastasia, […] Miklós thanks heaps!! I assume since you +2 that means testing successful? (so I resolve this comment) Patchset: PS4: I rebased this one and previous patches in the chain, so it is nice and ready :) This would not happen without all your help! -- To view, visit https://review.coreboot.org/c/flashrom/+/56103 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ib60300f9ddb295a255d5ef3f8da0e07064207140 Gerrit-Change-Number: 56103 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: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> 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: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Wed, 28 Jul 2021 03:50:40 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Miklós Márton <martonmiklosqdev(a)gmail.com> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: tests: Add tests to erase a chip
by Anastasia Klimchuk (Code Review) 28 Jul '21

28 Jul '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/+/56501 ) Change subject: tests: Add tests to erase a chip ...................................................................... Patch Set 5: (6 comments) Patchset: PS2: > > The newest test that I have added uses real spi_send_command, however some previous tests are mock […] Yes, I remember that when function is wrapped, the real one is meant to be available as __real_function. Which would work if the function is explicitly called in test code, but in this case spi_send_command is called somewhere deep inside in the middle of do_erase operation, the test doesn't know when and how (and shouldn't know). I did spend some time few months ago exploring wrap vs real situation, when I was writing my first tests. I never sent those very first tests though, because they were wrapping register_shutdown, and that was a conflict with init-shutdown tests that do real shutdown. When I was trying to alternate between wrap and mock in different tests (in the same executable), the code was quickly getting convoluted, but I couldn't make it work anyway :\ (searching the internet didn't help) I decided init-shutdown tests were more useful and chose them. That was few months ago, when I was younger and less experienced (especially the latter), so maybe now I could make it work... but the code would have to be convoluted, I am worried about it a bit. I am marking this comment unresolved, because there is a TODO for this patch one way or the other. Myself leaning to have two executables, but please tell me if you strongly disagree! Patchset: PS4: > Definitely split this like you suggested but the first test looks great now! thanks! File tests/chip.c: https://review.coreboot.org/c/flashrom/+/56501/comment/d5062766_b7fd379b PS4, Line 25: int > `unsigned` unless negative unlocks encode something special? Done https://review.coreboot.org/c/flashrom/+/56501/comment/fe0f1521_98143b4f PS4, Line 52: memcpy(&g_chip_state.buf[start], buf, len); > validate len does not exceed the actual buffer length. […] Done https://review.coreboot.org/c/flashrom/+/56501/comment/cb0eca80_5dd8a180 PS4, Line 59: g_chip_state.unlock_calls++; : return 0; > this does depend on the chip I suppose but another possible behavior is the following: […] I added this check, it makes sense to me. For the existing test, unlock only called once (we know because it is logged). If at some point in future this becomes an issue, we can have a look later then. https://review.coreboot.org/c/flashrom/+/56501/comment/499a6fc9_f7449f95 PS4, Line 71: memset(&g_chip_state.buf[blockaddr], 0xff, blocklen); > ditto, ensure blocklen does not exceed the global buffer size. Done -- To view, visit https://review.coreboot.org/c/flashrom/+/56501 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I6f74bfe4e02244d24d6c837cc3d551251e7b4898 Gerrit-Change-Number: 56501 Gerrit-PatchSet: 5 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-Comment-Date: Wed, 28 Jul 2021 03:20:55 +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]: ene_lpc: remove ENE LPC programmer
by Edward O'Callaghan (Code Review) 28 Jul '21

28 Jul '21

Attention is currently required from: Mike Banon, Nico Huber, Victor Ding, Angel Pons, Anastasia Klimchuk. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/56475 ) Change subject: ene_lpc: remove ENE LPC programmer ...................................................................... Patch Set 2: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/56475 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I3f40db22c42c04ce029c4defd837e05ebb550c9b Gerrit-Change-Number: 56475 Gerrit-PatchSet: 2 Gerrit-Owner: Victor Ding <victording(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: Mike Banon <mikebdp2(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: Mike Banon <mikebdp2(a)gmail.com> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Victor Ding <victording(a)google.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Wed, 28 Jul 2021 03:19:19 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: tests: Add tests to erase a chip
by Anastasia Klimchuk (Code Review) 28 Jul '21

28 Jul '21

Attention is currently required from: Angel Pons, Anastasia Klimchuk. 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/+/56501 to look at the new patch set (#5). Change subject: tests: Add tests to erase a chip ...................................................................... tests: Add tests to erase a chip Two tests cover the code which parforms do_erase operation. First one works with fake chip and dummy programmer. Fake chip has all operations defined, they all log and return 0. Read operation always initialises buffer with one and the same value. Second one uses the chip which is closer to the real one, because read/write/unlock/erase operations are real. The tests takes the advantage of dummyflasher's capability of emulating a W25Q128.V chip. BUG=b:181803212 TEST=builds and ninja test Change-Id: I6f74bfe4e02244d24d6c837cc3d551251e7b4898 Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org> --- M meson.build A tests/chip.c M tests/meson.build M tests/tests.c M tests/tests.h A tests/wrap_send_tests.c A tests/wrap_send_tests.h 7 files changed, 346 insertions(+), 28 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/01/56501/5 -- To view, visit https://review.coreboot.org/c/flashrom/+/56501 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I6f74bfe4e02244d24d6c837cc3d551251e7b4898 Gerrit-Change-Number: 56501 Gerrit-PatchSet: 5 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: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: meson/: Add missing config for NI845_SPI drv
by Edward O'Callaghan (Code Review) 28 Jul '21

28 Jul '21

Attention is currently required from: Miklós Márton. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/54849 ) Change subject: meson/: Add missing config for NI845_SPI drv ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: > Hi folks, […] Any recent build should do? -- To view, visit https://review.coreboot.org/c/flashrom/+/54849 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I85387e94f1b159733d0124f1a6dfde1e3d16827f Gerrit-Change-Number: 54849 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-Comment-Date: Wed, 28 Jul 2021 02:24:27 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ni845x_spi: Fix signed - unsigned comparisons
by Anastasia Klimchuk (Code Review) 27 Jul '21

27 Jul '21

Attention is currently required from: Nico Huber, Miklós Márton. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/56637 ) Change subject: ni845x_spi: Fix signed - unsigned comparisons ...................................................................... Patch Set 1: (1 comment) File ni845x_spi.c: https://review.coreboot.org/c/flashrom/+/56637/comment/48704037_4085d45b PS1, Line 556: CS_number = CS_str[0] - '0'; This line converts string to number, but only two lines below the condition is checked whether the string is valid for conversion. -- To view, visit https://review.coreboot.org/c/flashrom/+/56637 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I48ef927aa28433fb0e3b3a1f3fb2e797095e53bd Gerrit-Change-Number: 56637 Gerrit-PatchSet: 1 Gerrit-Owner: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-Comment-Date: Tue, 27 Jul 2021 23:39:12 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: treewide: Use calloc() where applicable
by Anastasia Klimchuk (Code Review) 27 Jul '21

27 Jul '21

Attention is currently required from: Miklós Márton, Edward O'Callaghan, Angel Pons, Arthur Heymans. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55269 ) Change subject: treewide: Use calloc() where applicable ...................................................................... Patch Set 2: (1 comment) Patchset: PS2: > Thanks for reminding me of this change. I'll update it once I have time (two or three days). Thanks! I didn't mean to rush :) I am interested because I learn from this and would use the end result of this patch as an example of how to allocate memory in similar cases. -- To view, visit https://review.coreboot.org/c/flashrom/+/55269 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Id6ef0d6d02c7b63baabc9a0087275cab22a48736 Gerrit-Change-Number: 55269 Gerrit-PatchSet: 2 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Miklós Márton <martonmiklosqdev(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: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Comment-Date: Tue, 27 Jul 2021 21:44:44 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: programmer.h,chipset_enable.c: Make penable cb more descript
by Anastasia Klimchuk (Code Review) 27 Jul '21

27 Jul '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/+/52364 ) Change subject: programmer.h,chipset_enable.c: Make penable cb more descript ...................................................................... Patch Set 3: (1 comment) File programmer.h: https://review.coreboot.org/c/flashrom/+/52364/comment/b33cedc2_b385ee50 PS1, Line 228: enable_flash_xxx > Anastasia, have you seen this thread? Thank you Angel! reading it again, I thought it was about _xxx only but missed the last paragraphs, I am sorry :\ I saw that _xxx is removed from the latest version and thought that's it. Ok, I understand that "enable" is already present in the struct name "penable", so doesn't need to be repeated. Also yes for command pattern, although in other places we have commands with the name of the action, like chip has read/write/unlock, masters also have read/write_256, programmers have init etc. Maybe enable or do_enable? -- To view, visit https://review.coreboot.org/c/flashrom/+/52364 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I8b3a4ec51ea488c40c20ccafea883f8dea93577d Gerrit-Change-Number: 52364 Gerrit-PatchSet: 3 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: 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-Comment-Date: Tue, 27 Jul 2021 20:55:52 +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: Angel Pons <th3fanbus(a)gmail.com> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ni845x_spi: handle PROGRAMFILES(X86) env var properly
by Miklós Márton (Code Review) 27 Jul '21

27 Jul '21

Attention is currently required from: Nico Huber, Angel Pons. Miklós Márton has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/56636 ) Change subject: ni845x_spi: handle PROGRAMFILES(X86) env var properly ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: > Would escaping the parentheses work? Unfortunately no. bash says the following from the terminal, similarly to the makefile approach: mm@DESKTOP-4AITTRD /c/Users/mm/Documents/flashrom $ echo ${PROGRAMFILES$X86$} sh: ${PROGRAMFILES$X86$}: bad substitution I have not managed to find a better way to get this variable, others faced with this too in the past: https://stackoverflow.com/questions/53563543/bash-shell-access-to-programfi… -- To view, visit https://review.coreboot.org/c/flashrom/+/56636 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I397619a5038567d649a417ce6b9d8ac9e1c8c67b Gerrit-Change-Number: 56636 Gerrit-PatchSet: 1 Gerrit-Owner: Miklós Márton <martonmiklosqdev(a)gmail.com> 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: Tue, 27 Jul 2021 14:09:20 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit July 2021