Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: CMSIS-DAP block I/O #1999

Merged
merged 16 commits into from
Nov 17, 2024
Merged

Fix: CMSIS-DAP block I/O #1999

merged 16 commits into from
Nov 17, 2024

Conversation

dragonmux
Copy link
Member

Detailed description

This PR addresses a series of bugs found by j4cbo and Xobs on Discord concerning CMSIS-DAP adaptors. They concern: how the backend handles FAULT responses; how the backend handles WAIT responses due to AP hangs during block transfers (and subsequently, short responses); how the backend handles full responses over Bulk (ZLP handling); how the backend chunks reads; how the backend chunks writes.

This fixes the behaviour of the backend so that when a FAULT response is encountered, the backend cleanly returns up the stack as in a normal transport request - the main code base knows how to handle this, so don't exit() out when it can usually be recovered, and otherwise reported.

This fixes the behaviour of the backend not aborting a transfer on encountering a WAIT response, which is roughly equivalent to the WAIT repsonse handling in adiv5_swd.c.

This fixes mischunking causing over-long read requests to be sent to the adaptor, and over-short write requests, both resulting in nasty data corruption issues. It also makes the buffer behaviour more consistent in initialisation and usage so when a fault like a WAIT response occurs, the data passed back to GDB is in an initialised state, and not just whatever garbage was on the stack.

Tested against the STM32H7 and STM32F1 - the F1's Flash routines trigger the read chunking issue, resulting in Flash data corruption; the H7's tracing components trigger the other conditions as when TRACECLKEN is not set in the DBGMCU, the tracing components are entirely unclocked and accessing their registers results in a permanent WAIT condition on the AP and a bus hang; and then the next request after that results in a FAULT response. Reading 512 byte blocks using GDB's Python interface triggers the over-long read requests. The following was the testing protocol for this:

python
target = gdb.selected_inferior()
gdb.write(target.read_memory(0x5c000000, 512).hex()) # Read the ROM table for the tracing block
gdb.write(target.read_memory(0x5c003000, 512).hex()) # Read the SWO registers - if TRACECLKEN is not set, this will fail but "succeed", WAIT response will be reported on BMDA console
gdb.write(target.read_memory(0x5c015000, 512).hex()) # Read the TPIU registers - if TRACECLKEN is not set, this will outright fail, FAULT response will be reported on BMDA console

Power cycling the target is required after doing this testing protocol in its failing state configuration, and writing DBGMCU's CR register to 0x00700007 with set required for a re-run to test the success state.

Your checklist for this pull request

Closing issues

@dragonmux dragonmux added Bug Confirmed bug BMP Firmware Black Magic Probe Firmware (not PC hosted software) labels Nov 11, 2024
@dragonmux dragonmux added this to the v2.0 release milestone Nov 11, 2024
@dragonmux dragonmux requested a review from esden November 11, 2024 08:47
@dragonmux dragonmux force-pushed the fix/cmsis-dap-block-io branch 3 times, most recently from bd6188b to cf98ee8 Compare November 15, 2024 20:49
…pecialised in mass data transfers so we get the the actual amount transferred
…s a full packet amount of data, messing up the Bulk transfer transport state
…perly detecting and handling short responses due to target errors, leading to very bad things happening on return
… resulted in it not properly taking into account the command byte prefix in responses
… resulted in it not properly taking into account the command byte prefix in responses
…o not crash when a fault occurs, but instead propagate it correctly
…p_transfer_block_read()` to be a little more sane
… failed partial read up the stack and handling WAIT responses in read as best as we can
…)` to exactly fit the max amount transferrable in a go and nothing more
…k adaptors is done so we don't clobber performance on adaptors that don't need it
@dragonmux dragonmux force-pushed the fix/cmsis-dap-block-io branch from d45942f to 9c9d26b Compare November 17, 2024 16:55
Copy link
Member

@esden esden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@esden esden merged commit 9c9d26b into main Nov 17, 2024
36 checks passed
@dragonmux dragonmux deleted the fix/cmsis-dap-block-io branch November 17, 2024 17:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BMP Firmware Black Magic Probe Firmware (not PC hosted software) Bug Confirmed bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants