nrf5x: Fix DMA access race condition #1280
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kasjer
force-pushed
the
kasjer/nrf5x-dma-race
branch
2 times, most recently
from
bad5610
to
9085b34
Compare
In multi-thread mode starting DMA in thread mode was prone to race condition resulting in infinite loop. It may happen on single core CPU with strict priority based tasks scheduler where ready high prio task never yields to ready low prio task (Mynewt). Sequence that failed (T1 - low priority task, T2 - high priority task) - T1 called start_dma() - T1 set _dcd.dma_running (DMA not started yet, context switch happens) - T2 took CPU and saw that _dcd.dma_running is set, so waits for _dcd.dma_running to be 0 - T1 never gets CPU again, DMA is not started T2 waits forever OSAL mutex resolves problem of DMA starting from thread-context.
kasjer
force-pushed
the
kasjer/nrf5x-dma-race
branch
from
9085b34
to
c16b56a
Compare
|
Additional mutex lock added during transfer setup to prevent premature interrupt enable that could happen if two tasks started two separate transfers. |
When two tasks entered dcd_edpt_xfer() it was possible that first disabled interrupt to setup total_len and actual_len but second task for another endpoint enabled interrupt between total_len and actual_len resulting in race condition with interrupt, hence mutex is added on top of interrupt being blocked.
kasjer
force-pushed
the
kasjer/nrf5x-dma-race
branch
from
c16b56a
to
36b6ed8
Compare
hathach
approved these changes
Feb 22, 2022
There was a problem hiding this comment.
Thank your for the PR and sorry for late response, this somehow falled off my radar. The current implementation of nrf5x indeed has issue with race condition in preempted RTOS. I was hoping to use LDREX/STREX to have mutex, but couldn't get those to work. In the future, I think we could make use of semaphore as resource management instead of mutex which make a little bit more sense.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Describe the PR
In multi-thread mode starting DMA in thread mode was
prone to race condition resulting in infinite loop.
It may happen on single core CPU with strict priority based
tasks scheduler where ready high prio task never yields to
ready low prio task (Mynewt).
Sequence that failed (T1 - low priority task, T2 - high priority task)
OSAL mutex resolves problem of DMA starting from thread-context.
Additional context
Happened while stress testing Nimble stack on Mynewt with USB transport.