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

Change in flashrom[master]: stlinkv3_spi.c: Refactor singleton states into reentrant pattern
by Nico Huber (Code Review) 11 May '21

11 May '21

Attention is currently required from: Edward O'Callaghan, Angel Pons, Anastasia Klimchuk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/54042 ) Change subject: stlinkv3_spi.c: Refactor singleton states into reentrant pattern ...................................................................... Patch Set 1: (1 comment) File stlinkv3_spi.c: https://review.coreboot.org/c/flashrom/+/54042/comment/1e755dd2_49382243 PS1, Line 533: stlinkv3_spi_open(sck_freq_kHz, stlinkv3_handle > +1 How is this not actionable? Before this commit, the handle was closed unless register_shutdown() failed. After this commit, the handle is closed unless calloc() failed or register_shutdown() failed. A nit, but still a regression. And blaming the existing code for it seems odd. -- To view, visit https://review.coreboot.org/c/flashrom/+/54042 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Id044661b864b506028720ea809bc524f0640469f Gerrit-Change-Number: 54042 Gerrit-PatchSet: 1 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: 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: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Tue, 11 May 2021 15:28:12 +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: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: dediprog: Add 4BA support on SF600 protocol version 3
by Patrick Rudolph (Code Review) 11 May '21

11 May '21

Hello Patrick Rudolph, I'd like you to do a code review. Please visit https://review.coreboot.org/c/flashrom/+/44811 to review the following change. Change subject: dediprog: Add 4BA support on SF600 protocol version 3 ...................................................................... dediprog: Add 4BA support on SF600 protocol version 3 Tested on SF600 protocol 3 V:7.2.45 Fixes the error message: "4-byte address requested but master can't handle 4-byte addresses." Change-Id: I2d91f940eb246b928a9d386eefb4195f9ccf1bb5 Signed-off-by: Patrick Rudolph <patrick.rudolph(a)9elements.com> --- M dediprog.c 1 file changed, 1 insertion(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/11/44811/1 diff --git a/dediprog.c b/dediprog.c index b847db4..6e3f098 100644 --- a/dediprog.c +++ b/dediprog.c @@ -1272,7 +1272,7 @@ (dediprog_devicetype == DEV_SF600 && protocol() == PROTOCOL_V3)) spi_master_dediprog.features &= ~SPI_MASTER_NO_4BA_MODES; - if (protocol() == PROTOCOL_V2) + if (protocol() >= PROTOCOL_V2) spi_master_dediprog.features |= SPI_MASTER_4BA; if (register_spi_master(&spi_master_dediprog) || dediprog_set_leds(LED_NONE)) -- To view, visit https://review.coreboot.org/c/flashrom/+/44811 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I2d91f940eb246b928a9d386eefb4195f9ccf1bb5 Gerrit-Change-Number: 44811 Gerrit-PatchSet: 1 Gerrit-Owner: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-MessageType: newchange

4 3

Change in flashrom[master]: dediprog: Enable 4BA for all protocols
by Patrick Rudolph (Code Review) 11 May '21

11 May '21

Hello Patrick Rudolph, I'd like you to do a code review. Please visit https://review.coreboot.org/c/flashrom/+/44776 to review the following change. Change subject: dediprog: Enable 4BA for all protocols ...................................................................... dediprog: Enable 4BA for all protocols * Tested on SF600 with protocol version 3 with "W25Q256.W" (32768 kB) * Tested on SF100 with protocol version 2 with "W25Q256.W" (32768 kB) As it works with all protocol version, assume that the SF200 works fine as well. Change-Id: Iee0ba972245b9317ef86345432fec5fc32614888 Signed-off-by: Patrick Rudolph <patrick.rudolph(a)9elements.com> --- M dediprog.c 1 file changed, 1 insertion(+), 7 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/76/44776/1 diff --git a/dediprog.c b/dediprog.c index 827d5e4..13365cd 100644 --- a/dediprog.c +++ b/dediprog.c @@ -993,7 +993,7 @@ } static struct spi_master spi_master_dediprog = { - .features = SPI_MASTER_NO_4BA_MODES, + .features = SPI_MASTER_4BA, .max_data_read = 16, /* 18 seems to work fine as well, but 19 times out sometimes with FW 5.15. */ .max_data_write = 16, .command = dediprog_spi_send_command, @@ -1268,12 +1268,6 @@ if (dediprog_standalone_mode()) return 1; - if (dediprog_devicetype == DEV_SF100 && protocol() == PROTOCOL_V1) - spi_master_dediprog.features &= ~SPI_MASTER_NO_4BA_MODES; - - if (protocol() == PROTOCOL_V2) - spi_master_dediprog.features |= SPI_MASTER_4BA; - if (register_spi_master(&spi_master_dediprog) || dediprog_set_leds(LED_NONE)) return 1; -- To view, visit https://review.coreboot.org/c/flashrom/+/44776 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Iee0ba972245b9317ef86345432fec5fc32614888 Gerrit-Change-Number: 44776 Gerrit-PatchSet: 1 Gerrit-Owner: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-MessageType: newchange

