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

Improve DMA pop() #1482

Open
Dominaezzz opened this issue Apr 20, 2024 · 1 comment · May be fixed by #1664
Open

Improve DMA pop() #1482

Dominaezzz opened this issue Apr 20, 2024 · 1 comment · May be fixed by #1664
Assignees
Labels
peripheral:dma DMA Peripheral status:needs-attention This should be prioritized

Comments

@Dominaezzz
Copy link
Contributor

fn pop(&mut self, data: &mut [u8]) -> Result<usize, DmaError> {
let avail = self.available;
if avail < data.len() {
return Err(DmaError::Exhausted);
}
unsafe {
let dst = data.as_mut_ptr();
let src = self.read_buffer_start;
let count = self.available;
core::ptr::copy_nonoverlapping(src, dst, count);
}
self.available = 0;
Ok(data.len())
}

Found this while I was writing the camera driver, if pop is called with data of size less than available:

  • You end up with a buffer overrun, right?
  • All the remaining available data seems to be discarded too.

My proposed fix for this is.

fn pop(&mut self, data: &mut [u8]) -> Result<usize, DmaError> {
    let avail = self.available;

    if avail < data.len() {
        return Err(DmaError::Exhausted);
    }
    
    let dma_buf = unsafe {
        core::slice::from_raw_parts(self.read_buffer_start, avail)
    };
    data.copy_from_slice(&dma_buf[..data.len()]);

    self.available -= data.len();
    Ok(data.len())
}

What I don't understand is why pop returns a usize if it's always the same as data? Was pop meant to also work when the data buf is more than the available data?

@bjoernQ
Copy link
Contributor

bjoernQ commented Apr 22, 2024

The current naive approach is that pop will always pop the buffer of one descriptor exactly.

I think for the fix we also would need to change available: Currently available will overwrite read_buffer_start and available.
Also pop would need to adjust read_buffer_start . We would need to also take wrapping into account

I think push is already working like that and probably it would be good to have them working similar

If the API works like this, we won't need the DmaError::Exhausted

@jessebraham jessebraham added bug Something isn't working peripheral:dma DMA Peripheral labels Apr 24, 2024
@MabezDev MabezDev added the status:needs-attention This should be prioritized label May 29, 2024
@bjoernQ bjoernQ changed the title Bugs in DMA pop() Improve DMA pop() Jun 5, 2024
@bjoernQ bjoernQ added enhancement and removed bug Something isn't working labels Jun 5, 2024
@bjoernQ bjoernQ linked a pull request Jun 6, 2024 that will close this issue
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
peripheral:dma DMA Peripheral status:needs-attention This should be prioritized
Projects
Status: Todo
Development

Successfully merging a pull request may close this issue.

4 participants