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

Change in flashrom[master]: [RFC] tests: Abstract port I/O and factor mec1308 specifics out
by Nico Huber (Code Review) 30 Apr '21

30 Apr '21

Attention is currently required from: Anastasia Klimchuk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52604 ) Change subject: [RFC] tests: Abstract port I/O and factor mec1308 specifics out ...................................................................... Patch Set 1: (1 comment) File tests/io_mock.h: https://review.coreboot.org/c/flashrom/+/52604/comment/2e6845de_2d7fe86b PS1, Line 1: #ifndef _IO_MOCK_H_ : #define _IO_MOCK_H_ > I just realised, this is a new file and it doesn't have Copyright header. […] Um, I don't think this is copyrightable. But arguing about it is probably more work than doing the patch. :D CB:52794 -- To view, visit https://review.coreboot.org/c/flashrom/+/52604 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ia3a424478456a7c968b3c736ba371d755c494f30 Gerrit-Change-Number: 52604 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> 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: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Fri, 30 Apr 2021 14:53:37 +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]: tests: Start port-i/o mocking framework
by Angel Pons (Code Review) 30 Apr '21

30 Apr '21

Attention is currently required from: Nico Huber. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52794 ) Change subject: tests: Start port-i/o mocking framework ...................................................................... Patch Set 1: Code-Review+1 -- To view, visit https://review.coreboot.org/c/flashrom/+/52794 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I8df02832deba80761b57435244a29d0d9b4e2649 Gerrit-Change-Number: 52794 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Comment-Date: Fri, 30 Apr 2021 14:53:33 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: tests: Start port-i/o mocking framework
by Nico Huber (Code Review) 30 Apr '21

30 Apr '21

Nico Huber has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/52794 ) Change subject: tests: Start port-i/o mocking framework ...................................................................... tests: Start port-i/o mocking framework This will be used to mock the i/o needs of indvidiual programmer drivers. For each programmer driver, a `struct io_mock` can be registered to dispatch inb()/outb() and friends. Change-Id: I8df02832deba80761b57435244a29d0d9b4e2649 Signed-off-by: Nico Huber <nico.h(a)gmx.de> --- A tests/io_mock.h 1 file changed, 49 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/94/52794/1 diff --git a/tests/io_mock.h b/tests/io_mock.h new file mode 100644 index 0000000..056e6bf --- /dev/null +++ b/tests/io_mock.h @@ -0,0 +1,49 @@ +/* + * This file is part of the flashrom project. + * + * Copyright (c) 2021 Nico Huber <nico.h(a)gmx.de> + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions + * are met: + * 1. Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * 2. Redistributions in binary form must reproduce the above copyright + * notice, this list of conditions and the following disclaimer in the + * documentation and/or other materials provided with the distribution. + * 3. Neither the name of the author nor the names of its contributors + * may be used to endorse or promote products derived from this + * software without specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE + * ARE DISCLAIMED. IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE + * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS + * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) + * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT + * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY + * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF + * SUCH DAMAGE. + */ + +#ifndef _IO_MOCK_H_ +#define _IO_MOCK_H_ + +struct io_mock { + void *priv; + + void (*outb)(void *priv, unsigned char value, unsigned short port); + unsigned char (*inb)(void *priv, unsigned short port); + + void (*outw)(void *priv, unsigned short value, unsigned short port); + unsigned short (*inw)(void *priv, unsigned short port); + + void (*outl)(void *priv, unsigned int value, unsigned short port); + unsigned int (*inl)(void *priv, unsigned short port); +}; + +void io_mock_register(const struct io_mock *io); + +#endif -- To view, visit https://review.coreboot.org/c/flashrom/+/52794 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I8df02832deba80761b57435244a29d0d9b4e2649 Gerrit-Change-Number: 52794 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: pickit2_spi.c: Refactor singleton states into reentrant pattern
by Edward O'Callaghan (Code Review) 30 Apr '21

