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

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

10 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/+/54033 ) Change subject: mstarddc_spi.c: Refactor singleton states into reentrant pattern ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/54033 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ia0e5f6879bfbfac591a40119ee3e0942a5cbc4b9 Gerrit-Change-Number: 54033 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: Tue, 11 May 2021 03:40:35 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: mstarddc_spi.c: Drop mstarddc_ prefix for spi data struct members
by Edward O'Callaghan (Code Review) 10 May '21

10 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/+/54034 ) Change subject: mstarddc_spi.c: Drop mstarddc_ prefix for spi data struct members ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/54034 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ic04b14ce502917ae3b959cf2acf23d58b8752d47 Gerrit-Change-Number: 54034 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: Tue, 11 May 2021 03:37:30 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

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

10 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/+/54044 ) Change subject: usbblaster_spi.c: Refactor singleton states into reentrant pattern ...................................................................... Patch Set 1: Code-Review+2 (1 comment) File usbblaster_spi.c: https://review.coreboot.org/c/flashrom/+/54044/comment/bfb66aee_114280f7 PS1, Line 165: static int usbblaster_shutdown(void *data) : { : free(data); : return 0; : } > My own comment on this. […] Seems fine to me. The ftdi resource leaks any way as it is never closed so the driver actually needed a shutdown func. -- 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: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Tue, 11 May 2021 03:36:34 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

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

10 May '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/+/54044 ) Change subject: usbblaster_spi.c: Refactor singleton states into reentrant pattern ...................................................................... Patch Set 1: (2 comments) Patchset: PS1: I have a message to reviewers, below. Thank you! File usbblaster_spi.c: https://review.coreboot.org/c/flashrom/+/54044/comment/9fd99056_b7c7b4a7 PS1, Line 165: static int usbblaster_shutdown(void *data) : { : free(data); : return 0; : } My own comment on this. I fully realise this patch is doing two things (removes global state and introduces shutdown function), spent some time thinking about it and decided this is the least of evil. Without shutdown function there is no way to free the spi data, so it seems relevant here (vs adding empty shutdown function in a separate patch which serves no purpose). Also usbblaster is doing neither cleanup on init failure nor cleanup on shutdown, but this definitely should go to a separate patch. Here I only free what I allocated myself. In any case, if people think this should be changed/split, I can do it. -- 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: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Tue, 11 May 2021 01:50:42 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

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

10 May '21

