[PATCH v2 0/4] Implement TryFrom<&Device> for bus specific devices

[PATCH v2 0/4] Implement TryFrom<&Device> for bus specific devices

[Danilo Krummrich]

This series provides a mechanism to safely convert a struct device into its
corresponding bus specific device instance, if any.

In C a generic struct device is typically converted to a specific bus device
with container_of(). This requires the caller to know whether the generic struct
device is indeed embedded within the expected bus specific device type.

In Rust we can do the same thing by implementing the TryFrom trait, e.g.

        impl TryFrom<&Device> for pci::Device

This is a safe operation, since we can check whether dev->bus equals the the
expected struct bus_type.

Additionally, provide an accessor for a device' parent.

A branch containing the patches can be found in [1].

This is needed for the auxiliary bus abstractions and connecting nova-core with
nova-drm. [2]

[1] https://web.git.kernel.org/pub/scm/linux/kernel/git/dakr/linux.git/log/?h=rust/device
[2] https://gitlab.freedesktop.org/drm/nova/-/tree/staging/nova-drm

Changes in v2:
  - s/unsafe { *self.as_raw() }.parent/unsafe { (*self.as_raw()).parent }/
  - expand safety comment on Device::bus_type_raw()

Danilo Krummrich (4):
  rust: device: implement Device::parent()
  rust: device: implement bus_type_raw()
  rust: pci: impl TryFrom<&Device> for &pci::Device
  rust: platform: impl TryFrom<&Device> for &platform::Device

 rust/kernel/device.rs   | 24 ++++++++++++++++++++++++
 rust/kernel/pci.rs      | 21 +++++++++++++++++++--
 rust/kernel/platform.rs | 22 ++++++++++++++++++++--
 3 files changed, 63 insertions(+), 4 deletions(-)


base-commit: 51d0de7596a458096756c895cfed6bc4a7ecac10
-- 
2.48.1

[PATCH v2 1/4] rust: device: implement Device::parent()

[Danilo Krummrich]

Device::parent() returns a reference to the device' parent device, if
any.

Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 rust/kernel/device.rs | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
index 21b343a1dc4d..f6bdc2646028 100644
--- a/rust/kernel/device.rs
+++ b/rust/kernel/device.rs
@@ -65,6 +65,21 @@ pub(crate) fn as_raw(&self) -> *mut bindings::device {
         self.0.get()
     }
 
+    /// Returns a reference to the parent device, if any.
+    pub fn parent<'a>(&self) -> Option<&'a Self> {
+        // SAFETY:
+        // - By the type invariant `self.as_raw()` is always valid.
+        // - The parent device is only ever set at device creation.
+        let parent = unsafe { (*self.as_raw()).parent };
+
+        if parent.is_null() {
+            None
+        } else {
+            // SAFETY: Since `parent` is not NULL, it must be a valid pointer to a `struct device`.
+            Some(unsafe { Self::as_ref(parent) })
+        }
+    }
+
     /// Convert a raw C `struct device` pointer to a `&'a Device`.
     ///
     /// # Safety
-- 
2.48.1

Re: [PATCH v2 1/4] rust: device: implement Device::parent()

[Benno Lossin]

On Thu Mar 20, 2025 at 11:27 PM CET, Danilo Krummrich wrote:
> Device::parent() returns a reference to the device' parent device, if
> any.
>
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> ---
>  rust/kernel/device.rs | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
>
> diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
> index 21b343a1dc4d..f6bdc2646028 100644
> --- a/rust/kernel/device.rs
> +++ b/rust/kernel/device.rs
> @@ -65,6 +65,21 @@ pub(crate) fn as_raw(&self) -> *mut bindings::device {
>          self.0.get()
>      }
>  
> +    /// Returns a reference to the parent device, if any.
> +    pub fn parent<'a>(&self) -> Option<&'a Self> {
> +        // SAFETY:
> +        // - By the type invariant `self.as_raw()` is always valid.
> +        // - The parent device is only ever set at device creation.
> +        let parent = unsafe { (*self.as_raw()).parent };
> +
> +        if parent.is_null() {
> +            None
> +        } else {
> +            // SAFETY: Since `parent` is not NULL, it must be a valid pointer to a `struct device`.
> +            Some(unsafe { Self::as_ref(parent) })

Why is this valid for `'static`? Since you declare the lifetime `'a`
independently from the elided one on `&self`, the user can set it to
`'static`.

---
Cheers,
Benno

> +        }
> +    }
> +
>      /// Convert a raw C `struct device` pointer to a `&'a Device`.
>      ///
>      /// # Safety