30 Apr '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/+/52774 ) Change subject: pickit2_spi.c: Refactor singleton states into reentrant pattern ...................................................................... Patch Set 1: (1 comment) File pickit2_spi.c: https://review.coreboot.org/c/flashrom/+/52774/comment/57117c56_466cc8fd PS1, Line 343: static struct spi_master spi_master_pickit2 = { > This will bug me with every single patch. I think the API is just broken […] Same and this is something Anastasia and I actively discussed. I suggested to Anastasia to focus on getting the easier boilerplate-like patches such as this one front loaded before moving forwards with the API change so that it is well informed, primarily by building heuristic. There were a few edge cases Anastasia identified upon reflection and so we wanted to bring the tree into a more consistent state. Specifically, two main things: i) get all the drivers to be reentrant by leveraging data in a consistent manner, ii) untangle the purpose of shutdown such that its purpose singular in its duties. One other dimension suggested was that the testing efforts running in tandem to this should be easier to write and have larger cov once drivers follow a more consistent pattern. The data field was a great introduction however was never fully realised so we agreed this would be a good milestone to achieve. I'll let Anastasia talk to the precise details however I wanted to chime in here Nico so you perhaps get some visibility into the strategy set out to help Anastasia make consistent progress. -- 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: 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: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Fri, 30 Apr 2021 12:30:04 +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]: pickit2_spi.c: Refactor singleton states into reentrant pattern
by Nico Huber (Code Review) 30 Apr '21

30 Apr '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 1: (3 comments) File pickit2_spi.c: https://review.coreboot.org/c/flashrom/+/52774/comment/39a5cbe4_1e92716f PS1, Line 57: }; Do we need this struct? I guess it would make it easier to extend in the future, but this seems hard to predict. We could also pass the libusb handle directly as `data`. https://review.coreboot.org/c/flashrom/+/52774/comment/e1d4b973_7f01382a PS1, Line 343: static struct spi_master spi_master_pickit2 = { This will bug me with every single patch. I think the API is just broken and maybe we should fix it before making more use of it. The per-instance `data` pointer should simply be passed to register_spi_master(), IMHO. This might actually be easier to address right now as there are not that many programmer drivers that use the `data` pointer yet. https://review.coreboot.org/c/flashrom/+/52774/comment/0b21cbcc_982ab4fb PS1, Line 472: pickit2_data = calloc(1, sizeof(*pickit2_data)); No NULL check? (Could be avoided by not using a struct.) -- 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: 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: 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: Fri, 30 Apr 2021 11:39:14 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: pickit2_spi.c: Separate shutdown from failed init cleanup
by Nico Huber (Code Review) 30 Apr '21

30 Apr '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: Code-Review+2 (1 comment) File pickit2_spi.c: https://review.coreboot.org/c/flashrom/+/52773/comment/deb669b6_f2664fc6 PS1, Line 482: goto init_err_cleanup_exit; Just one comment to make this review not too boring: This actually changes behavior. The original version failed to call pickit2_shutdown() if its registration failed. It's a minor thing in this case (why would registration fail?). Generally it's prefered though to focus on the functional changes in the commit message (or just have them in a separate change). -- 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: Fri, 30 Apr 2021 11:22:24 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: [RFC] tests: Abstract port I/O and factor mec1308 specifics out
by Anastasia Klimchuk (Code Review) 30 Apr '21

30 Apr '21

Attention is currently required from: Nico Huber. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52604 ) Change subject: [RFC] tests: Abstract port I/O and factor mec1308 specifics out ...................................................................... Patch Set 1: (1 comment) File tests/io_mock.h: https://review.coreboot.org/c/flashrom/+/52604/comment/d117d6c2_bc06a158 PS1, Line 1: #ifndef _IO_MOCK_H_ : #define _IO_MOCK_H_ I just realised, this is a new file and it doesn't have Copyright header. Would you like to create a patch with this file and submit? I did little modifications for 51487, but I can rebase 51487 on the top and add my modifications. What do you think? -- To view, visit https://review.coreboot.org/c/flashrom/+/52604 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ia3a424478456a7c968b3c736ba371d755c494f30 Gerrit-Change-Number: 52604 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> 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: Nico Huber <nico.h(a)gmx.de> Gerrit-Comment-Date: Fri, 30 Apr 2021 09:20:05 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: pickit2_spi.c: Refactor singleton states into reentrant pattern
by Anastasia Klimchuk (Code Review) 30 Apr '21

30 Apr '21

