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

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

03 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/+/52774 ) Change subject: pickit2_spi.c: Refactor singleton states into reentrant pattern ...................................................................... Patch Set 2: Code-Review+1 (2 comments) File pickit2_spi.c: https://review.coreboot.org/c/flashrom/+/52774/comment/4b51141e_2977d293 PS1, Line 57: }; > Oh I am so happy that you asked! We haven't discussed this aspect in details, which I was slightly w […] I see what you are getting at. Still, in theory, a struct with a single member is storage-wise the same as that member without a struct. If you'd let the framework allocate a pointer (instead of a struct with only a pointer within) it wouldn't make a difference. `data` would then be of type `libusb_device_handle **`. A pointer to a struct is nicer than that, so struct seems good! https://review.coreboot.org/c/flashrom/+/52774/comment/2bf8b7dd_2b612051 PS1, Line 343: static struct spi_master spi_master_pickit2 = { > >>> The per-instance `data` pointer should simply be passed to register_spi_master() […] Ack -- To view, visit https://review.coreboot.org/c/flashrom/+/52774 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ibacc4738bee02c371c41583d321e0337128ad18a Gerrit-Change-Number: 52774 Gerrit-PatchSet: 2 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: 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: Mon, 03 May 2021 16:07:59 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes 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]: pickit2_spi.c: Separate shutdown from failed init cleanup
by Nico Huber (Code Review) 03 May '21

03 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/+/52773 ) Change subject: pickit2_spi.c: Separate shutdown from failed init cleanup ...................................................................... Patch Set 1: (1 comment) File pickit2_spi.c: https://review.coreboot.org/c/flashrom/+/52773/comment/f84d14d9_bd9f498e PS1, Line 482: goto init_err_cleanup_exit; > At the same time, what was before: if failure happened between pickit2_get_firmware_version() and register_spi_master() then pickit2_shutdown() would be called (because at the point when pickit2_get_firmware_version() run, shutdown function has already been registered). This stays the same: all failures before pickit2_get_firmware_version() do cleanup for themselves, all failures after pickit2_get_firmware_version() rely on pickit2_shutdown(). I think you are right wrt. the state of the hardware. But software-wise, in case register_shutdown() failed, we'd have left dangling resources (e.g. libusb_exit() wasn't called). > What do you think? I can change code here if you think I should do that, no problem! The patch is fine, hence the +2. I only wanted to point our that behavioral changes usually have priority in commit messages. Generally, this can also be achieved by splitting them out into separate commits. Nothing to do here. -- To view, visit https://review.coreboot.org/c/flashrom/+/52773 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I1b672b33169a7a1b6ceab190ad3f48c2f35c3a1f Gerrit-Change-Number: 52773 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: 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: Mon, 03 May 2021 15:56:06 +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> Gerrit-MessageType: comment

1 0

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

03 May '21

Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons, Anastasia Klimchuk. Simon Glass 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 10: (2 comments) File tests/init_shutdown.c: https://review.coreboot.org/c/flashrom/+/51487/comment/cb2965ae_12d7618c PS10, Line 53: MEC1308_SIO_PORT1 > These are not enums, these are #define values in mec1308. […] Well you can move them to a header file File tests/tests.c: https://review.coreboot.org/c/flashrom/+/51487/comment/12490f1d_179d94bb PS10, Line 92: if (current_io && current_io->outb) > There are drivers where "doing nothing" mocking strategy is sufficient. […] OK, so do you mean to allow current_io to be NULL as well? -- 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: 10 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-CC: Simon Glass <sjg(a)chromium.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-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Mon, 03 May 2021 15:00:51 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No 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]: lspcon_i2c_spi: Extract I2C bus parameter handling
by Edward O'Callaghan (Code Review) 03 May '21

03 May '21

Attention is currently required from: Nico Huber, Angel Pons, Anastasia Klimchuk. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52830 ) Change subject: lspcon_i2c_spi: Extract I2C bus parameter handling ...................................................................... Patch Set 2: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/52830 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I006b311c88feea37fe4b217f769b21ca1505def9 Gerrit-Change-Number: 52830 Gerrit-PatchSet: 2 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.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: Peter Marheine <pmarheine(a)chromium.org> 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-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Mon, 03 May 2021 12:19:25 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: lspcon_i2c_spi: Extract I2C bus parameter handling
by Angel Pons (Code Review) 03 May '21

03 May '21

Attention is currently required from: Nico Huber, Edward O'Callaghan, Anastasia Klimchuk. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52830 ) Change subject: lspcon_i2c_spi: Extract I2C bus parameter handling ...................................................................... Patch Set 2: (1 comment) File i2c_helper_linux.c: https://review.coreboot.org/c/flashrom/+/52830/comment/2f4ff52d_5a0c704b PS1, Line 80: * TODO: Move this function to a common place, it is not specific to I2C */ > Just drop it all together I suppose. Gone -- To view, visit https://review.coreboot.org/c/flashrom/+/52830 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I006b311c88feea37fe4b217f769b21ca1505def9 Gerrit-Change-Number: 52830 Gerrit-PatchSet: 2 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.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: Peter Marheine <pmarheine(a)chromium.org> 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: Mon, 03 May 2021 12:09:05 +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> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: lspcon_i2c_spi: Extract I2C bus parameter handling
by Edward O'Callaghan (Code Review) 03 May '21