Re: [PATCH v2 1/4] rust: device: implement Device::parent()

[Danilo Krummrich]

On Thu, Mar 20, 2025 at 10:44:38PM +0000, Benno Lossin wrote:
> On Thu Mar 20, 2025 at 11:27 PM CET, Danilo Krummrich wrote:
> > Device::parent() returns a reference to the device' parent device, if
> > any.
> >
> > Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> > Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> > ---
> >  rust/kernel/device.rs | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> >
> > diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
> > index 21b343a1dc4d..f6bdc2646028 100644
> > --- a/rust/kernel/device.rs
> > +++ b/rust/kernel/device.rs
> > @@ -65,6 +65,21 @@ pub(crate) fn as_raw(&self) -> *mut bindings::device {
> >          self.0.get()
> >      }
> >  
> > +    /// Returns a reference to the parent device, if any.
> > +    pub fn parent<'a>(&self) -> Option<&'a Self> {
> > +        // SAFETY:
> > +        // - By the type invariant `self.as_raw()` is always valid.
> > +        // - The parent device is only ever set at device creation.
> > +        let parent = unsafe { (*self.as_raw()).parent };
> > +
> > +        if parent.is_null() {
> > +            None
> > +        } else {
> > +            // SAFETY: Since `parent` is not NULL, it must be a valid pointer to a `struct device`.
> > +            Some(unsafe { Self::as_ref(parent) })
> 
> Why is this valid for `'static`? Since you declare the lifetime `'a`
> independently from the elided one on `&self`, the user can set it to
> `'static`.

Good catch -- this is indeed a problem when the &Device comes from an
ARef<Device>, rather than Device::as_ref(), which is what I had in mind
originally.

Re: [PATCH v2 1/4] rust: device: implement Device::parent()

[Greg KH]

On Thu, Mar 20, 2025 at 11:27:43PM +0100, Danilo Krummrich wrote:
> Device::parent() returns a reference to the device' parent device, if
> any.

Ok, but why?  You don't use it in this series, or did I miss it?

A driver shouldn't care about the parent of a device, as it shouldn't
really know what it is.  So what is this needed for?

thanks,

greg k-h

Re: [PATCH v2 1/4] rust: device: implement Device::parent()

[Danilo Krummrich]

On Thu, Mar 20, 2025 at 06:40:56PM -0700, Greg KH wrote:
> On Thu, Mar 20, 2025 at 11:27:43PM +0100, Danilo Krummrich wrote:
> > Device::parent() returns a reference to the device' parent device, if
> > any.
> 
> Ok, but why?  You don't use it in this series, or did I miss it?

Indeed, it should rather be at the auxbus series.

> A driver shouldn't care about the parent of a device, as it shouldn't
> really know what it is.  So what is this needed for?

Generally, that's true. I use in the auxbus example and in nova-drm [1] (which
is connected through the auxbus to nova-core) to gather some information about
the device managed by nova-core.

Later on, since that's surely not enough, we'll have an interface to nova-core
that takes the corresponding auxiliary device. nova-core can then check whether
the given device originates from nova-core, and through the parent find its own
device.

[1] https://gitlab.freedesktop.org/drm/nova/-/blob/staging/nova-drm/drivers/gpu/drm/nova/driver.rs

Re: [PATCH v2 1/4] rust: device: implement Device::parent()

[Danilo Krummrich]

On Fri, Mar 21, 2025 at 10:04:26AM +0100, Danilo Krummrich wrote:
> On Thu, Mar 20, 2025 at 06:40:56PM -0700, Greg KH wrote:
> > On Thu, Mar 20, 2025 at 11:27:43PM +0100, Danilo Krummrich wrote:
> > > Device::parent() returns a reference to the device' parent device, if
> > > any.
> > 
> > Ok, but why?  You don't use it in this series, or did I miss it?
> 
> Indeed, it should rather be at the auxbus series.
> 
> > A driver shouldn't care about the parent of a device, as it shouldn't
> > really know what it is.  So what is this needed for?
> 
> Generally, that's true. I use in the auxbus example and in nova-drm [1] (which
> is connected through the auxbus to nova-core) to gather some information about
> the device managed by nova-core.
> 
> Later on, since that's surely not enough, we'll have an interface to nova-core
> that takes the corresponding auxiliary device. nova-core can then check whether
> the given device originates from nova-core, and through the parent find its own
> device.
> 
> [1] https://gitlab.freedesktop.org/drm/nova/-/blob/staging/nova-drm/drivers/gpu/drm/nova/driver.rs

Another category of drivers that came to my mind and seems valid for this is
MFD.

