From 3b78b5c1db68841fdc18ee48b6011f4affff2bfd Mon Sep 17 00:00:00 2001 From: Antonio Borneo Date: Sun, 2 Jul 2023 23:48:42 +0200 Subject: [PATCH] libusb_helper: split error and returned value The USB control transfer can be executed without any data. The libusb API libusb_control_transfer() can thus be called with zero 'size', thus returning zero byte transferred when succeed. The OpenOCD API jtag_libusb_control_transfer() returns zero either in case of transfer error and in case of libusb_control_transfer() returning zero, making impossible discriminating the two cases. Extend jtag_libusb_control_transfer() with separate return value for error code and explicit parameter's pointer for transferred bytes. Make the transferred pointer optional, as many callers do not properly handle the returned value. Use 'int' type pointer for transferred, instead of the 'uint16_t' that would have matched the type of 'size'. This can simplify the caller's code by using a single 'int transferred' variable shared with other jtag_libusb_bulk_read|write, while keeping possible the comparison int vs uint16_t without cast. This change is inspired from commit d612baacaa3f ("jtag_libusb_bulk_read|write: return error code instead of size") Change-Id: I14d9bff3e845675be03465c307a136e69eebc317 Signed-off-by: Antonio Borneo Reviewed-on: https://review.openocd.org/c/openocd/+/7756 Tested-by: jenkins Reviewed-by: ahmed BOUDJELIDA --- src/jtag/drivers/esp_usb_jtag.c | 19 +++++---- src/jtag/drivers/ft232r.c | 12 +++--- src/jtag/drivers/kitprog.c | 40 +++++++++---------- src/jtag/drivers/libusb_helper.c | 19 +++++---- src/jtag/drivers/libusb_helper.h | 3 +- src/jtag/drivers/opendous.c | 16 ++++++-- src/jtag/drivers/openjtag.c | 26 ++++++------ .../usb_blaster/ublast2_access_libusb.c | 12 ++++-- 8 files changed, 85 insertions(+), 62 deletions(-) diff --git a/src/jtag/drivers/esp_usb_jtag.c b/src/jtag/drivers/esp_usb_jtag.c index dd96f4b395..2ed0f58fd1 100644 --- a/src/jtag/drivers/esp_usb_jtag.c +++ b/src/jtag/drivers/esp_usb_jtag.c @@ -506,11 +506,13 @@ static int esp_usb_jtag_init(void) * 1- With the minimum size required to get to know the total length of that struct, * 2- Then exactly the length of that struct. */ uint8_t jtag_caps_desc[JTAG_PROTO_CAPS_DATA_LEN]; - int jtag_caps_read_len = jtag_libusb_control_transfer(priv->usb_device, + int jtag_caps_read_len; + r = jtag_libusb_control_transfer(priv->usb_device, LIBUSB_ENDPOINT_IN | LIBUSB_REQUEST_TYPE_STANDARD | LIBUSB_RECIPIENT_DEVICE, LIBUSB_REQUEST_GET_DESCRIPTOR, esp_usb_jtag_caps, 0, - (char *)jtag_caps_desc, JTAG_PROTO_CAPS_DATA_LEN, LIBUSB_TIMEOUT_MS); - if (jtag_caps_read_len <= 0) { + (char *)jtag_caps_desc, JTAG_PROTO_CAPS_DATA_LEN, LIBUSB_TIMEOUT_MS, + &jtag_caps_read_len); + if (r != ERROR_OK) { LOG_ERROR("esp_usb_jtag: could not retrieve jtag_caps descriptor!"); goto out; } @@ -580,7 +582,8 @@ static int esp_usb_jtag_init(void) 0, NULL, 0, - LIBUSB_TIMEOUT_MS); + LIBUSB_TIMEOUT_MS, + NULL); return ERROR_OK; @@ -637,7 +640,7 @@ static int esp_usb_jtag_speed(int divisor) LOG_DEBUG("esp_usb_jtag: setting divisor %d", divisor); jtag_libusb_control_transfer(priv->usb_device, - LIBUSB_REQUEST_TYPE_VENDOR, VEND_JTAG_SETDIV, divisor, 0, NULL, 0, LIBUSB_TIMEOUT_MS); + LIBUSB_REQUEST_TYPE_VENDOR, VEND_JTAG_SETDIV, divisor, 0, NULL, 0, LIBUSB_TIMEOUT_MS, NULL); return ERROR_OK; } @@ -648,8 +651,8 @@ COMMAND_HANDLER(esp_usb_jtag_tdo_cmd) if (!priv->usb_device) return ERROR_FAIL; int r = jtag_libusb_control_transfer(priv->usb_device, - LIBUSB_ENDPOINT_IN | LIBUSB_REQUEST_TYPE_VENDOR, VEND_JTAG_GETTDO, 0, 0, &tdo, 1, LIBUSB_TIMEOUT_MS); - if (r < 1) + LIBUSB_ENDPOINT_IN | LIBUSB_REQUEST_TYPE_VENDOR, VEND_JTAG_GETTDO, 0, 0, &tdo, 1, LIBUSB_TIMEOUT_MS, NULL); + if (r != ERROR_OK) return r; command_print(CMD, "%d", tdo); @@ -685,7 +688,7 @@ COMMAND_HANDLER(esp_usb_jtag_setio_cmd) d |= VEND_JTAG_SETIO_SRST; jtag_libusb_control_transfer(priv->usb_device, - 0x40, VEND_JTAG_SETIO, d, 0, NULL, 0, LIBUSB_TIMEOUT_MS); + 0x40, VEND_JTAG_SETIO, d, 0, NULL, 0, LIBUSB_TIMEOUT_MS, NULL); return ERROR_OK; } diff --git a/src/jtag/drivers/ft232r.c b/src/jtag/drivers/ft232r.c index 1d73af4e5c..c2ec78ad88 100644 --- a/src/jtag/drivers/ft232r.c +++ b/src/jtag/drivers/ft232r.c @@ -235,7 +235,7 @@ static int ft232r_speed(int divisor) if (jtag_libusb_control_transfer(adapter, LIBUSB_REQUEST_TYPE_VENDOR | LIBUSB_RECIPIENT_DEVICE | LIBUSB_ENDPOINT_OUT, - SIO_SET_BAUD_RATE, divisor, 0, NULL, 0, 1000) != 0) { + SIO_SET_BAUD_RATE, divisor, 0, NULL, 0, 1000, NULL) != ERROR_OK) { LOG_ERROR("cannot set baud rate"); return ERROR_JTAG_DEVICE_ERROR; } @@ -266,7 +266,7 @@ static int ft232r_init(void) /* Reset the device. */ if (jtag_libusb_control_transfer(adapter, LIBUSB_REQUEST_TYPE_VENDOR | LIBUSB_RECIPIENT_DEVICE | LIBUSB_ENDPOINT_OUT, - SIO_RESET, 0, 0, NULL, 0, 1000) != 0) { + SIO_RESET, 0, 0, NULL, 0, 1000, NULL) != ERROR_OK) { LOG_ERROR("unable to reset device"); return ERROR_JTAG_INIT_FAILED; } @@ -275,7 +275,7 @@ static int ft232r_init(void) if (jtag_libusb_control_transfer(adapter, LIBUSB_REQUEST_TYPE_VENDOR | LIBUSB_RECIPIENT_DEVICE | LIBUSB_ENDPOINT_OUT, SIO_SET_BITMODE, (1<usb_handle, + int retval = jtag_libusb_control_transfer(kitprog_handle->usb_handle, LIBUSB_ENDPOINT_IN | LIBUSB_REQUEST_TYPE_VENDOR | LIBUSB_RECIPIENT_DEVICE, CONTROL_TYPE_WRITE, (CONTROL_MODE_SET_PROGRAMMER_PROTOCOL << 8) | CONTROL_COMMAND_PROGRAM, - protocol, &status, 1, 0); + protocol, &status, 1, 0, &transferred); - if (transferred == 0) { + if (retval != ERROR_OK || transferred == 0) { LOG_DEBUG("Zero bytes transferred"); return ERROR_FAIL; } @@ -440,11 +440,11 @@ static int kitprog_get_status(void) /* Try a maximum of three times */ for (int i = 0; (i < 3) && (transferred == 0); i++) { - transferred = jtag_libusb_control_transfer(kitprog_handle->usb_handle, + jtag_libusb_control_transfer(kitprog_handle->usb_handle, LIBUSB_ENDPOINT_IN | LIBUSB_REQUEST_TYPE_VENDOR | LIBUSB_RECIPIENT_DEVICE, CONTROL_TYPE_READ, (CONTROL_MODE_POLL_PROGRAMMER_STATUS << 8) | CONTROL_COMMAND_PROGRAM, - 0, &status, 1, 0); + 0, &status, 1, 0, &transferred); jtag_sleep(1000); } @@ -466,13 +466,13 @@ static int kitprog_set_unknown(void) int transferred; char status = PROGRAMMER_NOK_NACK; - transferred = jtag_libusb_control_transfer(kitprog_handle->usb_handle, + int retval = jtag_libusb_control_transfer(kitprog_handle->usb_handle, LIBUSB_ENDPOINT_IN | LIBUSB_REQUEST_TYPE_VENDOR | LIBUSB_RECIPIENT_DEVICE, CONTROL_TYPE_WRITE, (0x03 << 8) | 0x04, - 0, &status, 1, 0); + 0, &status, 1, 0, &transferred); - if (transferred == 0) { + if (retval != ERROR_OK || transferred == 0) { LOG_DEBUG("Zero bytes transferred"); return ERROR_FAIL; } @@ -491,13 +491,13 @@ static int kitprog_acquire_psoc(uint8_t psoc_type, uint8_t acquire_mode, int transferred; char status = PROGRAMMER_NOK_NACK; - transferred = jtag_libusb_control_transfer(kitprog_handle->usb_handle, + int retval = jtag_libusb_control_transfer(kitprog_handle->usb_handle, LIBUSB_ENDPOINT_IN | LIBUSB_REQUEST_TYPE_VENDOR | LIBUSB_RECIPIENT_DEVICE, CONTROL_TYPE_WRITE, (CONTROL_MODE_ACQUIRE_SWD_TARGET << 8) | CONTROL_COMMAND_PROGRAM, - (max_attempts << 8) | (acquire_mode << 4) | psoc_type, &status, 1, 0); + (max_attempts << 8) | (acquire_mode << 4) | psoc_type, &status, 1, 0, &transferred); - if (transferred == 0) { + if (retval != ERROR_OK || transferred == 0) { LOG_DEBUG("Zero bytes transferred"); return ERROR_FAIL; } @@ -515,13 +515,13 @@ static int kitprog_reset_target(void) int transferred; char status = PROGRAMMER_NOK_NACK; - transferred = jtag_libusb_control_transfer(kitprog_handle->usb_handle, + int retval = jtag_libusb_control_transfer(kitprog_handle->usb_handle, LIBUSB_ENDPOINT_IN | LIBUSB_REQUEST_TYPE_VENDOR | LIBUSB_RECIPIENT_DEVICE, CONTROL_TYPE_WRITE, (CONTROL_MODE_RESET_TARGET << 8) | CONTROL_COMMAND_PROGRAM, - 0, &status, 1, 0); + 0, &status, 1, 0, &transferred); - if (transferred == 0) { + if (retval != ERROR_OK || transferred == 0) { LOG_DEBUG("Zero bytes transferred"); return ERROR_FAIL; } @@ -539,13 +539,13 @@ static int kitprog_swd_sync(void) int transferred; char status = PROGRAMMER_NOK_NACK; - transferred = jtag_libusb_control_transfer(kitprog_handle->usb_handle, + int retval = jtag_libusb_control_transfer(kitprog_handle->usb_handle, LIBUSB_ENDPOINT_IN | LIBUSB_REQUEST_TYPE_VENDOR | LIBUSB_RECIPIENT_DEVICE, CONTROL_TYPE_WRITE, (CONTROL_MODE_SYNCHRONIZE_TRANSFER << 8) | CONTROL_COMMAND_PROGRAM, - 0, &status, 1, 0); + 0, &status, 1, 0, &transferred); - if (transferred == 0) { + if (retval != ERROR_OK || transferred == 0) { LOG_DEBUG("Zero bytes transferred"); return ERROR_FAIL; } @@ -563,13 +563,13 @@ static int kitprog_swd_seq(uint8_t seq_type) int transferred; char status = PROGRAMMER_NOK_NACK; - transferred = jtag_libusb_control_transfer(kitprog_handle->usb_handle, + int retval = jtag_libusb_control_transfer(kitprog_handle->usb_handle, LIBUSB_ENDPOINT_IN | LIBUSB_REQUEST_TYPE_VENDOR | LIBUSB_RECIPIENT_DEVICE, CONTROL_TYPE_WRITE, (CONTROL_MODE_SEND_SWD_SEQUENCE << 8) | CONTROL_COMMAND_PROGRAM, - seq_type, &status, 1, 0); + seq_type, &status, 1, 0, &transferred); - if (transferred == 0) { + if (retval != ERROR_OK || transferred == 0) { LOG_DEBUG("Zero bytes transferred"); return ERROR_FAIL; } diff --git a/src/jtag/drivers/libusb_helper.c b/src/jtag/drivers/libusb_helper.c index 4b098b482f..c77fe78c28 100644 --- a/src/jtag/drivers/libusb_helper.c +++ b/src/jtag/drivers/libusb_helper.c @@ -216,17 +216,22 @@ void jtag_libusb_close(struct libusb_device_handle *dev) int jtag_libusb_control_transfer(struct libusb_device_handle *dev, uint8_t request_type, uint8_t request, uint16_t value, uint16_t index, char *bytes, - uint16_t size, unsigned int timeout) + uint16_t size, unsigned int timeout, int *transferred) { - int transferred = 0; - - transferred = libusb_control_transfer(dev, request_type, request, value, index, + int retval = libusb_control_transfer(dev, request_type, request, value, index, (unsigned char *)bytes, size, timeout); - if (transferred < 0) - transferred = 0; + if (retval < 0) { + LOG_ERROR("libusb_control_transfer error: %s", libusb_error_name(retval)); + if (transferred) + *transferred = 0; + return jtag_libusb_error(retval); + } - return transferred; + if (transferred) + *transferred = retval; + + return ERROR_OK; } int jtag_libusb_bulk_write(struct libusb_device_handle *dev, int ep, char *bytes, diff --git a/src/jtag/drivers/libusb_helper.h b/src/jtag/drivers/libusb_helper.h index 799e3e6a91..75f133519a 100644 --- a/src/jtag/drivers/libusb_helper.h +++ b/src/jtag/drivers/libusb_helper.h @@ -38,7 +38,8 @@ int jtag_libusb_open(const uint16_t vids[], const uint16_t pids[], void jtag_libusb_close(struct libusb_device_handle *dev); int jtag_libusb_control_transfer(struct libusb_device_handle *dev, uint8_t request_type, uint8_t request, uint16_t value, - uint16_t index, char *bytes, uint16_t size, unsigned int timeout); + uint16_t index, char *bytes, uint16_t size, unsigned int timeout, + int *transferred); int jtag_libusb_bulk_write(struct libusb_device_handle *dev, int ep, char *bytes, int size, int timeout, int *transferred); int jtag_libusb_bulk_read(struct libusb_device_handle *dev, int ep, diff --git a/src/jtag/drivers/opendous.c b/src/jtag/drivers/opendous.c index ad980bf238..2e1d648147 100644 --- a/src/jtag/drivers/opendous.c +++ b/src/jtag/drivers/opendous.c @@ -735,7 +735,7 @@ int opendous_usb_message(struct opendous_jtag *opendous_jtag, int out_length, in /* Write data from out_buffer to USB. */ int opendous_usb_write(struct opendous_jtag *opendous_jtag, int out_length) { - int result; + int result, transferred; if (out_length > OPENDOUS_OUT_BUFFER_SIZE) { LOG_ERROR("opendous_jtag_write illegal out_length=%d (max=%d)", out_length, OPENDOUS_OUT_BUFFER_SIZE); @@ -748,7 +748,11 @@ int opendous_usb_write(struct opendous_jtag *opendous_jtag, int out_length) if (opendous_probe->CONTROL_TRANSFER) { result = jtag_libusb_control_transfer(opendous_jtag->usb_handle, LIBUSB_REQUEST_TYPE_VENDOR | LIBUSB_RECIPIENT_DEVICE | LIBUSB_ENDPOINT_OUT, - FUNC_WRITE_DATA, 0, 0, (char *) usb_out_buffer, out_length, OPENDOUS_USB_TIMEOUT); + FUNC_WRITE_DATA, 0, 0, (char *)usb_out_buffer, out_length, OPENDOUS_USB_TIMEOUT, + &transferred); + /* FIXME: propagate error separately from transferred */ + if (result == ERROR_OK) + result = transferred; } else { jtag_libusb_bulk_write(opendous_jtag->usb_handle, OPENDOUS_WRITE_ENDPOINT, (char *)usb_out_buffer, out_length, OPENDOUS_USB_TIMEOUT, &result); @@ -768,6 +772,8 @@ int opendous_usb_write(struct opendous_jtag *opendous_jtag, int out_length) /* Read data from USB into in_buffer. */ int opendous_usb_read(struct opendous_jtag *opendous_jtag) { + int transferred; + #ifdef _DEBUG_USB_COMMS_ LOG_DEBUG("USB read begin"); #endif @@ -775,7 +781,11 @@ int opendous_usb_read(struct opendous_jtag *opendous_jtag) if (opendous_probe->CONTROL_TRANSFER) { result = jtag_libusb_control_transfer(opendous_jtag->usb_handle, LIBUSB_REQUEST_TYPE_VENDOR | LIBUSB_RECIPIENT_DEVICE | LIBUSB_ENDPOINT_IN, - FUNC_READ_DATA, 0, 0, (char *) usb_in_buffer, OPENDOUS_IN_BUFFER_SIZE, OPENDOUS_USB_TIMEOUT); + FUNC_READ_DATA, 0, 0, (char *)usb_in_buffer, OPENDOUS_IN_BUFFER_SIZE, OPENDOUS_USB_TIMEOUT, + &transferred); + /* FIXME: propagate error separately from transferred */ + if (result == ERROR_OK) + result = transferred; } else { jtag_libusb_bulk_read(opendous_jtag->usb_handle, OPENDOUS_READ_ENDPOINT, (char *)usb_in_buffer, OPENDOUS_IN_BUFFER_SIZE, OPENDOUS_USB_TIMEOUT, &result); diff --git a/src/jtag/drivers/openjtag.c b/src/jtag/drivers/openjtag.c index 12ea463302..fe3a8ff7f9 100644 --- a/src/jtag/drivers/openjtag.c +++ b/src/jtag/drivers/openjtag.c @@ -239,10 +239,10 @@ static int openjtag_buf_write_cy7c65215( ret = jtag_libusb_control_transfer(usbh, CY7C65215_JTAG_REQUEST, CY7C65215_JTAG_WRITE, size, 0, - NULL, 0, CY7C65215_USB_TIMEOUT); - if (ret < 0) { - LOG_ERROR("vendor command failed, error %d", ret); - return ERROR_JTAG_DEVICE_ERROR; + NULL, 0, CY7C65215_USB_TIMEOUT, NULL); + if (ret != ERROR_OK) { + LOG_ERROR("vendor command failed"); + return ret; } if (jtag_libusb_bulk_write(usbh, ep_out, (char *)buf, size, @@ -306,10 +306,10 @@ static int openjtag_buf_read_cy7c65215( ret = jtag_libusb_control_transfer(usbh, CY7C65215_JTAG_REQUEST, CY7C65215_JTAG_READ, qty, 0, - NULL, 0, CY7C65215_USB_TIMEOUT); - if (ret < 0) { - LOG_ERROR("vendor command failed, error %d", ret); - return ERROR_JTAG_DEVICE_ERROR; + NULL, 0, CY7C65215_USB_TIMEOUT, NULL); + if (ret != ERROR_OK) { + LOG_ERROR("vendor command failed"); + return ret; } if (jtag_libusb_bulk_read(usbh, ep_in, (char *)buf, qty, @@ -455,8 +455,8 @@ static int openjtag_init_cy7c65215(void) ret = jtag_libusb_control_transfer(usbh, CY7C65215_JTAG_REQUEST, CY7C65215_JTAG_ENABLE, - 0, 0, NULL, 0, CY7C65215_USB_TIMEOUT); - if (ret < 0) { + 0, 0, NULL, 0, CY7C65215_USB_TIMEOUT, NULL); + if (ret != ERROR_OK) { LOG_ERROR("could not enable JTAG module"); goto err; } @@ -466,7 +466,7 @@ static int openjtag_init_cy7c65215(void) err: if (usbh) jtag_libusb_close(usbh); - return ERROR_JTAG_INIT_FAILED; + return ret; } static int openjtag_init(void) @@ -508,8 +508,8 @@ static int openjtag_quit_cy7c65215(void) ret = jtag_libusb_control_transfer(usbh, CY7C65215_JTAG_REQUEST, CY7C65215_JTAG_DISABLE, - 0, 0, NULL, 0, CY7C65215_USB_TIMEOUT); - if (ret < 0) + 0, 0, NULL, 0, CY7C65215_USB_TIMEOUT, NULL); + if (ret != ERROR_OK) LOG_WARNING("could not disable JTAG module"); jtag_libusb_close(usbh); diff --git a/src/jtag/drivers/usb_blaster/ublast2_access_libusb.c b/src/jtag/drivers/usb_blaster/ublast2_access_libusb.c index 7f9781869c..f5e0026a7b 100644 --- a/src/jtag/drivers/usb_blaster/ublast2_access_libusb.c +++ b/src/jtag/drivers/usb_blaster/ublast2_access_libusb.c @@ -103,7 +103,8 @@ static int ublast2_write_firmware_section(struct libusb_device_handle *libusb_de 0, (char *)data_ptr, chunk_size, - 100); + 100, + NULL); bytes_remaining -= chunk_size; addr += chunk_size; @@ -154,7 +155,8 @@ static int load_usb_blaster_firmware(struct libusb_device_handle *libusb_dev, 0, &value, 1, - 100); + 100, + NULL); /* Download all sections in the image to ULINK */ for (unsigned int i = 0; i < ublast2_firmware_image.num_sections; i++) { @@ -175,7 +177,8 @@ static int load_usb_blaster_firmware(struct libusb_device_handle *libusb_dev, 0, &value, 1, - 100); + 100, + NULL); error_close_firmware: image_close(&ublast2_firmware_image); @@ -245,7 +248,8 @@ static int ublast2_libusb_init(struct ublast_lowlevel *low) 0, buffer, 5, - 100); + 100, + NULL); LOG_INFO("Altera USB-Blaster II found (Firm. rev. = %s)", buffer); -- 2.30.2