Anastasia Klimchuk has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/52774 ) Change subject: pickit2_spi.c: Refactor singleton states into reentrant pattern ...................................................................... pickit2_spi.c: Refactor singleton states into reentrant pattern Move global singleton states into a struct and store within the spi_master data field for the life-time of the driver. This is one of the steps on the way to move spi_master data memory management behind the initialisation API, for more context see other patches under the same topic "register_master_api". TEST=builds BUG=b:185191942 Change-Id: Ibacc4738bee02c371c41583d321e0337128ad18a Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org> --- M pickit2_spi.c 1 file changed, 31 insertions(+), 16 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/74/52774/1 diff --git a/pickit2_spi.c b/pickit2_spi.c index 367d0fc..461f6cb 100644 --- a/pickit2_spi.c +++ b/pickit2_spi.c @@ -52,7 +52,9 @@ {0} }; -static libusb_device_handle *pickit2_handle; +struct pickit2_spi_data { + libusb_device_handle *pickit2_handle; +}; /* Default USB transaction timeout in ms */ #define DFLT_TIMEOUT 10000 @@ -89,7 +91,7 @@ #define SCR_VDD_OFF 0xFE #define SCR_VDD_ON 0xFF -static int pickit2_get_firmware_version(void) +static int pickit2_get_firmware_version(libusb_device_handle *pickit2_handle) { int ret; uint8_t command[CMD_LENGTH] = {CMD_GET_VERSION, CMD_END_OF_BUFFER}; @@ -113,7 +115,7 @@ return 0; } -static int pickit2_set_spi_voltage(int millivolt) +static int pickit2_set_spi_voltage(libusb_device_handle *pickit2_handle, int millivolt) { double voltage_selector; switch (millivolt) { @@ -172,7 +174,7 @@ { NULL, 0x0 }, }; -static int pickit2_set_spi_speed(unsigned int spispeed_idx) +static int pickit2_set_spi_speed(libusb_device_handle *pickit2_handle, unsigned int spispeed_idx) { msg_pdbg("SPI speed is %sHz\n", spispeeds[spispeed_idx].name); @@ -198,6 +200,7 @@ static int pickit2_spi_send_command(const struct flashctx *flash, unsigned int writecnt, unsigned int readcnt, const unsigned char *writearr, unsigned char *readarr) { + struct pickit2_spi_data *pickit2_data = flash->mst->spi.data; /* Maximum number of bytes per transaction (including command overhead) is 64. Lets play it safe * and always assume the worst case scenario of 20 bytes command overhead. @@ -255,7 +258,8 @@ buf[i++] = CMD_END_OF_BUFFER; int transferred; - int ret = libusb_interrupt_transfer(pickit2_handle, ENDPOINT_OUT, buf, CMD_LENGTH, &transferred, DFLT_TIMEOUT); + int ret = libusb_interrupt_transfer(pickit2_data->pickit2_handle, + ENDPOINT_OUT, buf, CMD_LENGTH, &transferred, DFLT_TIMEOUT); if (ret != 0) { msg_perr("Send SPI failed!\n"); @@ -264,7 +268,8 @@ if (readcnt) { int length = 0; - ret = libusb_interrupt_transfer(pickit2_handle, ENDPOINT_IN, buf, CMD_LENGTH, &length, DFLT_TIMEOUT); + ret = libusb_interrupt_transfer(pickit2_data->pickit2_handle, + ENDPOINT_IN, buf, CMD_LENGTH, &length, DFLT_TIMEOUT); if (length == 0 || ret != 0) { msg_perr("Receive SPI failed\n"); @@ -335,7 +340,7 @@ return millivolt; } -static const struct spi_master spi_master_pickit2 = { +static struct spi_master spi_master_pickit2 = { .max_data_read = 40, .max_data_write = 40, .command = pickit2_spi_send_command, @@ -347,6 +352,8 @@ static int pickit2_shutdown(void *data) { + struct pickit2_spi_data *pickit2_data = data; + /* Set all pins to float and turn voltages off */ uint8_t command[CMD_LENGTH] = { CMD_EXEC_SCRIPT, @@ -363,18 +370,21 @@ }; int transferred; - int ret = libusb_interrupt_transfer(pickit2_handle, ENDPOINT_OUT, command, CMD_LENGTH, &transferred, DFLT_TIMEOUT); + int ret = libusb_interrupt_transfer(pickit2_data->pickit2_handle, + ENDPOINT_OUT, command, CMD_LENGTH, &transferred, DFLT_TIMEOUT); if (ret != 0) { msg_perr("Command Shutdown failed!\n"); ret = 1; } - if (libusb_release_interface(pickit2_handle, 0) != 0) { + if (libusb_release_interface(pickit2_data->pickit2_handle, 0) != 0) { msg_perr("Could not release USB interface!\n"); ret = 1; } - libusb_close(pickit2_handle); + libusb_close(pickit2_data->pickit2_handle); libusb_exit(NULL); + + free(data); return ret; } @@ -397,7 +407,8 @@ CMD_END_OF_BUFFER }; - + libusb_device_handle *pickit2_handle; + struct pickit2_spi_data *pickit2_data; int spispeed_idx = 0; char *spispeed = extract_programmer_param("spispeed"); if (spispeed != NULL) { @@ -458,16 +469,20 @@ return 1; } - if (pickit2_get_firmware_version()) + pickit2_data = calloc(1, sizeof(*pickit2_data)); + pickit2_data->pickit2_handle = pickit2_handle; + spi_master_pickit2.data = pickit2_data; + + if (pickit2_get_firmware_version(pickit2_handle)) goto init_err_cleanup_exit; /* Command Set SPI Speed */ - if (pickit2_set_spi_speed(spispeed_idx)) + if (pickit2_set_spi_speed(pickit2_handle, spispeed_idx)) goto init_err_cleanup_exit; /* Command Set SPI Voltage */ msg_pdbg("Setting voltage to %i mV.\n", millivolt); - if (pickit2_set_spi_voltage(millivolt) != 0) + if (pickit2_set_spi_voltage(pickit2_handle, millivolt) != 0) goto init_err_cleanup_exit; /* Perform basic setup. @@ -478,13 +493,13 @@ goto init_err_cleanup_exit; } - if (register_shutdown(pickit2_shutdown, NULL)) + if (register_shutdown(pickit2_shutdown, pickit2_data)) goto init_err_cleanup_exit; register_spi_master(&spi_master_pickit2); return 0; init_err_cleanup_exit: - pickit2_shutdown(NULL); + pickit2_shutdown(pickit2_data); return 1; } -- 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: 1 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: pickit2_spi.c: Separate shutdown from failed init cleanup
by Anastasia Klimchuk (Code Review) 30 Apr '21