3 3

Change in flashrom[master]: usbblaster_spi.c: Refactor singleton states into reentrant pattern
by Angel Pons (Code Review) 11 May '21

11 May '21

Attention is currently required from: Nico Huber, Anastasia Klimchuk. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/54044 ) Change subject: usbblaster_spi.c: Refactor singleton states into reentrant pattern ...................................................................... Patch Set 1: Code-Review+2 (3 comments) File usbblaster_spi.c: https://review.coreboot.org/c/flashrom/+/54044/comment/d203e14c_eb4938d8 PS1, Line 55: struct ftdi_context ftdic; Having a struct with only one member looks odd, but it makes sense for consistency with other programmers. (No action needed) https://review.coreboot.org/c/flashrom/+/54044/comment/9275999f_04022c25 PS1, Line 165: static int usbblaster_shutdown(void *data) : { : free(data); : return 0; : } > Seems fine to me. […] Makes sense. This patch changes programmer context storage from a global variable to dynamically-allocated memory, and one needs to free() this memory to avoid memory leaks. Adding a shutdown function to do this is a very reasonable thing to do. https://review.coreboot.org/c/flashrom/+/54044/comment/c6a69f1c_77219d65 PS1, Line 185: struct ftdi_context ftdic; We could allocate `usbblaster_data` early and use `usbblaster_data->ftdic` instead of having a second local variable. Can be done in a separate patch. -- To view, visit https://review.coreboot.org/c/flashrom/+/54044 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ia81f9f40c7eab430a8b304d0b197ce7c75bf5ace Gerrit-Change-Number: 54044 Gerrit-PatchSet: 1 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: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Tue, 11 May 2021 07:07:01 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes 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]: stlinkv3_spi.c: Drop stlinkv3_ prefix for spi data struct member
by Miklós Márton (Code Review) 11 May '21

11 May '21

Attention is currently required from: Nico Huber, Anastasia Klimchuk. Miklós Márton has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/54043 ) Change subject: stlinkv3_spi.c: Drop stlinkv3_ prefix for spi data struct member ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/54043 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ibddac9371ab8f08276d499642a9bdd6dbecea0ca Gerrit-Change-Number: 54043 Gerrit-PatchSet: 1 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: 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: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Tue, 11 May 2021 07:06:06 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: stlinkv3_spi.c: Refactor singleton states into reentrant pattern
by Miklós Márton (Code Review) 11 May '21

11 May '21

Attention is currently required from: Nico Huber, Anastasia Klimchuk. Miklós Márton has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/54042 ) Change subject: stlinkv3_spi.c: Refactor singleton states into reentrant pattern ...................................................................... Patch Set 1: Code-Review+2 (1 comment) Patchset: PS1: LGTM -- To view, visit https://review.coreboot.org/c/flashrom/+/54042 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Id044661b864b506028720ea809bc524f0640469f Gerrit-Change-Number: 54042 Gerrit-PatchSet: 1 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: 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: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Tue, 11 May 2021 07:05:37 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: stlinkv3_spi.c: Refactor singleton states into reentrant pattern
by Angel Pons (Code Review) 10 May '21

10 May '21