Other than that I found a couple of cases where platform drivers interact with
their corresponding parent devices (mostly embedded platforms where the topology
is known), as well as a couple of HID devices that access their parent to issue
USB transactions etc., which all seems more like an abuse due to a lack of
proper APIs, which may or may not exist at the time the corresponding driver was
written.

So, maybe we should make Device::parent() crate private instead, such that it
can't be accessed by drivers, but only the core abstractions and instead only
provide accessors for the parent device for specific bus devices, where this is
reasonable to be used by drivers, e.g. auxiliary.

Re: [PATCH v2 1/4] rust: device: implement Device::parent()

[Greg KH]

On Fri, Mar 21, 2025 at 02:03:30PM +0100, Danilo Krummrich wrote:
> So, maybe we should make Device::parent() crate private instead, such that it
> can't be accessed by drivers, but only the core abstractions and instead only
> provide accessors for the parent device for specific bus devices, where this is
> reasonable to be used by drivers, e.g. auxiliary.
> 

That sounds reasonable, thanks!

greg k-h

Re: [PATCH v2 1/4] rust: device: implement Device::parent()

[Danilo Krummrich]

On Fri, Mar 21, 2025 at 06:09:06AM -0700, Greg KH wrote:
> On Fri, Mar 21, 2025 at 02:03:30PM +0100, Danilo Krummrich wrote:
> > So, maybe we should make Device::parent() crate private instead, such that it
> > can't be accessed by drivers, but only the core abstractions and instead only
> > provide accessors for the parent device for specific bus devices, where this is
> > reasonable to be used by drivers, e.g. auxiliary.
> > 
> 
> That sounds reasonable, thanks!

Cool, I will drop the patch from this series then, since when being crate
private it needs an #[expect(unused)] until actually being used. So, I rather
add it to the auxbus series.

Re: [PATCH v2 1/4] rust: device: implement Device::parent()

[Boqun Feng]

On Thu, Mar 20, 2025 at 11:27:43PM +0100, Danilo Krummrich wrote:
> Device::parent() returns a reference to the device' parent device, if
> any.
> 
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> ---
>  rust/kernel/device.rs | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
> index 21b343a1dc4d..f6bdc2646028 100644
> --- a/rust/kernel/device.rs
> +++ b/rust/kernel/device.rs
> @@ -65,6 +65,21 @@ pub(crate) fn as_raw(&self) -> *mut bindings::device {
>          self.0.get()
>      }
>  
> +    /// Returns a reference to the parent device, if any.
> +    pub fn parent<'a>(&self) -> Option<&'a Self> {
> +        // SAFETY:
> +        // - By the type invariant `self.as_raw()` is always valid.
> +        // - The parent device is only ever set at device creation.
> +        let parent = unsafe { (*self.as_raw()).parent };
> +
> +        if parent.is_null() {
> +            None
> +        } else {
> +            // SAFETY: Since `parent` is not NULL, it must be a valid pointer to a `struct device`.
> +            Some(unsafe { Self::as_ref(parent) })

The safety comment also needs to explain why the parent device won't be
gone, I assume a struct device holds a refcount of its parent? Therefore
the borrow checker would ensure the parent exists as long as the Device
is borrowed.

Regards,
Boqun

> +        }
> +    }
> +
>      /// Convert a raw C `struct device` pointer to a `&'a Device`.
>      ///
>      /// # Safety
> -- 
> 2.48.1
> 

Re: [PATCH v2 1/4] rust: device: implement Device::parent()

[Danilo Krummrich]

