[PATCH 1/2] rust: Add dma_set_mask() and dma_set_coherent_mask() bindings

[PATCH 1/2] rust: Add dma_set_mask() and dma_set_coherent_mask() bindings

[Alistair Popple]

Add bindings to allow setting the DMA masks for both a generic device
and a PCI device.

Signed-off-by: Alistair Popple <apopple@nvidia.com>
Cc: Danilo Krummrich <dakr@kernel.org>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: "Krzysztof Wilczyński" <kwilczynski@kernel.org>
Cc: Miguel Ojeda <ojeda@kernel.org>
Cc: Alex Gaynor <alex.gaynor@gmail.com>
Cc: Boqun Feng <boqun.feng@gmail.com>
Cc: Gary Guo <gary@garyguo.net>
Cc: "Björn Roy Baron" <bjorn3_gh@protonmail.com>
Cc: Benno Lossin <lossin@kernel.org>
Cc: Andreas Hindborg <a.hindborg@kernel.org>
Cc: Alice Ryhl <aliceryhl@google.com>
Cc: Trevor Gross <tmgross@umich.edu>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: John Hubbard <jhubbard@nvidia.com>
Cc: Alexandre Courbot <acourbot@nvidia.com>
Cc: linux-pci@vger.kernel.org
Cc: linux-kernel@vger.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(())
+        }
+    }
+
+    /// 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(())
+        }
+    }
+
     /// 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 _) }
+    }
+}
+
 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)
+    }
+
+    /// Set the coherent DMA mask for the PCI device.
+    pub fn dma_set_coherent_mask(&self, mask: u64) -> Result {
+        let dev: &device::Device = self.into();
+        dev.dma_set_coherent_mask(mask)
+    }
 }
 
 impl Device<device::Bound> {
-- 
2.47.2

[PATCH 2/2] rust: Add several miscellaneous PCI helpers

[Alistair Popple]

Add bindings to obtain a PCI device's resource start address, bus/
device function, revision ID and subsystem device and vendor IDs.

Signed-off-by: Alistair Popple <apopple@nvidia.com>
Cc: Danilo Krummrich <dakr@kernel.org>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: "Krzysztof Wilczyński" <kwilczynski@kernel.org>
Cc: Miguel Ojeda <ojeda@kernel.org>
Cc: Alex Gaynor <alex.gaynor@gmail.com>
Cc: Boqun Feng <boqun.feng@gmail.com>
Cc: Gary Guo <gary@garyguo.net>
Cc: "Björn Roy Baron" <bjorn3_gh@protonmail.com>
Cc: Benno Lossin <lossin@kernel.org>
Cc: Andreas Hindborg <a.hindborg@kernel.org>
Cc: Alice Ryhl <aliceryhl@google.com>
Cc: Trevor Gross <tmgross@umich.edu>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: John Hubbard <jhubbard@nvidia.com>
Cc: Alexandre Courbot <acourbot@nvidia.com>
Cc: linux-pci@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 rust/helpers/pci.c | 10 ++++++++++
 rust/kernel/pci.rs | 36 ++++++++++++++++++++++++++++++++++++
 2 files changed, 46 insertions(+)

diff --git a/rust/helpers/pci.c b/rust/helpers/pci.c
index cd0e6bf2cc4d..59d15bd4bdb1 100644
--- a/rust/helpers/pci.c
+++ b/rust/helpers/pci.c
@@ -12,6 +12,16 @@ void *rust_helper_pci_get_drvdata(struct pci_dev *pdev)
 	return pci_get_drvdata(pdev);
 }
 
