Skip to content

libusbclient/stm32n6: fix interrupt handler races#650

Open
jakubsmolaga wants to merge 1 commit into
masterfrom
jakubsmolaga/usb-stm32n6-intr-races
Open

libusbclient/stm32n6: fix interrupt handler races#650
jakubsmolaga wants to merge 1 commit into
masterfrom
jakubsmolaga/usb-stm32n6-intr-races

Conversation

@jakubsmolaga

Copy link
Copy Markdown
Contributor

TASK: PP-403

Description

This change fixes potential read-modify-write races inside the low frequency interrupt handler

Motivation and Context

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Chore (refactoring, style fixes, git/CI config, submodule management, no code logic changes)

How Has This Been Tested?

  • Already covered by automatic testing.
  • New test added: (add PR link here).
  • Tested by hand on: armv8m-stm32n6.

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing linter checks and tests passed.
  • My changes generate no new compilation warnings for any of the targets.

Special treatment

  • This PR needs additional PRs to work (list the PRs, preferably in merge-order).
  • I will merge this PR by myself when appropriate.

gemini-code-assist[bot]

This comment was marked as outdated.

@jakubsmolaga jakubsmolaga force-pushed the jakubsmolaga/usb-stm32n6-intr-races branch from 713816b to e3e4844 Compare April 12, 2026 22:53
@github-actions

Copy link
Copy Markdown

Unit Test Results

10 463 tests  ±0   9 801 ✅ ±0   52m 32s ⏱️ -17s
   629 suites ±0     662 💤 ±0 
     1 files   ±0       0 ❌ ±0 

Results for commit e3e4844. ± Comparison against base commit 9888dbe.

@jakubsmolaga

This comment was marked as outdated.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request introduces atomic operations for interrupt handling in the STM32N6 USB client driver. Key changes include converting pending_event, irqPendingDIEPINT, and irqPendingDOEPINT to _Atomic types and using atomic_exchange to safely retrieve and clear pending flags in a single operation. As a result, ctrl_lifiq_handler has been updated to accept the interrupt status by value, and manual bit-clearing logic within the interrupt handlers has been removed to simplify the code and improve thread safety. I have no feedback to provide.

@jakubsmolaga jakubsmolaga marked this pull request as ready for review April 13, 2026 13:00

@agkaminski agkaminski left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

_Atomic uint32_t daintClear;
uint32_t irqPendingDIEPINT[ENDPOINTS_NUMBER];
uint32_t irqPendingDOEPINT[ENDPOINTS_NUMBER];
_Atomic uint32_t irqPendingDIEPINT[ENDPOINTS_NUMBER];

@jmaksymowicz jmaksymowicz Apr 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe use the atomic_uint type from stdatomic.h instead? It's best to avoid using attributes that start with with underscores directly.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I just wanted to clearly communicate the intention that "this must be a 32bit unsigned integer", since it reflects the value of hw registers. Obviously atomic_uint will be the exact same type since unsigned int is 32bit on pretty much every sensible platform but I just wasn't sure if the intention is clear enough that way.

From my understanding the main problem with using _Atomic is that it breaks compatibility with C++ since C++ uses std::atomic<T> instead of _Atomic. I'm not sure how much of an issue that is for us.

Anyway, I'm ok with changing it but I just wanted to express my thoughts and hear what you have to say before committing the change blindly.

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.

3 participants