On Fri, Mar 21, 2025 at 07:40:57AM -0700, Boqun Feng wrote:
> On Thu, Mar 20, 2025 at 11:27:43PM +0100, Danilo Krummrich wrote:
> > Device::parent() returns a reference to the device' parent device, if
> > any.
> > 
> > Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> > Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> > ---
> >  rust/kernel/device.rs | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> > 
> > diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
> > index 21b343a1dc4d..f6bdc2646028 100644
> > --- a/rust/kernel/device.rs
> > +++ b/rust/kernel/device.rs
> > @@ -65,6 +65,21 @@ pub(crate) fn as_raw(&self) -> *mut bindings::device {
> >          self.0.get()
> >      }
> >  
> > +    /// Returns a reference to the parent device, if any.
> > +    pub fn parent<'a>(&self) -> Option<&'a Self> {
> > +        // SAFETY:
> > +        // - By the type invariant `self.as_raw()` is always valid.
> > +        // - The parent device is only ever set at device creation.
> > +        let parent = unsafe { (*self.as_raw()).parent };
> > +
> > +        if parent.is_null() {
> > +            None
> > +        } else {
> > +            // SAFETY: Since `parent` is not NULL, it must be a valid pointer to a `struct device`.
> > +            Some(unsafe { Self::as_ref(parent) })
> 
> The safety comment also needs to explain why the parent device won't be
> gone, I assume a struct device holds a refcount of its parent?

Correct, this is taken generically in device_add().

> Therefore
> the borrow checker would ensure the parent exists as long as the Device
> is borrowed.

Yes.

[PATCH v2 2/4] rust: device: implement bus_type_raw()

[Danilo Krummrich]

Implement bus_type_raw(), which returns a raw pointer to the device'
struct bus_type.

This is useful for bus devices, to implement the following trait.

	impl TryFrom<&Device> for &pci::Device

With this a caller can try to get the bus specific device from a generic
device in a safe way. try_from() will only succeed if the generic
device' bus type pointer matches the pointer of the bus' type.

Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 rust/kernel/device.rs | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
index f6bdc2646028..1b554fdd93e9 100644
--- a/rust/kernel/device.rs
+++ b/rust/kernel/device.rs
@@ -80,6 +80,16 @@ pub fn parent<'a>(&self) -> Option<&'a Self> {
         }
     }
 
+    /// Returns a raw pointer to the device' bus type.
+    #[expect(unused)]
+    pub(crate) fn bus_type_raw(&self) -> *const bindings::bus_type {
+        // SAFETY:
+        // - By the type invariants, `self.as_raw()` is a valid pointer to a `struct device`.
+        // - `dev->bus` is a pointer to a `const struct bus_type`, which is only ever set at device
+        //    creation.
+        unsafe { (*self.as_raw()).bus }
+    }
+
     /// Convert a raw C `struct device` pointer to a `&'a Device`.
     ///
     /// # Safety
-- 
2.48.1

Re: [PATCH v2 2/4] rust: device: implement bus_type_raw()

[Benno Lossin]

On Thu Mar 20, 2025 at 11:27 PM CET, Danilo Krummrich wrote:
> Implement bus_type_raw(), which returns a raw pointer to the device'
> struct bus_type.
>
> This is useful for bus devices, to implement the following trait.
>
> 	impl TryFrom<&Device> for &pci::Device
>
> With this a caller can try to get the bus specific device from a generic
> device in a safe way. try_from() will only succeed if the generic
> device' bus type pointer matches the pointer of the bus' type.
>
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>

Reviewed-by: Benno Lossin <benno.lossin@proton.me>

---
Cheers,
Benno

> ---
>  rust/kernel/device.rs | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
> index f6bdc2646028..1b554fdd93e9 100644
> --- a/rust/kernel/device.rs
> +++ b/rust/kernel/device.rs
> @@ -80,6 +80,16 @@ pub fn parent<'a>(&self) -> Option<&'a Self> {
>          }
>      }
>  
> +    /// Returns a raw pointer to the device' bus type.
> +    #[expect(unused)]
> +    pub(crate) fn bus_type_raw(&self) -> *const bindings::bus_type {
> +        // SAFETY:
> +        // - By the type invariants, `self.as_raw()` is a valid pointer to a `struct device`.
> +        // - `dev->bus` is a pointer to a `const struct bus_type`, which is only ever set at device
> +        //    creation.
> +        unsafe { (*self.as_raw()).bus }
> +    }
> +
>      /// Convert a raw C `struct device` pointer to a `&'a Device`.
>      ///
>      /// # Safety

[PATCH v2 3/4] rust: pci: impl TryFrom<&Device> for &pci::Device

[Danilo Krummrich]

Implement TryFrom<&device::Device> for &Device.

This allows us to get a &pci::Device from a generic &Device in a safe
way; the conversion fails if the device' bus type does not match with
the PCI bus type.

Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 rust/kernel/device.rs |  1 -
 rust/kernel/pci.rs    | 21 +++++++++++++++++++--
 2 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
index 1b554fdd93e9..190719a04686 100644
--- a/rust/kernel/device.rs
+++ b/rust/kernel/device.rs
@@ -81,7 +81,6 @@ pub fn parent<'a>(&self) -> Option<&'a Self> {
     }
 
     /// Returns a raw pointer to the device' bus type.