Anastasia Klimchuk has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/54044 ) Change subject: usbblaster_spi.c: Refactor singleton states into reentrant pattern ...................................................................... usbblaster_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". This patch also introduces shutdown function for usbblaster, because there was none previously and without shutdown function there is no way to free spi_master data at the end of driver lifecycle. BUG=b:185191942 TEST=builds Change-Id: Ia81f9f40c7eab430a8b304d0b197ce7c75bf5ace Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org> --- M usbblaster_spi.c 1 file changed, 30 insertions(+), 8 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/44/54044/1 diff --git a/usbblaster_spi.c b/usbblaster_spi.c index 020c3bf..c92efe6 100644 --- a/usbblaster_spi.c +++ b/usbblaster_spi.c @@ -51,7 +51,9 @@ {0} }; -static struct ftdi_context ftdic; +struct usbblaster_spi_data { + struct ftdi_context ftdic; +}; // command bytes #define BIT_BYTE (1<<7) // byte mode (rather than bitbang) @@ -70,7 +72,7 @@ return ((b * 0x0802LU & 0x22110LU) | (b * 0x8020LU & 0x88440LU)) * 0x10101LU >> 16; } -static int send_write(unsigned int writecnt, const unsigned char *writearr) +static int send_write(unsigned int writecnt, const unsigned char *writearr, struct ftdi_context ftdic) { uint8_t buf[BUF_SIZE]; @@ -95,7 +97,7 @@ return 0; } -static int send_read(unsigned int readcnt, unsigned char *readarr) +static int send_read(unsigned int readcnt, unsigned char *readarr, struct ftdi_context ftdic) { int i; unsigned int n_read; @@ -135,23 +137,24 @@ static int usbblaster_spi_send_command(const struct flashctx *flash, unsigned int writecnt, unsigned int readcnt, const unsigned char *writearr, unsigned char *readarr) { + struct usbblaster_spi_data *usbblaster_data = flash->mst->spi.data; uint8_t cmd; int ret = 0; cmd = BIT_LED; // asserts /CS - if (ftdi_write_data(&ftdic, &cmd, 1) < 0) { + if (ftdi_write_data(&usbblaster_data->ftdic, &cmd, 1) < 0) { msg_perr("USB-Blaster enable chip select failed\n"); ret = -1; } if (!ret && writecnt) - ret = send_write(writecnt, writearr); + ret = send_write(writecnt, writearr, usbblaster_data->ftdic); if (!ret && readcnt) - ret = send_read(readcnt, readarr); + ret = send_read(readcnt, readarr, usbblaster_data->ftdic); cmd = BIT_CS; - if (ftdi_write_data(&ftdic, &cmd, 1) < 0) { + if (ftdi_write_data(&usbblaster_data->ftdic, &cmd, 1) < 0) { msg_perr("USB-Blaster disable chip select failed\n"); ret = -1; } @@ -159,8 +162,13 @@ return ret; } +static int usbblaster_shutdown(void *data) +{ + free(data); + return 0; +} -static const struct spi_master spi_master_usbblaster = { +static struct spi_master spi_master_usbblaster = { .max_data_read = 256, .max_data_write = 256, .command = usbblaster_spi_send_command, @@ -174,6 +182,8 @@ int usbblaster_spi_init(void) { uint8_t buf[BUF_SIZE + 1]; + struct ftdi_context ftdic; + struct usbblaster_spi_data *usbblaster_data; if (ftdi_init(&ftdic) < 0) return -1; @@ -210,6 +220,18 @@ return -1; } + usbblaster_data = calloc(1, sizeof(*usbblaster_data)); + if (!usbblaster_data) { + msg_perr("Unable to allocate space for SPI master data\n"); + return -1; + } + usbblaster_data->ftdic = ftdic; + spi_master_usbblaster.data = usbblaster_data; + + if (register_shutdown(usbblaster_shutdown, usbblaster_data)) { + free(usbblaster_data); + return -1; + } register_spi_master(&spi_master_usbblaster); return 0; } -- 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-MessageType: newchange

1 0

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

10 May '21

