[PATCH 0/4] Improve soundness of bus device abstractions

[PATCH 0/4] Improve soundness of bus device abstractions

[Danilo Krummrich]

Currently, when sharing references of bus devices (e.g. ARef<pci::Device>), we
do not have a way to restrict which functions of a bus device can be called.

Consequently, it is possible to call all bus device functions concurrently from
any context. This includes functions, which access fields of the (bus) device,
which are not protected against concurrent access.

This is improved by applying an execution context to the bus device in form of a
generic type.

For instance, the PCI device reference that is passed to probe() has the type
pci::Device<Core>, which implements all functions that are only allowed to be
called from bus callbacks.

The implementation for the default context (pci::Device) contains all functions
that are safe to call from any context concurrently.

The context types can be extended as required, e.g. to limit availability  of
certain (bus) device functions to probe().

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

[1] https://web.git.kernel.org/pub/scm/linux/kernel/git/dakr/linux.git/log/?h=rust/device

Danilo Krummrich (4):
  rust: pci: use to_result() in enable_device_mem()
  rust: device: implement device context marker
  rust: pci: fix unrestricted &mut pci::Device
  rust: platform: fix unrestricted &mut platform::Device

 drivers/gpu/nova-core/driver.rs      |   4 +-
 rust/kernel/device.rs                |  18 ++++
 rust/kernel/pci.rs                   | 131 ++++++++++++++++-----------
 rust/kernel/platform.rs              |  93 ++++++++++++-------
 samples/rust/rust_driver_pci.rs      |   8 +-
 samples/rust/rust_driver_platform.rs |  16 +++-
 6 files changed, 177 insertions(+), 93 deletions(-)


base-commit: b28786b190d1ae2df5e6a5181ad78c6f226ea3e1
-- 
2.48.1

[PATCH 1/4] rust: pci: use to_result() in enable_device_mem()

[Danilo Krummrich]