03 May '21

Attention is currently required from: Nico Huber, Angel Pons, Anastasia Klimchuk. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52830 ) Change subject: lspcon_i2c_spi: Extract I2C bus parameter handling ...................................................................... Patch Set 1: (1 comment) File i2c_helper_linux.c: https://review.coreboot.org/c/flashrom/+/52830/comment/a113fbe7_f0fbf72d PS1, Line 80: * TODO: Move this function to a common place, it is not specific to I2C */ > If one removes all the references to "bus" from this function, what remains is a function that parse […] Just drop it all together I suppose. -- To view, visit https://review.coreboot.org/c/flashrom/+/52830 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I006b311c88feea37fe4b217f769b21ca1505def9 Gerrit-Change-Number: 52830 Gerrit-PatchSet: 1 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.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: Peter Marheine <pmarheine(a)chromium.org> 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-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Mon, 03 May 2021 11:59: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> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: realtek_mst_i2c_spi: Add missing braces
by Edward O'Callaghan (Code Review) 03 May '21

03 May '21

Attention is currently required from: Nico Huber, Angel Pons, Anastasia Klimchuk. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52827 ) Change subject: realtek_mst_i2c_spi: Add missing braces ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/52827 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I69b762391165177857e9331f79f54b01149cf339 Gerrit-Change-Number: 52827 Gerrit-PatchSet: 1 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.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: Peter Marheine <pmarheine(a)chromium.org> 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-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Mon, 03 May 2021 11:56:47 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: programmer.h: Convert anon union to anon struct
by Edward O'Callaghan (Code Review) 03 May '21

03 May '21

Attention is currently required from: Nico Huber, Stefan Reinauer, Anastasia Klimchuk. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/50246 ) Change subject: programmer.h: Convert anon union to anon struct ...................................................................... Patch Set 4: (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/50246/comment/9f5e1e45_c1f72a38 PS4, Line 14: The user of the registered_masters type is therefore : responsible for querying the buses_supported field before : attempting to dereference a ptr field in the anon struct. > Can the user check which one out of three is not null? It's a good question. I guess I mean "user" here to mean things that call "spi_send_command()" such as in spi25.c. The actual case where this exploded was when w25q_read_status_register_2() used spi_send_command(), which requires that the master be a spi_master where as Hardware-sequenced ICH that is an opaque master was being used. Essentially the union is a bit of a trap. -- To view, visit https://review.coreboot.org/c/flashrom/+/50246 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I576967a8599b923c902e39f177f39146291cc242 Gerrit-Change-Number: 50246 Gerrit-PatchSet: 4 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Alan Green <avg(a)google.com> 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: Sam McNally <sammc(a)google.com> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Mon, 03 May 2021 09:22:17 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: realtek_mst_i2c_spi: Add missing braces
by Angel Pons (Code Review) 03 May '21

03 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/+/52827 ) Change subject: realtek_mst_i2c_spi: Add missing braces ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: > Is this Linux kernel coding style? Asking for myself, I will gradually read it. Yes, but with an exception about line length: https://www.flashrom.org/Development_Guidelines#Coding_style -- To view, visit https://review.coreboot.org/c/flashrom/+/52827 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I69b762391165177857e9331f79f54b01149cf339 Gerrit-Change-Number: 52827 Gerrit-PatchSet: 1 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.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: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Mon, 03 May 2021 09:07:41 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: lspcon_i2c_spi: Extract I2C bus parameter handling
by Angel Pons (Code Review) 03 May '21

03 May '21

Attention is currently required from: Nico Huber, Edward O'Callaghan, Anastasia Klimchuk. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52830 ) Change subject: lspcon_i2c_spi: Extract I2C bus parameter handling ...................................................................... Patch Set 1: (1 comment) File i2c_helper_linux.c: https://review.coreboot.org/c/flashrom/+/52830/comment/530d31ed_c4e9217c PS1, Line 80: * TODO: Move this function to a common place, it is not specific to I2C */ > I guess this todo is fixed now in this commit? If one removes all the references to "bus" from this function, what remains is a function that parses a string to obtain an integer. This new function is not specific to I2C. Reading my comment again, I agree that it doesn't make much sense. Any suggestions on how to reword it, or should I drop it altogether? -- To view, visit https://review.coreboot.org/c/flashrom/+/52830 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I006b311c88feea37fe4b217f769b21ca1505def9 Gerrit-Change-Number: 52830 Gerrit-PatchSet: 1 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.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: Peter Marheine <pmarheine(a)chromium.org> 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: Mon, 03 May 2021 09:06:19 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: comment

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit May 2021