From fed42ccfd3dff0c3dcfa7f017bbd26eff3d4f15c Mon Sep 17 00:00:00 2001 From: Adrian Negreanu Date: Fri, 6 Nov 2020 11:57:04 +0200 Subject: [PATCH] cmsis-dap: don't update the packet size across backends. The hidapi cmsis-dap backend is using a packet_size of 64+1: 64 is the bMaxPacketSize0 and 1 is the hid Report-Id. In hidapi::hid_write(), the packet_size is decremented by 1 and stored for the next transfer. The packet_size is now valid bMaxPacketSize0=64, so when hid_read() is called, libusb_bulk_transfer() finishes w/o timeout. For the libusb bulk backend, the same packet_size of 64+1 is used, but there's no hid_write() to decrement and store it for the next read. So the next time a read is done, it will try to read 64+1 bytes. Fix this by putting the packet logic within each backend. Use calloc() to allocate the struct cmsis_dap to be on safer side. Change-Id: I0c450adbc7674d5fcd8208dd23062d5cdd209efd Signed-off-by: Adrian Negreanu Reviewed-on: http://openocd.zylin.com/5920 Tested-by: jenkins Reviewed-by: Tarek BOCHKATI Reviewed-by: Tomas Vanek --- src/jtag/drivers/cmsis_dap.c | 44 ++++++++------------------- src/jtag/drivers/cmsis_dap.h | 4 +++ src/jtag/drivers/cmsis_dap_usb_bulk.c | 37 ++++++++++++++++++++-- src/jtag/drivers/cmsis_dap_usb_hid.c | 40 +++++++++++++++++++----- 4 files changed, 84 insertions(+), 41 deletions(-) diff --git a/src/jtag/drivers/cmsis_dap.c b/src/jtag/drivers/cmsis_dap.c index fd565398ff..7a5acc3140 100644 --- a/src/jtag/drivers/cmsis_dap.c +++ b/src/jtag/drivers/cmsis_dap.c @@ -225,16 +225,12 @@ static int cmsis_dap_open(void) { const struct cmsis_dap_backend *backend = NULL; - struct cmsis_dap *dap = malloc(sizeof(struct cmsis_dap)); + struct cmsis_dap *dap = calloc(1, sizeof(struct cmsis_dap)); if (dap == NULL) { LOG_ERROR("unable to allocate memory"); return ERROR_FAIL; } - dap->caps = 0; - dap->mode = 0; - dap->packet_size = 0; /* initialized by backend */ - if (cmsis_dap_backend >= 0) { /* Use forced backend */ backend = cmsis_dap_backends[cmsis_dap_backend]; @@ -257,17 +253,7 @@ static int cmsis_dap_open(void) return ERROR_FAIL; } - assert(dap->packet_size > 0); - dap->backend = backend; - dap->packet_buffer = malloc(dap->packet_size); - - if (dap->packet_buffer == NULL) { - LOG_ERROR("unable to allocate memory"); - dap->backend->close(dap); - free(dap); - return ERROR_FAIL; - } cmsis_dap_handle = dap; @@ -929,24 +915,20 @@ static int cmsis_dap_init(void) if (data[0] == 2) { /* short */ uint16_t pkt_sz = data[1] + (data[2] << 8); + if (pkt_sz != cmsis_dap_handle->packet_size) { - /* 4 bytes of command header + 5 bytes per register - * write. For bulk read sequences just 4 bytes are - * needed per transfer, so this is suboptimal. */ - pending_queue_len = (pkt_sz - 4) / 5; - - if (cmsis_dap_handle->packet_size != pkt_sz + 1) { - /* reallocate buffer */ - cmsis_dap_handle->packet_size = pkt_sz + 1; - cmsis_dap_handle->packet_buffer = realloc(cmsis_dap_handle->packet_buffer, - cmsis_dap_handle->packet_size); - if (cmsis_dap_handle->packet_buffer == NULL) { - LOG_ERROR("unable to reallocate memory"); - return ERROR_FAIL; - } - } + /* 4 bytes of command header + 5 bytes per register + * write. For bulk read sequences just 4 bytes are + * needed per transfer, so this is suboptimal. */ + pending_queue_len = (pkt_sz - 4) / 5; - LOG_DEBUG("CMSIS-DAP: Packet Size = %" PRId16, pkt_sz); + free(cmsis_dap_handle->packet_buffer); + retval = cmsis_dap_handle->backend->packet_buffer_alloc(cmsis_dap_handle, pkt_sz); + if (retval != ERROR_OK) + return retval; + + LOG_DEBUG("CMSIS-DAP: Packet Size = %" PRIu16, pkt_sz); + } } /* INFO_ID_PKT_CNT - byte */ diff --git a/src/jtag/drivers/cmsis_dap.h b/src/jtag/drivers/cmsis_dap.h index 054621cd51..a096a95c0d 100644 --- a/src/jtag/drivers/cmsis_dap.h +++ b/src/jtag/drivers/cmsis_dap.h @@ -13,6 +13,7 @@ struct cmsis_dap { uint16_t packet_size; int packet_count; uint8_t *packet_buffer; + uint16_t packet_buffer_size; uint8_t caps; uint8_t mode; }; @@ -23,10 +24,13 @@ struct cmsis_dap_backend { void (*close)(struct cmsis_dap *dap); int (*read)(struct cmsis_dap *dap, int timeout_ms); int (*write)(struct cmsis_dap *dap, int len, int timeout_ms); + int (*packet_buffer_alloc)(struct cmsis_dap *dap, unsigned int pkt_sz); }; extern const struct cmsis_dap_backend cmsis_dap_hid_backend; extern const struct cmsis_dap_backend cmsis_dap_usb_backend; extern const struct command_registration cmsis_dap_usb_subcommand_handlers[]; +#define REPORT_ID_SIZE 1 + #endif diff --git a/src/jtag/drivers/cmsis_dap_usb_bulk.c b/src/jtag/drivers/cmsis_dap_usb_bulk.c index 0134c03765..7efea51666 100644 --- a/src/jtag/drivers/cmsis_dap_usb_bulk.c +++ b/src/jtag/drivers/cmsis_dap_usb_bulk.c @@ -50,6 +50,9 @@ struct cmsis_dap_backend_data { static int cmsis_dap_usb_interface = -1; +static void cmsis_dap_usb_close(struct cmsis_dap *dap); +static int cmsis_dap_usb_alloc(struct cmsis_dap *dap, unsigned int pkt_sz); + static int cmsis_dap_usb_open(struct cmsis_dap *dap, uint16_t vids[], uint16_t pids[], char *serial) { int err; @@ -356,12 +359,21 @@ static int cmsis_dap_usb_open(struct cmsis_dap *dap, uint16_t vids[], uint16_t p return ERROR_FAIL; } - dap->packet_size = packet_size + 1; /* "+ 1" for compatibility with the HID backend */ + dap->packet_size = packet_size; + dap->packet_buffer_size = packet_size + REPORT_ID_SIZE; dap->bdata->usb_ctx = ctx; dap->bdata->dev_handle = dev_handle; dap->bdata->ep_out = ep_out; dap->bdata->ep_in = ep_in; dap->bdata->interface = interface_num; + + dap->packet_buffer = malloc(dap->packet_buffer_size); + if (dap->packet_buffer == NULL) { + LOG_ERROR("unable to allocate memory"); + cmsis_dap_usb_close(dap); + return ERROR_FAIL; + } + return ERROR_OK; } @@ -381,6 +393,8 @@ static void cmsis_dap_usb_close(struct cmsis_dap *dap) libusb_exit(dap->bdata->usb_ctx); free(dap->bdata); dap->bdata = NULL; + free(dap->packet_buffer); + dap->packet_buffer = NULL; } static int cmsis_dap_usb_read(struct cmsis_dap *dap, int timeout_ms) @@ -399,7 +413,7 @@ static int cmsis_dap_usb_read(struct cmsis_dap *dap, int timeout_ms) } } - memset(&dap->packet_buffer[transferred], 0, dap->packet_size - transferred); + memset(&dap->packet_buffer[transferred], 0, dap->packet_buffer_size - transferred); return transferred; } @@ -411,7 +425,7 @@ static int cmsis_dap_usb_write(struct cmsis_dap *dap, int txlen, int timeout_ms) /* skip the first byte that is only used by the HID backend */ err = libusb_bulk_transfer(dap->bdata->dev_handle, dap->bdata->ep_out, - dap->packet_buffer + 1, txlen - 1, &transferred, timeout_ms); + dap->packet_buffer + REPORT_ID_SIZE, txlen - REPORT_ID_SIZE, &transferred, timeout_ms); if (err) { if (err == LIBUSB_ERROR_TIMEOUT) { return ERROR_TIMEOUT_REACHED; @@ -424,6 +438,22 @@ static int cmsis_dap_usb_write(struct cmsis_dap *dap, int txlen, int timeout_ms) return transferred; } +static int cmsis_dap_usb_alloc(struct cmsis_dap *dap, unsigned int pkt_sz) +{ + unsigned int packet_buffer_size = pkt_sz + REPORT_ID_SIZE; + uint8_t *buf = malloc(packet_buffer_size); + if (buf == NULL) { + LOG_ERROR("unable to allocate CMSIS-DAP packet buffer"); + return ERROR_FAIL; + } + + dap->packet_buffer = buf; + dap->packet_size = pkt_sz; + dap->packet_buffer_size = packet_buffer_size; + + return ERROR_OK; +} + COMMAND_HANDLER(cmsis_dap_handle_usb_interface_command) { if (CMD_ARGC == 1) @@ -451,4 +481,5 @@ const struct cmsis_dap_backend cmsis_dap_usb_backend = { .close = cmsis_dap_usb_close, .read = cmsis_dap_usb_read, .write = cmsis_dap_usb_write, + .packet_buffer_alloc = cmsis_dap_usb_alloc, }; diff --git a/src/jtag/drivers/cmsis_dap_usb_hid.c b/src/jtag/drivers/cmsis_dap_usb_hid.c index 681aef1712..b290d6f818 100644 --- a/src/jtag/drivers/cmsis_dap_usb_hid.c +++ b/src/jtag/drivers/cmsis_dap_usb_hid.c @@ -40,12 +40,13 @@ #include "cmsis_dap.h" -#define PACKET_SIZE (64 + 1) /* 64 bytes plus report id */ - struct cmsis_dap_backend_data { hid_device *dev_handle; }; +static void cmsis_dap_hid_close(struct cmsis_dap *dap); +static int cmsis_dap_hid_alloc(struct cmsis_dap *dap, unsigned int pkt_sz); + static int cmsis_dap_hid_open(struct cmsis_dap *dap, uint16_t vids[], uint16_t pids[], char *serial) { hid_device *dev = NULL; @@ -145,7 +146,7 @@ static int cmsis_dap_hid_open(struct cmsis_dap *dap, uint16_t vids[], uint16_t p * without this info we cannot communicate with the adapter. * For the moment we have to hard code the packet size */ - dap->packet_size = PACKET_SIZE; + unsigned int packet_size = 64; /* atmel cmsis-dap uses 512 byte reports */ /* except when it doesn't e.g. with mEDBG on SAMD10 Xplained @@ -153,10 +154,16 @@ static int cmsis_dap_hid_open(struct cmsis_dap *dap, uint16_t vids[], uint16_t p /* TODO: HID report descriptor should be parsed instead of * hardcoding a match by VID */ if (target_vid == 0x03eb && target_pid != 0x2145 && target_pid != 0x2175) - dap->packet_size = 512 + 1; + packet_size = 512; dap->bdata->dev_handle = dev; + int retval = cmsis_dap_hid_alloc(dap, packet_size); + if (retval != ERROR_OK) { + cmsis_dap_hid_close(dap); + return ERROR_FAIL; + } + return ERROR_OK; } @@ -166,11 +173,13 @@ static void cmsis_dap_hid_close(struct cmsis_dap *dap) hid_exit(); free(dap->bdata); dap->bdata = NULL; + free(dap->packet_buffer); + dap->packet_buffer = NULL; } static int cmsis_dap_hid_read(struct cmsis_dap *dap, int timeout_ms) { - int retval = hid_read_timeout(dap->bdata->dev_handle, dap->packet_buffer, dap->packet_size, timeout_ms); + int retval = hid_read_timeout(dap->bdata->dev_handle, dap->packet_buffer, dap->packet_buffer_size, timeout_ms); if (retval == 0) { return ERROR_TIMEOUT_REACHED; @@ -187,10 +196,10 @@ static int cmsis_dap_hid_write(struct cmsis_dap *dap, int txlen, int timeout_ms) (void) timeout_ms; /* Pad the rest of the TX buffer with 0's */ - memset(dap->packet_buffer + txlen, 0, dap->packet_size - txlen); + memset(dap->packet_buffer + txlen, 0, dap->packet_buffer_size - txlen); /* write data to device */ - int retval = hid_write(dap->bdata->dev_handle, dap->packet_buffer, dap->packet_size); + int retval = hid_write(dap->bdata->dev_handle, dap->packet_buffer, dap->packet_buffer_size); if (retval == -1) { LOG_ERROR("error writing data: %ls", hid_error(dap->bdata->dev_handle)); return ERROR_FAIL; @@ -199,10 +208,27 @@ static int cmsis_dap_hid_write(struct cmsis_dap *dap, int txlen, int timeout_ms) return retval; } +static int cmsis_dap_hid_alloc(struct cmsis_dap *dap, unsigned int pkt_sz) +{ + unsigned int packet_buffer_size = pkt_sz + REPORT_ID_SIZE; + uint8_t *buf = malloc(packet_buffer_size); + if (buf == NULL) { + LOG_ERROR("unable to allocate CMSIS-DAP packet buffer"); + return ERROR_FAIL; + } + + dap->packet_buffer = buf; + dap->packet_size = pkt_sz; + dap->packet_buffer_size = packet_buffer_size; + + return ERROR_OK; +} + const struct cmsis_dap_backend cmsis_dap_hid_backend = { .name = "hid", .open = cmsis_dap_hid_open, .close = cmsis_dap_hid_close, .read = cmsis_dap_hid_read, .write = cmsis_dap_hid_write, + .packet_buffer_alloc = cmsis_dap_hid_alloc, }; -- 2.30.2