From a1fc87fdfbe4121c478d6c1dc9007d64fe714333 Mon Sep 17 00:00:00 2001 From: dragonmux Date: Wed, 6 Nov 2024 22:14:16 +0000 Subject: [PATCH 01/16] hosted/cmsis_dap: Fixed the nomenclature of the CMSIS-DAP packet size variable --- src/platforms/hosted/cmsis_dap.c | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/src/platforms/hosted/cmsis_dap.c b/src/platforms/hosted/cmsis_dap.c index e226f2d5c95..71c8b371911 100644 --- a/src/platforms/hosted/cmsis_dap.c +++ b/src/platforms/hosted/cmsis_dap.c @@ -103,9 +103,9 @@ static hid_device *handle = NULL; static uint8_t buffer[1025U]; /* * Start by defaulting this to the typical size of `DAP_PACKET_SIZE` for FS USB. This value is pulled from here: - * https://www.keil.com/pack/doc/CMSIS/DAP/html/group__DAP__Config__Debug__gr.html#gaa28bb1da2661291634c4a8fb3e227404 + * https://arm-software.github.io/CMSIS-DAP/latest/group__DAP__Config__Debug__gr.html#gaa28bb1da2661291634c4a8fb3e227404 */ -static size_t packet_size = 64U; +static size_t dap_packet_size = 64U; bool dap_has_swd_sequence = false; dap_version_s dap_adaptor_version(dap_info_e version_kind); @@ -157,7 +157,7 @@ static inline bool dap_version_compare_le(const dap_version_s lhs, const dap_ver */ static inline size_t dap_max_transfer_data(size_t command_header_len) { - const size_t result = packet_size - command_header_len; + const size_t result = dap_packet_size - command_header_len; /* Allow for an additional byte of payload overhead when sending data in HID Report payloads */ if (type == CMSIS_TYPE_HID) @@ -223,11 +223,11 @@ static bool dap_init_hid(void) * Base the report length information for the device on the max packet length from its descriptors. * Add 1 to account for HIDAPI's need to prefix with a report type byte. */ - packet_size = bmda_probe_info.max_packet_length + 1U; + dap_packet_size = bmda_probe_info.max_packet_length + 1U; /* Handle the NXP LPC11U3x CMSIS-DAP v1.0.7 implementation needing a 64 byte report length */ if (bmda_probe_info.vid == 0x1fc9U && bmda_probe_info.pid == 0x0132U) - packet_size = 64U + 1U; + dap_packet_size = 64U + 1U; /* Now open the device with HIDAPI so we can talk with it */ handle = hid_open(bmda_probe_info.vid, bmda_probe_info.pid, serial[0] ? serial : NULL); @@ -255,7 +255,7 @@ static bool dap_init_bulk(void) return false; } /* Base the packet size on the one retrieved from the device descriptors */ - packet_size = bmda_probe_info.max_packet_length; + dap_packet_size = bmda_probe_info.max_packet_length; in_ep = bmda_probe_info.in_ep; out_ep = bmda_probe_info.out_ep; return true; @@ -309,7 +309,7 @@ bool dap_init(bool allow_fallback) /* Report the failure */ DEBUG_WARN("Failed to get adaptor packet size, assuming descriptor provided size\n"); else - packet_size = dap_packet_size + (type == CMSIS_TYPE_HID ? 1U : 0U); + dap_packet_size = dap_packet_size + (type == CMSIS_TYPE_HID ? 1U : 0U); /* Try to get the device's capabilities */ const size_t size = dap_info(DAP_INFO_CAPABILITIES, &dap_caps, sizeof(dap_caps)); @@ -459,17 +459,17 @@ ssize_t dbg_dap_cmd_hid(const uint8_t *const request_data, const size_t request_ const size_t response_length) { // Need room to prepend HID Report ID byte - if (request_length + 1U > packet_size) { - DEBUG_ERROR( - "Attempted to make over-long request of %zu bytes, max length is %zu\n", request_length + 1U, packet_size); + if (request_length + 1U > dap_packet_size) { + DEBUG_ERROR("Attempted to make over-long request of %zu bytes, max length is %zu\n", request_length + 1U, + dap_packet_size); exit(-1); } - memset(buffer + request_length + 1U, 0xff, packet_size - (request_length + 1U)); + memset(buffer + request_length + 1U, 0xff, dap_packet_size - (request_length + 1U)); buffer[0] = 0x00; // Report ID memcpy(buffer + 1, request_data, request_length); - const int result = hid_write(handle, buffer, packet_size); + const int result = hid_write(handle, buffer, dap_packet_size); if (result < 0) { DEBUG_ERROR("CMSIS-DAP write error: %ls\n", hid_error(handle)); exit(-1); @@ -526,16 +526,16 @@ static ssize_t dap_run_cmd_raw(const uint8_t *const request_data, const size_t r /* Provide enough space for up to a HS USB HID payload */ uint8_t data[1024]; /* Make sure that we're not about to blow this buffer when we request data back */ - if (sizeof(data) < packet_size) { + if (sizeof(data) < dap_packet_size) { DEBUG_ERROR("CMSIS-DAP request would exceed response buffer\n"); return -1; } ssize_t response = -1; if (type == CMSIS_TYPE_HID) - response = dbg_dap_cmd_hid(request_data, request_length, data, packet_size); + response = dbg_dap_cmd_hid(request_data, request_length, data, dap_packet_size); else if (type == CMSIS_TYPE_BULK) - response = dbg_dap_cmd_bulk(request_data, request_length, data, packet_size); + response = dbg_dap_cmd_bulk(request_data, request_length, data, dap_packet_size); if (response < 0) return response; const size_t result = (size_t)response; From 6c05e1ebc6daa749c10cc640176f7b49c5bcead6 Mon Sep 17 00:00:00 2001 From: dragonmux Date: Wed, 6 Nov 2024 22:17:59 +0000 Subject: [PATCH 02/16] hosted/cmsis_dap: Implemented a variant on `dap_run_cmd()` which is specialised in mass data transfers so we get the the actual amount transferred --- src/platforms/hosted/cmsis_dap.c | 17 +++++++++++++++++ src/platforms/hosted/dap.h | 2 ++ 2 files changed, 19 insertions(+) diff --git a/src/platforms/hosted/cmsis_dap.c b/src/platforms/hosted/cmsis_dap.c index 71c8b371911..b02fea4fdcb 100644 --- a/src/platforms/hosted/cmsis_dap.c +++ b/src/platforms/hosted/cmsis_dap.c @@ -561,6 +561,23 @@ bool dap_run_cmd(const void *const request_data, const size_t request_length, vo return (size_t)result >= response_length; } +bool dap_run_transfer(const void *const request_data, const size_t request_length, void *const response_data, + const size_t response_length, size_t *const actual_length) +{ + /* + * This function works almost exactly the same as dap_run_cmd(), but captures and preserves the resulting + * response length if the result is not an outright failure. It sets the actual response length to 0 when it is. + */ + const ssize_t result = + dap_run_cmd_raw((const uint8_t *)request_data, request_length, (uint8_t *)response_data, response_length) - 1U; + if (result < 0) { + *actual_length = 0U; + return false; + } + *actual_length = (size_t)result; + return *actual_length >= response_length; +} + static void dap_adiv5_mem_read(adiv5_access_port_s *ap, void *dest, target_addr64_t src, size_t len) { if (len == 0U) diff --git a/src/platforms/hosted/dap.h b/src/platforms/hosted/dap.h index f7065530ad5..cd066c7e24e 100644 --- a/src/platforms/hosted/dap.h +++ b/src/platforms/hosted/dap.h @@ -95,6 +95,8 @@ bool dap_mem_read_block(adiv5_access_port_s *target_ap, void *dest, target_addr6 bool dap_mem_write_block( adiv5_access_port_s *target_ap, target_addr64_t dest, const void *src, size_t len, align_e align); bool dap_run_cmd(const void *request_data, size_t request_length, void *response_data, size_t response_length); +bool dap_run_transfer(const void *request_data, size_t request_length, void *response_data, size_t response_length, + size_t *actual_length); bool dap_jtag_configure(void); void dap_dp_abort(adiv5_debug_port_s *target_dp, uint32_t abort); From 0bf6b959b8aded3cf39f00b1b2d2c440b271df76 Mon Sep 17 00:00:00 2001 From: dragonmux Date: Wed, 6 Nov 2024 22:21:07 +0000 Subject: [PATCH 03/16] hosted/cmsis_dap: Fixed the lack of ZLP handling when a transfer moves a full packet amount of data, messing up the Bulk transfer transport state --- src/platforms/hosted/cmsis_dap.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/platforms/hosted/cmsis_dap.c b/src/platforms/hosted/cmsis_dap.c index b02fea4fdcb..d1d9e71cf62 100644 --- a/src/platforms/hosted/cmsis_dap.c +++ b/src/platforms/hosted/cmsis_dap.c @@ -64,6 +64,7 @@ #include #include #include +#include #include "bmp_hosted.h" #include "dap.h" @@ -512,6 +513,13 @@ ssize_t dbg_dap_cmd_bulk(const uint8_t *const request_data, const size_t request return response_result; } } while (response_data[0] != request_data[0]); + /* If the response requested is the size of the packet size for the adaptor, generate a ZLP read to clean state */ + if (transferred == (int)dap_packet_size) { + uint8_t zlp; + int zlp_read = 0; + libusb_bulk_transfer(usb_handle, in_ep, &zlp, sizeof(zlp), &zlp_read, TRANSFER_TIMEOUT_MS); + assert(zlp_read == 0); + } return transferred; } From 06eda378b9db5cbfe4167759a9864948e0aa772e Mon Sep 17 00:00:00 2001 From: dragonmux Date: Wed, 6 Nov 2024 22:22:28 +0000 Subject: [PATCH 04/16] hosted/dap_command: Fixed `perform_dap_transfer_block_read()` not properly detecting and handling short responses due to target errors, leading to very bad things happening on return --- src/platforms/hosted/dap_command.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/platforms/hosted/dap_command.c b/src/platforms/hosted/dap_command.c index df56d2d129b..81df01e03bb 100644 --- a/src/platforms/hosted/dap_command.c +++ b/src/platforms/hosted/dap_command.c @@ -161,9 +161,16 @@ bool perform_dap_transfer_block_read( write_le2(request.block_count, 0, block_count); dap_transfer_block_response_read_s response; + size_t response_length; /* Run the request having set up the request buffer */ - if (!dap_run_cmd(&request, sizeof(request), &response, 3U + (block_count * 4U))) + if (!dap_run_transfer(&request, sizeof(request), &response, 3U + (block_count * 4U), &response_length)) { + /* Check if we got any response bytes back and if we got enough, extract the status. */ + if (response_length < 3U) + exit(1); + /* We got enough response bytes back for the status to be valid, so put that in the DP's fault member */ + target_dp->fault = response.status & DAP_TRANSFER_STATUS_MASK; return false; + } /* Check the response over */ const uint16_t blocks_read = read_le2(response.count, 0); From 1b8f10296b5247142069e42a0469952ab90c9f84 Mon Sep 17 00:00:00 2001 From: dragonmux Date: Wed, 6 Nov 2024 23:51:53 +0000 Subject: [PATCH 05/16] hosted/cmsis_dap: Fixed a small error in `dap_adiv5_mem_read()` which resulted in it not properly taking into account the command byte prefix in responses --- src/platforms/hosted/cmsis_dap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/platforms/hosted/cmsis_dap.c b/src/platforms/hosted/cmsis_dap.c index d1d9e71cf62..2b862a05fd6 100644 --- a/src/platforms/hosted/cmsis_dap.c +++ b/src/platforms/hosted/cmsis_dap.c @@ -598,7 +598,7 @@ static void dap_adiv5_mem_read(adiv5_access_port_s *ap, void *dest, target_addr6 return; } /* Otherwise proceed blockwise */ - const size_t blocks_per_transfer = dap_max_transfer_data(DAP_CMD_BLOCK_READ_HDR_LEN) >> 2U; + const size_t blocks_per_transfer = dap_max_transfer_data(DAP_CMD_BLOCK_READ_HDR_LEN + 1U) >> 2U; uint8_t *const data = (uint8_t *)dest; for (size_t offset = 0; offset < len;) { /* Setup AP_TAR every loop as failing to do so results in it wrapping */ From 658f2d2269a55f6f97cba5b5713852e2f23835d6 Mon Sep 17 00:00:00 2001 From: dragonmux Date: Wed, 6 Nov 2024 23:55:22 +0000 Subject: [PATCH 06/16] hosted/cmsis_dap: Fixed a small error in `dap_adiv6_mem_read()` which resulted in it not properly taking into account the command byte prefix in responses --- src/platforms/hosted/cmsis_dap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/platforms/hosted/cmsis_dap.c b/src/platforms/hosted/cmsis_dap.c index 2b862a05fd6..d8dec507f14 100644 --- a/src/platforms/hosted/cmsis_dap.c +++ b/src/platforms/hosted/cmsis_dap.c @@ -679,7 +679,7 @@ static void dap_adiv6_mem_read( return; } /* Otherwise proceed blockwise */ - const size_t blocks_per_transfer = dap_max_transfer_data(DAP_CMD_BLOCK_READ_HDR_LEN) >> 2U; + const size_t blocks_per_transfer = dap_max_transfer_data(DAP_CMD_BLOCK_READ_HDR_LEN + 1U) >> 2U; uint8_t *const data = (uint8_t *)dest; for (size_t offset = 0; offset < len;) { /* Setup AP_TAR every loop as failing to do so results in it wrapping */ From fa97ac73c6f939130da529104f8bde8e9ff2476d Mon Sep 17 00:00:00 2001 From: dragonmux Date: Wed, 6 Nov 2024 23:59:06 +0000 Subject: [PATCH 07/16] hosted/dap_command: Fixed a clang-tidy lint about a widening cast in `perform_dap_transfer_block_write()` --- src/platforms/hosted/dap_command.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/platforms/hosted/dap_command.c b/src/platforms/hosted/dap_command.c index 81df01e03bb..c30feaad37f 100644 --- a/src/platforms/hosted/dap_command.c +++ b/src/platforms/hosted/dap_command.c @@ -207,7 +207,7 @@ bool perform_dap_transfer_block_write( dap_transfer_block_response_write_s response; /* Run the request having set up the request buffer */ - if (!dap_run_cmd(&request, DAP_CMD_BLOCK_WRITE_HDR_LEN + (block_count * 4U), &response, sizeof(response))) + if (!dap_run_cmd(&request, DAP_CMD_BLOCK_WRITE_HDR_LEN + (size_t)(block_count * 4U), &response, sizeof(response))) return false; /* Check the response over */ From 03fb9f727bd9a94d4308bdf6371915286715d68d Mon Sep 17 00:00:00 2001 From: dragonmux Date: Fri, 8 Nov 2024 02:41:20 +0000 Subject: [PATCH 08/16] hosted/dap: Fixed the behaviour of the memory access setup routines to not crash when a fault occurs, but instead propagate it correctly --- src/platforms/hosted/dap.c | 26 ++++++-------------------- src/platforms/hosted/dap.h | 4 ++-- 2 files changed, 8 insertions(+), 22 deletions(-) diff --git a/src/platforms/hosted/dap.c b/src/platforms/hosted/dap.c index cb16e6a4367..8f9f2a82ad6 100644 --- a/src/platforms/hosted/dap.c +++ b/src/platforms/hosted/dap.c @@ -340,21 +340,14 @@ static size_t dap_adiv5_mem_access_build(const adiv5_access_port_s *const target } } -void dap_adiv5_mem_access_setup(adiv5_access_port_s *const target_ap, const target_addr64_t addr, const align_e align) +bool dap_adiv5_mem_access_setup(adiv5_access_port_s *const target_ap, const target_addr64_t addr, const align_e align) { /* Start by setting up the transfer and attempting it */ dap_transfer_request_s requests[4]; const size_t requests_count = dap_adiv5_mem_access_build(target_ap, requests, addr, align); adiv5_debug_port_s *const target_dp = target_ap->dp; - const bool result = perform_dap_transfer_recoverable(target_dp, requests, requests_count, NULL, 0U); - /* If it didn't go well, say something and abort */ - if (!result) { - if (target_dp->fault != DAP_TRANSFER_NO_RESPONSE) - DEBUG_ERROR("Transport error (%u), aborting\n", target_dp->fault); - else - DEBUG_ERROR("Transaction unrecoverably failed\n"); - exit(-1); - } + /* The result of this call is then fed up the stack for proper handling */ + return perform_dap_transfer_recoverable(target_dp, requests, requests_count, NULL, 0U); } static size_t dap_adiv6_mem_access_build(const adiv6_access_port_s *const target_ap, @@ -398,21 +391,14 @@ static size_t dap_adiv6_mem_access_build(const adiv6_access_port_s *const target } } -void dap_adiv6_mem_access_setup(adiv6_access_port_s *const target_ap, const target_addr64_t addr, const align_e align) +bool dap_adiv6_mem_access_setup(adiv6_access_port_s *const target_ap, const target_addr64_t addr, const align_e align) { /* Start by setting up the transfer and attempting it */ dap_transfer_request_s requests[6]; const size_t requests_count = dap_adiv6_mem_access_build(target_ap, requests, addr, align); adiv5_debug_port_s *const target_dp = target_ap->base.dp; - const bool result = perform_dap_transfer_recoverable(target_dp, requests, requests_count, NULL, 0U); - /* If it didn't go well, say something and abort */ - if (!result) { - if (target_dp->fault != DAP_TRANSFER_NO_RESPONSE) - DEBUG_ERROR("Transport error (%u), aborting\n", target_dp->fault); - else - DEBUG_ERROR("Transaction unrecoverably failed\n"); - exit(-1); - } + /* The result of this call is then fed up the stack for proper handling */ + return perform_dap_transfer_recoverable(target_dp, requests, requests_count, NULL, 0U); } uint32_t dap_adiv5_ap_read(adiv5_access_port_s *const target_ap, const uint16_t addr) diff --git a/src/platforms/hosted/dap.h b/src/platforms/hosted/dap.h index cd066c7e24e..f810b9238de 100644 --- a/src/platforms/hosted/dap.h +++ b/src/platforms/hosted/dap.h @@ -87,10 +87,10 @@ uint32_t dap_adiv6_ap_read(adiv5_access_port_s *base_ap, uint16_t addr); void dap_adiv6_ap_write(adiv5_access_port_s *base_ap, uint16_t addr, uint32_t value); void dap_adiv5_mem_read_single(adiv5_access_port_s *target_ap, void *dest, target_addr64_t src, align_e align); void dap_adiv5_mem_write_single(adiv5_access_port_s *target_ap, target_addr64_t dest, const void *src, align_e align); -void dap_adiv5_mem_access_setup(adiv5_access_port_s *target_ap, target_addr64_t addr, align_e align); +bool dap_adiv5_mem_access_setup(adiv5_access_port_s *target_ap, target_addr64_t addr, align_e align); void dap_adiv6_mem_read_single(adiv6_access_port_s *target_ap, void *dest, target_addr64_t src, align_e align); void dap_adiv6_mem_write_single(adiv6_access_port_s *target_ap, target_addr64_t dest, const void *src, align_e align); -void dap_adiv6_mem_access_setup(adiv6_access_port_s *target_ap, target_addr64_t addr, align_e align); +bool dap_adiv6_mem_access_setup(adiv6_access_port_s *target_ap, target_addr64_t addr, align_e align); bool dap_mem_read_block(adiv5_access_port_s *target_ap, void *dest, target_addr64_t src, size_t len, align_e align); bool dap_mem_write_block( adiv5_access_port_s *target_ap, target_addr64_t dest, const void *src, size_t len, align_e align); From d3fcb173a855cf70f02187c1e642db28ce6b32ca Mon Sep 17 00:00:00 2001 From: dragonmux Date: Fri, 8 Nov 2024 02:43:07 +0000 Subject: [PATCH 09/16] hosted/cmsis_dap: Early exit on trying to memory access when the setup for the access goes wrong --- src/platforms/hosted/cmsis_dap.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/platforms/hosted/cmsis_dap.c b/src/platforms/hosted/cmsis_dap.c index d8dec507f14..ba5bce8975e 100644 --- a/src/platforms/hosted/cmsis_dap.c +++ b/src/platforms/hosted/cmsis_dap.c @@ -602,7 +602,8 @@ static void dap_adiv5_mem_read(adiv5_access_port_s *ap, void *dest, target_addr6 uint8_t *const data = (uint8_t *)dest; for (size_t offset = 0; offset < len;) { /* Setup AP_TAR every loop as failing to do so results in it wrapping */ - dap_adiv5_mem_access_setup(ap, src + offset, align); + if (!dap_adiv5_mem_access_setup(ap, src + offset, align)) + return; /* * src can start out unaligned to a 1024 byte chunk size, * so we have to calculate how much is left of the chunk. @@ -640,7 +641,8 @@ static void dap_adiv5_mem_write( const uint8_t *const data = (const uint8_t *)src; for (size_t offset = 0; offset < len;) { /* Setup AP_TAR every loop as failing to do so results in it wrapping */ - dap_adiv5_mem_access_setup(ap, dest + offset, align); + if (!dap_adiv5_mem_access_setup(ap, dest + offset, align)) + return; /* * dest can start out unaligned to a 1024 byte chunk size, * so we have to calculate how much is left of the chunk. @@ -683,7 +685,8 @@ static void dap_adiv6_mem_read( uint8_t *const data = (uint8_t *)dest; for (size_t offset = 0; offset < len;) { /* Setup AP_TAR every loop as failing to do so results in it wrapping */ - dap_adiv6_mem_access_setup(ap, src + offset, align); + if (!dap_adiv6_mem_access_setup(ap, src + offset, align)) + return; /* * src can start out unaligned to a 1024 byte chunk size, * so we have to calculate how much is left of the chunk. @@ -722,7 +725,8 @@ static void dap_adiv6_mem_write( const uint8_t *const data = (const uint8_t *)src; for (size_t offset = 0; offset < len;) { /* Setup AP_TAR every loop as failing to do so results in it wrapping */ - dap_adiv6_mem_access_setup(ap, dest + offset, align); + if (!dap_adiv6_mem_access_setup(ap, dest + offset, align)) + return; /* * dest can start out unaligned to a 1024 byte chunk size, * so we have to calculate how much is left of the chunk. From 3fac021d153f499adc41a36d1da46df9c79887cc Mon Sep 17 00:00:00 2001 From: dragonmux Date: Sat, 9 Nov 2024 02:19:23 +0000 Subject: [PATCH 10/16] hosted/dap_command: Rewrote the post-request handling for `perform_dap_transfer_block_read()` to be a little more sane --- src/platforms/hosted/dap_command.c | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/src/platforms/hosted/dap_command.c b/src/platforms/hosted/dap_command.c index c30feaad37f..5ab6f429854 100644 --- a/src/platforms/hosted/dap_command.c +++ b/src/platforms/hosted/dap_command.c @@ -172,20 +172,22 @@ bool perform_dap_transfer_block_read( return false; } - /* Check the response over */ + /* Check the response over, starting by extracting how much data was returned and the response status */ const uint16_t blocks_read = read_le2(response.count, 0); - if (blocks_read == block_count && (response.status & DAP_TRANSFER_STATUS_MASK) == DAP_TRANSFER_OK) { - for (size_t i = 0; i < block_count; ++i) + const uint8_t result = response.status & DAP_TRANSFER_STATUS_MASK; + /* If the request went okay */ + if (result == DAP_TRANSFER_OK) { + /* Extract what data we can */ + const uint16_t blocks_copy = MIN(blocks_read, block_count); + for (size_t i = 0U; i < blocks_copy; ++i) blocks[i] = read_le4(response.data[i], 0); - return true; + } else { + /* If the target didn't like something about what we asked it to do, mark the DP with the status code */ + target_dp->fault = result & DAP_TRANSFER_STATUS_MASK; + DEBUG_PROBE("-> transfer failed with %u after processing %u blocks\n", response.status, blocks_read); } - if ((response.status & DAP_TRANSFER_STATUS_MASK) != DAP_TRANSFER_OK) - target_dp->fault = response.status & DAP_TRANSFER_STATUS_MASK; - else - target_dp->fault = 0; - - DEBUG_PROBE("-> transfer failed with %u after processing %u blocks\n", response.status, blocks_read); - return false; + /* Let the caller know if things went okay or not */ + return result == DAP_TRANSFER_OK; } bool perform_dap_transfer_block_write( From b5869eac1edf6ff3584bddde1e73a2a939728b37 Mon Sep 17 00:00:00 2001 From: dragonmux Date: Sat, 9 Nov 2024 04:00:38 +0000 Subject: [PATCH 11/16] hosted/dap: Improved the data flows to properly allow the result of a failed partial read up the stack and handling WAIT responses in read as best as we can --- src/platforms/hosted/dap.c | 20 +++++++++++++------- src/platforms/hosted/dap_command.c | 12 +++++++++++- 2 files changed, 24 insertions(+), 8 deletions(-) diff --git a/src/platforms/hosted/dap.c b/src/platforms/hosted/dap.c index 8f9f2a82ad6..e2bc50fb0d5 100644 --- a/src/platforms/hosted/dap.c +++ b/src/platforms/hosted/dap.c @@ -265,13 +265,12 @@ void dap_write_reg(adiv5_debug_port_s *target_dp, const uint8_t reg, const uint3 bool dap_mem_read_block( adiv5_access_port_s *const target_ap, void *dest, target_addr64_t src, const size_t len, const align_e align) { + /* Try to read the 32-bit blocks requested */ const size_t blocks = len >> MIN(align, 2U); - uint32_t data[256]; - if (!perform_dap_transfer_block_read(target_ap->dp, SWD_AP_DRW, blocks, data)) { - DEBUG_ERROR("dap_read_block failed\n"); - return false; - } + uint32_t data[256U] = {0U}; + const bool result = perform_dap_transfer_block_read(target_ap->dp, SWD_AP_DRW, blocks, data); + /* Unpack the data from those blocks */ if (align > ALIGN_16BIT) memcpy(dest, data, len); else { @@ -280,15 +279,20 @@ bool dap_mem_read_block( src += 1U << align; } } - return true; + + /* Report if it actually failed and then propagate the failure up accordingly */ + if (!result) + DEBUG_ERROR("dap_read_block failed\n"); + return result; } bool dap_mem_write_block( adiv5_access_port_s *const target_ap, target_addr64_t dest, const void *src, const size_t len, const align_e align) { const size_t blocks = len >> MAX(align, 2U); - uint32_t data[256]; + uint32_t data[256U]; + /* Pack the data to send into 32-bit blocks */ if (align > ALIGN_16BIT) memcpy(data, src, len); else { @@ -298,7 +302,9 @@ bool dap_mem_write_block( } } + /* Try to write the blocks to the target's memory */ const bool result = perform_dap_transfer_block_write(target_ap->dp, SWD_AP_DRW, blocks, data); + /* Report if it actually failed and then propagate the failure up accordingly */ if (!result) DEBUG_ERROR("dap_write_block failed\n"); return result; diff --git a/src/platforms/hosted/dap_command.c b/src/platforms/hosted/dap_command.c index 5ab6f429854..739e6b5f888 100644 --- a/src/platforms/hosted/dap_command.c +++ b/src/platforms/hosted/dap_command.c @@ -167,8 +167,17 @@ bool perform_dap_transfer_block_read( /* Check if we got any response bytes back and if we got enough, extract the status. */ if (response_length < 3U) exit(1); + /* Extract the number of blocks of data we got back and copy them out into the block buffer */ + const uint16_t blocks_read = read_le2(response.count, 0); + for (size_t i = 0U; i < blocks_read; ++i) + blocks[i] = read_le4(response.data[i], 0); /* We got enough response bytes back for the status to be valid, so put that in the DP's fault member */ target_dp->fault = response.status & DAP_TRANSFER_STATUS_MASK; + /* If the reason we're here is a WAIT timeout, abort the ongoing transaction to bring the AP back to sanity */ + if (target_dp->fault == DAP_TRANSFER_WAIT) { + DEBUG_ERROR("SWD access resulted in wait, aborting\n"); + target_dp->abort(target_dp, ADIV5_DP_ABORT_DAPABORT); + } return false; } @@ -177,7 +186,7 @@ bool perform_dap_transfer_block_read( const uint8_t result = response.status & DAP_TRANSFER_STATUS_MASK; /* If the request went okay */ if (result == DAP_TRANSFER_OK) { - /* Extract what data we can */ + /* Extract what data we can to the block buffer */ const uint16_t blocks_copy = MIN(blocks_read, block_count); for (size_t i = 0U; i < blocks_copy; ++i) blocks[i] = read_le4(response.data[i], 0); @@ -216,6 +225,7 @@ bool perform_dap_transfer_block_write( const uint16_t blocks_written = read_le2(response.count, 0); if (blocks_written == block_count && (response.status & DAP_TRANSFER_STATUS_MASK) == DAP_TRANSFER_OK) return true; + /* If the target didn't like something about what we asked it to do, mark the DP with the status code */ if ((response.status & DAP_TRANSFER_STATUS_MASK) != DAP_TRANSFER_OK) target_dp->fault = response.status & DAP_TRANSFER_STATUS_MASK; else From e97537876da3c5fd9990b0d26531f93a7a80ea56 Mon Sep 17 00:00:00 2001 From: dragonmux Date: Sat, 9 Nov 2024 04:06:00 +0000 Subject: [PATCH 12/16] hosted/cmsis_dap: Fixed up all the copyright headers --- src/platforms/hosted/cmsis_dap.c | 2 +- src/platforms/hosted/cmsis_dap.h | 2 ++ src/platforms/hosted/dap.c | 9 +++------ src/platforms/hosted/dap.h | 4 ++++ src/platforms/hosted/dap_command.c | 2 +- src/platforms/hosted/dap_jtag.c | 2 +- src/platforms/hosted/dap_swd.c | 2 +- 7 files changed, 13 insertions(+), 10 deletions(-) diff --git a/src/platforms/hosted/cmsis_dap.c b/src/platforms/hosted/cmsis_dap.c index ba5bce8975e..30d053e5e31 100644 --- a/src/platforms/hosted/cmsis_dap.c +++ b/src/platforms/hosted/cmsis_dap.c @@ -1,6 +1,6 @@ /* * Copyright (c) 2013-2019, Alex Taradov - * Copyright (C) 2023 1BitSquared + * Copyright (C) 2023-2024 1BitSquared * Modified by Rachel Mant * All rights reserved. * diff --git a/src/platforms/hosted/cmsis_dap.h b/src/platforms/hosted/cmsis_dap.h index 94ccb62e273..fa9751bceed 100644 --- a/src/platforms/hosted/cmsis_dap.h +++ b/src/platforms/hosted/cmsis_dap.h @@ -2,6 +2,8 @@ * This file is part of the Black Magic Debug project. * * Copyright (C) 2019-2021 Uwe Bonnes + * Copyright (C) 2023-2024 1BitSquared + * Modified by Rachel Mant * * This program is free software: you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by diff --git a/src/platforms/hosted/dap.c b/src/platforms/hosted/dap.c index e2bc50fb0d5..835ef6dce13 100644 --- a/src/platforms/hosted/dap.c +++ b/src/platforms/hosted/dap.c @@ -1,6 +1,7 @@ /* - * Copyright (c) 2013-2015, Alex Taradov - * Copyright (C) 2023 1BitSquared + * Copyright (C) 2013-2015 Alex Taradov + * Copyright (C) 2020-2021 Uwe Bonnes + * Copyright (C) 2023-2024 1BitSquared * Modified by Rachel Mant * All rights reserved. * @@ -28,10 +29,6 @@ * POSSIBILITY OF SUCH DAMAGE. */ -/* Modified for Blackmagic Probe - * Copyright (c) 2020-21 Uwe Bonnes bon@elektron.ikp.physik.tu-darmstadt.de - */ - #include #include "general.h" #include "exception.h" diff --git a/src/platforms/hosted/dap.h b/src/platforms/hosted/dap.h index f810b9238de..aef0beaaa14 100644 --- a/src/platforms/hosted/dap.h +++ b/src/platforms/hosted/dap.h @@ -1,5 +1,9 @@ /* + * This file is part of the Black Magic Debug project. + * * Copyright (c) 2013-2015, Alex Taradov + * Copyright (C) 2023-2024 1BitSquared + * Modified by Rachel Mant * All rights reserved. * * Redistribution and use in source and binary forms, with or without diff --git a/src/platforms/hosted/dap_command.c b/src/platforms/hosted/dap_command.c index 739e6b5f888..30c363e9e80 100644 --- a/src/platforms/hosted/dap_command.c +++ b/src/platforms/hosted/dap_command.c @@ -1,7 +1,7 @@ /* * This file is part of the Black Magic Debug project. * - * Copyright (C) 2022-2023 1BitSquared + * Copyright (C) 2022-2024 1BitSquared * Written by Rachel Mant * All rights reserved. * diff --git a/src/platforms/hosted/dap_jtag.c b/src/platforms/hosted/dap_jtag.c index 1590a9f1567..fcf5bf8a26a 100644 --- a/src/platforms/hosted/dap_jtag.c +++ b/src/platforms/hosted/dap_jtag.c @@ -1,7 +1,7 @@ /* * This file is part of the Black Magic Debug project. * - * Copyright (C) 2023 1BitSquared + * Copyright (C) 2023-2024 1BitSquared * Written by Rachel Mant * All rights reserved. * diff --git a/src/platforms/hosted/dap_swd.c b/src/platforms/hosted/dap_swd.c index 36d33b20bf7..6dc7c6fd8fc 100644 --- a/src/platforms/hosted/dap_swd.c +++ b/src/platforms/hosted/dap_swd.c @@ -1,7 +1,7 @@ /* * This file is part of the Black Magic Debug project. * - * Copyright (C) 2023 1BitSquared + * Copyright (C) 2023-2024 1BitSquared * Written by Rachel Mant * All rights reserved. * From c62dac24b2bc4302ab59a561175c815177352c58 Mon Sep 17 00:00:00 2001 From: dragonmux Date: Mon, 11 Nov 2024 08:23:32 +0000 Subject: [PATCH 13/16] hosted/dap: Fixed the blocks calculation in `dap_mem_write_block()` --- src/platforms/hosted/dap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/platforms/hosted/dap.c b/src/platforms/hosted/dap.c index 835ef6dce13..8fc51ce306e 100644 --- a/src/platforms/hosted/dap.c +++ b/src/platforms/hosted/dap.c @@ -286,7 +286,7 @@ bool dap_mem_read_block( bool dap_mem_write_block( adiv5_access_port_s *const target_ap, target_addr64_t dest, const void *src, const size_t len, const align_e align) { - const size_t blocks = len >> MAX(align, 2U); + const size_t blocks = len >> MIN(align, 2U); uint32_t data[256U]; /* Pack the data to send into 32-bit blocks */ From ed0ae20015edc79ab01a95e2b97e3f2abdd7461d Mon Sep 17 00:00:00 2001 From: dragonmux Date: Mon, 11 Nov 2024 08:24:15 +0000 Subject: [PATCH 14/16] hosted/dap: Resized the block buffers in `dap_mem_{read,write}_block()` to exactly fit the max amount transferrable in a go and nothing more --- src/platforms/hosted/dap.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/platforms/hosted/dap.c b/src/platforms/hosted/dap.c index 8fc51ce306e..253a102e4a0 100644 --- a/src/platforms/hosted/dap.c +++ b/src/platforms/hosted/dap.c @@ -264,7 +264,7 @@ bool dap_mem_read_block( { /* Try to read the 32-bit blocks requested */ const size_t blocks = len >> MIN(align, 2U); - uint32_t data[256U] = {0U}; + uint32_t data[127U] = {0U}; const bool result = perform_dap_transfer_block_read(target_ap->dp, SWD_AP_DRW, blocks, data); /* Unpack the data from those blocks */ @@ -287,7 +287,7 @@ bool dap_mem_write_block( adiv5_access_port_s *const target_ap, target_addr64_t dest, const void *src, const size_t len, const align_e align) { const size_t blocks = len >> MIN(align, 2U); - uint32_t data[256U]; + uint32_t data[126U]; /* Pack the data to send into 32-bit blocks */ if (align > ALIGN_16BIT) From f46b6d1f88392c32e791c00640fa7c3e511aa518 Mon Sep 17 00:00:00 2001 From: dragonmux Date: Mon, 11 Nov 2024 08:26:45 +0000 Subject: [PATCH 15/16] hosted/cmsis_dap: Limit the max packet size to 512 bytes to not overflow any buffers anywhere --- src/platforms/hosted/cmsis_dap.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/platforms/hosted/cmsis_dap.c b/src/platforms/hosted/cmsis_dap.c index 30d053e5e31..6ab7decdab6 100644 --- a/src/platforms/hosted/cmsis_dap.c +++ b/src/platforms/hosted/cmsis_dap.c @@ -222,9 +222,9 @@ static bool dap_init_hid(void) /* * Base the report length information for the device on the max packet length from its descriptors. - * Add 1 to account for HIDAPI's need to prefix with a report type byte. + * Add 1 to account for HIDAPI's need to prefix with a report type byte. Limit to at most 512 bytes. */ - dap_packet_size = bmda_probe_info.max_packet_length + 1U; + dap_packet_size = MIN(bmda_probe_info.max_packet_length + 1U, 512U); /* Handle the NXP LPC11U3x CMSIS-DAP v1.0.7 implementation needing a 64 byte report length */ if (bmda_probe_info.vid == 0x1fc9U && bmda_probe_info.pid == 0x0132U) From 9c9d26b87e6ec05f5764c1116251b11022d4ee4d Mon Sep 17 00:00:00 2001 From: dragonmux Date: Sun, 17 Nov 2024 13:43:51 +0000 Subject: [PATCH 16/16] hosted/cmsis_dap: Added a quirk restricting when the ZLP read for Bulk adaptors is done so we don't clobber performance on adaptors that don't need it --- src/platforms/hosted/cmsis_dap.c | 7 ++++++- src/platforms/hosted/dap.h | 1 + 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/src/platforms/hosted/cmsis_dap.c b/src/platforms/hosted/cmsis_dap.c index 6ab7decdab6..82c572d8dcb 100644 --- a/src/platforms/hosted/cmsis_dap.c +++ b/src/platforms/hosted/cmsis_dap.c @@ -351,6 +351,10 @@ bool dap_init(bool allow_fallback) dap_version_compare_le(adaptor_version, (dap_version_s){1, 3, 1})) dap_quirks |= DAP_QUIRK_BAD_SWD_NO_RESP_DATA_PHASE; + /* ORBTrace needs an extra ZLP read done on full packet reception */ + if (strcmp(bmda_probe_info.product, "Orbtrace") == 0) + dap_quirks |= DAP_QUIRK_NEEDS_EXTRA_ZLP_READ; + return true; } @@ -513,8 +517,9 @@ ssize_t dbg_dap_cmd_bulk(const uint8_t *const request_data, const size_t request return response_result; } } while (response_data[0] != request_data[0]); + /* If the response requested is the size of the packet size for the adaptor, generate a ZLP read to clean state */ - if (transferred == (int)dap_packet_size) { + if ((dap_quirks & DAP_QUIRK_NEEDS_EXTRA_ZLP_READ) && transferred == (int)dap_packet_size) { uint8_t zlp; int zlp_read = 0; libusb_bulk_transfer(usb_handle, in_ep, &zlp, sizeof(zlp), &zlp_read, TRANSFER_TIMEOUT_MS); diff --git a/src/platforms/hosted/dap.h b/src/platforms/hosted/dap.h index aef0beaaa14..e698ca6de0d 100644 --- a/src/platforms/hosted/dap.h +++ b/src/platforms/hosted/dap.h @@ -72,6 +72,7 @@ typedef enum dap_led_type { #define DAP_QUIRK_NO_JTAG_MUTLI_TAP (1U << 0U) #define DAP_QUIRK_BAD_SWD_NO_RESP_DATA_PHASE (1U << 1U) #define DAP_QUIRK_BROKEN_SWD_SEQUENCE (1U << 2U) +#define DAP_QUIRK_NEEDS_EXTRA_ZLP_READ (1U << 3U) extern uint8_t dap_caps; extern dap_cap_e dap_mode;