+u16 rust_helper_pci_dev_id(struct pci_dev *dev)
+{
+	return PCI_DEVID(dev->bus->number, dev->devfn);
+}
+
+resource_size_t rust_helper_pci_resource_start(struct pci_dev *pdev, int bar)
+{
+	return pci_resource_start(pdev, bar);
+}
+
 resource_size_t rust_helper_pci_resource_len(struct pci_dev *pdev, int bar)
 {
 	return pci_resource_len(pdev, bar);
diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
index 7f640ba8f19c..f41fd9facb90 100644
--- a/rust/kernel/pci.rs
+++ b/rust/kernel/pci.rs
@@ -393,6 +393,42 @@ pub fn device_id(&self) -> u16 {
         unsafe { (*self.as_raw()).device }
     }
 
+    /// Returns the PCI revision ID.
+    pub fn revision_id(&self) -> u8 {
+        // SAFETY: `self.as_raw` is a valid pointer to a `struct pci_dev`.
+        unsafe { (*self.as_raw()).revision }
+    }
+
+    /// Returns the PCI bus device/function.
+    pub fn dev_id(&self) -> u16 {
+        // SAFETY: `self.as_raw` is a valid pointer to a `struct pci_dev`.
+        unsafe { bindings::pci_dev_id(self.as_raw()) }
+    }
+
+    /// Returns the PCI subsystem vendor ID.
+    pub fn subsystem_vendor_id(&self) -> u16 {
+        // SAFETY: `self.as_raw` is a valid pointer to a `struct pci_dev`.
+        unsafe { (*self.as_raw()).subsystem_vendor }
+    }
+
+    /// Returns the PCI subsystem device ID.
+    pub fn subsystem_device_id(&self) -> u16 {
+        // SAFETY: `self.as_raw` is a valid pointer to a `struct pci_dev`.
+        unsafe { (*self.as_raw()).subsystem_device }
+    }
+
+    /// Returns the start of the given PCI bar resource.
+    pub fn resource_start(&self, bar: u32) -> Result<bindings::resource_size_t> {
+        if !Bar::index_is_valid(bar) {
+            return Err(EINVAL);
+        }
+
+        // SAFETY:
+        // - `bar` is a valid bar number, as guaranteed by the above call to `Bar::index_is_valid`,
+        // - by its type invariant `self.as_raw` is always a valid pointer to a `struct pci_dev`.
+        Ok(unsafe { bindings::pci_resource_start(self.as_raw(), bar.try_into()?) })
+    }
+
     /// Returns the size of the given PCI bar resource.
     pub fn resource_len(&self, bar: u32) -> Result<bindings::resource_size_t> {
         if !Bar::index_is_valid(bar) {
-- 
2.47.2

Re: [PATCH 1/2] rust: Add dma_set_mask() and dma_set_coherent_mask() bindings

[Alexandre Courbot]

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@nvidia.com>
> Cc: Danilo Krummrich <dakr@kernel.org>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: "Krzysztof Wilczyński" <kwilczynski@kernel.org>
> Cc: Miguel Ojeda <ojeda@kernel.org>
> Cc: Alex Gaynor <alex.gaynor@gmail.com>
> Cc: Boqun Feng <boqun.feng@gmail.com>
> Cc: Gary Guo <gary@garyguo.net>
> Cc: "Björn Roy Baron" <bjorn3_gh@protonmail.com>
> Cc: Benno Lossin <lossin@kernel.org>
> Cc: Andreas Hindborg <a.hindborg@kernel.org>
> Cc: Alice Ryhl <aliceryhl@google.com>
> Cc: Trevor Gross <tmgross@umich.edu>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Cc: John Hubbard <jhubbard@nvidia.com>
> Cc: Alexandre Courbot <acourbot@nvidia.com>
> Cc: linux-pci@vger.kernel.org
> Cc: linux-kernel@vger.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?

> +    }
> +
> +    /// 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.

Re: [PATCH 2/2] rust: Add several miscellaneous PCI helpers

[Alexandre Courbot]

On Tue Jul 8, 2025 at 3:04 PM JST, Alistair Popple wrote:
> Add bindings to obtain a PCI device's resource start address, bus/
> device function, revision ID and subsystem device and vendor IDs.

FWIW,

Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>

Re: [PATCH 1/2] rust: Add dma_set_mask() and dma_set_coherent_mask() bindings

[Alistair Popple]

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@nvidia.com>
> > Cc: Danilo Krummrich <dakr@kernel.org>
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Cc: "Krzysztof Wilczyński" <kwilczynski@kernel.org>
> > Cc: Miguel Ojeda <ojeda@kernel.org>
> > Cc: Alex Gaynor <alex.gaynor@gmail.com>
> > Cc: Boqun Feng <boqun.feng@gmail.com>
> > Cc: Gary Guo <gary@garyguo.net>
> > Cc: "Björn Roy Baron" <bjorn3_gh@protonmail.com>
> > Cc: Benno Lossin <lossin@kernel.org>
> > Cc: Andreas Hindborg <a.hindborg@kernel.org>
> > Cc: Alice Ryhl <aliceryhl@google.com>
> > Cc: Trevor Gross <tmgross@umich.edu>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> > Cc: John Hubbard <jhubbard@nvidia.com>
> > Cc: Alexandre Courbot <acourbot@nvidia.com>
> > Cc: linux-pci@vger.kernel.org
> > Cc: linux-kernel@vger.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.

Re: [PATCH 2/2] rust: Add several miscellaneous PCI helpers

[Greg Kroah-Hartman]

On Tue, Jul 08, 2025 at 04:04:51PM +1000, Alistair Popple wrote:
> Add bindings to obtain a PCI device's resource start address, bus/
> device function, revision ID and subsystem device and vendor IDs.


Do we have a user for these new helpers already?

thanks,

greg k-h

Re: [PATCH 1/2] rust: Add dma_set_mask() and dma_set_coherent_mask() bindings

[Danilo Krummrich]

On Tue Jul 8, 2025 at 8:04 AM CEST, Alistair Popple wrote:
> Add bindings to allow setting the DMA masks for both a generic device
> and a PCI device.

Nice coincidence, I was about to get back to this. I already implemented this in
a previous patch [1], but didn't apply it yet.

I think the approach below is thought a bit too simple:

  (1) We want the DMA mask methods to be implemented by a trait in dma.rs.
      Subsequently, the trait should only be implemented by bus devices where
      the bus actually supports DMA. Allowing to set the DMA mask on any device
      doesn't make sense.

  (2) We need to consider that with this we do no prevent
      dma_set_coherent_mask() to concurrently with dma_alloc_coherent() (not
      even if we'd add a new `Probe` device context).

(2) is the main reason why I didn't follow up yet. So far I haven't found a nice
    solution for a sound API that doesn't need unsafe.

One thing I did consider was to have some kind of per device table (similar to
the device ID table) for drivers to specify the DMA mask already at compile
time. However, I'm pretty sure there are cases where the DMA mask has to derived
dynamically from probe().

I think I have to think a bit more about it.

[1] https://lore.kernel.org/all/20250317185345.2608976-7-abdiel.janulgue@gmail.com/

Re: [PATCH 2/2] rust: Add several miscellaneous PCI helpers

[Danilo Krummrich]

On Tue Jul 8, 2025 at 8:04 AM CEST, Alistair Popple wrote:
> diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
> index 7f640ba8f19c..f41fd9facb90 100644
> --- a/rust/kernel/pci.rs
> +++ b/rust/kernel/pci.rs
> @@ -393,6 +393,42 @@ pub fn device_id(&self) -> u16 {
>          unsafe { (*self.as_raw()).device }
>      }
>  
> +    /// Returns the PCI revision ID.
> +    pub fn revision_id(&self) -> u8 {

We should add a compiler hint for those methods to be inlined.

> +        // SAFETY: `self.as_raw` is a valid pointer to a `struct pci_dev`.

Let's refer to the type invariant for the validity of self.as_raw().

> +        unsafe { (*self.as_raw()).revision }
> +    }

Both is also true for the existing methods vendor_id() and device_id(). Can you
please fix them up in a separate patch as well?

Also, please add a brief note in the commit message where those will be used
(even though I obviously know). :)

Re: [PATCH 1/2] rust: Add dma_set_mask() and dma_set_coherent_mask() bindings

[Danilo Krummrich]

On Tue Jul 8, 2025 at 10:40 AM CEST, Danilo Krummrich wrote:
> On Tue Jul 8, 2025 at 8:04 AM CEST, Alistair Popple wrote:
>> Add bindings to allow setting the DMA masks for both a generic device
>> and a PCI device.
>
> Nice coincidence, I was about to get back to this. I already implemented this in
> a previous patch [1], but didn't apply it yet.
>
> I think the approach below is thought a bit too simple:
>
>   (1) We want the DMA mask methods to be implemented by a trait in dma.rs.
>       Subsequently, the trait should only be implemented by bus devices where
>       the bus actually supports DMA. Allowing to set the DMA mask on any device
>       doesn't make sense.

Forgot to mention, another reason for a trait is that we can also use it as a
trait bound on dma::CoherentAllocation::new(), such that people can't pass
arbitrary devices to dma::CoherentAllocation::new(), but only those that
actually sit on a DMA capable bus.

>
>   (2) We need to consider that with this we do no prevent
>       dma_set_coherent_mask() to concurrently with dma_alloc_coherent() (not
>       even if we'd add a new `Probe` device context).
>
> (2) is the main reason why I didn't follow up yet. So far I haven't found a nice
>     solution for a sound API that doesn't need unsafe.
>
> One thing I did consider was to have some kind of per device table (similar to
> the device ID table) for drivers to specify the DMA mask already at compile
> time. However, I'm pretty sure there are cases where the DMA mask has to derived
> dynamically from probe().
>
> I think I have to think a bit more about it.
>
> [1] https://lore.kernel.org/all/20250317185345.2608976-7-abdiel.janulgue@gmail.com/

Re: [PATCH 1/2] rust: Add dma_set_mask() and dma_set_coherent_mask() bindings

[Danilo Krummrich]

On Tue Jul 8, 2025 at 11:48 AM CEST, Danilo Krummrich wrote:
> On Tue Jul 8, 2025 at 10:40 AM CEST, Danilo Krummrich wrote:
>> On Tue Jul 8, 2025 at 8:04 AM CEST, Alistair Popple wrote:
>>> Add bindings to allow setting the DMA masks for both a generic device
>>> and a PCI device.
>>
>> Nice coincidence, I was about to get back to this. I already implemented this in
>> a previous patch [1], but didn't apply it yet.
>>
>> I think the approach below is thought a bit too simple:
>>
>>   (1) We want the DMA mask methods to be implemented by a trait in dma.rs.
>>       Subsequently, the trait should only be implemented by bus devices where
>>       the bus actually supports DMA. Allowing to set the DMA mask on any device
>>       doesn't make sense.
>
> Forgot to mention, another reason for a trait is that we can also use it as a
> trait bound on dma::CoherentAllocation::new(), such that people can't pass
> arbitrary devices to dma::CoherentAllocation::new(), but only those that
> actually sit on a DMA capable bus.
>
>>
>>   (2) We need to consider that with this we do no prevent
>>       dma_set_coherent_mask() to concurrently with dma_alloc_coherent() (not
>>       even if we'd add a new `Probe` device context).
>>
>> (2) is the main reason why I didn't follow up yet. So far I haven't found a nice
>>     solution for a sound API that doesn't need unsafe.
>>
>> One thing I did consider was to have some kind of per device table (similar to
>> the device ID table) for drivers to specify the DMA mask already at compile
>> time. However, I'm pretty sure there are cases where the DMA mask has to derived
>> dynamically from probe().
>>
>> I think I have to think a bit more about it.

Ok, there are multiple things to consider in the context of (2) above.

  (a) We have to ensure that the dev->dma_mask pointer is properly initialized,
      which happens when the corresponding bus device is initialized. This is
      definitely the case when probe() is called, i.e. when the device is bound.

      So the solutions here is simple, we just implement the dma::Device trait
      (which implements dma_set_mask() and dma_set_coherent_mask()) for
      &Device<Bound>.

  (b) When dma_set_mask() or dma_set_coherent_mask() are called concurrently
      with e.g. dma_alloc_coherent(), there is a data race with dev->dma_mask,
      dev->coherent_dma_mask and dev->dma_skip_sync (also set by
      dma_set_mask()).

      However, AFAICT, this does not necessarily make the Rust API unsafe in the
      sense of Rust's requirements. I.e. a potential data race does not lead to
      undefined behavior on the CPU side of things, but may result into a not
      properly functioning device.

      It would be possible to declare dma_set_mask() and dma_set_coherent_mask()
      Rust accessors as safe with the caveat that the device may not be able to
      use the memory concurrently allocated with e.g.
      dma::CoherentAllocation::new() properly.

      The alternative would be to make dma_set_mask() and
      dma_set_coherent_mask() unsafe to begin with.

      I don't think there's a reasonable alternative given that the mask may be
      derived on runtime in probe() by probing the device itself.

      I guess we could do something with type states and cookie values etc., but
      that's unreasonable overhead for something that is clearly more a
      theoretical than a practical concern.

      My conclusion is that we should just declare dma_set_mask() and
      dma_set_coherent_mask() as safe functions (with proper documentation on
      the pitfalls), given that the device is equally malfunctioning if they're
      not called at all.

@Alistair: If that is fine for you I'll pick up my old patches ([1] and related
ones) and re-send them.

If there is more discussion on (b) I'm happy to follow up either here or in the
mentioned patches once I re-visited and re-sent them.

>> [1] https://lore.kernel.org/all/20250317185345.2608976-7-abdiel.janulgue@gmail.com/

Re: [PATCH 1/2] rust: Add dma_set_mask() and dma_set_coherent_mask() bindings

[Alistair Popple]

On Tue, Jul 08, 2025 at 10:44:53PM +0200, Danilo Krummrich wrote:
> On Tue Jul 8, 2025 at 11:48 AM CEST, Danilo Krummrich wrote:
> > On Tue Jul 8, 2025 at 10:40 AM CEST, Danilo Krummrich wrote:
> >> On Tue Jul 8, 2025 at 8:04 AM CEST, Alistair Popple wrote:
> >>> Add bindings to allow setting the DMA masks for both a generic device
> >>> and a PCI device.
> >>
> >> Nice coincidence, I was about to get back to this. I already implemented this in
> >> a previous patch [1], but didn't apply it yet.
> >>
> >> I think the approach below is thought a bit too simple:
> >>
> >>   (1) We want the DMA mask methods to be implemented by a trait in dma.rs.
> >>       Subsequently, the trait should only be implemented by bus devices where
> >>       the bus actually supports DMA. Allowing to set the DMA mask on any device
> >>       doesn't make sense.
> >
> > Forgot to mention, another reason for a trait is that we can also use it as a
> > trait bound on dma::CoherentAllocation::new(), such that people can't pass
> > arbitrary devices to dma::CoherentAllocation::new(), but only those that
> > actually sit on a DMA capable bus.
> >
> >>
> >>   (2) We need to consider that with this we do no prevent
> >>       dma_set_coherent_mask() to concurrently with dma_alloc_coherent() (not
> >>       even if we'd add a new `Probe` device context).
> >>
> >> (2) is the main reason why I didn't follow up yet. So far I haven't found a nice
> >>     solution for a sound API that doesn't need unsafe.
> >>
> >> One thing I did consider was to have some kind of per device table (similar to
> >> the device ID table) for drivers to specify the DMA mask already at compile
> >> time. However, I'm pretty sure there are cases where the DMA mask has to derived
> >> dynamically from probe().
> >>
> >> I think I have to think a bit more about it.
> 
> Ok, there are multiple things to consider in the context of (2) above.
> 
>   (a) We have to ensure that the dev->dma_mask pointer is properly initialized,
>       which happens when the corresponding bus device is initialized. This is
>       definitely the case when probe() is called, i.e. when the device is bound.
> 
>       So the solutions here is simple, we just implement the dma::Device trait
>       (which implements dma_set_mask() and dma_set_coherent_mask()) for
>       &Device<Bound>.
> 
>   (b) When dma_set_mask() or dma_set_coherent_mask() are called concurrently
>       with e.g. dma_alloc_coherent(), there is a data race with dev->dma_mask,
>       dev->coherent_dma_mask and dev->dma_skip_sync (also set by
>       dma_set_mask()).
> 
>       However, AFAICT, this does not necessarily make the Rust API unsafe in the
>       sense of Rust's requirements. I.e. a potential data race does not lead to
>       undefined behavior on the CPU side of things, but may result into a not
>       properly functioning device.
> 
>       It would be possible to declare dma_set_mask() and dma_set_coherent_mask()
>       Rust accessors as safe with the caveat that the device may not be able to
>       use the memory concurrently allocated with e.g.
>       dma::CoherentAllocation::new() properly.
> 
>       The alternative would be to make dma_set_mask() and
>       dma_set_coherent_mask() unsafe to begin with.
> 
>       I don't think there's a reasonable alternative given that the mask may be
>       derived on runtime in probe() by probing the device itself.
> 
>       I guess we could do something with type states and cookie values etc., but
>       that's unreasonable overhead for something that is clearly more a
>       theoretical than a practical concern.
> 
>       My conclusion is that we should just declare dma_set_mask() and
>       dma_set_coherent_mask() as safe functions (with proper documentation on
>       the pitfalls), given that the device is equally malfunctioning if they're
>       not called at all.
> 
> @Alistair: If that is fine for you I'll pick up my old patches ([1] and related
> ones) and re-send them.

Fine with me, and I agree with your conclusion that these should just be
declared as safe functions with proper documentation. I think there are cases
where the mask is dynamic and in practice I think basically all drivers just set
the mask in their probe routine before doing any DMA allocations anyway so it
does seem more theoretical.

> If there is more discussion on (b) I'm happy to follow up either here or in the
> mentioned patches once I re-visited and re-sent them.

Sounds good. My motivation for sending this was simply that we will need it to
start initialising the GPU in nova-core.
 
> >> [1] https://lore.kernel.org/all/20250317185345.2608976-7-abdiel.janulgue@gmail.com/

Re: [PATCH 2/2] rust: Add several miscellaneous PCI helpers

[Alistair Popple]

On Tue, Jul 08, 2025 at 11:32:24AM +0200, Danilo Krummrich wrote:
> On Tue Jul 8, 2025 at 8:04 AM CEST, Alistair Popple wrote:
> > diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
> > index 7f640ba8f19c..f41fd9facb90 100644
> > --- a/rust/kernel/pci.rs
> > +++ b/rust/kernel/pci.rs
> > @@ -393,6 +393,42 @@ pub fn device_id(&self) -> u16 {
> >          unsafe { (*self.as_raw()).device }
> >      }
> >  
> > +    /// Returns the PCI revision ID.
> > +    pub fn revision_id(&self) -> u8 {
> 
> We should add a compiler hint for those methods to be inlined.
> 
> > +        // SAFETY: `self.as_raw` is a valid pointer to a `struct pci_dev`.
> 
> Let's refer to the type invariant for the validity of self.as_raw().
> 
> > +        unsafe { (*self.as_raw()).revision }
> > +    }
> 
> Both is also true for the existing methods vendor_id() and device_id(). Can you
> please fix them up in a separate patch as well?

Sure.

> Also, please add a brief note in the commit message where those will be used
> (even though I obviously know). :)

:) Will do.

Re: [PATCH 2/2] rust: Add several miscellaneous PCI helpers

[Alistair Popple]

On Tue, Jul 08, 2025 at 10:07:11AM +0200, Greg Kroah-Hartman wrote:
> On Tue, Jul 08, 2025 at 04:04:51PM +1000, Alistair Popple wrote:
> > Add bindings to obtain a PCI device's resource start address, bus/
> > device function, revision ID and subsystem device and vendor IDs.
> 
> 
> Do we have a user for these new helpers already?

We need them for nova-core. I'm still cleaning up the patch series that uses
these but figured these particular bindings should be pretty uncontroversial so
could be sent ahead of that.

Danilo has requested some changes so I will note that when I send v2.

Thanks.

> thanks,
> 
> greg k-h

Re: [PATCH 1/2] rust: Add dma_set_mask() and dma_set_coherent_mask() bindings

[Danilo Krummrich]

On Tue Jul 8, 2025 at 10:44 PM CEST, Danilo Krummrich wrote:
> On Tue Jul 8, 2025 at 11:48 AM CEST, Danilo Krummrich wrote:
>> On Tue Jul 8, 2025 at 10:40 AM CEST, Danilo Krummrich wrote:
>>> On Tue Jul 8, 2025 at 8:04 AM CEST, Alistair Popple wrote:
>>>> Add bindings to allow setting the DMA masks for both a generic device
>>>> and a PCI device.
>>>
>>> Nice coincidence, I was about to get back to this. I already implemented this in
>>> a previous patch [1], but didn't apply it yet.
>>>
>>> I think the approach below is thought a bit too simple:
>>>
>>>   (1) We want the DMA mask methods to be implemented by a trait in dma.rs.
>>>       Subsequently, the trait should only be implemented by bus devices where
>>>       the bus actually supports DMA. Allowing to set the DMA mask on any device
>>>       doesn't make sense.
>>
>> Forgot to mention, another reason for a trait is that we can also use it as a
>> trait bound on dma::CoherentAllocation::new(), such that people can't pass
>> arbitrary devices to dma::CoherentAllocation::new(), but only those that
>> actually sit on a DMA capable bus.
>>
>>>
>>>   (2) We need to consider that with this we do no prevent
>>>       dma_set_coherent_mask() to concurrently with dma_alloc_coherent() (not
>>>       even if we'd add a new `Probe` device context).
>>>
>>> (2) is the main reason why I didn't follow up yet. So far I haven't found a nice
>>>     solution for a sound API that doesn't need unsafe.
>>>
>>> One thing I did consider was to have some kind of per device table (similar to
>>> the device ID table) for drivers to specify the DMA mask already at compile
>>> time. However, I'm pretty sure there are cases where the DMA mask has to derived
>>> dynamically from probe().
>>>
>>> I think I have to think a bit more about it.
>
> Ok, there are multiple things to consider in the context of (2) above.
>
>   (a) We have to ensure that the dev->dma_mask pointer is properly initialized,
>       which happens when the corresponding bus device is initialized. This is
>       definitely the case when probe() is called, i.e. when the device is bound.
>
>       So the solutions here is simple, we just implement the dma::Device trait
>       (which implements dma_set_mask() and dma_set_coherent_mask()) for
>       &Device<Bound>.
>
>   (b) When dma_set_mask() or dma_set_coherent_mask() are called concurrently
>       with e.g. dma_alloc_coherent(), there is a data race with dev->dma_mask,
>       dev->coherent_dma_mask and dev->dma_skip_sync (also set by
>       dma_set_mask()).
>
>       However, AFAICT, this does not necessarily make the Rust API unsafe in the
>       sense of Rust's requirements. I.e. a potential data race does not lead to
>       undefined behavior on the CPU side of things, but may result into a not
>       properly functioning device.

Apparently, this is wrong, and it might indeed result in undefined behavior on
the CPU side of things. :(

>
>       It would be possible to declare dma_set_mask() and dma_set_coherent_mask()
>       Rust accessors as safe with the caveat that the device may not be able to
>       use the memory concurrently allocated with e.g.
>       dma::CoherentAllocation::new() properly.
>
>       The alternative would be to make dma_set_mask() and
>       dma_set_coherent_mask() unsafe to begin with.
>
>       I don't think there's a reasonable alternative given that the mask may be
>       derived on runtime in probe() by probing the device itself.
>
>       I guess we could do something with type states and cookie values etc., but
>       that's unreasonable overhead for something that is clearly more a
>       theoretical than a practical concern.
>
>       My conclusion is that we should just declare dma_set_mask() and
>       dma_set_coherent_mask() as safe functions (with proper documentation on
>       the pitfalls), given that the device is equally malfunctioning if they're
>       not called at all.
>
> @Alistair: If that is fine for you I'll pick up my old patches ([1] and related
> ones) and re-send them.
>
> If there is more discussion on (b) I'm happy to follow up either here or in the
> mentioned patches once I re-visited and re-sent them.
>
>>> [1] https://lore.kernel.org/all/20250317185345.2608976-7-abdiel.janulgue@gmail.com/