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

Prevent modifying INT_ENA (and other shared registers) in a way allowing race-conditions #1874

Closed
bjoernQ opened this issue Jul 29, 2024 · 3 comments · Fixed by #2051
Closed
Labels
peripheral:interrupt Interrupt peripheral(s) status:needs-attention This should be prioritized

Comments

@bjoernQ
Copy link
Contributor

bjoernQ commented Jul 29, 2024

Some drivers allow "splitting" the resource into parts (e.g. SYSTIMER/TIMGs - probably more).

Those parts usually allow modifying shared registers in some way but we need to make sure to atomically modify them (e.g. modifying it in a critical-section)

@bjoernQ bjoernQ changed the title Prevent modifying INT_ENA in a way allowing race-conditions Prevent modifying INT_ENA (and other shared registers) in a way allowing race-conditions Jul 29, 2024
@Dominaezzz
Copy link
Collaborator

Ideally the atomicity should be achieved with a more granular construct than critical-section (on multi core chips).

critical_section::with(|| { .... }) disables interrupts on the current core and sets a global "locked" flag, and if the other core also does critical_section::with, it'll disable interrupts there and spin on this global "locked" flag until it is unlocked.
This is effectively a global mutex which is not ideal as it can unnecessarily hold up the other core with the interrupts disabled/delayed.

What should be used instead is some kind of mutex construct that works the same way as critical section but instead of using a global flag, is uses a non-global flag instead. A flag per mutex. Or a flag per register to protect. This way both both cores can lock two separate/unrelated mutexes without having to unnecessarily wait for one another.

Going even a step further it'd be even more ideal if the locking mechanism were left to the user to deal with by providing a &mut only API. I don't want to pay for a lock I don't need one.

@bjoernQ
Copy link
Contributor Author

bjoernQ commented Jul 29, 2024

Sure - critical_section.with is to bring the big guns in - ideally there would be CAS on registers or something like bit-banding - but unfortunately we don't have that

@jessebraham jessebraham added the peripheral:interrupt Interrupt peripheral(s) label Jul 29, 2024
@JurajSadel JurajSadel added the status:needs-attention This should be prioritized label Jul 29, 2024
@Dominaezzz Dominaezzz mentioned this issue Aug 7, 2024
5 tasks
@MabezDev
Copy link
Member

The current precedent is to use critical_section for this kind of synchronization, we can re open this if this changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
peripheral:interrupt Interrupt peripheral(s) status:needs-attention This should be prioritized
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants