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

Change in flashrom[master]: ni845x_spi: add missing const specifier to the spi_transmit function
by Angel Pons (Code Review) 18 Aug '21

18 Aug '21

Attention is currently required from: Nico Huber, Miklós Márton, Edward O'Callaghan, Anastasia Klimchuk. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/57003 ) Change subject: ni845x_spi: add missing const specifier to the spi_transmit function ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: > Many thanks for the quick review! > > Well the internal API seems to be changed since the ni845x_spi introduction and I have not built flashrom on Windows since then. As we discussed before it might be useful to create a mock C file for the ni845x.h library header to being able to perfrom least compilation tests (without Windows, mingw, and installed NI drivers). I actually started to work on this yesterday. Speaking of mocks, this gives me an idea: how about making something that not only serves to build-test the code, but also allows running cmocka tests on ni845x? -- To view, visit https://review.coreboot.org/c/flashrom/+/57003 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I2c14361283b2da3725d9ba2cae0e1007e6be3d66 Gerrit-Change-Number: 57003 Gerrit-PatchSet: 1 Gerrit-Owner: Miklós Márton <martonmiklosqdev(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-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Wed, 18 Aug 2021 06:59:08 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Miklós Márton <martonmiklosqdev(a)gmail.com> Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ni845x_spi: add missing const specifier to the spi_transmit function
by Miklós Márton (Code Review) 18 Aug '21

18 Aug '21

Attention is currently required from: Nico Huber, Edward O'Callaghan, Anastasia Klimchuk. Miklós Márton has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/57003 ) Change subject: ni845x_spi: add missing const specifier to the spi_transmit function ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: > I see, the function pointer declaration in `struct spi_master` has `const struct flashctx *flash`. Many thanks for the quick review! Well the internal API seems to be changed since the ni845x_spi introduction and I have not built flashrom on Windows since then. As we discussed before it might be useful to create a mock C file for the ni845x.h library header to being able to perfrom least compilation tests (without Windows, mingw, and installed NI drivers). I actually started to work on this yesterday. -- To view, visit https://review.coreboot.org/c/flashrom/+/57003 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I2c14361283b2da3725d9ba2cae0e1007e6be3d66 Gerrit-Change-Number: 57003 Gerrit-PatchSet: 1 Gerrit-Owner: Miklós Márton <martonmiklosqdev(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-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Wed, 18 Aug 2021 06:24:27 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ni845x_spi: add missing const specifier to the spi_transmit function
by Angel Pons (Code Review) 18 Aug '21

18 Aug '21

Attention is currently required from: Nico Huber, Miklós Márton, Edward O'Callaghan, Anastasia Klimchuk. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/57003 ) Change subject: ni845x_spi: add missing const specifier to the spi_transmit function ...................................................................... Patch Set 1: Code-Review+2 (1 comment) Patchset: PS1: I see, the function pointer declaration in `struct spi_master` has `const struct flashctx *flash`. -- To view, visit https://review.coreboot.org/c/flashrom/+/57003 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I2c14361283b2da3725d9ba2cae0e1007e6be3d66 Gerrit-Change-Number: 57003 Gerrit-PatchSet: 1 Gerrit-Owner: Miklós Márton <martonmiklosqdev(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-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Wed, 18 Aug 2021 06:07:15 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: opaque_master: Use new API to register shutdown function
by Anastasia Klimchuk (Code Review) 18 Aug '21

18 Aug '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/+/56825 ) Change subject: opaque_master: Use new API to register shutdown function ...................................................................... Patch Set 1: (1 comment) File linux_mtd.c: https://review.coreboot.org/c/flashrom/+/56825/comment/0c5fdf33_bf4b5a85 PS1, Line 421: if (register_shutdown(linux_mtd_shutdown, (void *)data)) { > Looks like this is also fixing leakage of a file descriptor. Yes right... I will update commit message. And I plan to test linux_mtd on device anyway, so marking this unresolved. -- To view, visit https://review.coreboot.org/c/flashrom/+/56825 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Id8471a117556edcbf9694752fabe05cf4501ce70 Gerrit-Change-Number: 56825 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: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Wed, 18 Aug 2021 02:16:56 +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]: ni845x_spi: Fix signed - unsigned comparisons
by Anastasia Klimchuk (Code Review) 18 Aug '21

