diff mbox series

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

Message ID 20250320222823.16509-4-dakr@kernel.org (mailing list archive)
State Superseded
Delegated to: Bjorn Helgaas
Headers show
Series Implement TryFrom<&Device> for bus specific devices | expand

Commit Message

Danilo Krummrich March 20, 2025, 10:27 p.m. UTC 
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(-)

Comments

Benno Lossin March 20, 2025, 11:44 p.m. UTC | #1 
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 {}
>
Danilo Krummrich March 20, 2025, 11:48 p.m. UTC | #2 
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 {}
> >  
> 
>
kernel test robot March 21, 2025, 4:56 p.m. UTC | #3 
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
Danilo Krummrich March 21, 2025, 5:44 p.m. UTC | #4 
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?
Miguel Ojeda March 21, 2025, 6:59 p.m. UTC | #5 
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
Danilo Krummrich March 21, 2025, 7:11 p.m. UTC | #6 
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.
Miguel Ojeda March 21, 2025, 7:37 p.m. UTC | #7 
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
Benno Lossin March 22, 2025, 10:08 a.m. UTC | #8 
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
diff mbox series

Patch

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