Skip to content

Commit

Permalink
Fix ringbuffer hardware race condition bug.
Browse files Browse the repository at this point in the history
When attempting to get a snapshot of the completion count and
position, the current ringbuffer implementation uses a critical section.
This does indeed prevent the completion_count from updating but does not
prevent the position from updating since that is fetched directly
from the DMA register.

When the code is called right at the end of the DMA cycle,
the position that is read may be that of the next cycle, while the
completion_count is still that of the previous cycle. As a result it
looks to the code as if we went backwards and an overrun error is
emitted.

In order to fix this we remove the critical section and get two
readings, comparing the two in order to deduce that a wrap-around
occured.
  • Loading branch information
liarokapisv committed Aug 9, 2024
1 parent b55726c commit dd837dc
Showing 1 changed file with 20 additions and 2 deletions.
22 changes: 20 additions & 2 deletions embassy-stm32/src/dma/ringbuffer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,22 @@ pub struct ReadableDmaRingBuffer<'a, W: Word> {
#[cfg_attr(feature = "defmt", derive(defmt::Format))]
pub struct OverrunError;

// We use this because it is not sufficient to use a critical section
// since the get_remaining_transfers fetches directly from the DMA register
// which is not affected by it.
//
// The main issue occurs when retrieving right at the cycle boundary
// where we can't know the cycle of the retrieved position.

fn get_pos_and_count(cap: usize, dma: &impl DmaCtrl) -> (usize, usize) {
let fst_pos = cap - dma.get_remaining_transfers();
let fst_count = dma.get_complete_count();
let pos = cap - dma.get_remaining_transfers();
let snd_count = dma.get_complete_count();
let count = if pos < fst_pos { snd_count } else { fst_count };
(pos, count)
}

pub trait DmaCtrl {
/// Get the NDTR register value, i.e. the space left in the underlying
/// buffer until the dma writer wraps.
Expand Down Expand Up @@ -155,7 +171,8 @@ impl<'a, W: Word> ReadableDmaRingBuffer<'a, W> {
then, get the current position of of the dma write and check
if it's inside data we could have copied
*/
let (pos, complete_count) = critical_section::with(|_| (self.pos(dma), dma.get_complete_count()));
let (pos, complete_count) = get_pos_and_count(self.cap(), dma);

if (pos >= self.start && pos < end) || (complete_count > 0 && pos >= end) || complete_count > 1 {
Err(OverrunError)
} else {
Expand Down Expand Up @@ -311,7 +328,8 @@ impl<'a, W: Word> WritableDmaRingBuffer<'a, W> {
compiler_fence(Ordering::SeqCst);

// Confirm that the DMA is not inside data we could have written
let (pos, complete_count) = critical_section::with(|_| (self.pos(dma), dma.get_complete_count()));
let (pos, complete_count) = get_pos_and_count(self.cap(), dma);

if (pos >= self.end && pos < start) || (complete_count > 0 && pos >= start) || complete_count > 1 {
Err(OverrunError)
} else {
Expand Down

0 comments on commit dd837dc

Please sign in to comment.