Skip to content

[update] port: hpmicro: dcd: using sutw semaphore by isr handler for … #410

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.

Sign up for GitHub

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

Merged
sakumisu merged 1 commit into from
Apr 13, 2026
Merged

[update] port: hpmicro: dcd: using sutw semaphore by isr handler for … #410

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 27 additions & 11 deletions port/hpmicro/usb_dc_hpm.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ struct hpm_udc {
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];
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Apr 13, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

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.


/* Index to bit position in register */
static inline uint8_t ep_idx2bit(uint8_t ep_idx)
Expand Down Expand Up @@ -234,12 +235,13 @@ int usbd_ep_start_write(uint8_t busid, const uint8_t ep, const uint8_t *data, ui
{
uint8_t ep_idx = USB_EP_GET_IDX(ep);
usb_device_handle_t *handle = g_hpm_udc[busid].handle;
bool ret;

if (!data && data_len) {
return -1;
return -USB_ERR_NOMEM;
Comment on lines 240 to +241
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Apr 13, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

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.

}
if (!g_hpm_udc[busid].in_ep[ep_idx].ep_enable) {
return -2;
return -USB_ERR_NOTCONN;
}

#ifdef CONFIG_USB_DCACHE_ENABLE
Expand All @@ -251,21 +253,22 @@ int usbd_ep_start_write(uint8_t busid, const uint8_t ep, const uint8_t *data, ui
g_hpm_udc[busid].in_ep[ep_idx].actual_xfer_len = 0;

usb_dcache_clean((uintptr_t)data, USB_ALIGN_UP(data_len, CONFIG_USB_ALIGN_SIZE));
usb_device_edpt_xfer(handle, ep, (uint8_t *)data, data_len);
ret = usb_device_edpt_xfer(handle, ep, (uint8_t *)data, data_len);

return 0;
return ret ? 0 : -USB_ERR_INVAL;
}

int usbd_ep_start_read(uint8_t busid, const uint8_t ep, uint8_t *data, uint32_t data_len)
{
uint8_t ep_idx = USB_EP_GET_IDX(ep);
usb_device_handle_t *handle = g_hpm_udc[busid].handle;
bool ret;

if (!data && data_len) {
return -1;
return -USB_ERR_NOMEM;
}
if (!g_hpm_udc[busid].out_ep[ep_idx].ep_enable) {
return -2;
return -USB_ERR_NOTCONN;
}

#ifdef CONFIG_USB_DCACHE_ENABLE
Expand All @@ -277,9 +280,9 @@ int usbd_ep_start_read(uint8_t busid, const uint8_t ep, uint8_t *data, uint32_t
g_hpm_udc[busid].out_ep[ep_idx].actual_xfer_len = 0;

usb_dcache_invalidate((uintptr_t)data, USB_ALIGN_UP(data_len, CONFIG_USB_ALIGN_SIZE));
usb_device_edpt_xfer(handle, ep, data, data_len);
ret = usb_device_edpt_xfer(handle, ep, data, data_len);

return 0;
return ret ? 0 : -USB_ERR_INVAL;
}

void USBD_IRQHandler(uint8_t busid)
Expand Down Expand Up @@ -310,6 +313,7 @@ void USBD_IRQHandler(uint8_t busid)
memset(g_hpm_udc[busid].out_ep, 0, sizeof(struct hpm_ep_state) * USB_NUM_BIDIR_ENDPOINTS);
usbd_event_reset_handler(busid);
usb_device_bus_reset(handle, g_hpm_udc[busid].in_ep[0].ep_mps);
return;
}

if (int_status & intr_suspend) {
Expand Down Expand Up @@ -359,6 +363,11 @@ void USBD_IRQHandler(uint8_t busid)
break;
} else {
transfer_len += p_qtd->expected_bytes - p_qtd->total_bytes;
if ((ep_idx & 0x01) != 0) {
g_hpm_udc[busid].in_ep[ep_idx / 2].actual_xfer_len = transfer_len;
} else {
g_hpm_udc[busid].out_ep[ep_idx / 2].actual_xfer_len = transfer_len;
}
p_qtd->in_use = false;
}

Expand All @@ -383,10 +392,17 @@ void USBD_IRQHandler(uint8_t busid)
}

if (edpt_setup_status) {
/*------------- Set up Received -------------*/
usb_device_clear_setup_status(handle, edpt_setup_status);
/*------------- Setup Received -------------*/
dcd_qhd_t *qhd0 = usb_device_qhd_get(handle, 0);
usbd_event_ep0_setup_complete_handler(busid, (uint8_t *)&qhd0->setup_request);

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);
Comment on lines +398 to +403
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Apr 13, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 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:

🏁 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
fi

Repository: 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=10

Repository: 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 -n

Repository: 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.c

Repository: 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.c

Repository: 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 -n

Repository: 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.c

Repository: 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.


usbd_event_ep0_setup_complete_handler(busid, _setup_data);
}
}
}
Loading