Message-ID: <5nvcgno3xgybfo4dncfcw6fdm76eawf4nagoerwekzqaeftqjo@eiux4q5afuaa>
Date: Tue, 8 Jul 2025 18:07:03 +1000
From: Alistair Popple <apopple@...dia.com>
To: Alexandre Courbot <acourbot@...dia.com>
Cc: rust-for-linux@...r.kernel.org, Danilo Krummrich <dakr@...nel.org>, 
	Bjorn Helgaas <bhelgaas@...gle.com>, Krzysztof Wilczyński <kwilczynski@...nel.org>, 
	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 <lossin@...nel.org>, 
	Andreas Hindborg <a.hindborg@...nel.org>, Alice Ryhl <aliceryhl@...gle.com>, 
	Trevor Gross <tmgross@...ch.edu>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>, 
	"Rafael J. Wysocki" <rafael@...nel.org>, John Hubbard <jhubbard@...dia.com>, linux-pci@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] rust: Add dma_set_mask() and dma_set_coherent_mask()
 bindings

On Tue, Jul 08, 2025 at 04:01:19PM +0900, Alexandre Courbot wrote:
> Hi Alistair,
> 
> On Tue Jul 8, 2025 at 3:04 PM JST, Alistair Popple wrote:
> > Add bindings to allow setting the DMA masks for both a generic device
> > and a PCI device.
> >
> > Signed-off-by: Alistair Popple <apopple@...dia.com>
> > Cc: Danilo Krummrich <dakr@...nel.org>
> > Cc: Bjorn Helgaas <bhelgaas@...gle.com>
> > Cc: "Krzysztof Wilczyński" <kwilczynski@...nel.org>
> > Cc: Miguel Ojeda <ojeda@...nel.org>
> > Cc: Alex Gaynor <alex.gaynor@...il.com>
> > Cc: Boqun Feng <boqun.feng@...il.com>
> > Cc: Gary Guo <gary@...yguo.net>
> > Cc: "Björn Roy Baron" <bjorn3_gh@...tonmail.com>
> > Cc: Benno Lossin <lossin@...nel.org>
> > Cc: Andreas Hindborg <a.hindborg@...nel.org>
> > Cc: Alice Ryhl <aliceryhl@...gle.com>
> > Cc: Trevor Gross <tmgross@...ch.edu>
> > Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> > Cc: "Rafael J. Wysocki" <rafael@...nel.org>
> > Cc: John Hubbard <jhubbard@...dia.com>
> > Cc: Alexandre Courbot <acourbot@...dia.com>
> > Cc: linux-pci@...r.kernel.org
> > Cc: linux-kernel@...r.kernel.org
> > ---
> >  rust/kernel/device.rs | 25 +++++++++++++++++++++++++
> >  rust/kernel/pci.rs    | 23 +++++++++++++++++++++++
> >  2 files changed, 48 insertions(+)
> >
> > diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
> > index dea06b79ecb5..77a1293a1c82 100644
> > --- a/rust/kernel/device.rs
> > +++ b/rust/kernel/device.rs
> > @@ -10,6 +10,7 @@
> >      types::{ARef, Opaque},
> >  };
> >  use core::{fmt, marker::PhantomData, ptr};
> > +use kernel::prelude::*;
> >  
> >  #[cfg(CONFIG_PRINTK)]
> >  use crate::c_str;
> > @@ -67,6 +68,30 @@ pub(crate) fn as_raw(&self) -> *mut bindings::device {
> >          self.0.get()
> >      }
> >  
> > +    /// Sets the DMA mask for the device.
> > +    pub fn dma_set_mask(&self, mask: u64) -> Result {
> > +        // SAFETY: By the type invariant of `device::Device`, `self.as_ref().as_raw()` is valid.
> > +        let ret = unsafe { bindings::dma_set_mask(&(*self.as_raw()) as *const _ as *mut _, mask) };
> > +        if ret != 0 {
> > +            Err(Error::from_errno(ret))
> > +        } else {
> > +            Ok(())
> > +        }
> 
> I think you want to use `kernel::error::to_result()` here?

Ok.

> > +    }
> > +
> > +    /// Sets the coherent DMA mask for the device.
> > +    pub fn dma_set_coherent_mask(&self, mask: u64) -> Result {
> > +        // SAFETY: By the type invariant of `device::Device`, `self.as_ref().as_raw()` is valid.
> > +        let ret = unsafe {
> > +            bindings::dma_set_coherent_mask(&(*self.as_raw()) as *const _ as *mut _, mask)
> > +        };
> > +        if ret != 0 {
> > +            Err(Error::from_errno(ret))
> > +        } else {
> > +            Ok(())
> > +        }
> 
> And here as well.
> 
> > +    }
> > +
> >      /// Returns a reference to the parent device, if any.
> >      #[cfg_attr(not(CONFIG_AUXILIARY_BUS), expect(dead_code))]
> >      pub(crate) fn parent(&self) -> Option<&Self> {
> > diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
> > index 8435f8132e38..7f640ba8f19c 100644
> > --- a/rust/kernel/pci.rs
> > +++ b/rust/kernel/pci.rs
> > @@ -369,6 +369,17 @@ fn as_raw(&self) -> *mut bindings::pci_dev {
> >      }
> >  }
> >  
> > +impl<'a, Ctx: device::DeviceContext> From<&'a kernel::pci::Device<Ctx>>
> > +    for &'a device::Device<Ctx>
> > +{
> > +    fn from(pdev: &kernel::pci::Device<Ctx>) -> &device::Device<Ctx> {
> > +        // SAFETY: The returned reference has the same lifetime as the
> > +        // pci::Device which holds a reference on the underlying device
> > +        // pointer.
> > +        unsafe { device::Device::as_ref(&(*pdev.as_raw()).dev as *const _ as *mut _) }
> > +    }
> > +}
> 
> IIUC pci::Device has an `AsRef<device::Device>` implementation, why not
> use that in the code below?
> 
> > +
> >  impl Device {
> >      /// Returns the PCI vendor ID.
> >      pub fn vendor_id(&self) -> u16 {
> > @@ -393,6 +404,18 @@ pub fn resource_len(&self, bar: u32) -> Result<bindings::resource_size_t> {
> >          // - by its type invariant `self.as_raw` is always a valid pointer to a `struct pci_dev`.
> >          Ok(unsafe { bindings::pci_resource_len(self.as_raw(), bar.try_into()?) })
> >      }
> > +
> > +    /// Set the DMA mask for PCI device.
> > +    pub fn dma_set_mask(&self, mask: u64) -> Result {
> > +        let dev: &device::Device = self.into();
> > +        dev.dma_set_mask(mask)
> 
> Yup, I have tried and `self.as_ref().dma_set_mask(mask)` works just
> fine, so I don't think we need the `From` implementation above.

Right you are. Will drop the `From` implementation for the next version, thanks.

Powered by blists - more mailing lists