Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/40105 )
Change subject: raiden_debug_spi.c: Clean up global state ......................................................................
raiden_debug_spi.c: Clean up global state
The Chromium flashrom fork has very poor dispatch logic whereas upstream has proper inversion of control with a generic 'data' void * member for stuff long-lived state in. Leverage the member to store the USB descriptor state in during the life-time of the spi master.
BUG=b:140394053 BRANCH=none TEST=builds
Change-Id: Ida9dce97fef2c6dfd68a278c879917fdd3ff7fef Signed-off-by: Edward O'Callaghan quasisec@google.com --- M raiden_debug_spi.c 1 file changed, 48 insertions(+), 15 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/05/40105/1
diff --git a/raiden_debug_spi.c b/raiden_debug_spi.c index 173e355..913ee45 100644 --- a/raiden_debug_spi.c +++ b/raiden_debug_spi.c @@ -161,9 +161,11 @@ */ #define TRANSFER_TIMEOUT_MS (200 + 800)
-struct usb_device *device = NULL; -uint8_t in_endpoint = 0; -uint8_t out_endpoint = 0; +struct raiden_debug_spi_data { + struct usb_device *dev; + uint8_t in_ep; + uint8_t out_ep; +};
typedef struct { int8_t write_count; @@ -200,6 +202,12 @@ return true; }
+static const struct raiden_debug_spi_data * + get_raiden_data_from_context(const struct flashctx *flash) +{ + return (const struct raiden_debug_spi_data *)flash->mst->spi.data; +} + static int write_command(const struct flashctx *flash, unsigned int write_count, unsigned int read_count, @@ -210,6 +218,7 @@ int transferred; int ret; usb_spi_command_t command_packet; + const struct raiden_debug_spi_data * ctx_data = get_raiden_data_from_context(flash);
if (write_count > PAYLOAD_SIZE) { msg_perr("Raiden: Invalid write_count of %d\n", write_count); @@ -226,8 +235,8 @@
memcpy(command_packet.data, write_buffer, write_count);
- ret = LIBUSB(libusb_bulk_transfer(device->handle, - out_endpoint, + ret = LIBUSB(libusb_bulk_transfer(ctx_data->dev->handle, + ctx_data->out_ep, (void*)&command_packet, write_count + PACKET_HEADER_SIZE, &transferred, @@ -258,9 +267,10 @@ int transferred; int ret; usb_spi_response_t response_packet; + const struct raiden_debug_spi_data * ctx_data = get_raiden_data_from_context(flash);
- ret = LIBUSB(libusb_bulk_transfer(device->handle, - in_endpoint, + ret = LIBUSB(libusb_bulk_transfer(ctx_data->dev->handle, + ctx_data->in_ep, (void*)&response_packet, read_count + PACKET_HEADER_SIZE, &transferred, @@ -403,16 +413,19 @@ return 0; }
-static int shutdown(void * data) +static int raiden_debug_spi_shutdown(void * data) { + struct raiden_debug_spi_data * ctx_data = + (struct raiden_debug_spi_data *)data; + int ret = LIBUSB(libusb_control_transfer( - device->handle, + ctx_data->dev->handle, LIBUSB_ENDPOINT_OUT | LIBUSB_REQUEST_TYPE_VENDOR | LIBUSB_RECIPIENT_INTERFACE, RAIDEN_DEBUG_SPI_REQ_DISABLE, 0, - device->interface_descriptor->bInterfaceNumber, + ctx_data->dev->interface_descriptor->bInterfaceNumber, NULL, 0, TRANSFER_TIMEOUT_MS)); @@ -421,10 +434,9 @@ return ret; }
- usb_device_free(device); - - device = NULL; + usb_device_free(ctx_data->dev); libusb_exit(NULL); + free(ctx_data);
return 0; } @@ -458,11 +470,20 @@ dev = usb_device_free(dev); }
+static int register_raiden_debug_spi_master(void *ptr) +{ + struct spi_master mst = spi_master_raiden_debug; + mst.data = ptr; + + return register_spi_master(&mst); +} + int raiden_debug_spi_init(void) { struct usb_match match; char *serial = extract_programmer_param("serial"); struct usb_device *current; + struct usb_device *device = NULL; int found = 0; int ret;
@@ -489,6 +510,8 @@ return ret; }
+ uint8_t in_endpoint = 0; + uint8_t out_endpoint = 0; while (current) { device = current;
@@ -576,8 +599,18 @@ (request_enable == RAIDEN_DEBUG_SPI_REQ_ENABLE_EC)) usleep(50 * 1000);
- register_spi_master(&spi_master_raiden_debug); - register_shutdown(shutdown, NULL); + struct raiden_debug_spi_data *data = calloc(1, sizeof(struct raiden_debug_spi_data)); + if (!data) { + msg_perr("Unable to allocate space for extra SPI master data.\n"); + return SPI_GENERIC_ERROR; + } + + data->dev = device; + data->in_ep = in_endpoint; + data->out_ep = out_endpoint; + + register_raiden_debug_spi_master(data); + register_shutdown(raiden_debug_spi_shutdown, data);
return 0; }
Sam McNally has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/40105 )
Change subject: raiden_debug_spi.c: Clean up global state ......................................................................
Patch Set 1: Code-Review+1
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/40105 )
Change subject: raiden_debug_spi.c: Clean up global state ......................................................................
Patch Set 1: Code-Review+1
(2 comments)
https://review.coreboot.org/c/flashrom/+/40105/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/flashrom/+/40105/1//COMMIT_MSG@11 PS1, Line 11: for to
https://review.coreboot.org/c/flashrom/+/40105/1//COMMIT_MSG@17 PS1, Line 17: builds Was this tested to check if it still works to detect a raiden?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/40105 )
Change subject: raiden_debug_spi.c: Clean up global state ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/flashrom/+/40105/5/raiden_debug_spi.c File raiden_debug_spi.c:
https://review.coreboot.org/c/flashrom/+/40105/5/raiden_debug_spi.c@473 PS5, Line 473: static int register_raiden_debug_spi_master(void *ptr) : { : struct spi_master mst = spi_master_raiden_debug; : mst.data = ptr; : : return register_spi_master(&mst); : } Given that CB:40470 removes this kind of construct, why add it here?
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/40105 )
Change subject: raiden_debug_spi.c: Clean up global state ......................................................................
Patch Set 5: Code-Review-2
Patch Set 5:
(1 comment)
yep, this patch needs revision. I will update it sometime next week, thanks for reminding me!
Hello Sam McNally, build bot (Jenkins), Stefan Reinauer, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/40105
to look at the new patch set (#6).
Change subject: raiden_debug_spi.c: Clean up global state ......................................................................
raiden_debug_spi.c: Clean up global state
The Chromium flashrom fork has very poor dispatch logic whereas upstream has proper inversion of control with a generic 'data' void * member to stuff long-lived state in. Leverage the member to store the USB descriptor state in during the life-time of the spi master.
V.2: Remove unnecessary indirection as is the case in commit a25c13cdb601f9d43b0f8edad96f9489efcb4b37.
BUG=b:140394053 BRANCH=none TEST=builds && detects flashchip name.
Change-Id: Ida9dce97fef2c6dfd68a278c879917fdd3ff7fef Signed-off-by: Edward O'Callaghan quasisec@google.com --- M raiden_debug_spi.c 1 file changed, 41 insertions(+), 14 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/05/40105/6
Hello Sam McNally, build bot (Jenkins), Stefan Reinauer, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/40105
to look at the new patch set (#7).
Change subject: raiden_debug_spi.c: Clean up global state ......................................................................
raiden_debug_spi.c: Clean up global state
The Chromium flashrom fork has very poor dispatch logic whereas upstream has proper inversion of control with a generic 'data' void * member to stuff long-lived state in. Leverage the member to store the USB descriptor state in during the life-time of the spi master.
V.2: Remove unnecessary indirection as is the case in commit a25c13cdb601f9d43b0f8edad96f9489efcb4b37.
BUG=b:140394053 BRANCH=none TEST=builds && detects flashchip name.
Change-Id: Ida9dce97fef2c6dfd68a278c879917fdd3ff7fef Signed-off-by: Edward O'Callaghan quasisec@google.com --- M raiden_debug_spi.c 1 file changed, 42 insertions(+), 15 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/05/40105/7
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/40105 )
Change subject: raiden_debug_spi.c: Clean up global state ......................................................................
Patch Set 7: -Code-Review
Sam McNally has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/40105 )
Change subject: raiden_debug_spi.c: Clean up global state ......................................................................
Patch Set 7: Code-Review+1
Stefan Reinauer has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/40105 )
Change subject: raiden_debug_spi.c: Clean up global state ......................................................................
Patch Set 8: Code-Review+2
Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/40105 )
Change subject: raiden_debug_spi.c: Clean up global state ......................................................................
raiden_debug_spi.c: Clean up global state
The Chromium flashrom fork has very poor dispatch logic whereas upstream has proper inversion of control with a generic 'data' void * member to stuff long-lived state in. Leverage the member to store the USB descriptor state in during the life-time of the spi master.
V.2: Remove unnecessary indirection as is the case in commit a25c13cdb601f9d43b0f8edad96f9489efcb4b37.
BUG=b:140394053 BRANCH=none TEST=builds && detects flashchip name.
Change-Id: Ida9dce97fef2c6dfd68a278c879917fdd3ff7fef Signed-off-by: Edward O'Callaghan quasisec@google.com Reviewed-on: https://review.coreboot.org/c/flashrom/+/40105 Reviewed-by: Stefan Reinauer stefan.reinauer@coreboot.org Reviewed-by: Sam McNally sammc@google.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M raiden_debug_spi.c 1 file changed, 42 insertions(+), 15 deletions(-)
Approvals: build bot (Jenkins): Verified Stefan Reinauer: Looks good to me, approved Sam McNally: Looks good to me, but someone else must approve
diff --git a/raiden_debug_spi.c b/raiden_debug_spi.c index 306d8fb..368476f 100644 --- a/raiden_debug_spi.c +++ b/raiden_debug_spi.c @@ -161,9 +161,11 @@ */ #define TRANSFER_TIMEOUT_MS (200 + 800)
-struct usb_device *device = NULL; -uint8_t in_endpoint = 0; -uint8_t out_endpoint = 0; +struct raiden_debug_spi_data { + struct usb_device *dev; + uint8_t in_ep; + uint8_t out_ep; +};
typedef struct { int8_t write_count; @@ -200,6 +202,12 @@ return true; }
+static const struct raiden_debug_spi_data * + get_raiden_data_from_context(const struct flashctx *flash) +{ + return (const struct raiden_debug_spi_data *)flash->mst->spi.data; +} + static int write_command(const struct flashctx *flash, unsigned int write_count, unsigned int read_count, @@ -210,6 +218,7 @@ int transferred; int ret; usb_spi_command_t command_packet; + const struct raiden_debug_spi_data * ctx_data = get_raiden_data_from_context(flash);
if (write_count > PAYLOAD_SIZE) { msg_perr("Raiden: Invalid write_count of %d\n", write_count); @@ -226,8 +235,8 @@
memcpy(command_packet.data, write_buffer, write_count);
- ret = LIBUSB(libusb_bulk_transfer(device->handle, - out_endpoint, + ret = LIBUSB(libusb_bulk_transfer(ctx_data->dev->handle, + ctx_data->out_ep, (void*)&command_packet, write_count + PACKET_HEADER_SIZE, &transferred, @@ -258,9 +267,10 @@ int transferred; int ret; usb_spi_response_t response_packet; + const struct raiden_debug_spi_data * ctx_data = get_raiden_data_from_context(flash);
- ret = LIBUSB(libusb_bulk_transfer(device->handle, - in_endpoint, + ret = LIBUSB(libusb_bulk_transfer(ctx_data->dev->handle, + ctx_data->in_ep, (void*)&response_packet, read_count + PACKET_HEADER_SIZE, &transferred, @@ -350,7 +360,7 @@ */ #define MAX_DATA_SIZE (PAYLOAD_SIZE - JEDEC_BYTE_PROGRAM_OUTSIZE)
-static const struct spi_master spi_master_raiden_debug = { +static struct spi_master spi_master_raiden_debug = { .features = SPI_MASTER_4BA, .max_data_read = MAX_DATA_SIZE, .max_data_write = MAX_DATA_SIZE, @@ -403,16 +413,19 @@ return 0; }
-static int shutdown(void * data) +static int raiden_debug_spi_shutdown(void * data) { + struct raiden_debug_spi_data * ctx_data = + (struct raiden_debug_spi_data *)data; + int ret = LIBUSB(libusb_control_transfer( - device->handle, + ctx_data->dev->handle, LIBUSB_ENDPOINT_OUT | LIBUSB_REQUEST_TYPE_VENDOR | LIBUSB_RECIPIENT_INTERFACE, RAIDEN_DEBUG_SPI_REQ_DISABLE, 0, - device->interface_descriptor->bInterfaceNumber, + ctx_data->dev->interface_descriptor->bInterfaceNumber, NULL, 0, TRANSFER_TIMEOUT_MS)); @@ -421,10 +434,9 @@ return ret; }
- usb_device_free(device); - - device = NULL; + usb_device_free(ctx_data->dev); libusb_exit(NULL); + free(ctx_data);
return 0; } @@ -463,6 +475,7 @@ struct usb_match match; char *serial = extract_programmer_param("serial"); struct usb_device *current; + struct usb_device *device = NULL; int found = 0; int ret;
@@ -491,6 +504,8 @@ return ret; }
+ uint8_t in_endpoint = 0; + uint8_t out_endpoint = 0; while (current) { device = current;
@@ -579,8 +594,20 @@ (request_enable == RAIDEN_DEBUG_SPI_REQ_ENABLE_EC)) usleep(50 * 1000);
+ struct raiden_debug_spi_data *data = calloc(1, sizeof(struct raiden_debug_spi_data)); + if (!data) { + msg_perr("Unable to allocate space for extra SPI master data.\n"); + return SPI_GENERIC_ERROR; + } + + data->dev = device; + data->in_ep = in_endpoint; + data->out_ep = out_endpoint; + + spi_master_raiden_debug.data = data; + register_spi_master(&spi_master_raiden_debug); - register_shutdown(shutdown, NULL); + register_shutdown(raiden_debug_spi_shutdown, data);
return 0; }
-
Angel Pons (Code Review) -
Edward O'Callaghan (Code Review)
-
Sam McNally (Code Review)
-
Stefan Reinauer (Code Review)