[update] port: hpmicro: dcd: using sutw semaphore by isr handler for …#410
Conversation
…setup Signed-off-by: Zhihong Chen <zhihong.chen@hpmicro.com>
📝 WalkthroughWalkthroughModified the USB device controller interrupt handler to introduce a static buffer for EP0 setup data with synchronization logic, update endpoint error codes to use standardized USB error constants, add an early return for bus reset events, and accumulate actual transfer lengths for completed QTDs. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@port/hpmicro/usb_dc_hpm.c`:
- Around line 240-241: The check "if (!data && data_len)" currently returns
-USB_ERR_NOMEM which is wrong; change it to return an invalid-argument error
(e.g., -USB_ERR_INVAL or the project-specific invalid-argument constant) so
callers get a parameter error instead of an out-of-memory error; update both
occurrences of this condition in usb_dc_hpm.c (the "if (!data && data_len)"
branches) to return the chosen invalid-argument error constant and keep the rest
of the logic unchanged.
- Line 49: _setup_data is a global 8-byte scratch buffer shared across
controller instances which can be clobbered if one ISR preempts another between
memcpy() and usbd_event_ep0_setup_complete_handler(); change it to be per-bus
instead: replace the global static uint8_t _setup_data[8] with either a local
stack buffer used where memcpy() is called and then immediately pass to
usbd_event_ep0_setup_complete_handler(), or allocate a per-bus buffer like
uint8_t setup_data[CONFIG_USBDEV_MAX_BUS][8] indexed by the controller/bus id
used by the port code, and update all uses (the memcpy call and any references
at the locations around lines 401-405 and the EP0 handling path) to use the
bus-specific buffer. Ensure the core still receives a synchronous copy so
semantics remain unchanged.
- Around line 398-403: Move the call to usb_device_clear_setup_status so it
happens after the SUTW-protected read to avoid a race where a second SETUP can
overwrite qhd0->setup_request during memcpy. Specifically, keep the do/while
that asserts usb_dcd_set_sutw(handle->regs, true), copies the 8 bytes from
qhd0->setup_request into _setup_data using memcpy, loops on
usb_dcd_get_sutw(handle->regs), and then clear SUTW and only after SUTW is
deasserted call usb_device_clear_setup_status(handle, edpt_setup_status); ensure
usb_dcd_set_sutw(handle->regs, false) remains adjacent to the protected read so
the setup-clear happens after the protected read completes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| static USB_NOCACHE_RAM_SECTION ATTR_ALIGN(USB_SOC_DCD_DATA_RAM_ADDRESS_ALIGNMENT) | ||
| uint8_t _dcd_data[CONFIG_USBDEV_MAX_BUS][HPM_ALIGN_UP(sizeof(dcd_data_t), USB_SOC_DCD_DATA_RAM_ADDRESS_ALIGNMENT)]; | ||
| static USB_NOCACHE_RAM_SECTION usb_device_handle_t usb_device_handle[CONFIG_USBDEV_MAX_BUS]; | ||
| static uint8_t _setup_data[8]; |
There was a problem hiding this comment.
Make the EP0 scratch buffer per bus, not global.
_setup_data is shared by every controller instance, but this port already supports multiple buses. If one USB ISR preempts another between the memcpy() and usbd_event_ep0_setup_complete_handler(), the later SETUP packet can overwrite the buffer and deliver the wrong request to core. core/usbd_core.c copies the 8 bytes synchronously, so a stack buffer or [CONFIG_USBDEV_MAX_BUS][8] buffer is enough.
Suggested fix
-static uint8_t _setup_data[8];
+static uint8_t _setup_data[CONFIG_USBDEV_MAX_BUS][8];
...
- memcpy(_setup_data, (uint8_t *)(&qhd0->setup_request), 8);
+ memcpy(_setup_data[busid], (uint8_t *)(&qhd0->setup_request), 8);
...
- usbd_event_ep0_setup_complete_handler(busid, _setup_data);
+ usbd_event_ep0_setup_complete_handler(busid, _setup_data[busid]);Also applies to: 401-405
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@port/hpmicro/usb_dc_hpm.c` at line 49, _setup_data is a global 8-byte scratch
buffer shared across controller instances which can be clobbered if one ISR
preempts another between memcpy() and usbd_event_ep0_setup_complete_handler();
change it to be per-bus instead: replace the global static uint8_t
_setup_data[8] with either a local stack buffer used where memcpy() is called
and then immediately pass to usbd_event_ep0_setup_complete_handler(), or
allocate a per-bus buffer like uint8_t setup_data[CONFIG_USBDEV_MAX_BUS][8]
indexed by the controller/bus id used by the port code, and update all uses (the
memcpy call and any references at the locations around lines 401-405 and the EP0
handling path) to use the bus-specific buffer. Ensure the core still receives a
synchronous copy so semantics remain unchanged.
| if (!data && data_len) { | ||
| return -1; | ||
| return -USB_ERR_NOMEM; |
There was a problem hiding this comment.
Return an invalid-argument error for NULL + nonzero length.
This is caller misuse, not allocator exhaustion. Reporting -USB_ERR_NOMEM makes it harder for callers to distinguish a bad API contract from real resource pressure.
Suggested fix
if (!data && data_len) {
- return -USB_ERR_NOMEM;
+ return -USB_ERR_INVAL;
}Also applies to: 267-268
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@port/hpmicro/usb_dc_hpm.c` around lines 240 - 241, The check "if (!data &&
data_len)" currently returns -USB_ERR_NOMEM which is wrong; change it to return
an invalid-argument error (e.g., -USB_ERR_INVAL or the project-specific
invalid-argument constant) so callers get a parameter error instead of an
out-of-memory error; update both occurrences of this condition in usb_dc_hpm.c
(the "if (!data && data_len)" branches) to return the chosen invalid-argument
error constant and keep the rest of the logic unchanged.
| usb_device_clear_setup_status(handle, edpt_setup_status); | ||
| do { | ||
| usb_dcd_set_sutw(handle->regs, true); | ||
| memcpy(_setup_data, (uint8_t *)(&qhd0->setup_request), 8); | ||
| } while (!usb_dcd_get_sutw(handle->regs)); | ||
| usb_dcd_set_sutw(handle->regs, false); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For the HPMicro USB device controller / EHCI device controller, what is the recommended ordering when reading EP0 setup data with the SUTW (setup tripwire) bit relative to clearing ENDPTSETUPSTAT / setup status?
💡 Result:
No definitive source explicitly states the recommended ordering for reading EP0 setup data with the SUTW (setup tripwire) bit relative to clearing ENDPTSETUPSTAT/setup status in the HPMicro USB device controller / EHCI device controller. The HPMicro SDK documentation and official website do not provide detailed register descriptions matching these exact terms (SUTW, ENDPTSETUPSTAT), which are typical of Freescale/NXP USB controllers (e.g., in Kinetis or i.MX series). HPMicro's USB IP appears to be a custom implementation supporting TinyUSB and USBX stacks, but specific programming guidance for setup packet handling is not publicly documented in searched sources. In standard Freescale/NXP USB device controllers (similar terminology), the recommended sequence is: 1. Read EP0 setup data from the OEPCTLx/SEPCTLx registers or FIFO. 2. Clear the setup status bit (ENDPTSETUPSTAT) in ENDPOINTSTATUS register. The SUTW bit prevents further setups until cleared, ensuring data integrity. Clearing before reading risks missing or corrupting the setup data. Without HPMicro-specific confirmation, follow the NXP/Freescale convention: read setup data first, then clear the setup status.
Citations:
- 1: https://hpm-sdk.readthedocs.io/en/v1.9.0/_static/api_doc/html/group__usb__interface.html
- 2: https://hpm-sdk.readthedocs.io/en/v1.10.0/_static/api_doc/html/group__usb__interface.html
- 3: https://hpm-sdk.readthedocs.io/en/latest/samples/tinyusb/index_en.html
- 4: https://www.hpmicro.com
- 5: https://hpm-sdk.readthedocs.io/zh-cn/latest/api_doc/HPM5300_2ip_2hpm__usb__regs_8h_source.html
- 6: https://hpm-sdk.readthedocs.io/zh-cn/latest/api_doc/HPM6300_2ip_2hpm__usb__regs_8h_source.html
- 7: https://mcuxpresso.nxp.com/mcuxsdk/25.09.00-pvw2/html/_static/usb/MCUXpresso_SDK_USB_Stack_Host_Reference_Manual.pdf
- 8: https://github.com/apache/nuttx/blob/master/include/nuttx/usb/ehci.h
🏁 Script executed:
# First, let's find and examine the file
file_path="port/hpmicro/usb_dc_hpm.c"
if [ -f "$file_path" ]; then
echo "=== File found, examining lines 390-410 ==="
sed -n '390,410p' "$file_path" | cat -n
else
echo "File not found at $file_path"
# Try to find it
echo "Searching for usb_dc_hpm.c..."
find . -name "usb_dc_hpm.c" -type f
fiRepository: cherry-embedded/CherryUSB
Length of output: 854
🏁 Script executed:
# Search for _setup_data declaration to understand its type
echo "=== Searching for _setup_data declaration ==="
rg "_setup_data" -B 2 -A 2 --max-count=10Repository: cherry-embedded/CherryUSB
Length of output: 1106
🏁 Script executed:
# Examine the function containing lines 398-403
echo "=== Examining function context around line 398 ==="
sed -n '370,430p' "$file_path" | cat -nRepository: cherry-embedded/CherryUSB
Length of output: 180
🏁 Script executed:
# Check if there are per-bus setup data buffers
rg "setup_data" -B 3 -A 3 port/hpmicro/usb_dc_hpm.cRepository: cherry-embedded/CherryUSB
Length of output: 899
🏁 Script executed:
# Check how CONFIG_USBDEV_MAX_BUS is used and if there should be per-bus buffers
rg "CONFIG_USBDEV_MAX_BUS" port/hpmicro/usb_dc_hpm.cRepository: cherry-embedded/CherryUSB
Length of output: 317
🏁 Script executed:
# Look at the function signature to understand busid context
sed -n '350,400p' port/hpmicro/usb_dc_hpm.c | cat -nRepository: cherry-embedded/CherryUSB
Length of output: 2819
🏁 Script executed:
# Double-check by looking at other similar USB controller drivers to see if _setup_data should be per-bus
rg "_setup_data\[" port/ || echo "No indexed _setup_data found"Repository: cherry-embedded/CherryUSB
Length of output: 125
🏁 Script executed:
# Check how this interrupt handler is structured and if busid race condition exists
grep -n "usbd_event_ep0_setup_complete_handler" port/hpmicro/usb_dc_hpm.cRepository: cherry-embedded/CherryUSB
Length of output: 143
Move usb_device_clear_setup_status() after the SUTW-protected read.
The current code acknowledges the EP0 setup status before the SUTW-protected read completes, opening a race condition where a second SETUP can corrupt qhd0->setup_request while the memcpy is in progress. This can cause the wrong request to be forwarded or a setup packet to be skipped entirely. The clear operation must be moved until after the do/while loop completes, while SUTW is still asserted.
Suggested fix
do {
usb_dcd_set_sutw(handle->regs, true);
memcpy(_setup_data, (uint8_t *)(&qhd0->setup_request), 8);
} while (!usb_dcd_get_sutw(handle->regs));
+ usb_device_clear_setup_status(handle, edpt_setup_status);
usb_dcd_set_sutw(handle->regs, false);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@port/hpmicro/usb_dc_hpm.c` around lines 398 - 403, Move the call to
usb_device_clear_setup_status so it happens after the SUTW-protected read to
avoid a race where a second SETUP can overwrite qhd0->setup_request during
memcpy. Specifically, keep the do/while that asserts
usb_dcd_set_sutw(handle->regs, true), copies the 8 bytes from
qhd0->setup_request into _setup_data using memcpy, loops on
usb_dcd_get_sutw(handle->regs), and then clear SUTW and only after SUTW is
deasserted call usb_device_clear_setup_status(handle, edpt_setup_status); ensure
usb_dcd_set_sutw(handle->regs, false) remains adjacent to the protected read so
the setup-clear happens after the protected read completes.
2 participants
…setup
Summary by CodeRabbit
Release Notes