18 Aug '21

Attention is currently required from: Nico Huber, Miklós Márton, Angel Pons. 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 3: Code-Review+1 -- 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: 3 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-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Wed, 18 Aug 2021 02:10:29 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

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

17 Aug '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 7: (4 comments) Patchset: PS6: > This also fixes a lot resource leakage in case register_shutdown() […] Yes that's a very good thing, I can boast in commit message, thanks :) Programmers which have tests and affected here: linux_spi, dediprog, mec1308 and ene_lpc (since they are not removed yet), also realtek has test which is under review CB:56911 but I verified it passes on the top of this patch. I have test which makes register_shutdown to fail, it is buried somewhere in my local repo, I never sent it. I can return back to it given that CB:56753 works. Totally makes sense to test error paths. Although it is very likely to take longer [to write proper test for error paths] than it seems to me now. Patchset: PS7: I rebased and updated commit message, and lost all the votes. File ch341a_spi.c: https://review.coreboot.org/c/flashrom/+/56103/comment/2ea2c6cd_072b87d7 PS6, Line 511: return 0; > Just raising an eyebrow: this changes unrelated behaviour. Maybe worth to […] This is right! Added to commit message. File dediprog.c: https://review.coreboot.org/c/flashrom/+/56103/comment/fa345d0b_fd2a3d71 PS6, Line 1276: /* shutdown function does cleanup */ > Still somewhat true, but less obvious what is meant. This is not a final state for dediprog, I want it to return register_spi_master() like the others. But I recalled there is CB:51706, I think we need to discuss and decide with the author of that patch what to do. -- 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: 7 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: Tue, 17 Aug 2021 23:55:58 +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]: spi_master: Use new API to register shutdown function
by Anastasia Klimchuk (Code Review) 17 Aug '21

17 Aug '21

Attention is currently required from: Thomas Heijligen, Angel Pons, Anastasia Klimchuk. Hello build bot (Jenkins), Nico Huber, Miklós Márton, Thomas Heijligen, Edward O'Callaghan, Angel Pons, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/56103 to look at the new patch set (#7). Change subject: spi_master: Use new API to register shutdown function ...................................................................... spi_master: Use new API to register shutdown function This allows spi masters to register shutdown function in spi_master struct, which means there is no need to call register_shutdown in init function, since this call is now a part of register_spi_master. As a consequence of using new API, two things are happening here: 1) No resource leakage anymore in case register_shutdown() would fail, 2) Fixed propagation of register_spi_master() return values. Basic testing: when I comment out free(data) in linux_spi_shutdown, test fails with error ../linux_spi.c:235: note: block 0x55a4db276510 allocated here ERROR: linux_spi_init_and_shutdown_test_success leaked 1 block(s) Means, shutdown function is invoked. BUG=b:185191942 TEST= 1) builds and ninja test including CB:56911 2) On ARMv7 device flashrom -p linux_spi -V -> using linux_spi, chip found 3) On x86_64 AMD device flashrom -p internal -V -> this is actually using sb600spi, chip found Change-Id: Ib60300f9ddb295a255d5ef3f8da0e07064207140 Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org> --- M bitbang_spi.c M buspirate_spi.c M ch341a_spi.c M dediprog.c M digilent_spi.c M ene_lpc.c M ft2232_spi.c M it85spi.c M it87spi.c M jlink_spi.c M linux_spi.c M lspcon_i2c_spi.c M mec1308.c M mstarddc_spi.c M ni845x_spi.c M pickit2_spi.c M realtek_mst_i2c_spi.c M sb600spi.c M stlinkv3_spi.c M usbblaster_spi.c M wbsio_spi.c 21 files changed, 40 insertions(+), 112 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/03/56103/7 -- 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: 7 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: Thomas Heijligen <src(a)posteo.de> 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]: ni845x_spi: add missing const specifier to the spi_transmit function
by Miklós Márton (Code Review) 17 Aug '21