30 Apr '21

Anastasia Klimchuk has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/52773 ) Change subject: pickit2_spi.c: Separate shutdown from failed init cleanup ...................................................................... pickit2_spi.c: Separate shutdown from failed init cleanup Shutdown function was covering two different jobs here: 1) the actual shutdown which is run at the end of the driver's lifecycle and 2) cleanup in cases when initialisation failed. Now, shutdown is only doing its main job (#1), and the driver itself is doing cleanup when init fails (#2). The good thing is that now resources are released/closed immediately in cases when init fails (vs shutdown function which was run at some point later), and the driver leaves clean space after itself if init fails. And very importantly this unlocks API change which plans to move register_shutdown inside register master API, see https://review.coreboot.org/c/flashrom/+/51761 TEST=builds BUG=b:185191942 Change-Id: I1b672b33169a7a1b6ceab190ad3f48c2f35c3a1f Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org> --- M pickit2_spi.c 1 file changed, 13 insertions(+), 14 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/73/52773/1 diff --git a/pickit2_spi.c b/pickit2_spi.c index 0bc17af..367d0fc 100644 --- a/pickit2_spi.c +++ b/pickit2_spi.c @@ -458,34 +458,33 @@ return 1; } - if (register_shutdown(pickit2_shutdown, NULL) != 0) { - return 1; - } - - if (pickit2_get_firmware_version()) { - return 1; - } + if (pickit2_get_firmware_version()) + goto init_err_cleanup_exit; /* Command Set SPI Speed */ - if (pickit2_set_spi_speed(spispeed_idx)) { - return 1; - } + if (pickit2_set_spi_speed(spispeed_idx)) + goto init_err_cleanup_exit; /* Command Set SPI Voltage */ msg_pdbg("Setting voltage to %i mV.\n", millivolt); - if (pickit2_set_spi_voltage(millivolt) != 0) { - return 1; - } + if (pickit2_set_spi_voltage(millivolt) != 0) + goto init_err_cleanup_exit; /* Perform basic setup. * Configure pin directions and logic levels, turn Vdd on, turn busy LED on and clear buffers. */ int transferred; if (libusb_interrupt_transfer(pickit2_handle, ENDPOINT_OUT, buf, CMD_LENGTH, &transferred, DFLT_TIMEOUT) != 0) { msg_perr("Command Setup failed!\n"); - return 1; + goto init_err_cleanup_exit; } + if (register_shutdown(pickit2_shutdown, NULL)) + goto init_err_cleanup_exit; register_spi_master(&spi_master_pickit2); return 0; + +init_err_cleanup_exit: + pickit2_shutdown(NULL); + return 1; } -- 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-MessageType: newchange

1 0

Change in flashrom[master]: cli_classic.c: add -x option for do_extract()
by Edward O'Callaghan (Code Review) 30 Apr '21

30 Apr '21

Attention is currently required from: Sam McNally, Daniel Campello. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52450 ) Change subject: cli_classic.c: add -x option for do_extract() ...................................................................... Patch Set 11: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/52450 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I8c69223fa92cf5b50abe070f1ab9f19d3b42f6ff Gerrit-Change-Number: 52450 Gerrit-PatchSet: 11 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: 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: Fri, 30 Apr 2021 02:27:11 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit April 2021