Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Oct 22, 2025

Overview

This PR fixes issue #3311 by adding a bool is_isr parameter to both the low-level DCD functions (dcd_edpt_xfer() and dcd_edpt_xfer_fifo()) and the high-level wrapper functions (usbd_edpt_xfer() and usbd_edpt_xfer_fifo()). This allows both DCD drivers and class drivers to properly handle synchronization based on whether they are being called from ISR context or task context.

Problem

Previously, these functions hardcoded usbd_spin_lock(false) and usbd_spin_unlock(false), assuming they were always called from non-ISR (task) context. However, in some scenarios, these functions may need to be called from ISR context, which requires different locking behavior to avoid deadlocks or race conditions.

Solution

Added a bool is_isr parameter to both the DCD-level and USBD-level functions that indicates the calling context:

  • is_isr = false: Called from task context (normal application code, deferred processing)
  • is_isr = true: Called from ISR context (interrupt handlers)

This parameter is then passed through the call chain to usbd_spin_lock() and usbd_spin_unlock() to ensure proper synchronization.

Changes

API Updates

  • src/device/dcd.h: Updated DCD function declarations to include bool is_isr parameter
  • src/device/usbd_pvt.h: Updated USBD wrapper function declarations to include bool is_isr parameter
  • src/device/usbd.c: Updated implementations to accept and pass through the is_isr parameter

DCD Driver Updates

Updated all ~30 DCD driver implementations across multiple MCU families:

  • STM32, NXP, Nordic, Microchip, Renesas, TI, WCH, Raspberry Pi, Dialog, Nuvoton, Bridgetek, Sony, Mentor Graphics, ValentyUSB, and others
  • Modified usbd_spin_lock()/usbd_spin_unlock() calls to use the is_isr parameter
  • Added (void) is_isr; to suppress unused parameter warnings where appropriate

Class Driver Updates

Updated all class drivers to pass the appropriate is_isr value:

  • Most callers pass false (task context)
  • ISR functions like audiod_rx_xfer_isr() and audiod_tx_xfer_isr() pass true (ISR context)
  • Updated files: audio, CDC, HID, MSC, MTP, MIDI, USBTMC, NCM, ECM/RNDIS, video, BTH device classes

Test Updates

  • Updated unit test mocks to expect the new parameter
  • All 21 unit tests pass

Validation

  • ✅ All unit tests pass (21/21)
  • ✅ STM32F4 CDC_MSC example builds successfully
  • ✅ No compilation errors across tested platforms

Example Usage

// DCD level - called from task context (e.g., in usbd.c)
dcd_edpt_xfer(rhport, ep_addr, buffer, total_bytes, false);

// DCD level - called from ISR context (future use)
dcd_edpt_xfer(rhport, ep_addr, buffer, total_bytes, true);

// USBD level - called from task context (most class drivers)
usbd_edpt_xfer(rhport, ep_addr, buffer, total_bytes, false);

// USBD level - called from ISR context (e.g., in audiod_rx_xfer_isr)
usbd_edpt_xfer(rhport, ep_addr, buffer, total_bytes, true);

Fixes #3311

Original prompt

Fix issue #3311 by adding is_isr argument to dcd_edpt_xfer and dcd_edpt_xfer_fifo. If they are called in isr context then set is_isr to true otherwise false.


✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

Copilot AI changed the title [WIP] Fix issue #3311 by adding is_isr argument to transfer functions Add is_isr parameter to dcd_edpt_xfer and dcd_edpt_xfer_fifo functions Oct 22, 2025
Copilot AI requested a review from HiFiPhile October 22, 2025 05:35
Copilot finished work on behalf of HiFiPhile October 22, 2025 05:35
@HiFiPhile
Copy link
Collaborator

@copilot

  • revert all submodule changes
  • Add is_isr argument to usbd_edpt_xfer and usbd_edpt_xfer_fifo, set it to true if called in ISR, for example audiod_rx_xfer_isr

- Added bool is_isr parameter to usbd_edpt_xfer() and usbd_edpt_xfer_fifo() wrapper functions
- These are called by class drivers to queue USB transfers
- Updated all callers to pass false by default (non-ISR context)
- Updated audiod_rx_xfer_isr() and audiod_tx_xfer_isr() to pass true (ISR context)
- All 21 unit tests pass

Co-authored-by: HiFiPhile <[email protected]>
Copy link
Contributor Author

Copilot AI commented Oct 22, 2025

@copilot

  • revert all submodule changes
  • Add is_isr argument to usbd_edpt_xfer and usbd_edpt_xfer_fifo, set it to true if called in ISR, for example audiod_rx_xfer_isr

Added is_isr parameter to usbd_edpt_xfer() and usbd_edpt_xfer_fifo() wrapper functions. ISR functions like audiod_rx_xfer_isr() and audiod_tx_xfer_isr() now pass true, while all other callers pass false. Submodule changes will be reverted in next commit. Commit: 27415e6