Simplify enable_device_mem() by using to_result() to handle the return
value of the corresponding FFI call.

Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 rust/kernel/pci.rs | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
index 4c98b5b9aa1e..386484dcf36e 100644
--- a/rust/kernel/pci.rs
+++ b/rust/kernel/pci.rs
@@ -382,12 +382,7 @@ pub fn device_id(&self) -> u16 {
     /// Enable memory resources for this device.
     pub fn enable_device_mem(&self) -> Result {
         // SAFETY: `self.as_raw` is guaranteed to be a pointer to a valid `struct pci_dev`.
-        let ret = unsafe { bindings::pci_enable_device_mem(self.as_raw()) };
-        if ret != 0 {
-            Err(Error::from_errno(ret))
-        } else {
-            Ok(())
-        }
+        to_result(unsafe { bindings::pci_enable_device_mem(self.as_raw()) })
     }
 
     /// Enable bus-mastering for this device.
-- 
2.48.1

[PATCH 2/4] rust: device: implement device context marker

[Danilo Krummrich]

Some bus device functions should only be called from bus callbacks,
such as probe(), remove(), resume(), suspend(), etc.

To ensure this add device context marker structs, that can be used as
generics for bus device implementations.

Suggested-by: Benno Lossin <benno.lossin@proton.me>
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 rust/kernel/device.rs | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
index db2d9658ba47..39793740a95c 100644
--- a/rust/kernel/device.rs
+++ b/rust/kernel/device.rs
@@ -209,6 +209,24 @@ unsafe impl Send for Device {}
 // synchronization in `struct device`.
 unsafe impl Sync for Device {}
 
+/// Marker trait for the context of a bus specific device.
+///
+/// Some functions of a bus specific device should only be called from a certain context, i.e. bus
+/// callbacks, such as `probe()`.
+///
+/// This is the marker trait for structures representing the context of a bus specific device.
+pub trait DeviceContext {}
+
+/// The [`Normal`] context is the context of a bus specific device when it is not an argument of
+/// any bus callback.
+pub struct Normal;
+impl DeviceContext for Normal {}
+
+/// The [`Core`] context is the context of a bus specific device when it is supplied as argument of
+/// any of the bus callbacks, such as `probe()`.
+pub struct Core;
+impl DeviceContext for Core {}
+
 #[doc(hidden)]
 #[macro_export]
 macro_rules! dev_printk {
-- 
2.48.1

[PATCH 3/4] rust: pci: fix unrestricted &mut pci::Device

[Danilo Krummrich]

As by now, pci::Device is implemented as:

	#[derive(Clone)]
	pub struct Device(ARef<device::Device>);

This may be convenient, but has the implication that drivers can call
device methods that require a mutable reference concurrently at any
point of time.

Instead define pci::Device as

	pub struct Device<Ctx: DeviceContext = Normal>(
		Opaque<bindings::pci_dev>,
		PhantomData<Ctx>,
	);

and manually implement the AlwaysRefCounted trait.

With this we can implement methods that should only be called from
bus callbacks (such as probe()) for pci::Device<Core>. Consequently, we
make this type accessible in bus callbacks only.

Arbitrary references taken by the driver are still of type
ARef<pci::Device> and hence don't provide access to methods that are
reserved for bus callbacks.

Fixes: 1bd8b6b2c5d3 ("rust: pci: add basic PCI device / driver abstractions")
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 drivers/gpu/nova-core/driver.rs |   4 +-
 rust/kernel/pci.rs              | 126 ++++++++++++++++++++------------
 samples/rust/rust_driver_pci.rs |   8 +-
 3 files changed, 85 insertions(+), 53 deletions(-)

diff --git a/drivers/gpu/nova-core/driver.rs b/drivers/gpu/nova-core/driver.rs
index 63c19f140fbd..a08fb6599267 100644
--- a/drivers/gpu/nova-core/driver.rs
+++ b/drivers/gpu/nova-core/driver.rs
@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 
-use kernel::{bindings, c_str, pci, prelude::*};
+use kernel::{bindings, c_str, device::Core, pci, prelude::*};
 
 use crate::gpu::Gpu;
 
@@ -27,7 +27,7 @@ impl pci::Driver for NovaCore {
     type IdInfo = ();
     const ID_TABLE: pci::IdTable<Self::IdInfo> = &PCI_TABLE;
 
-    fn probe(pdev: &mut pci::Device, _info: &Self::IdInfo) -> Result<Pin<KBox<Self>>> {
+    fn probe(pdev: &pci::Device<Core>, _info: &Self::IdInfo) -> Result<Pin<KBox<Self>>> {
         dev_dbg!(pdev.as_ref(), "Probe Nova Core GPU driver.\n");
 
         pdev.enable_device_mem()?;
diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
index 386484dcf36e..6357b4ff8d65 100644
--- a/rust/kernel/pci.rs
+++ b/rust/kernel/pci.rs
@@ -6,7 +6,7 @@
 
 use crate::{
     alloc::flags::*,
-    bindings, container_of, device,
+    bindings, device,
     device_id::RawDeviceId,
     devres::Devres,
     driver,
@@ -17,7 +17,11 @@
     types::{ARef, ForeignOwnable, Opaque},
     ThisModule,
 };
-use core::{ops::Deref, ptr::addr_of_mut};
+use core::{
+    marker::PhantomData,
+    ops::Deref,
+    ptr::{addr_of_mut, NonNull},
+};
 use kernel::prelude::*;
 
 /// An adapter for the registration of PCI drivers.
@@ -60,17 +64,16 @@ extern "C" fn probe_callback(
     ) -> kernel::ffi::c_int {
         // SAFETY: The PCI bus only ever calls the probe callback with a valid pointer to a
         // `struct pci_dev`.
-        let dev = unsafe { device::Device::get_device(addr_of_mut!((*pdev).dev)) };
-        // SAFETY: `dev` is guaranteed to be embedded in a valid `struct pci_dev` by the call
-        // above.
-        let mut pdev = unsafe { Device::from_dev(dev) };
+        //
+        // INVARIANT: `pdev` is valid for the duration of `probe_callback()`.
+        let pdev = unsafe { &*pdev.cast::<Device<device::Core>>() };
 
         // SAFETY: `DeviceId` is a `#[repr(transparent)` wrapper of `struct pci_device_id` and
         // does not add additional invariants, so it's safe to transmute.
         let id = unsafe { &*id.cast::<DeviceId>() };
         let info = T::ID_TABLE.info(id.index());
 
-        match T::probe(&mut pdev, info) {
+        match T::probe(pdev, info) {
             Ok(data) => {
                 // Let the `struct pci_dev` own a reference of the driver's private data.
                 // SAFETY: By the type invariant `pdev.as_raw` returns a valid pointer to a
@@ -192,7 +195,7 @@ macro_rules! pci_device_table {
 /// # Example
 ///
 ///```
-/// # use kernel::{bindings, pci};
+/// # use kernel::{bindings, device::Core, pci};
 ///
 /// struct MyDriver;
 ///
@@ -210,7 +213,7 @@ macro_rules! pci_device_table {
 ///     const ID_TABLE: pci::IdTable<Self::IdInfo> = &PCI_TABLE;
 ///
 ///     fn probe(
-///         _pdev: &mut pci::Device,
+///         _pdev: &pci::Device<Core>,
 ///         _id_info: &Self::IdInfo,
 ///     ) -> Result<Pin<KBox<Self>>> {
 ///         Err(ENODEV)
@@ -234,20 +237,23 @@ pub trait Driver {
     ///
     /// Called when a new platform device is added or discovered.
     /// Implementers should attempt to initialize the device here.
-    fn probe(dev: &mut Device, id_info: &Self::IdInfo) -> Result<Pin<KBox<Self>>>;
+    fn probe(dev: &Device<device::Core>, id_info: &Self::IdInfo) -> Result<Pin<KBox<Self>>>;
 }
 
 /// The PCI device representation.
 ///
-/// A PCI device is based on an always reference counted `device:Device` instance. Cloning a PCI
-/// device, hence, also increments the base device' reference count.
+/// This structure represents the Rust abstraction for a C `struct pci_dev`. The implementation
+/// abstracts the usage of an already existing C `struct pci_dev` within Rust code that we get
+/// passed from the C side.
 ///
 /// # Invariants
 ///
-/// `Device` hold a valid reference of `ARef<device::Device>` whose underlying `struct device` is a
-/// member of a `struct pci_dev`.
-#[derive(Clone)]
-pub struct Device(ARef<device::Device>);
+/// A [`Device`] instance represents a valid `struct device` created by the C portion of the kernel.
+#[repr(transparent)]
+pub struct Device<Ctx: device::DeviceContext = device::Normal>(
+    Opaque<bindings::pci_dev>,
+    PhantomData<Ctx>,
+);
 
 /// A PCI BAR to perform I/O-Operations on.
 ///
@@ -256,13 +262,13 @@ pub trait Driver {
 /// `Bar` always holds an `IoRaw` inststance that holds a valid pointer to the start of the I/O
 /// memory mapped PCI bar and its size.
 pub struct Bar<const SIZE: usize = 0> {
-    pdev: Device,
+    pdev: ARef<Device>,
     io: IoRaw<SIZE>,
     num: i32,
 }
 
 impl<const SIZE: usize> Bar<SIZE> {
-    fn new(pdev: Device, num: u32, name: &CStr) -> Result<Self> {
+    fn new(pdev: &Device, num: u32, name: &CStr) -> Result<Self> {
         let len = pdev.resource_len(num)?;
         if len == 0 {
             return Err(ENOMEM);
@@ -300,12 +306,16 @@ fn new(pdev: Device, num: u32, name: &CStr) -> Result<Self> {
                 // `pdev` is valid by the invariants of `Device`.
                 // `ioptr` is guaranteed to be the start of a valid I/O mapped memory region.
                 // `num` is checked for validity by a previous call to `Device::resource_len`.
-                unsafe { Self::do_release(&pdev, ioptr, num) };
+                unsafe { Self::do_release(pdev, ioptr, num) };
                 return Err(err);
             }
         };
 
-        Ok(Bar { pdev, io, num })
+        Ok(Bar {
+            pdev: pdev.into(),
+            io,
+            num,
+        })
     }
 
     /// # Safety
@@ -351,20 +361,8 @@ fn deref(&self) -> &Self::Target {
 }
 
 impl Device {
-    /// Create a PCI Device instance from an existing `device::Device`.
-    ///
-    /// # Safety
-    ///
-    /// `dev` must be an `ARef<device::Device>` whose underlying `bindings::device` is a member of
-    /// a `bindings::pci_dev`.
-    pub unsafe fn from_dev(dev: ARef<device::Device>) -> Self {
-        Self(dev)
-    }
-
     fn as_raw(&self) -> *mut bindings::pci_dev {
-        // SAFETY: By the type invariant `self.0.as_raw` is a pointer to the `struct device`
-        // embedded in `struct pci_dev`.
-        unsafe { container_of!(self.0.as_raw(), bindings::pci_dev, dev) as _ }
+        self.0.get()
     }
 
     /// Returns the PCI vendor ID.
@@ -379,18 +377,6 @@ pub fn device_id(&self) -> u16 {
         unsafe { (*self.as_raw()).device }
     }
 
-    /// Enable memory resources for this device.
-    pub fn enable_device_mem(&self) -> Result {
-        // SAFETY: `self.as_raw` is guaranteed to be a pointer to a valid `struct pci_dev`.
-        to_result(unsafe { bindings::pci_enable_device_mem(self.as_raw()) })
-    }
-
-    /// Enable bus-mastering for this device.
-    pub fn set_master(&self) {
-        // SAFETY: `self.as_raw` is guaranteed to be a pointer to a valid `struct pci_dev`.
-        unsafe { bindings::pci_set_master(self.as_raw()) };
-    }
-
     /// 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) {
@@ -410,7 +396,7 @@ pub fn iomap_region_sized<const SIZE: usize>(
         bar: u32,
         name: &CStr,
     ) -> Result<Devres<Bar<SIZE>>> {
-        let bar = Bar::<SIZE>::new(self.clone(), bar, name)?;
+        let bar = Bar::<SIZE>::new(self, bar, name)?;
         let devres = Devres::new(self.as_ref(), bar, GFP_KERNEL)?;
 
         Ok(devres)
@@ -422,8 +408,54 @@ pub fn iomap_region(&self, bar: u32, name: &CStr) -> Result<Devres<Bar>> {
     }
 }
 
+impl Device<device::Core> {
+    /// Enable memory resources for this device.
+    pub fn enable_device_mem(&self) -> Result {
+        // SAFETY: `self.as_raw` is guaranteed to be a pointer to a valid `struct pci_dev`.
+        to_result(unsafe { bindings::pci_enable_device_mem(self.as_raw()) })
+    }
+
+    /// Enable bus-mastering for this device.
+    pub fn set_master(&self) {
+        // SAFETY: `self.as_raw` is guaranteed to be a pointer to a valid `struct pci_dev`.
+        unsafe { bindings::pci_set_master(self.as_raw()) };
+    }
+}
+
+impl Deref for Device<device::Core> {
+    type Target = Device;
+
+    fn deref(&self) -> &Self::Target {
+        let ptr: *const Self = self;
+
+        // CAST: `Device<Ctx>` is a transparent wrapper of `Opaque<bindings::pci_dev>`.
+        let ptr = ptr.cast::<Device>();
+
+        // SAFETY: `ptr` was derived from `&self`.
+        unsafe { &*ptr }
+    }
+}
+
+// SAFETY: Instances of `Device` are always reference-counted.
+unsafe impl crate::types::AlwaysRefCounted for Device {
+    fn inc_ref(&self) {
+        // SAFETY: The existence of a shared reference guarantees that the refcount is non-zero.
+        unsafe { bindings::pci_dev_get(self.as_raw()) };
+    }
+
+    unsafe fn dec_ref(obj: NonNull<Self>) {
+        // SAFETY: The safety requirements guarantee that the refcount is non-zero.
+        unsafe { bindings::pci_dev_put(obj.cast().as_ptr()) }
+    }
+}
+
 impl AsRef<device::Device> for Device {
     fn as_ref(&self) -> &device::Device {
-        &self.0
+        // SAFETY: By the type invariant of `Self`, `self.as_raw()` is a pointer to a valid
+        // `struct pci_dev`.
+        let dev = unsafe { addr_of_mut!((*self.as_raw()).dev) };
+
+        // SAFETY: `dev` points to a valid `struct device`.
+        unsafe { device::Device::as_ref(dev) }
     }
 }
diff --git a/samples/rust/rust_driver_pci.rs b/samples/rust/rust_driver_pci.rs
index 1fb6e44f3395..b90df5f9d1d0 100644
--- a/samples/rust/rust_driver_pci.rs
+++ b/samples/rust/rust_driver_pci.rs
@@ -4,7 +4,7 @@
 //!
 //! To make this driver probe, QEMU must be run with `-device pci-testdev`.
 
-use kernel::{bindings, c_str, devres::Devres, pci, prelude::*};
+use kernel::{bindings, c_str, device::Core, devres::Devres, pci, prelude::*, types::ARef};
 
 struct Regs;
 
@@ -26,7 +26,7 @@ impl TestIndex {
 }
 
 struct SampleDriver {
-    pdev: pci::Device,
+    pdev: ARef<pci::Device>,
     bar: Devres<Bar0>,
 }
 
@@ -62,7 +62,7 @@ impl pci::Driver for SampleDriver {
 
     const ID_TABLE: pci::IdTable<Self::IdInfo> = &PCI_TABLE;
 
-    fn probe(pdev: &mut pci::Device, info: &Self::IdInfo) -> Result<Pin<KBox<Self>>> {
+    fn probe(pdev: &pci::Device<Core>, info: &Self::IdInfo) -> Result<Pin<KBox<Self>>> {
         dev_dbg!(
             pdev.as_ref(),
             "Probe Rust PCI driver sample (PCI ID: 0x{:x}, 0x{:x}).\n",
@@ -77,7 +77,7 @@ fn probe(pdev: &mut pci::Device, info: &Self::IdInfo) -> Result<Pin<KBox<Self>>>
 
         let drvdata = KBox::new(
             Self {
-                pdev: pdev.clone(),
+                pdev: (&**pdev).into(),
                 bar,
             },
             GFP_KERNEL,
-- 
2.48.1

[PATCH 4/4] rust: platform: fix unrestricted &mut platform::Device

[Danilo Krummrich]

As by now, platform::Device is implemented as:

	#[derive(Clone)]
	pub struct Device(ARef<device::Device>);

This may be convenient, but has the implication that drivers can call
device methods that require a mutable reference concurrently at any
point of time.

Instead define platform::Device as

	pub struct Device<Ctx: DeviceContext = Normal>(
		Opaque<bindings::platform_dev>,
		PhantomData<Ctx>,
	);

and manually implement the AlwaysRefCounted trait.

With this we can implement methods that should only be called from
bus callbacks (such as probe()) for platform::Device<Core>. Consequently,
we make this type accessible in bus callbacks only.

Arbitrary references taken by the driver are still of type
ARef<platform::Device> and hence don't provide access to methods that are
reserved for bus callbacks.

Fixes: 683a63befc73 ("rust: platform: add basic platform device / driver abstractions")
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 rust/kernel/platform.rs              | 93 ++++++++++++++++++----------
 samples/rust/rust_driver_platform.rs | 16 +++--
 2 files changed, 74 insertions(+), 35 deletions(-)

diff --git a/rust/kernel/platform.rs b/rust/kernel/platform.rs
index 50e6b0421813..f7a055739143 100644
--- a/rust/kernel/platform.rs
+++ b/rust/kernel/platform.rs
@@ -5,16 +5,20 @@
 //! C header: [`include/linux/platform_device.h`](srctree/include/linux/platform_device.h)
 
 use crate::{
-    bindings, container_of, device, driver,
+    bindings, device, driver,
     error::{to_result, Result},
     of,
     prelude::*,
     str::CStr,
-    types::{ARef, ForeignOwnable, Opaque},
+    types::{ForeignOwnable, Opaque},
     ThisModule,
 };
 
-use core::ptr::addr_of_mut;
+use core::{
+    marker::PhantomData,
+    ops::Deref,
+    ptr::{addr_of_mut, NonNull},
+};
 
 /// An adapter for the registration of platform drivers.
 pub struct Adapter<T: Driver>(T);
@@ -54,14 +58,14 @@ unsafe fn unregister(pdrv: &Opaque<Self::RegType>) {
 
 impl<T: Driver + 'static> Adapter<T> {
     extern "C" fn probe_callback(pdev: *mut bindings::platform_device) -> kernel::ffi::c_int {
-        // SAFETY: The platform bus only ever calls the probe callback with a valid `pdev`.
-        let dev = unsafe { device::Device::get_device(addr_of_mut!((*pdev).dev)) };
-        // SAFETY: `dev` is guaranteed to be embedded in a valid `struct platform_device` by the
-        // call above.
-        let mut pdev = unsafe { Device::from_dev(dev) };
+        // SAFETY: The platform bus only ever calls the probe callback with a valid pointer to a
+        // `struct platform_device`.
+        //
+        // INVARIANT: `pdev` is valid for the duration of `probe_callback()`.
+        let pdev = unsafe { &*pdev.cast::<Device<device::Core>>() };
 
         let info = <Self as driver::Adapter>::id_info(pdev.as_ref());
-        match T::probe(&mut pdev, info) {
+        match T::probe(pdev, info) {
             Ok(data) => {
                 // Let the `struct platform_device` own a reference of the driver's private data.
                 // SAFETY: By the type invariant `pdev.as_raw` returns a valid pointer to a
@@ -120,7 +124,7 @@ macro_rules! module_platform_driver {
 /// # Example
 ///
 ///```
-/// # use kernel::{bindings, c_str, of, platform};
+/// # use kernel::{bindings, c_str, device::Core, of, platform};
 ///
 /// struct MyDriver;
 ///
@@ -138,7 +142,7 @@ macro_rules! module_platform_driver {
 ///     const OF_ID_TABLE: Option<of::IdTable<Self::IdInfo>> = Some(&OF_TABLE);
 ///
 ///     fn probe(
-///         _pdev: &mut platform::Device,
+///         _pdev: &platform::Device<Core>,
 ///         _id_info: Option<&Self::IdInfo>,
 ///     ) -> Result<Pin<KBox<Self>>> {
 ///         Err(ENODEV)
@@ -160,41 +164,68 @@ pub trait Driver {
     ///
     /// Called when a new platform device is added or discovered.
     /// Implementers should attempt to initialize the device here.
-    fn probe(dev: &mut Device, id_info: Option<&Self::IdInfo>) -> Result<Pin<KBox<Self>>>;
+    fn probe(
+        dev: &Device<device::Core>,
+        id_info: Option<&Self::IdInfo>,
+    ) -> Result<Pin<KBox<Self>>>;
 }
 
 /// The platform device representation.
 ///
-/// A platform device is based on an always reference counted `device:Device` instance. Cloning a
-/// platform device, hence, also increments the base device' reference count.
+/// This structure represents the Rust abstraction for a C `struct platform_device`. The
+/// implementation abstracts the usage of an already existing C `struct platform_device` within Rust
+/// code that we get passed from the C side.
 ///
 /// # Invariants
 ///
-/// `Device` holds a valid reference of `ARef<device::Device>` whose underlying `struct device` is a
-/// member of a `struct platform_device`.
-#[derive(Clone)]
-pub struct Device(ARef<device::Device>);
+/// A [`Device`] instance represents a valid `struct platform_device` created by the C portion of
+/// the kernel.
+#[repr(transparent)]
+pub struct Device<Ctx: device::DeviceContext = device::Normal>(
+    Opaque<bindings::platform_device>,
+    PhantomData<Ctx>,
+);
 
 impl Device {
-    /// Convert a raw kernel device into a `Device`
-    ///
-    /// # Safety
-    ///
-    /// `dev` must be an `Aref<device::Device>` whose underlying `bindings::device` is a member of a
-    /// `bindings::platform_device`.
-    unsafe fn from_dev(dev: ARef<device::Device>) -> Self {
-        Self(dev)
+    fn as_raw(&self) -> *mut bindings::platform_device {
+        self.0.get()
     }
+}
 
-    fn as_raw(&self) -> *mut bindings::platform_device {
-        // SAFETY: By the type invariant `self.0.as_raw` is a pointer to the `struct device`
-        // embedded in `struct platform_device`.
-        unsafe { container_of!(self.0.as_raw(), bindings::platform_device, dev) }.cast_mut()
+impl Deref for Device<device::Core> {
+    type Target = Device;
+
+    fn deref(&self) -> &Self::Target {
+        let ptr: *const Self = self;
+
+        // CAST: `Device<Ctx>` is a transparent wrapper of `Opaque<bindings::platform_device>`.
+        let ptr = ptr.cast::<Device>();
+
+        // SAFETY: `ptr` was derived from `&self`.
+        unsafe { &*ptr }
+    }
+}
+
+// SAFETY: Instances of `Device` are always reference-counted.
+unsafe impl crate::types::AlwaysRefCounted for Device {
+    fn inc_ref(&self) {
+        // SAFETY: The existence of a shared reference guarantees that the refcount is non-zero.
+        unsafe { bindings::get_device(self.as_ref().as_raw()) };
+    }
+
+    unsafe fn dec_ref(obj: NonNull<Self>) {
+        // SAFETY: The safety requirements guarantee that the refcount is non-zero.
+        unsafe { bindings::platform_device_put(obj.cast().as_ptr()) }
     }
 }
 
 impl AsRef<device::Device> for Device {
     fn as_ref(&self) -> &device::Device {
-        &self.0
+        // SAFETY: By the type invariant of `Self`, `self.as_raw()` is a pointer to a valid
+        // `struct platform_device`.
+        let dev = unsafe { addr_of_mut!((*self.as_raw()).dev) };
+
+        // SAFETY: `dev` points to a valid `struct device`.
+        unsafe { device::Device::as_ref(dev) }
     }
 }
diff --git a/samples/rust/rust_driver_platform.rs b/samples/rust/rust_driver_platform.rs
index 8120609e2940..0fb4c6033d60 100644
--- a/samples/rust/rust_driver_platform.rs
+++ b/samples/rust/rust_driver_platform.rs
@@ -2,10 +2,10 @@
 
 //! Rust Platform driver sample.
 
-use kernel::{c_str, of, platform, prelude::*};
+use kernel::{c_str, device::Core, of, platform, prelude::*, types::ARef};
 
 struct SampleDriver {
-    pdev: platform::Device,
+    pdev: ARef<platform::Device>,
 }
 
 struct Info(u32);
@@ -21,14 +21,22 @@ impl platform::Driver for SampleDriver {
     type IdInfo = Info;
     const OF_ID_TABLE: Option<of::IdTable<Self::IdInfo>> = Some(&OF_TABLE);
 
-    fn probe(pdev: &mut platform::Device, info: Option<&Self::IdInfo>) -> Result<Pin<KBox<Self>>> {
+    fn probe(
+        pdev: &platform::Device<Core>,
+        info: Option<&Self::IdInfo>,
+    ) -> Result<Pin<KBox<Self>>> {
         dev_dbg!(pdev.as_ref(), "Probe Rust Platform driver sample.\n");
 
         if let Some(info) = info {
             dev_info!(pdev.as_ref(), "Probed with info: '{}'.\n", info.0);
         }
 
-        let drvdata = KBox::new(Self { pdev: pdev.clone() }, GFP_KERNEL)?;
+        let drvdata = KBox::new(
+            Self {
+                pdev: (&**pdev).into(),
+            },
+            GFP_KERNEL,
+        )?;
 
         Ok(drvdata.into())
     }
-- 
2.48.1

Re: [PATCH 0/4] Improve soundness of bus device abstractions

[Boqun Feng]

On Thu, Mar 13, 2025 at 03:13:30AM +0100, Danilo Krummrich wrote:
> Currently, when sharing references of bus devices (e.g. ARef<pci::Device>), we
> do not have a way to restrict which functions of a bus device can be called.
> 
> Consequently, it is possible to call all bus device functions concurrently from
> any context. This includes functions, which access fields of the (bus) device,
> which are not protected against concurrent access.
> 
> This is improved by applying an execution context to the bus device in form of a
> generic type.
> 
> For instance, the PCI device reference that is passed to probe() has the type
> pci::Device<Core>, which implements all functions that are only allowed to be
> called from bus callbacks.
> 
> The implementation for the default context (pci::Device) contains all functions
> that are safe to call from any context concurrently.
> 
> The context types can be extended as required, e.g. to limit availability  of
> certain (bus) device functions to probe().
> 

For the whole series:

Acked-by: Boqun Feng <boqun.feng@gmail.com>

Regards,
Boqun

> A branch containing the patches can be found in [1].
> 
> [1] https://web.git.kernel.org/pub/scm/linux/kernel/git/dakr/linux.git/log/?h=rust/device
> 
> Danilo Krummrich (4):
>   rust: pci: use to_result() in enable_device_mem()
>   rust: device: implement device context marker
>   rust: pci: fix unrestricted &mut pci::Device
>   rust: platform: fix unrestricted &mut platform::Device
> 
>  drivers/gpu/nova-core/driver.rs      |   4 +-
>  rust/kernel/device.rs                |  18 ++++
>  rust/kernel/pci.rs                   | 131 ++++++++++++++++-----------
>  rust/kernel/platform.rs              |  93 ++++++++++++-------
>  samples/rust/rust_driver_pci.rs      |   8 +-
>  samples/rust/rust_driver_platform.rs |  16 +++-
>  6 files changed, 177 insertions(+), 93 deletions(-)
> 
> 
> base-commit: b28786b190d1ae2df5e6a5181ad78c6f226ea3e1
> -- 
> 2.48.1
> 

Re: [PATCH 1/4] rust: pci: use to_result() in enable_device_mem()

[Benno Lossin]

On Thu Mar 13, 2025 at 3:13 AM CET, Danilo Krummrich wrote:
> Simplify enable_device_mem() by using to_result() to handle the return
> value of the corresponding FFI call.
>
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>

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

---
Cheers,
Benno

> ---
>  rust/kernel/pci.rs | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
> index 4c98b5b9aa1e..386484dcf36e 100644
> --- a/rust/kernel/pci.rs
> +++ b/rust/kernel/pci.rs
> @@ -382,12 +382,7 @@ pub fn device_id(&self) -> u16 {
>      /// Enable memory resources for this device.
>      pub fn enable_device_mem(&self) -> Result {
>          // SAFETY: `self.as_raw` is guaranteed to be a pointer to a valid `struct pci_dev`.
> -        let ret = unsafe { bindings::pci_enable_device_mem(self.as_raw()) };
> -        if ret != 0 {
> -            Err(Error::from_errno(ret))
> -        } else {
> -            Ok(())
> -        }
> +        to_result(unsafe { bindings::pci_enable_device_mem(self.as_raw()) })
>      }
>  
>      /// Enable bus-mastering for this device.

Re: [PATCH 2/4] rust: device: implement device context marker

[Benno Lossin]

On Thu Mar 13, 2025 at 3:13 AM CET, Danilo Krummrich wrote:
> Some bus device functions should only be called from bus callbacks,
> such as probe(), remove(), resume(), suspend(), etc.
>
> To ensure this add device context marker structs, that can be used as
> generics for bus device implementations.
>
> Suggested-by: Benno Lossin <benno.lossin@proton.me>
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>

I would have folded this into #3, but if you prefer it being split, then
it's also fine.

> ---
>  rust/kernel/device.rs | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>
> diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
> index db2d9658ba47..39793740a95c 100644
> --- a/rust/kernel/device.rs
> +++ b/rust/kernel/device.rs
> @@ -209,6 +209,24 @@ unsafe impl Send for Device {}
>  // synchronization in `struct device`.
>  unsafe impl Sync for Device {}
>  
> +/// Marker trait for the context of a bus specific device.
> +///
> +/// Some functions of a bus specific device should only be called from a certain context, i.e. bus
> +/// callbacks, such as `probe()`.
> +///
> +/// This is the marker trait for structures representing the context of a bus specific device.
> +pub trait DeviceContext {}

I would make this trait sealed. ie:

    pub trait DeviceContext: private::Sealed {}
    
    mod private {
        pub trait Sealed {}

        impl Sealed for super::Core {}
        impl Sealed for super::Normal {}
    }

Since currently a user can create a custom context (it will be useless,
but then I think it still is better to give them a compile error).

If you make it sealed,

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

---
Cheers,
Benno

> +
> +/// The [`Normal`] context is the context of a bus specific device when it is not an argument of
> +/// any bus callback.
> +pub struct Normal;
> +impl DeviceContext for Normal {}
> +
> +/// The [`Core`] context is the context of a bus specific device when it is supplied as argument of
> +/// any of the bus callbacks, such as `probe()`.
> +pub struct Core;
> +impl DeviceContext for Core {}
> +
>  #[doc(hidden)]
>  #[macro_export]
>  macro_rules! dev_printk {

Re: [PATCH 2/4] rust: device: implement device context marker

[Danilo Krummrich]

On Thu, Mar 13, 2025 at 10:29:35AM +0000, Benno Lossin wrote:
> On Thu Mar 13, 2025 at 3:13 AM CET, Danilo Krummrich wrote:
> > +/// Marker trait for the context of a bus specific device.
> > +///
> > +/// Some functions of a bus specific device should only be called from a certain context, i.e. bus
> > +/// callbacks, such as `probe()`.
> > +///
> > +/// This is the marker trait for structures representing the context of a bus specific device.
> > +pub trait DeviceContext {}
> 
> I would make this trait sealed. ie:
> 
>     pub trait DeviceContext: private::Sealed {}
>     
>     mod private {
>         pub trait Sealed {}
> 
>         impl Sealed for super::Core {}
>         impl Sealed for super::Normal {}
>     }
> 
> Since currently a user can create a custom context (it will be useless,
> but then I think it still is better to give them a compile error).

That is intentional, some busses have bus specific callbacks, hence we may want
a bus specific context at some point.

However, we can always change visibility as needed, so I'm fine making it sealed
for now.

> 
> If you make it sealed,
> 
> Reviewed-by: Benno Lossin <benno.lossin@proton.me>

Re: [PATCH 3/4] rust: pci: fix unrestricted &mut pci::Device

[Benno Lossin]

On Thu Mar 13, 2025 at 3:13 AM CET, Danilo Krummrich wrote:
> As by now, pci::Device is implemented as:
>
> 	#[derive(Clone)]
> 	pub struct Device(ARef<device::Device>);
>
> This may be convenient, but has the implication that drivers can call
> device methods that require a mutable reference concurrently at any
> point of time.

Which methods take mutable references? The `set_master` method you
mentioned also took a shared reference before this patch.

> Instead define pci::Device as
>
> 	pub struct Device<Ctx: DeviceContext = Normal>(
> 		Opaque<bindings::pci_dev>,
> 		PhantomData<Ctx>,
> 	);
>
> and manually implement the AlwaysRefCounted trait.
>
> With this we can implement methods that should only be called from
> bus callbacks (such as probe()) for pci::Device<Core>. Consequently, we
> make this type accessible in bus callbacks only.
>
> Arbitrary references taken by the driver are still of type
> ARef<pci::Device> and hence don't provide access to methods that are
> reserved for bus callbacks.
>
> Fixes: 1bd8b6b2c5d3 ("rust: pci: add basic PCI device / driver abstractions")
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>

Two small nits below, but it already looks good:

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

> ---
>  drivers/gpu/nova-core/driver.rs |   4 +-
>  rust/kernel/pci.rs              | 126 ++++++++++++++++++++------------
>  samples/rust/rust_driver_pci.rs |   8 +-
>  3 files changed, 85 insertions(+), 53 deletions(-)
>

> @@ -351,20 +361,8 @@ fn deref(&self) -> &Self::Target {
>  }
>  
>  impl Device {

One alternative to implementing `Deref` below would be to change this to
`impl<Ctx: DeviceContext> Device<Ctx>`. But then one would lose the
ability to just do `&pdev` to get a `Device` from a `Device<Core>`... So
I think the deref way is better. Just wanted to mention this in case
someone re-uses this pattern.

> -    /// Create a PCI Device instance from an existing `device::Device`.
> -    ///
> -    /// # Safety
> -    ///
> -    /// `dev` must be an `ARef<device::Device>` whose underlying `bindings::device` is a member of
> -    /// a `bindings::pci_dev`.
> -    pub unsafe fn from_dev(dev: ARef<device::Device>) -> Self {
> -        Self(dev)
> -    }
> -
>      fn as_raw(&self) -> *mut bindings::pci_dev {
> -        // SAFETY: By the type invariant `self.0.as_raw` is a pointer to the `struct device`
> -        // embedded in `struct pci_dev`.
> -        unsafe { container_of!(self.0.as_raw(), bindings::pci_dev, dev) as _ }
> +        self.0.get()
>      }
>  
>      /// Returns the PCI vendor ID.

>  impl AsRef<device::Device> for Device {
>      fn as_ref(&self) -> &device::Device {
> -        &self.0
> +        // SAFETY: By the type invariant of `Self`, `self.as_raw()` is a pointer to a valid
> +        // `struct pci_dev`.
> +        let dev = unsafe { addr_of_mut!((*self.as_raw()).dev) };
> +
> +        // SAFETY: `dev` points to a valid `struct device`.
> +        unsafe { device::Device::as_ref(dev) }

Why not use `&**self` instead (ie go through the `Deref` impl)?

> @@ -77,7 +77,7 @@ fn probe(pdev: &mut pci::Device, info: &Self::IdInfo) -> Result<Pin<KBox<Self>>>
>  
>          let drvdata = KBox::new(
>              Self {
> -                pdev: pdev.clone(),
> +                pdev: (&**pdev).into(),

It might be possible to do:

    impl From<&pci::Device<Core>> for ARef<pci::Device> { ... }

Then this line could become `pdev: pdev.into()`.

---
Cheers,
Benno

>                  bar,
>              },
>              GFP_KERNEL,

Re: [PATCH 2/4] rust: device: implement device context marker

[Benno Lossin]

On Thu Mar 13, 2025 at 11:29 AM CET, Benno Lossin wrote:
> On Thu Mar 13, 2025 at 3:13 AM CET, Danilo Krummrich wrote:
>> Some bus device functions should only be called from bus callbacks,
>> such as probe(), remove(), resume(), suspend(), etc.
>>
>> To ensure this add device context marker structs, that can be used as
>> generics for bus device implementations.
>>
>> Suggested-by: Benno Lossin <benno.lossin@proton.me>
>> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
>
> I would have folded this into #3, but if you prefer it being split, then
> it's also fine.

Ah I didn't look at patch #4, then I would also keep this separate.

---
Cheers,
Benno

Re: [PATCH 4/4] rust: platform: fix unrestricted &mut platform::Device

[Benno Lossin]

On Thu Mar 13, 2025 at 3:13 AM CET, Danilo Krummrich wrote:
> As by now, platform::Device is implemented as:
>
> 	#[derive(Clone)]
> 	pub struct Device(ARef<device::Device>);
>
> This may be convenient, but has the implication that drivers can call
> device methods that require a mutable reference concurrently at any
> point of time.

Similar to the other patch, I didn't find any methods taking `&mut self`
but I might have missed them.

> Instead define platform::Device as
>
> 	pub struct Device<Ctx: DeviceContext = Normal>(
> 		Opaque<bindings::platform_dev>,
> 		PhantomData<Ctx>,
> 	);
>
> and manually implement the AlwaysRefCounted trait.
>
> With this we can implement methods that should only be called from
> bus callbacks (such as probe()) for platform::Device<Core>. Consequently,
> we make this type accessible in bus callbacks only.
>
> Arbitrary references taken by the driver are still of type
> ARef<platform::Device> and hence don't provide access to methods that are
> reserved for bus callbacks.
>
> Fixes: 683a63befc73 ("rust: platform: add basic platform device / driver abstractions")
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>

The same two nits from patch #3 also apply.

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

---
Cheers,
Benno

> ---
>  rust/kernel/platform.rs              | 93 ++++++++++++++++++----------
>  samples/rust/rust_driver_platform.rs | 16 +++--
>  2 files changed, 74 insertions(+), 35 deletions(-)

Re: [PATCH 2/4] rust: device: implement device context marker

[Benno Lossin]

On Thu Mar 13, 2025 at 11:41 AM CET, Danilo Krummrich wrote:
> On Thu, Mar 13, 2025 at 10:29:35AM +0000, Benno Lossin wrote:
>> On Thu Mar 13, 2025 at 3:13 AM CET, Danilo Krummrich wrote:
>> > +/// Marker trait for the context of a bus specific device.
>> > +///
>> > +/// Some functions of a bus specific device should only be called from a certain context, i.e. bus
>> > +/// callbacks, such as `probe()`.
>> > +///
>> > +/// This is the marker trait for structures representing the context of a bus specific device.
>> > +pub trait DeviceContext {}
>> 
>> I would make this trait sealed. ie:
>> 
>>     pub trait DeviceContext: private::Sealed {}
>>     
>>     mod private {
>>         pub trait Sealed {}
>> 
>>         impl Sealed for super::Core {}
>>         impl Sealed for super::Normal {}
>>     }
>> 
>> Since currently a user can create a custom context (it will be useless,
>> but then I think it still is better to give them a compile error).
>
> That is intentional, some busses have bus specific callbacks, hence we may want
> a bus specific context at some point.

Then it's even better we went with the generics vs using mutable
references :)

> However, we can always change visibility as needed, so I'm fine making it sealed
> for now.

Couldn't you just add them here? Then sealing wouldn't be a problem.

---
Cheers,
Benno

Re: [PATCH 2/4] rust: device: implement device context marker

[Danilo Krummrich]

On Thu, Mar 13, 2025 at 10:52:34AM +0000, Benno Lossin wrote:
> On Thu Mar 13, 2025 at 11:41 AM CET, Danilo Krummrich wrote:
> 
> > However, we can always change visibility as needed, so I'm fine making it sealed
> > for now.
> 
> Couldn't you just add them here? Then sealing wouldn't be a problem.

Yes, but I wouldn't want bus specific stuff to reside in device.rs.

Re: [PATCH 3/4] rust: pci: fix unrestricted &mut pci::Device

[Danilo Krummrich]

On Thu, Mar 13, 2025 at 10:44:38AM +0000, Benno Lossin wrote:
> On Thu Mar 13, 2025 at 3:13 AM CET, Danilo Krummrich wrote:
> > As by now, pci::Device is implemented as:
> >
> > 	#[derive(Clone)]
> > 	pub struct Device(ARef<device::Device>);
> >
> > This may be convenient, but has the implication that drivers can call
> > device methods that require a mutable reference concurrently at any
> > point of time.
> 
> Which methods take mutable references? The `set_master` method you
> mentioned also took a shared reference before this patch.

Yeah, that's basically a bug that I never fixed (until now), since making it
take a mutable reference would not have changed anything in terms of
accessibility.

> 
> > Instead define pci::Device as
> >
> > 	pub struct Device<Ctx: DeviceContext = Normal>(
> > 		Opaque<bindings::pci_dev>,
> > 		PhantomData<Ctx>,
> > 	);
> >
> > and manually implement the AlwaysRefCounted trait.
> >
> > With this we can implement methods that should only be called from
> > bus callbacks (such as probe()) for pci::Device<Core>. Consequently, we
> > make this type accessible in bus callbacks only.
> >
> > Arbitrary references taken by the driver are still of type
> > ARef<pci::Device> and hence don't provide access to methods that are
> > reserved for bus callbacks.
> >
> > Fixes: 1bd8b6b2c5d3 ("rust: pci: add basic PCI device / driver abstractions")
> > Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> 
> Two small nits below, but it already looks good:
> 
> Reviewed-by: Benno Lossin <benno.lossin@proton.me>
> 
> > ---
> >  drivers/gpu/nova-core/driver.rs |   4 +-
> >  rust/kernel/pci.rs              | 126 ++++++++++++++++++++------------
> >  samples/rust/rust_driver_pci.rs |   8 +-
> >  3 files changed, 85 insertions(+), 53 deletions(-)
> >
> 
> > @@ -351,20 +361,8 @@ fn deref(&self) -> &Self::Target {
> >  }
> >  
> >  impl Device {
> 
> One alternative to implementing `Deref` below would be to change this to
> `impl<Ctx: DeviceContext> Device<Ctx>`. But then one would lose the
> ability to just do `&pdev` to get a `Device` from a `Device<Core>`... So
> I think the deref way is better. Just wanted to mention this in case
> someone re-uses this pattern.
> 
> > -    /// Create a PCI Device instance from an existing `device::Device`.
> > -    ///
> > -    /// # Safety
> > -    ///
> > -    /// `dev` must be an `ARef<device::Device>` whose underlying `bindings::device` is a member of
> > -    /// a `bindings::pci_dev`.
> > -    pub unsafe fn from_dev(dev: ARef<device::Device>) -> Self {
> > -        Self(dev)
> > -    }
> > -
> >      fn as_raw(&self) -> *mut bindings::pci_dev {
> > -        // SAFETY: By the type invariant `self.0.as_raw` is a pointer to the `struct device`
> > -        // embedded in `struct pci_dev`.
> > -        unsafe { container_of!(self.0.as_raw(), bindings::pci_dev, dev) as _ }
> > +        self.0.get()
> >      }
> >  
> >      /// Returns the PCI vendor ID.
> 
> >  impl AsRef<device::Device> for Device {
> >      fn as_ref(&self) -> &device::Device {
> > -        &self.0
> > +        // SAFETY: By the type invariant of `Self`, `self.as_raw()` is a pointer to a valid
> > +        // `struct pci_dev`.
> > +        let dev = unsafe { addr_of_mut!((*self.as_raw()).dev) };
> > +
> > +        // SAFETY: `dev` points to a valid `struct device`.
> > +        unsafe { device::Device::as_ref(dev) }
> 
> Why not use `&**self` instead (ie go through the `Deref` impl)?

`&**self` gives us a `Device` (i.e. `pci::Device`), not a `device::Device`.

> 
> > @@ -77,7 +77,7 @@ fn probe(pdev: &mut pci::Device, info: &Self::IdInfo) -> Result<Pin<KBox<Self>>>
> >  
> >          let drvdata = KBox::new(
> >              Self {
> > -                pdev: pdev.clone(),
> > +                pdev: (&**pdev).into(),
> 
> It might be possible to do:
> 
>     impl From<&pci::Device<Core>> for ARef<pci::Device> { ... }
> 
> Then this line could become `pdev: pdev.into()`.

Yeah, having to write `&**pdev` was bothering me too, and I actually tried what
you suggest, but it didn't compile -- I'll double check.

Re: [PATCH 4/4] rust: platform: fix unrestricted &mut platform::Device

[Danilo Krummrich]

On Thu, Mar 13, 2025 at 10:49:59AM +0000, Benno Lossin wrote:
> On Thu Mar 13, 2025 at 3:13 AM CET, Danilo Krummrich wrote:
> > As by now, platform::Device is implemented as:
> >
> > 	#[derive(Clone)]
> > 	pub struct Device(ARef<device::Device>);
> >
> > This may be convenient, but has the implication that drivers can call
> > device methods that require a mutable reference concurrently at any
> > point of time.
> 
> Similar to the other patch, I didn't find any methods taking `&mut self`
> but I might have missed them.

`platform::Device` does not have any yet. But we still need the pattern. Once we
land the `dma::Device` trait, we'll need:

	impl dma::Device for platform::Device<Core> {}

to derive the DMA methods.

Besides that, I want bus device implementations to be consistent.

> 
> > Instead define platform::Device as
> >
> > 	pub struct Device<Ctx: DeviceContext = Normal>(
> > 		Opaque<bindings::platform_dev>,
> > 		PhantomData<Ctx>,
> > 	);
> >
> > and manually implement the AlwaysRefCounted trait.
> >
> > With this we can implement methods that should only be called from
> > bus callbacks (such as probe()) for platform::Device<Core>. Consequently,
> > we make this type accessible in bus callbacks only.
> >
> > Arbitrary references taken by the driver are still of type
> > ARef<platform::Device> and hence don't provide access to methods that are
> > reserved for bus callbacks.
> >
> > Fixes: 683a63befc73 ("rust: platform: add basic platform device / driver abstractions")
> > Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> 
> The same two nits from patch #3 also apply.
> 
> Reviewed-by: Benno Lossin <benno.lossin@proton.me>
> 
> ---
> Cheers,
> Benno
> 
> > ---
> >  rust/kernel/platform.rs              | 93 ++++++++++++++++++----------
> >  samples/rust/rust_driver_platform.rs | 16 +++--
> >  2 files changed, 74 insertions(+), 35 deletions(-)
> 

Re: [PATCH 3/4] rust: pci: fix unrestricted &mut pci::Device

[Benno Lossin]

On Thu Mar 13, 2025 at 3:25 PM CET, Danilo Krummrich wrote:
> On Thu, Mar 13, 2025 at 10:44:38AM +0000, Benno Lossin wrote:
>> On Thu Mar 13, 2025 at 3:13 AM CET, Danilo Krummrich wrote:
>> > As by now, pci::Device is implemented as:
>> >
>> > 	#[derive(Clone)]
>> > 	pub struct Device(ARef<device::Device>);
>> >
>> > This may be convenient, but has the implication that drivers can call
>> > device methods that require a mutable reference concurrently at any
>> > point of time.
>> 
>> Which methods take mutable references? The `set_master` method you
>> mentioned also took a shared reference before this patch.
>
> Yeah, that's basically a bug that I never fixed (until now), since making it
> take a mutable reference would not have changed anything in terms of
> accessibility.

Gotcha.

>> >  impl AsRef<device::Device> for Device {
>> >      fn as_ref(&self) -> &device::Device {
>> > -        &self.0
>> > +        // SAFETY: By the type invariant of `Self`, `self.as_raw()` is a pointer to a valid
>> > +        // `struct pci_dev`.
>> > +        let dev = unsafe { addr_of_mut!((*self.as_raw()).dev) };
>> > +
>> > +        // SAFETY: `dev` points to a valid `struct device`.
>> > +        unsafe { device::Device::as_ref(dev) }
>> 
>> Why not use `&**self` instead (ie go through the `Deref` impl)?
>
> `&**self` gives us a `Device` (i.e. `pci::Device`), not a `device::Device`.

Ah, yeah then you'll have to use `unsafe`.

---
Cheers,
Benno

Re: [PATCH 2/4] rust: device: implement device context marker

[Benno Lossin]

On Thu Mar 13, 2025 at 3:20 PM CET, Danilo Krummrich wrote:
> On Thu, Mar 13, 2025 at 10:52:34AM +0000, Benno Lossin wrote:
>> On Thu Mar 13, 2025 at 11:41 AM CET, Danilo Krummrich wrote:
>> 
>> > However, we can always change visibility as needed, so I'm fine making it sealed
>> > for now.
>> 
>> Couldn't you just add them here? Then sealing wouldn't be a problem.
>
> Yes, but I wouldn't want bus specific stuff to reside in device.rs.

Sure, we can always make the sealed trait crate-public, since my main
concern is that a driver author tries to add a new context.

---
Cheers,
Benno

Re: [PATCH 4/4] rust: platform: fix unrestricted &mut platform::Device

[Benno Lossin]

On Thu Mar 13, 2025 at 3:28 PM CET, Danilo Krummrich wrote:
> On Thu, Mar 13, 2025 at 10:49:59AM +0000, Benno Lossin wrote:
>> On Thu Mar 13, 2025 at 3:13 AM CET, Danilo Krummrich wrote:
>> > As by now, platform::Device is implemented as:
>> >
>> > 	#[derive(Clone)]
>> > 	pub struct Device(ARef<device::Device>);
>> >
>> > This may be convenient, but has the implication that drivers can call
>> > device methods that require a mutable reference concurrently at any
>> > point of time.
>> 
>> Similar to the other patch, I didn't find any methods taking `&mut self`
>> but I might have missed them.
>
> `platform::Device` does not have any yet. But we still need the pattern. Once we
> land the `dma::Device` trait, we'll need:
>
> 	impl dma::Device for platform::Device<Core> {}
>
> to derive the DMA methods.
>
> Besides that, I want bus device implementations to be consistent.

Yeah I think we should have this patch, just was confused by the commit
message.

---
Cheers,
Benno