-    #[expect(unused)]
     pub(crate) fn bus_type_raw(&self) -> *const bindings::bus_type {
         // SAFETY:
         // - By the type invariants, `self.as_raw()` is a valid pointer to a `struct device`.
diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
index 22a32172b108..b717bcdb9abf 100644
--- a/rust/kernel/pci.rs
+++ b/rust/kernel/pci.rs
@@ -6,7 +6,7 @@
 
 use crate::{
     alloc::flags::*,
-    bindings, device,
+    bindings, container_of, device,
     device_id::RawDeviceId,
     devres::Devres,
     driver,
@@ -20,7 +20,7 @@
 use core::{
     marker::PhantomData,
     ops::Deref,
-    ptr::{addr_of_mut, NonNull},
+    ptr::{addr_of, addr_of_mut, NonNull},
 };
 use kernel::prelude::*;
 
@@ -466,6 +466,23 @@ fn as_ref(&self) -> &device::Device {
     }
 }
 
+impl TryFrom<&device::Device> for &Device {
+    type Error = kernel::error::Error;
+
+    fn try_from(dev: &device::Device) -> Result<Self, Self::Error> {
+        if dev.bus_type_raw() != addr_of!(bindings::pci_bus_type) {
+            return Err(EINVAL);
+        }
+
+        // SAFETY: We've just verified that the bus type of `dev` equals `bindings::pci_bus_type`,
+        // hence `dev` must be embedded in a valid `struct pci_dev`.
+        let pdev = unsafe { container_of!(dev.as_raw(), bindings::pci_dev, dev) };
+
+        // SAFETY: `pdev` is a valid pointer to a `struct pci_dev`.
+        Ok(unsafe { &*pdev.cast() })
+    }
+}
+
 // SAFETY: A `Device` is always reference-counted and can be released from any thread.
 unsafe impl Send for Device {}
 
-- 
2.48.1

Re: [PATCH v2 3/4] rust: pci: impl TryFrom<&Device> for &pci::Device

[Benno Lossin]

On Thu Mar 20, 2025 at 11:27 PM CET, Danilo Krummrich wrote:
> @@ -466,6 +466,23 @@ fn as_ref(&self) -> &device::Device {
>      }
>  }
>  
> +impl TryFrom<&device::Device> for &Device {
> +    type Error = kernel::error::Error;
> +
> +    fn try_from(dev: &device::Device) -> Result<Self, Self::Error> {
> +        if dev.bus_type_raw() != addr_of!(bindings::pci_bus_type) {
> +            return Err(EINVAL);
> +        }
> +
> +        // SAFETY: We've just verified that the bus type of `dev` equals `bindings::pci_bus_type`,
> +        // hence `dev` must be embedded in a valid `struct pci_dev`.

I think it'd be a good idea to mention that this is something guaranteed
by the C side. Something like "... must be embedded in a valid `struct
pci_dev` by the C side." or similar.

With that:

Reviewed-by: Benno Lossin <benno.lossin@proton.me>

---
Cheers,
Benno

> +        let pdev = unsafe { container_of!(dev.as_raw(), bindings::pci_dev, dev) };
> +
> +        // SAFETY: `pdev` is a valid pointer to a `struct pci_dev`.
> +        Ok(unsafe { &*pdev.cast() })
> +    }
> +}
> +
>  // SAFETY: A `Device` is always reference-counted and can be released from any thread.
>  unsafe impl Send for Device {}
>  

Re: [PATCH v2 3/4] rust: pci: impl TryFrom<&Device> for &pci::Device

[Danilo Krummrich]

On Thu, Mar 20, 2025 at 11:44:01PM +0000, Benno Lossin wrote:
> On Thu Mar 20, 2025 at 11:27 PM CET, Danilo Krummrich wrote:
> > @@ -466,6 +466,23 @@ fn as_ref(&self) -> &device::Device {
> >      }
> >  }
> >  
> > +impl TryFrom<&device::Device> for &Device {
> > +    type Error = kernel::error::Error;
> > +
> > +    fn try_from(dev: &device::Device) -> Result<Self, Self::Error> {
> > +        if dev.bus_type_raw() != addr_of!(bindings::pci_bus_type) {
> > +            return Err(EINVAL);
> > +        }
> > +
> > +        // SAFETY: We've just verified that the bus type of `dev` equals `bindings::pci_bus_type`,
> > +        // hence `dev` must be embedded in a valid `struct pci_dev`.
> 
> I think it'd be a good idea to mention that this is something guaranteed
> by the C side. Something like "... must be embedded in a valid `struct
> pci_dev` by the C side." or similar.

Sure, sounds reasonable.

> 
> With that:
> 
> Reviewed-by: Benno Lossin <benno.lossin@proton.me>
> 
> ---
> Cheers,
> Benno
> 
> > +        let pdev = unsafe { container_of!(dev.as_raw(), bindings::pci_dev, dev) };
> > +
> > +        // SAFETY: `pdev` is a valid pointer to a `struct pci_dev`.
> > +        Ok(unsafe { &*pdev.cast() })
> > +    }
> > +}
> > +
> >  // SAFETY: A `Device` is always reference-counted and can be released from any thread.
> >  unsafe impl Send for Device {}
> >  
> 
> 

Re: [PATCH v2 3/4] rust: pci: impl TryFrom<&Device> for &pci::Device

[kernel test robot]

Hi Danilo,

kernel test robot noticed the following build errors:

[auto build test ERROR on 51d0de7596a458096756c895cfed6bc4a7ecac10]

url:    https://github.com/intel-lab-lkp/linux/commits/Danilo-Krummrich/rust-device-implement-Device-parent/20250321-063101
base:   51d0de7596a458096756c895cfed6bc4a7ecac10
patch link:    https://lore.kernel.org/r/20250320222823.16509-4-dakr%40kernel.org
patch subject: [PATCH v2 3/4] rust: pci: impl TryFrom<&Device> for &pci::Device
config: x86_64-buildonly-randconfig-005-20250321 (https://download.01.org/0day-ci/archive/20250322/202503220040.TDePlxma-lkp@intel.com/config)
compiler: clang version 20.1.1 (https://github.com/llvm/llvm-project 424c2d9b7e4de40d0804dd374721e6411c27d1d1)
rustc: rustc 1.78.0 (9b00956e5 2024-04-29)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250322/202503220040.TDePlxma-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202503220040.TDePlxma-lkp@intel.com/

All errors (new ones prefixed by >>):

   ***
   *** Rust bindings generator 'bindgen' < 0.69.5 together with libclang >= 19.1
   *** may not work due to a bug (https://github.com/rust-lang/rust-bindgen/pull/2824),
   *** unless patched (like Debian's).
   ***   Your bindgen version:  0.65.1
   ***   Your libclang version: 20.1.1
   ***
   ***
   *** Please see Documentation/rust/quick-start.rst for details
   *** on how to set up the Rust support.
   ***
>> error[E0133]: use of extern static is unsafe and requires unsafe block
   --> rust/kernel/pci.rs:473:43
   |
   473 |         if dev.bus_type_raw() != addr_of!(bindings::pci_bus_type) {
   |                                           ^^^^^^^^^^^^^^^^^^^^^^ use of extern static
   |
   = note: extern statics are not controlled by the Rust type system: invalid data, aliasing violations or data races will cause undefined behavior

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v2 3/4] rust: pci: impl TryFrom<&Device> for &pci::Device

[Danilo Krummrich]

On Sat, Mar 22, 2025 at 12:56:58AM +0800, kernel test robot wrote:
> >> error[E0133]: use of extern static is unsafe and requires unsafe block
>    --> rust/kernel/pci.rs:473:43
>    |
>    473 |         if dev.bus_type_raw() != addr_of!(bindings::pci_bus_type) {
>    |                                           ^^^^^^^^^^^^^^^^^^^^^^ use of extern static
>    |
>    = note: extern statics are not controlled by the Rust type system: invalid data, aliasing violations or data races will cause undefined behavior

This requires an unsafe block for compilers < 1.82. For compilers >= 1.82 it
turns into a warning *if* using an unsafe block.

*Not* requiring unsafe for this seems like the correct thing -- was this a
bugfix in the compiler?

I guess to make it work for all compiler versions supported by the kernel we
have to use unsafe and suppress the warning?

Re: [PATCH v2 3/4] rust: pci: impl TryFrom<&Device> for &pci::Device

[Miguel Ojeda]

On Fri, Mar 21, 2025 at 6:44 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> This requires an unsafe block for compilers < 1.82. For compilers >= 1.82 it
> turns into a warning *if* using an unsafe block.
>
> *Not* requiring unsafe for this seems like the correct thing -- was this a
> bugfix in the compiler?
>
> I guess to make it work for all compiler versions supported by the kernel we
> have to use unsafe and suppress the warning?

It was a feature, but it has been fairly annoying -- it affected
several series, e.g. the latest KUnit one as well as:

    https://lore.kernel.org/rust-for-linux/CANiq72kuebpOa4aPxmTXNMA0eo-SLL+Ht9u1SGHymXBF5_92eA@mail.gmail.com/

Please see:

    https://blog.rust-lang.org/2024/10/17/Rust-1.82.0.html#safely-addressing-unsafe-statics

So, yeah, we use `allow(unused_unsafe)` (no `expect`, since it depends
on the version).

I hope that helps.

Cheers,
Miguel

Re: [PATCH v2 3/4] rust: pci: impl TryFrom<&Device> for &pci::Device

[Danilo Krummrich]

On Fri, Mar 21, 2025 at 07:59:08PM +0100, Miguel Ojeda wrote:
> On Fri, Mar 21, 2025 at 6:44 PM Danilo Krummrich <dakr@kernel.org> wrote:
> >
> > This requires an unsafe block for compilers < 1.82. For compilers >= 1.82 it
> > turns into a warning *if* using an unsafe block.
> >
> > *Not* requiring unsafe for this seems like the correct thing -- was this a
> > bugfix in the compiler?
> >
> > I guess to make it work for all compiler versions supported by the kernel we
> > have to use unsafe and suppress the warning?
> 
> It was a feature, but it has been fairly annoying -- it affected
> several series, e.g. the latest KUnit one as well as:

From the second link:

"Previously, the compiler's safety checks were not aware that the raw ref
operator did not actually affect the operand's place, treating it as a possible
read or write to a pointer. No unsafety is actually present, however, as it just
creates a pointer.

That sounds like it was a bug, or do I miss anything?

> 
>     https://lore.kernel.org/rust-for-linux/CANiq72kuebpOa4aPxmTXNMA0eo-SLL+Ht9u1SGHymXBF5_92eA@mail.gmail.com/
> 
> Please see:
> 
>     https://blog.rust-lang.org/2024/10/17/Rust-1.82.0.html#safely-addressing-unsafe-statics
> 
> So, yeah, we use `allow(unused_unsafe)` (no `expect`, since it depends
> on the version).
> 
> I hope that helps.

Yeah, thanks a lot. Especially for the second link, I couldn't find it even
after quite a while of searching.

I will respin right away, since otherwise the patches of v3 are reviewed.

Re: [PATCH v2 3/4] rust: pci: impl TryFrom<&Device> for &pci::Device

[Miguel Ojeda]

On Fri, Mar 21, 2025 at 8:11 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> From the second link:
>
> "Previously, the compiler's safety checks were not aware that the raw ref
> operator did not actually affect the operand's place, treating it as a possible
> read or write to a pointer. No unsafety is actually present, however, as it just
> creates a pointer.
>
> That sounds like it was a bug, or do I miss anything?

Yeah, if they didn't intend it originally, then I would call it a bug
-- they also seemed to considered it a bug themselves in the end, so I
think you are right.

I meant it from the point of view of the language, i.e. in the sense
that it was a restriction before, and now they relaxed it, so more
programs are accepted, and the feature would be "you need less
`unsafe`" etc.

The blog post seems to mention both sides of the coin ("This code is
now allowed", "Relaxed this", "A future version of Rust is expected to
generalize this").

> Yeah, thanks a lot. Especially for the second link, I couldn't find it even
> after quite a while of searching.

You're welcome!

Cheers,
Miguel

Re: [PATCH v2 3/4] rust: pci: impl TryFrom<&Device> for &pci::Device

[Benno Lossin]

On Fri Mar 21, 2025 at 5:56 PM CET, kernel test robot wrote:
> Hi Danilo,
>
> kernel test robot noticed the following build errors:
>
> [auto build test ERROR on 51d0de7596a458096756c895cfed6bc4a7ecac10]
>
> url:    https://github.com/intel-lab-lkp/linux/commits/Danilo-Krummrich/rust-device-implement-Device-parent/20250321-063101
> base:   51d0de7596a458096756c895cfed6bc4a7ecac10
> patch link:    https://lore.kernel.org/r/20250320222823.16509-4-dakr%40kernel.org
> patch subject: [PATCH v2 3/4] rust: pci: impl TryFrom<&Device> for &pci::Device
> config: x86_64-buildonly-randconfig-005-20250321 (https://download.01.org/0day-ci/archive/20250322/202503220040.TDePlxma-lkp@intel.com/config)
> compiler: clang version 20.1.1 (https://github.com/llvm/llvm-project 424c2d9b7e4de40d0804dd374721e6411c27d1d1)
> rustc: rustc 1.78.0 (9b00956e5 2024-04-29)
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250322/202503220040.TDePlxma-lkp@intel.com/reproduce)
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202503220040.TDePlxma-lkp@intel.com/
>
> All errors (new ones prefixed by >>):
>
>    ***
>    *** Rust bindings generator 'bindgen' < 0.69.5 together with libclang >= 19.1
>    *** may not work due to a bug (https://github.com/rust-lang/rust-bindgen/pull/2824),
>    *** unless patched (like Debian's).
>    ***   Your bindgen version:  0.65.1
>    ***   Your libclang version: 20.1.1
>    ***
>    ***
>    *** Please see Documentation/rust/quick-start.rst for details
>    *** on how to set up the Rust support.
>    ***
>>> error[E0133]: use of extern static is unsafe and requires unsafe block
>    --> rust/kernel/pci.rs:473:43
>    |
>    473 |         if dev.bus_type_raw() != addr_of!(bindings::pci_bus_type) {
>    |                                           ^^^^^^^^^^^^^^^^^^^^^^ use of extern static
>    |
>    = note: extern statics are not controlled by the Rust type system: invalid data, aliasing violations or data races will cause undefined behavior

This is just a minor annoyance with these error reports, but I would
very much like the error to have the correct indentation:

>> error[E0133]: use of extern static is unsafe and requires unsafe block
    --> rust/kernel/pci.rs:473:43
        |
    473 |         if dev.bus_type_raw() != addr_of!(bindings::pci_bus_type) {
        |                                           ^^^^^^^^^^^^^^^^^^^^^^ use of extern static
        |

Would that be possible to fix? That way the `^^^^` align with the item
they are referencing.

Otherwise they are very helpful!

---
Cheers,
Benno

[PATCH v2 4/4] rust: platform: impl TryFrom<&Device> for &platform::Device

[Danilo Krummrich]

Implement TryFrom<&device::Device> for &Device.

This allows us to get a &platform::Device from a generic &Device in a safe
way; the conversion fails if the device' bus type does not match with
the platform bus type.

Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 rust/kernel/platform.rs | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/rust/kernel/platform.rs b/rust/kernel/platform.rs
index e37531bae8e9..c17fc6e7c596 100644
--- a/rust/kernel/platform.rs
+++ b/rust/kernel/platform.rs
@@ -5,7 +5,7 @@
 //! C header: [`include/linux/platform_device.h`](srctree/include/linux/platform_device.h)
 
 use crate::{
-    bindings, device, driver,
+    bindings, container_of, device, driver,
     error::{to_result, Result},
     of,
     prelude::*,
@@ -17,7 +17,7 @@
 use core::{
     marker::PhantomData,
     ops::Deref,
-    ptr::{addr_of_mut, NonNull},
+    ptr::{addr_of, addr_of_mut, NonNull},
 };
 
 /// An adapter for the registration of platform drivers.
@@ -234,6 +234,24 @@ fn as_ref(&self) -> &device::Device {
     }
 }
 
+impl TryFrom<&device::Device> for &Device {
+    type Error = kernel::error::Error;
+
+    fn try_from(dev: &device::Device) -> Result<Self, Self::Error> {
+        if dev.bus_type_raw() != addr_of!(bindings::platform_bus_type) {
+            return Err(EINVAL);
+        }
+
+        // SAFETY: We've just verified that the bus type of `dev` equals
+        // `bindings::platform_bus_type`, hence `dev` must be embedded in a valid
+        // `struct platform_device`.
+        let pdev = unsafe { container_of!(dev.as_raw(), bindings::platform_device, dev) };
+
+        // SAFETY: `pdev` is a valid pointer to a `struct platform_device`.
+        Ok(unsafe { &*pdev.cast() })
+    }
+}
+
 // SAFETY: A `Device` is always reference-counted and can be released from any thread.
 unsafe impl Send for Device {}
 
-- 
2.48.1

Re: [PATCH v2 4/4] rust: platform: impl TryFrom<&Device> for &platform::Device

[Benno Lossin]

On Thu Mar 20, 2025 at 11:27 PM CET, Danilo Krummrich wrote:
> @@ -234,6 +234,24 @@ fn as_ref(&self) -> &device::Device {
>      }
>  }
>  
> +impl TryFrom<&device::Device> for &Device {
> +    type Error = kernel::error::Error;
> +
> +    fn try_from(dev: &device::Device) -> Result<Self, Self::Error> {
> +        if dev.bus_type_raw() != addr_of!(bindings::platform_bus_type) {
> +            return Err(EINVAL);
> +        }
> +
> +        // SAFETY: We've just verified that the bus type of `dev` equals
> +        // `bindings::platform_bus_type`, hence `dev` must be embedded in a valid
> +        // `struct platform_device`.

Same change here as in patch 3. With that:

Reviewed-by: Benno Lossin <benno.lossin@proton.me>

---
Cheers,
Benno

> +        let pdev = unsafe { container_of!(dev.as_raw(), bindings::platform_device, dev) };
> +
> +        // SAFETY: `pdev` is a valid pointer to a `struct platform_device`.
> +        Ok(unsafe { &*pdev.cast() })
> +    }
> +}
> +
>  // SAFETY: A `Device` is always reference-counted and can be released from any thread.
>  unsafe impl Send for Device {}
>