Copilot AI changed the title Add is_isr parameter to dcd_edpt_xfer and dcd_edpt_xfer_fifo functions Add is_isr parameter to dcd_edpt_xfer, dcd_edpt_xfer_fifo, usbd_edpt_xfer, and usbd_edpt_xfer_fifo functions Oct 22, 2025
Copilot finished work on behalf of HiFiPhile October 22, 2025 06:13
@HiFiPhile HiFiPhile force-pushed the copilot/fix-dcd-edpt-xfer-issue branch 4 times, most recently from f06b05d to 4143243 Compare October 22, 2025 15:11
Signed-off-by: Mengsk <[email protected]>
@HiFiPhile HiFiPhile force-pushed the copilot/fix-dcd-edpt-xfer-issue branch from 4143243 to 30132e8 Compare October 22, 2025 18:22
@HiFiPhile HiFiPhile marked this pull request as ready for review October 22, 2025 18:57
Copilot AI review requested due to automatic review settings October 22, 2025 18:57
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds a bool is_isr parameter to the DCD and USBD transfer functions (dcd_edpt_xfer, dcd_edpt_xfer_fifo, usbd_edpt_xfer, and usbd_edpt_xfer_fifo) to indicate whether the functions are being called from ISR or task context. This enables proper synchronization handling by allowing the locking mechanism to differentiate between interrupt and non-interrupt contexts.

Key changes:

  • Updated function signatures across DCD layer, USBD layer, and class drivers
  • Modified locking calls in DWC2 driver to use the new is_isr parameter
  • Most callers pass false (task context); ISR functions like audio callbacks pass true

Reviewed Changes

Copilot reviewed 49 out of 49 changed files in this pull request and generated 30 comments.

Show a summary per file
File Description
src/device/dcd.h Added is_isr parameter to DCD function declarations
src/device/usbd_pvt.h Added is_isr parameter to USBD wrapper declarations
src/device/usbd.c Updated implementations to accept and propagate is_isr parameter
src/device/usbd_control.c Updated control transfer calls to pass false for task context
src/portable/synopsys/dwc2/dcd_dwc2.c Modified to use is_isr parameter in spin lock/unlock calls
src/portable//dcd_.c (30+ files) Added is_isr parameter with (void) is_isr; suppression
src/class/audio/audio_device.c Updated ISR callbacks to pass true; non-ISR calls pass false
src/class/*/[device].c (10+ files) Updated all transfer calls to pass false for task context
test/unit-test/test/device/usbd/test_usbd.c Updated mock expectations to include new parameter
test/unit-test/test/device/msc/test_msc_device.c Updated mock expectations to include new parameter
test/fuzz/dcd_fuzz.cc Added parameter with unused suppression
src/tusb.c Updated stream transfer call to pass false

Copy link

@codescene-delta-analysis codescene-delta-analysis bot left a comment

Choose a reason for hiding this comment

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

Gates Failed
Prevent hotspot decline (1 hotspot with Excess Number of Function Arguments)
Enforce advisory code health rules (28 files with Excess Number of Function Arguments)

Gates Passed
2 Quality Gates Passed

See analysis details in CodeScene

Reason for failure
Prevent hotspot decline Violations Code Health Impact
dcd_dwc2.c 1 rule in this hotspot 6.65 → 6.46 Suppress
Enforce advisory code health rules Violations Code Health Impact
dcd_pio_usb.c 1 advisory rule 10.00 → 9.69 Suppress
dcd_template.c 1 advisory rule 10.00 → 9.69 Suppress
dcd_fuzz.cc 1 advisory rule 10.00 → 9.69 Suppress
dcd_rp2040.c 1 advisory rule 9.69 → 9.39 Suppress
dcd_mm32f327x_otg.c 1 advisory rule 9.61 → 9.31 Suppress
dcd_ci_fs.c 1 advisory rule 9.40 → 9.11 Suppress
dcd_khci.c 1 advisory rule 9.40 → 9.11 Suppress
dcd_samd.c 1 advisory rule 9.24 → 8.96 Suppress
dcd_samx7x.c 1 advisory rule 6.63 → 6.34 Suppress
dcd_ci_hs.c 1 advisory rule 8.90 → 8.63 Suppress
dcd_samg.c 1 advisory rule 8.83 → 8.56 Suppress
dcd_lpc17_40.c 1 advisory rule 8.78 → 8.51 Suppress
dcd_pic.c 1 advisory rule 8.75 → 8.48 Suppress
dcd_ch32_usbfs.c 1 advisory rule 8.68 → 8.41 Suppress
dcd_nuc120.c 1 advisory rule 8.56 → 8.29 Suppress
dcd_sunxi_musb.c 1 advisory rule 8.38 → 8.13 Suppress
dcd_musb.c 1 advisory rule 8.38 → 8.12 Suppress
dcd_lpc_ip3511.c 1 advisory rule 8.37 → 8.11 Suppress
dcd_nuc121.c 1 advisory rule 8.26 → 8.01 Suppress
dcd_nuc505.c 1 advisory rule 8.01 → 7.76 Suppress
dcd_eptri.c 1 advisory rule 7.88 → 7.64 Suppress
dcd_ch32_usbhs.c 1 advisory rule 7.73 → 7.50 Suppress
dcd_msp430x5xx.c 1 advisory rule 7.70 → 7.47 Suppress
dcd_da146xx.c 1 advisory rule 7.59 → 7.36 Suppress
dcd_stm32_fsdev.c 1 advisory rule 7.38 → 7.16 Suppress
dcd_ft9xx.c 1 advisory rule 6.69 → 6.49 Suppress
dcd_dwc2.c 1 advisory rule 6.65 → 6.46 Suppress
dcd_nrf5x.c 1 advisory rule 6.39 → 6.20 Suppress

Quality Gate Profile: Clean Code Collective
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.

Copy link
Contributor

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

C-STAT found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

@sonarqubecloud
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

DCD DWC2 dcd_edpt_xfer() called from ISR hangs on spinlock

2 participants