[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <D3CC752A-8FE3-4B2A-93A2-7FEC2EC3940E@collabora.com>
Date: Tue, 28 Jan 2025 07:36:17 -0300
From: Daniel Almeida <daniel.almeida@...labora.com>
To: Alice Ryhl <aliceryhl@...gle.com>
Cc: Abdiel Janulgue <abdiel.janulgue@...il.com>,
rust-for-linux@...r.kernel.org,
dakr@...nel.org,
robin.murphy@....com,
Miguel Ojeda <ojeda@...nel.org>,
Alex Gaynor <alex.gaynor@...il.com>,
Boqun Feng <boqun.feng@...il.com>,
Gary Guo <gary@...yguo.net>,
Björn Roy Baron <bjorn3_gh@...tonmail.com>,
Benno Lossin <benno.lossin@...ton.me>,
Andreas Hindborg <a.hindborg@...nel.org>,
Trevor Gross <tmgross@...ch.edu>,
Valentin Obst <kernel@...entinobst.de>,
open list <linux-kernel@...r.kernel.org>,
Christoph Hellwig <hch@....de>,
Marek Szyprowski <m.szyprowski@...sung.com>,
airlied@...hat.com,
"open list:DMA MAPPING HELPERS" <iommu@...ts.linux.dev>
Subject: Re: [PATCH v11 2/3] rust: add dma coherent allocator abstraction.
>
> I mean, I imagine that you could make the syntax
> `dma_read!(my_alloc[7])` read the entire struct. I'm not sure which
> safe methods you are referring to, because right now there is only the
> unsafe as_slice().
>
> Alice
I mean this:
+ /// Writes data to the region starting from `offset`. `offset` is in units of `T`, not the
+ /// number of bytes.
+ pub fn write(&self, src: &[T], offset: usize) -> Result {
+ if offset + src.len() >= self.count {
+ return Err(EINVAL);
+ }
+ // SAFETY:
+ // - The pointer is valid due to type invariant on `CoherentAllocation`
+ // and we've just checked that the range and index is within bounds.
+ // - `offset` can't overflow since it is smaller than `self.count` and we've checked
+ // that `self.count` won't overflow early in the constructor.
+ unsafe {
+ core::ptr::copy_nonoverlapping(src.as_ptr(), self.cpu_addr.add(offset), src.len())
+ };
+ Ok(())
+ }
+}
…and the similar read() method that was apparently removed, i.e.:
+ /// Reads data from the region starting from `offset` as a slice.
+ /// `offset` and `count` are in units of `T`, not the number of bytes.
+ ///
+ /// Due to the safety requirements of slice, the data returned should be regarded by the
+ /// caller as a snapshot of the region when this function is called, as the region could
+ /// be modified by the device at anytime. For ringbuffer type of r/w access or use-cases
+ /// where the pointer to the live data is needed, `start_ptr()` or `start_ptr_mut()`
+ /// could be used instead.
+ ///
+ /// # Safety
+ ///
+ /// Callers must ensure that no hardware operations that involve the buffer are currently
+ /// taking place while the returned slice is live.
+ pub unsafe fn read(&self, offset: usize, count: usize) -> Result<&[T]> {
+ if offset + count >= self.count {
+ return Err(EINVAL);
+ }
+ // SAFETY: The pointer is valid due to type invariant on `CoherentAllocation`,
+ // we've just checked that the range and index is within bounds. The immutability of the
+ // of data is also guaranteed by the safety requirements of the function.
+ Ok(unsafe { core::slice::from_raw_parts(self.cpu_addr.wrapping_add(offset), count) })
+ }
I don’t think there’s anything wrong with these, so maybe your solution can come in as a convenience
when one wants to save on the amount of copying? IMHO the two methods above co-exist with what
you propose.
The unsafe `as_slice()` and `as_slice_mut()` should also stay, in my opinion. They can be used, for example,
in codec drivers.
— Daniel
Powered by blists - more mailing lists