Attention is currently required from: Nico Huber, Miklós Márton, Anastasia Klimchuk. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/54042 ) Change subject: stlinkv3_spi.c: Refactor singleton states into reentrant pattern ...................................................................... Patch Set 1: Code-Review+2 (1 comment) File stlinkv3_spi.c: https://review.coreboot.org/c/flashrom/+/54042/comment/a64d5993_c6d4bd5d PS1, Line 533: stlinkv3_spi_open(sck_freq_kHz, stlinkv3_handle > Yes :\ on my observation, half of init error paths in the tree are not doing proper cleanup. […] +1 -- To view, visit https://review.coreboot.org/c/flashrom/+/54042 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Id044661b864b506028720ea809bc524f0640469f Gerrit-Change-Number: 54042 Gerrit-PatchSet: 1 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: 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: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Tue, 11 May 2021 06:46:35 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes 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]: stlinkv3_spi.c: Drop stlinkv3_ prefix for spi data struct member
by Angel Pons (Code Review) 10 May '21

10 May '21

Attention is currently required from: Nico Huber, Anastasia Klimchuk. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/54043 ) Change subject: stlinkv3_spi.c: Drop stlinkv3_ prefix for spi data struct member ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/54043 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ibddac9371ab8f08276d499642a9bdd6dbecea0ca Gerrit-Change-Number: 54043 Gerrit-PatchSet: 1 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: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Tue, 11 May 2021 06:44:10 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: tests: Add unit test to run init/shutdown for mec1308.c, ene_lpc.c
by Anastasia Klimchuk (Code Review) 10 May '21

10 May '21

Attention is currently required from: Nico Huber, Angel Pons, Simon Glass. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/51487 ) Change subject: tests: Add unit test to run init/shutdown for mec1308.c, ene_lpc.c ...................................................................... Patch Set 14: (4 comments) Patchset: PS13: > another option is just to submit it. […] thank you! :) PS13: > I rebased this on the top of io_mock.h. […] update: #3 done. For #1 and #2 I would do them separately after this one, but tell me what you all think. File tests/io_mock.h: https://review.coreboot.org/c/flashrom/+/51487/comment/c23930c3_c0306274 PS13, Line 37: unsigned char > I took these types from a Linux manual page, is there something […] I took these types from hwaccess_x86_io.h because I thought, these are mocks for outx/inx and the mocks should have the same signatures as real functions. Do you know why hwaccess has different types than Linux manual page? Maybe I am looking in the wrong place? But technically, we are mocking functions in hwaccess? File tests/tests.c: https://review.coreboot.org/c/flashrom/+/51487/comment/77e9edcf_0e98bf38 PS13, Line 26: static struct io_mock *current_io = NULL; > This could just be `const`, I guess, as there is no intention […] oh yes thank you... warning gone! -- To view, visit https://review.coreboot.org/c/flashrom/+/51487 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I3af612defe1af3850dfc1626a208d873e3a3eddc Gerrit-Change-Number: 51487 Gerrit-PatchSet: 14 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: Simon Glass <sjg(a)chromium.org> 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: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Simon Glass <sjg(a)chromium.org> Gerrit-Comment-Date: Tue, 11 May 2021 04:26:19 +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: Simon Glass <sjg(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: tests: Add unit test to run init/shutdown for mec1308.c, ene_lpc.c
by Anastasia Klimchuk (Code Review) 10 May '21

10 May '21

Attention is currently required from: Angel Pons, Anastasia Klimchuk. Hello build bot (Jenkins), Nico Huber, Edward O'Callaghan, Angel Pons, Simon Glass, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/51487 to look at the new patch set (#14). Change subject: tests: Add unit test to run init/shutdown for mec1308.c, ene_lpc.c ...................................................................... tests: Add unit test to run init/shutdown for mec1308.c, ene_lpc.c This patch includes mocks for io operations in hwaccess_x86_io.h because those are needed to test lifecycle of mec1308.c and ene_lpc.c BUG=b:181803212 TEST=builds and ninja test Change-Id: I3af612defe1af3850dfc1626a208d873e3a3eddc Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org> --- A hwaccess_x86_io_unittest.h M meson.build M tests/init_shutdown.c M tests/io_mock.h M tests/meson.build M tests/tests.c M tests/tests.h 7 files changed, 249 insertions(+), 7 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/87/51487/14 -- To view, visit https://review.coreboot.org/c/flashrom/+/51487 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I3af612defe1af3850dfc1626a208d873e3a3eddc Gerrit-Change-Number: 51487 Gerrit-PatchSet: 14 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: Simon Glass <sjg(a)chromium.org> 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

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit May 2021