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 d612baa
("jtag_libusb_bulk_read|write: return error code instead of size")

Change-Id: I14d9bff3e845675be03465c307a136e69eebc317
Signed-off-by: Antonio Borneo <[email protected]>
Reviewed-on: https://review.openocd.org/c/openocd/+/7756
Tested-by: jenkins
Reviewed-by: ahmed BOUDJELIDA <[email protected]>

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;
 }

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<<tck_gpio) | (1<<tdi_gpio) | (1<<tms_gpio) | (1<<ntrst_gpio) | (1<<nsysrst_gpio) | 0x400,
-		0, NULL, 0, 1000) != 0) {
+		0, NULL, 0, 1000, NULL) != ERROR_OK) {
 		LOG_ERROR("cannot set sync bitbang mode");
 		return ERROR_JTAG_INIT_FAILED;
 	}
@@ -288,13 +288,13 @@ static int ft232r_init(void)
 	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) {
+		0, NULL, 0, 1000, NULL) != ERROR_OK) {
 		LOG_ERROR("cannot set baud rate");
 		return ERROR_JTAG_INIT_FAILED;
 	}
 	if (jtag_libusb_control_transfer(adapter,
 		LIBUSB_REQUEST_TYPE_VENDOR | LIBUSB_RECIPIENT_DEVICE | LIBUSB_ENDPOINT_OUT,
-		SIO_SET_LATENCY_TIMER, latency_timer, 0, NULL, 0, 1000) != 0) {
+		SIO_SET_LATENCY_TIMER, latency_timer, 0, NULL, 0, 1000, NULL) != ERROR_OK) {
 		LOG_ERROR("unable to set latency timer");
 		return ERROR_JTAG_INIT_FAILED;
 	}
@@ -315,7 +315,7 @@ static int ft232r_quit(void)
 		if (jtag_libusb_control_transfer(adapter,
 			LIBUSB_REQUEST_TYPE_VENDOR | LIBUSB_RECIPIENT_DEVICE | LIBUSB_ENDPOINT_OUT,
 			SIO_SET_BITMODE, ft232r_restore_bitmode,
-			0, NULL, 0, 1000) != 0) {
+			0, NULL, 0, 1000, NULL) != ERROR_OK) {
 			LOG_ERROR("cannot set bitmode to restore serial port");
 		}
 	}

src/jtag/drivers/kitprog.c

@@ -414,13 +414,13 @@ static int kitprog_set_protocol(uint8_t protocol)
 	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_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;
 	}

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,

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,

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,14 +772,20 @@ 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
 	int result;
 	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);

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);