17 Aug '21

Miklós Márton has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/57003 ) Change subject: ni845x_spi: add missing const specifier to the spi_transmit function ...................................................................... ni845x_spi: add missing const specifier to the spi_transmit function Change-Id: I2c14361283b2da3725d9ba2cae0e1007e6be3d66 Signed-off-by: Miklós Márton <martonmiklosqdev(a)gmail.com> --- M ni845x_spi.c 1 file changed, 1 insertion(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/03/57003/1 diff --git a/ni845x_spi.c b/ni845x_spi.c index 2153510..9d09f30 100644 --- a/ni845x_spi.c +++ b/ni845x_spi.c @@ -480,7 +480,7 @@ return 0; } -static int ni845x_spi_transmit(struct flashctx *flash, +static int ni845x_spi_transmit(const struct flashctx *flash, unsigned int write_cnt, unsigned int read_cnt, const unsigned char *write_arr, -- To view, visit https://review.coreboot.org/c/flashrom/+/57003 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I2c14361283b2da3725d9ba2cae0e1007e6be3d66 Gerrit-Change-Number: 57003 Gerrit-PatchSet: 1 Gerrit-Owner: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: ni845x_spi: Fix signed - unsigned comparisons
by Miklós Márton (Code Review) 17 Aug '21

17 Aug '21

Attention is currently required from: Nico Huber, Angel Pons, Anastasia Klimchuk. Miklós Márton has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/56637 ) Change subject: ni845x_spi: Fix signed - unsigned comparisons ...................................................................... Patch Set 3: (5 comments) File ni845x_spi.c: https://review.coreboot.org/c/flashrom/+/56637/comment/9f31cd3a_30b28bd7 PS1, Line 188: strtol > +1 Done https://review.coreboot.org/c/flashrom/+/56637/comment/453d7f54_d7742601 PS1, Line 483: const > Unrelated change? Yepp, submitting it an another PR. https://review.coreboot.org/c/flashrom/+/56637/comment/f9d763d1_59860a5f PS1, Line 556: CS_number = CS_str[0] - '0'; > Interesting, thank you very much! Done https://review.coreboot.org/c/flashrom/+/56637/comment/6ebc5c05_4a3db031 PS1, Line 558: strlen(CS_str) > Note that the alternative approach proposed below avoids the use-after-free bug. Done https://review.coreboot.org/c/flashrom/+/56637/comment/562322e8_be858425 PS1, Line 556: CS_number = CS_str[0] - '0'; : free(CS_str); : if (strlen(CS_str) > 1 || 7 < CS_number) { : msg_perr("Only CS 0-7 supported\n"); : return 1; : } > It's simpler, sounds good. Done -- 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: 3 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: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Tue, 17 Aug 2021 20:50:33 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> 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: Fix signed - unsigned comparisons
by Miklós Márton (Code Review) 17 Aug '21

17 Aug '21

Attention is currently required from: Nico Huber, Miklós Márton, Anastasia Klimchuk. Hello build bot (Jenkins), Nico Huber, Anastasia Klimchuk, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/56637 to look at the new patch set (#3). Change subject: ni845x_spi: Fix signed - unsigned comparisons ...................................................................... ni845x_spi: Fix signed - unsigned comparisons Change-Id: I48ef927aa28433fb0e3b3a1f3fb2e797095e53bd Signed-off-by: Miklós Márton <martonmiklosqdev(a)gmail.com> --- M ni845x_spi.c 1 file changed, 4 insertions(+), 4 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/37/56637/3 -- 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: 3 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-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit August 2021