mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
From: Daniel Almeida <daniel.almeida@collabora.com>
To: Alice Ryhl <aliceryhl@google.com>
Cc: "Abdiel Janulgue" <abdiel.janulgue@gmail.com>,
	rust-for-linux@vger.kernel.org, dakr@kernel.org,
	robin.murphy@arm.com, "Miguel Ojeda" <ojeda@kernel.org>,
	"Alex Gaynor" <alex.gaynor@gmail.com>,
	"Boqun Feng" <boqun.feng@gmail.com>,
	"Gary Guo" <gary@garyguo.net>,
	"Björn Roy Baron" <bjorn3_gh@protonmail.com>,
	"Benno Lossin" <benno.lossin@proton.me>,
	"Andreas Hindborg" <a.hindborg@kernel.org>,
	"Trevor Gross" <tmgross@umich.edu>,
	"Valentin Obst" <kernel@valentinobst.de>,
	"open list" <linux-kernel@vger.kernel.org>,
	"Christoph Hellwig" <hch@lst.de>,
	"Marek Szyprowski" <m.szyprowski@samsung.com>,
	airlied@redhat.com,
	"open list:DMA MAPPING HELPERS" <iommu@lists.linux.dev>
Subject: Re: [PATCH v11 2/3] rust: add dma coherent allocator abstraction.
Date: Tue, 28 Jan 2025 07:36:17 -0300	[thread overview]
Message-ID: <D3CC752A-8FE3-4B2A-93A2-7FEC2EC3940E@collabora.com> (raw)
In-Reply-To: <CAH5fLgjZiux1_=dNuZN+7SEY6fUGMDNLoJNTwmHcB=WbKChwzw@mail.gmail.com>

> 
> 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


  reply	other threads:[~2025-01-28 10:37 UTC|newest]

Thread overview: 41+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-01-23 10:42 [PATCH v11 0/3] Add " Abdiel Janulgue
2025-01-23 10:42 ` [PATCH v11 1/3] rust: error: Add EOVERFLOW Abdiel Janulgue
2025-01-23 10:42 ` [PATCH v11 2/3] rust: add dma coherent allocator abstraction Abdiel Janulgue
2025-01-23 12:30   ` Petr Tesařík
2025-01-23 13:38     ` Abdiel Janulgue
2025-01-23 14:44       ` Miguel Ojeda
2025-01-23 22:54         ` Daniel Almeida
2025-01-24  7:27   ` Alice Ryhl
2025-01-27  6:16     ` Petr Tesařík
2025-01-27  9:45       ` Alice Ryhl
2025-01-27 10:37     ` Danilo Krummrich
2025-01-27 10:43       ` Alice Ryhl
2025-01-27 12:14         ` Danilo Krummrich
2025-01-27 13:25           ` Alice Ryhl
2025-01-27 13:34             ` Danilo Krummrich
2025-01-27 13:42               ` Alice Ryhl
2025-01-27 16:59         ` Boqun Feng
2025-01-27 18:32           ` Alice Ryhl
2025-01-27 18:38             ` Boqun Feng
2025-01-27 18:46               ` Alice Ryhl
2025-01-27 19:01                 ` Boqun Feng
2025-01-27 19:05                   ` Alice Ryhl
2025-01-27 19:21                     ` Boqun Feng
2025-01-27 19:37                       ` Boqun Feng
2025-01-27 10:52       ` Daniel Almeida
2025-01-27 10:59     ` Daniel Almeida
2025-01-27 11:34       ` Alice Ryhl
2025-01-28 10:22         ` Daniel Almeida
2025-01-28 10:25           ` Alice Ryhl
2025-01-28 10:36             ` Daniel Almeida [this message]
2025-01-28 11:28               ` Alice Ryhl
2025-02-15 21:40   ` Daniel Almeida
2025-02-17 13:52     ` Robin Murphy
2025-02-17 17:37       ` Abdiel Janulgue
2025-02-18  9:58       ` Abdiel Janulgue
2025-02-18 12:19         ` Daniel Almeida
2025-02-18 12:44           ` Robin Murphy
2025-01-23 10:42 ` [PATCH v11 3/3] MAINTAINERS: add entry for Rust dma mapping helpers device driver API Abdiel Janulgue
2025-01-30 11:49   ` Robin Murphy
2025-01-30 12:00     ` Danilo Krummrich
2025-02-03  8:37     ` Abdiel Janulgue

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=D3CC752A-8FE3-4B2A-93A2-7FEC2EC3940E@collabora.com \
    --to=daniel.almeida@collabora.com \
    --cc=a.hindborg@kernel.org \
    --cc=abdiel.janulgue@gmail.com \
    --cc=airlied@redhat.com \
    --cc=alex.gaynor@gmail.com \
    --cc=aliceryhl@google.com \
    --cc=benno.lossin@proton.me \
    --cc=bjorn3_gh@protonmail.com \
    --cc=boqun.feng@gmail.com \
    --cc=dakr@kernel.org \
    --cc=gary@garyguo.net \
    --cc=hch@lst.de \
    --cc=iommu@lists.linux.dev \
    --cc=kernel@valentinobst.de \
    --cc=linux-kernel@vger.kernel.org \
    --cc=m.szyprowski@samsung.com \
    --cc=ojeda@kernel.org \
    --cc=robin.murphy@arm.com \
    --cc=rust-for-linux@vger.kernel.org \
    --cc=tmgross@umich.edu \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®