Attention is currently required from: Nico Huber, Angel Pons. Anastasia Klimchuk 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/f745928a_bb768882 PS1, Line 533: stlinkv3_spi_open(sck_freq_kHz, stlinkv3_handle > not actionable in this commit however I noticed the handle isn't closed on the error paths below. Yes :\ on my observation, half of init error paths in the tree are not doing proper cleanup. I think this is going to be my next project. -- 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: 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: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Tue, 11 May 2021 00:26: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

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

10 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/+/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/381e3897_14e8c186 PS1, Line 533: stlinkv3_spi_open(sck_freq_kHz, stlinkv3_handle not actionable in this commit however I noticed the handle isn't closed on the error paths below. -- 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: 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: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Tue, 11 May 2021 00:08:42 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: stlinkv3_spi.c: Drop stlinkv3_ prefix for spi data struct member
by Edward O'Callaghan (Code Review) 10 May '21

10 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/+/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: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Tue, 11 May 2021 00:04:32 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: stlinkv3_spi.c: Drop stlinkv3_ prefix for spi data struct member
by Anastasia Klimchuk (Code Review) 10 May '21

10 May '21

Anastasia Klimchuk has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/54043 ) Change subject: stlinkv3_spi.c: Drop stlinkv3_ prefix for spi data struct member ...................................................................... stlinkv3_spi.c: Drop stlinkv3_ prefix for spi data struct member The name of the struct already contains stlinkv3_ prefix, so prefix doesn't need to be repeated in members name. BUG=b:185191942 TEST=builds Change-Id: Ibddac9371ab8f08276d499642a9bdd6dbecea0ca Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org> --- M stlinkv3_spi.c 1 file changed, 13 insertions(+), 13 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/43/54043/1 diff --git a/stlinkv3_spi.c b/stlinkv3_spi.c index f1e455b..1bb8525 100644 --- a/stlinkv3_spi.c +++ b/stlinkv3_spi.c @@ -121,7 +121,7 @@ struct stlinkv3_spi_data { struct libusb_context *usb_ctx; - libusb_device_handle *stlinkv3_handle; + libusb_device_handle *handle; }; static int stlinkv3_command(uint8_t *command, size_t command_length, @@ -355,7 +355,7 @@ uint32_t rw_status = 0; unsigned int i; - if (stlinkv3_spi_set_SPI_NSS(SPI_NSS_LOW, stlinkv3_data->stlinkv3_handle)) { + if (stlinkv3_spi_set_SPI_NSS(SPI_NSS_LOW, stlinkv3_data->handle)) { msg_perr("Failed to set the NSS pin to low\n"); return -1; } @@ -370,7 +370,7 @@ for (i = 0; (i < 8) && (i < write_cnt); i++) command[4+i] = write_arr[i]; - rc = libusb_bulk_transfer(stlinkv3_data->stlinkv3_handle, STLINK_EP_OUT, + rc = libusb_bulk_transfer(stlinkv3_data->handle, STLINK_EP_OUT, command, sizeof(command), &actual_length, USB_TIMEOUT_IN_MS); if (rc != LIBUSB_TRANSFER_COMPLETED || actual_length != sizeof(command)) { @@ -380,7 +380,7 @@ } if (write_cnt > 8) { - rc = libusb_bulk_transfer(stlinkv3_data->stlinkv3_handle, + rc = libusb_bulk_transfer(stlinkv3_data->handle, STLINK_EP_OUT, (unsigned char *)&write_arr[8], (unsigned int)(write_cnt - 8), @@ -393,7 +393,7 @@ } } - if (stlinkv3_get_last_readwrite_status(&rw_status, stlinkv3_data->stlinkv3_handle)) + if (stlinkv3_get_last_readwrite_status(&rw_status, stlinkv3_data->handle)) return -1; if (rw_status != 0) { @@ -406,7 +406,7 @@ command[2] = (uint8_t)read_cnt; command[3] = (uint8_t)(read_cnt >> 8); - rc = libusb_bulk_transfer(stlinkv3_data->stlinkv3_handle, STLINK_EP_OUT, + rc = libusb_bulk_transfer(stlinkv3_data->handle, STLINK_EP_OUT, command, sizeof(command), &actual_length, USB_TIMEOUT_IN_MS); if (rc != LIBUSB_TRANSFER_COMPLETED || (unsigned int)actual_length != sizeof(command)) { @@ -415,7 +415,7 @@ goto transmit_err; } - rc = libusb_bulk_transfer(stlinkv3_data->stlinkv3_handle, + rc = libusb_bulk_transfer(stlinkv3_data->handle, STLINK_EP_IN, (unsigned char *)read_arr, (int)read_cnt, @@ -428,7 +428,7 @@ } } - if (stlinkv3_get_last_readwrite_status(&rw_status, stlinkv3_data->stlinkv3_handle)) + if (stlinkv3_get_last_readwrite_status(&rw_status, stlinkv3_data->handle)) goto transmit_err; if (rw_status != 0) { @@ -436,14 +436,14 @@ goto transmit_err; } - if (stlinkv3_spi_set_SPI_NSS(SPI_NSS_HIGH, stlinkv3_data->stlinkv3_handle)) { + if (stlinkv3_spi_set_SPI_NSS(SPI_NSS_HIGH, stlinkv3_data->handle)) { msg_perr("Failed to set the NSS pin to high\n"); return -1; } return 0; transmit_err: - if (stlinkv3_spi_set_SPI_NSS(SPI_NSS_HIGH, stlinkv3_data->stlinkv3_handle)) + if (stlinkv3_spi_set_SPI_NSS(SPI_NSS_HIGH, stlinkv3_data->handle)) msg_perr("Failed to set the NSS pin to high\n"); return -1; } @@ -463,9 +463,9 @@ stlinkv3_command(command, sizeof(command), answer, sizeof(answer), "STLINK_BRIDGE_CLOSE", - stlinkv3_data->stlinkv3_handle); + stlinkv3_data->handle); - libusb_close(stlinkv3_data->stlinkv3_handle); + libusb_close(stlinkv3_data->handle); libusb_exit(stlinkv3_data->usb_ctx); free(data); @@ -540,7 +540,7 @@ } stlinkv3_data->usb_ctx = usb_ctx; - stlinkv3_data->stlinkv3_handle = stlinkv3_handle; + stlinkv3_data->handle = stlinkv3_handle; spi_programmer_stlinkv3.data = stlinkv3_data; if (register_shutdown(stlinkv3_spi_shutdown, stlinkv3_data)) { -- 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-MessageType: newchange

1 0

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

10 May '21

Anastasia Klimchuk has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/54042 ) Change subject: stlinkv3_spi.c: Refactor singleton states into reentrant pattern ...................................................................... stlinkv3_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". BUG=b:185191942 TEST=builds Change-Id: Id044661b864b506028720ea809bc524f0640469f Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org> --- M stlinkv3_spi.c 1 file changed, 74 insertions(+), 33 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/42/54042/1 diff --git a/stlinkv3_spi.c b/stlinkv3_spi.c index 7e8336e..f1e455b 100644 --- a/stlinkv3_spi.c +++ b/stlinkv3_spi.c @@ -119,11 +119,14 @@ {0} }; -static struct libusb_context *usb_ctx; -static libusb_device_handle *stlinkv3_handle; +struct stlinkv3_spi_data { + struct libusb_context *usb_ctx; + libusb_device_handle *stlinkv3_handle; +}; static int stlinkv3_command(uint8_t *command, size_t command_length, - uint8_t *answer, size_t answer_length, const char *command_name) + uint8_t *answer, size_t answer_length, const char *command_name, + libusb_device_handle *stlinkv3_handle) { int actual_length = 0; int rc = libusb_bulk_transfer(stlinkv3_handle, STLINK_EP_OUT, @@ -151,7 +154,7 @@ /** * @param[out] bridge_input_clk Current input frequency in kHz of the given com. */ -static int stlinkv3_get_clk(uint32_t *bridge_input_clk) +static int stlinkv3_get_clk(uint32_t *bridge_input_clk, libusb_device_handle *stlinkv3_handle) { uint8_t command[16]; uint8_t answer[12]; @@ -165,7 +168,10 @@ command[1] = STLINK_BRIDGE_GET_CLOCK; command[2] = STLINK_SPI_COM; - if (stlinkv3_command(command, sizeof(command), answer, sizeof(answer), "STLINK_BRIDGE_GET_CLOCK") != 0) + if (stlinkv3_command(command, sizeof(command), + answer, sizeof(answer), + "STLINK_BRIDGE_GET_CLOCK", + stlinkv3_handle) != 0) return -1; *bridge_input_clk = (uint32_t)answer[4] @@ -178,13 +184,14 @@ static int stlinkv3_spi_calc_prescaler(uint16_t reqested_freq_in_kHz, enum spi_prescaler *prescaler, - uint16_t *calculated_freq_in_kHz) + uint16_t *calculated_freq_in_kHz, + libusb_device_handle *stlinkv3_handle) { uint32_t bridge_clk_in_kHz; uint32_t calculated_prescaler = 1; uint16_t prescaler_value; - if (stlinkv3_get_clk(&bridge_clk_in_kHz)) + if (stlinkv3_get_clk(&bridge_clk_in_kHz, stlinkv3_handle)) return -1; calculated_prescaler = bridge_clk_in_kHz/reqested_freq_in_kHz; @@ -224,7 +231,7 @@ return 0; } -static int stlinkv3_check_version(enum fw_version_check_result *result) +static int stlinkv3_check_version(enum fw_version_check_result *result, libusb_device_handle *stlinkv3_handle) { uint8_t answer[12]; uint8_t command[16]; @@ -234,7 +241,10 @@ command[0] = ST_GETVERSION_EXT; command[1] = 0x80; - if (stlinkv3_command(command, sizeof(command), answer, sizeof(answer), "ST_GETVERSION_EXT") != 0) + if (stlinkv3_command(command, sizeof(command), + answer, sizeof(answer), + "ST_GETVERSION_EXT", + stlinkv3_handle) != 0) return -1; msg_pinfo("Connected to STLink V3 with bridge FW version: %d\n", answer[4]); @@ -244,7 +254,7 @@ return 0; } -static int stlinkv3_spi_open(uint16_t reqested_freq_in_kHz) +static int stlinkv3_spi_open(uint16_t reqested_freq_in_kHz, libusb_device_handle *stlinkv3_handle) { uint8_t command[16]; uint8_t answer[2]; @@ -252,7 +262,7 @@ enum spi_prescaler prescaler; enum fw_version_check_result fw_check_result; - if (stlinkv3_check_version(&fw_check_result)) { + if (stlinkv3_check_version(&fw_check_result, stlinkv3_handle)) { msg_perr("Failed to query FW version\n"); return -1; } @@ -267,7 +277,8 @@ if (stlinkv3_spi_calc_prescaler(reqested_freq_in_kHz, &prescaler, - &SCK_freq_in_kHz)) { + &SCK_freq_in_kHz, + stlinkv3_handle)) { msg_perr("Failed to calculate SPI clock prescaler\n"); return -1; } @@ -286,10 +297,13 @@ command[5] = SPI_NSS_SOFT; command[6] = (uint8_t)prescaler; - return stlinkv3_command(command, sizeof(command), answer, sizeof(answer), "STLINK_BRIDGE_INIT_SPI"); + return stlinkv3_command(command, sizeof(command), + answer, sizeof(answer), + "STLINK_BRIDGE_INIT_SPI", + stlinkv3_handle); } -static int stlinkv3_get_last_readwrite_status(uint32_t *status) +static int stlinkv3_get_last_readwrite_status(uint32_t *status, libusb_device_handle *stlinkv3_handle) { uint8_t command[16]; uint16_t answer[4]; @@ -301,14 +315,15 @@ if (stlinkv3_command(command, sizeof(command), (uint8_t *)answer, sizeof(answer), - "STLINK_BRIDGE_GET_RWCMD_STATUS") != 0) + "STLINK_BRIDGE_GET_RWCMD_STATUS", + stlinkv3_handle) != 0) return -1; *status = (uint32_t)answer[2] | (uint32_t)answer[3]<<16; return 0; } -static int stlinkv3_spi_set_SPI_NSS(enum spi_nss_level nss_level) +static int stlinkv3_spi_set_SPI_NSS(enum spi_nss_level nss_level, libusb_device_handle *stlinkv3_handle) { uint8_t command[16]; uint8_t answer[2]; @@ -319,7 +334,10 @@ command[1] = STLINK_BRIDGE_CS_SPI; command[2] = (uint8_t) (nss_level); - if (stlinkv3_command(command, sizeof(command), answer, sizeof(answer), "STLINK_BRIDGE_CS_SPI") != 0) + if (stlinkv3_command(command, sizeof(command), + answer, sizeof(answer), + "STLINK_BRIDGE_CS_SPI", + stlinkv3_handle) != 0) return -1; return 0; } @@ -330,13 +348,14 @@ const unsigned char *write_arr, unsigned char *read_arr) { + struct stlinkv3_spi_data *stlinkv3_data = flash->mst->spi.data; uint8_t command[16]; int rc = 0; int actual_length = 0; uint32_t rw_status = 0; unsigned int i; - if (stlinkv3_spi_set_SPI_NSS(SPI_NSS_LOW)) { + if (stlinkv3_spi_set_SPI_NSS(SPI_NSS_LOW, stlinkv3_data->stlinkv3_handle)) { msg_perr("Failed to set the NSS pin to low\n"); return -1; } @@ -351,7 +370,7 @@ for (i = 0; (i < 8) && (i < write_cnt); i++) command[4+i] = write_arr[i]; - rc = libusb_bulk_transfer(stlinkv3_handle, STLINK_EP_OUT, + rc = libusb_bulk_transfer(stlinkv3_data->stlinkv3_handle, STLINK_EP_OUT, command, sizeof(command), &actual_length, USB_TIMEOUT_IN_MS); if (rc != LIBUSB_TRANSFER_COMPLETED || actual_length != sizeof(command)) { @@ -361,7 +380,7 @@ } if (write_cnt > 8) { - rc = libusb_bulk_transfer(stlinkv3_handle, + rc = libusb_bulk_transfer(stlinkv3_data->stlinkv3_handle, STLINK_EP_OUT, (unsigned char *)&write_arr[8], (unsigned int)(write_cnt - 8), @@ -374,7 +393,7 @@ } } - if (stlinkv3_get_last_readwrite_status(&rw_status)) + if (stlinkv3_get_last_readwrite_status(&rw_status, stlinkv3_data->stlinkv3_handle)) return -1; if (rw_status != 0) { @@ -387,7 +406,7 @@ command[2] = (uint8_t)read_cnt; command[3] = (uint8_t)(read_cnt >> 8); - rc = libusb_bulk_transfer(stlinkv3_handle, STLINK_EP_OUT, + rc = libusb_bulk_transfer(stlinkv3_data->stlinkv3_handle, STLINK_EP_OUT, command, sizeof(command), &actual_length, USB_TIMEOUT_IN_MS); if (rc != LIBUSB_TRANSFER_COMPLETED || (unsigned int)actual_length != sizeof(command)) { @@ -396,7 +415,7 @@ goto transmit_err; } - rc = libusb_bulk_transfer(stlinkv3_handle, + rc = libusb_bulk_transfer(stlinkv3_data->stlinkv3_handle, STLINK_EP_IN, (unsigned char *)read_arr, (int)read_cnt, @@ -409,7 +428,7 @@ } } - if (stlinkv3_get_last_readwrite_status(&rw_status)) + if (stlinkv3_get_last_readwrite_status(&rw_status, stlinkv3_data->stlinkv3_handle)) goto transmit_err; if (rw_status != 0) { @@ -417,20 +436,21 @@ goto transmit_err; } - if (stlinkv3_spi_set_SPI_NSS(SPI_NSS_HIGH)) { + if (stlinkv3_spi_set_SPI_NSS(SPI_NSS_HIGH, stlinkv3_data->stlinkv3_handle)) { msg_perr("Failed to set the NSS pin to high\n"); return -1; } return 0; transmit_err: - if (stlinkv3_spi_set_SPI_NSS(SPI_NSS_HIGH)) + if (stlinkv3_spi_set_SPI_NSS(SPI_NSS_HIGH, stlinkv3_data->stlinkv3_handle)) msg_perr("Failed to set the NSS pin to high\n"); return -1; } static int stlinkv3_spi_shutdown(void *data) { + struct stlinkv3_spi_data *stlinkv3_data = data; uint8_t command[16]; uint8_t answer[2]; @@ -440,15 +460,19 @@ command[1] = STLINK_BRIDGE_CLOSE; command[2] = STLINK_SPI_COM; - stlinkv3_command(command, sizeof(command), answer, sizeof(answer), "STLINK_BRIDGE_CLOSE"); + stlinkv3_command(command, sizeof(command), + answer, sizeof(answer), + "STLINK_BRIDGE_CLOSE", + stlinkv3_data->stlinkv3_handle); - libusb_close(stlinkv3_handle); - libusb_exit(usb_ctx); + libusb_close(stlinkv3_data->stlinkv3_handle); + libusb_exit(stlinkv3_data->usb_ctx); + free(data); return 0; } -static const struct spi_master spi_programmer_stlinkv3 = { +static struct spi_master spi_programmer_stlinkv3 = { .max_data_read = UINT16_MAX, .max_data_write = UINT16_MAX, .command = stlinkv3_spi_transmit, @@ -464,6 +488,9 @@ char *speed_str = NULL; char *serialno = NULL; char *endptr = NULL; + struct libusb_context *usb_ctx; + libusb_device_handle *stlinkv3_handle; + struct stlinkv3_spi_data *stlinkv3_data; libusb_init(&usb_ctx); if (!usb_ctx) { @@ -503,14 +530,28 @@ free(speed_str); } - if (stlinkv3_spi_open(sck_freq_kHz)) + if (stlinkv3_spi_open(sck_freq_kHz, stlinkv3_handle)) goto err_exit; - if (register_shutdown(stlinkv3_spi_shutdown, NULL)) + stlinkv3_data = calloc(1, sizeof(*stlinkv3_data)); + if (!stlinkv3_data) { + msg_perr("Unable to allocate space for SPI master data\n"); goto err_exit; + } - if (register_spi_master(&spi_programmer_stlinkv3)) + stlinkv3_data->usb_ctx = usb_ctx; + stlinkv3_data->stlinkv3_handle = stlinkv3_handle; + spi_programmer_stlinkv3.data = stlinkv3_data; + + if (register_shutdown(stlinkv3_spi_shutdown, stlinkv3_data)) { + free(stlinkv3_data); goto err_exit; + } + + if (register_spi_master(&spi_programmer_stlinkv3)) { + free(stlinkv3_data); + goto err_exit; + } return 0; -- 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-MessageType: newchange

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit May 2021