From 0242623384c767b1156b61b67894b4ecf6682b8b Mon Sep 17 00:00:00 2001 From: Danilo Krummrich Date: Thu, 16 Oct 2025 14:55:28 +0200 Subject: rust: driver: let probe() return impl PinInit The driver model defines the lifetime of the private data stored in (and owned by) a bus device to be valid from when the driver is bound to a device (i.e. from successful probe()) until the driver is unbound from the device. This is already taken care of by the Rust implementation of the driver model. However, we still ask drivers to return a Result>> from probe(). Unlike in C, where we do not have the concept of initializers, but rather deal with uninitialized memory, drivers can just return an impl PinInit instead. This contributes to more clarity to the fact that a driver returns it's device private data in probe() and the Rust driver model owns the data, manages the lifetime and - considering the lifetime - provides (safe) accessors for the driver. Hence, let probe() functions return an impl PinInit instead of Result>>. Reviewed-by: Alice Ryhl Acked-by: Viresh Kumar Reviewed-by: Alexandre Courbot Acked-by: Greg Kroah-Hartman Signed-off-by: Danilo Krummrich --- samples/rust/rust_driver_auxiliary.rs | 25 +++++++++---------------- 1 file changed, 9 insertions(+), 16 deletions(-) (limited to 'samples/rust/rust_driver_auxiliary.rs') diff --git a/samples/rust/rust_driver_auxiliary.rs b/samples/rust/rust_driver_auxiliary.rs index 55ece336ee45..0e221abe4936 100644 --- a/samples/rust/rust_driver_auxiliary.rs +++ b/samples/rust/rust_driver_auxiliary.rs @@ -27,7 +27,7 @@ impl auxiliary::Driver for AuxiliaryDriver { const ID_TABLE: auxiliary::IdTable = &AUX_TABLE; - fn probe(adev: &auxiliary::Device, _info: &Self::IdInfo) -> Result>> { + fn probe(adev: &auxiliary::Device, _info: &Self::IdInfo) -> impl PinInit { dev_info!( adev.as_ref(), "Probing auxiliary driver for auxiliary device with id={}\n", @@ -36,9 +36,7 @@ impl auxiliary::Driver for AuxiliaryDriver { ParentDriver::connect(adev)?; - let this = KBox::new(Self, GFP_KERNEL)?; - - Ok(this.into()) + Ok(Self) } } @@ -58,18 +56,13 @@ impl pci::Driver for ParentDriver { const ID_TABLE: pci::IdTable = &PCI_TABLE; - fn probe(pdev: &pci::Device, _info: &Self::IdInfo) -> Result>> { - let this = KBox::new( - Self { - _reg: [ - auxiliary::Registration::new(pdev.as_ref(), AUXILIARY_NAME, 0, MODULE_NAME)?, - auxiliary::Registration::new(pdev.as_ref(), AUXILIARY_NAME, 1, MODULE_NAME)?, - ], - }, - GFP_KERNEL, - )?; - - Ok(this.into()) + fn probe(pdev: &pci::Device, _info: &Self::IdInfo) -> impl PinInit { + Ok(Self { + _reg: [ + auxiliary::Registration::new(pdev.as_ref(), AUXILIARY_NAME, 0, MODULE_NAME)?, + auxiliary::Registration::new(pdev.as_ref(), AUXILIARY_NAME, 1, MODULE_NAME)?, + ], + }) } } -- cgit v1.2.3 From 589b061975db3c7e87b819cc9a8006eb99ac4b5f Mon Sep 17 00:00:00 2001 From: Danilo Krummrich Date: Tue, 21 Oct 2025 00:34:25 +0200 Subject: rust: auxiliary: consider auxiliary devices always have a parent An auxiliary device is guaranteed to always have a parent device (both in C and Rust), hence don't return an Option<&auxiliary::Device> in auxiliary::Device::parent(). Reviewed-by: Alice Ryhl Reviewed-by: Greg Kroah-Hartman Signed-off-by: Danilo Krummrich --- drivers/gpu/drm/nova/file.rs | 2 +- rust/kernel/auxiliary.rs | 7 ++++--- samples/rust/rust_driver_auxiliary.rs | 2 +- 3 files changed, 6 insertions(+), 5 deletions(-) (limited to 'samples/rust/rust_driver_auxiliary.rs') diff --git a/drivers/gpu/drm/nova/file.rs b/drivers/gpu/drm/nova/file.rs index 90b9d2d0ec4a..a3b7bd36792c 100644 --- a/drivers/gpu/drm/nova/file.rs +++ b/drivers/gpu/drm/nova/file.rs @@ -28,7 +28,7 @@ impl File { _file: &drm::File, ) -> Result { let adev = &dev.adev; - let parent = adev.parent().ok_or(ENOENT)?; + let parent = adev.parent(); let pdev: &pci::Device = parent.try_into()?; let value = match getparam.param as u32 { diff --git a/rust/kernel/auxiliary.rs b/rust/kernel/auxiliary.rs index a6a2b23befce..e5bddb738d58 100644 --- a/rust/kernel/auxiliary.rs +++ b/rust/kernel/auxiliary.rs @@ -215,9 +215,10 @@ impl Device { unsafe { (*self.as_raw()).id } } - /// Returns a reference to the parent [`device::Device`], if any. - pub fn parent(&self) -> Option<&device::Device> { - self.as_ref().parent() + /// Returns a reference to the parent [`device::Device`]. + pub fn parent(&self) -> &device::Device { + // SAFETY: A `struct auxiliary_device` always has a parent. + unsafe { self.as_ref().parent().unwrap_unchecked() } } } diff --git a/samples/rust/rust_driver_auxiliary.rs b/samples/rust/rust_driver_auxiliary.rs index 0e221abe4936..2e9afeb83d4f 100644 --- a/samples/rust/rust_driver_auxiliary.rs +++ b/samples/rust/rust_driver_auxiliary.rs @@ -68,7 +68,7 @@ impl pci::Driver for ParentDriver { impl ParentDriver { fn connect(adev: &auxiliary::Device) -> Result<()> { - let parent = adev.parent().ok_or(EINVAL)?; + let parent = adev.parent(); let pdev: &pci::Device = parent.try_into()?; let vendor = pdev.vendor_id(); -- cgit v1.2.3 From e4e679c8608e5c747081cc6ce63ee0b0e524c68d Mon Sep 17 00:00:00 2001 From: Danilo Krummrich Date: Tue, 21 Oct 2025 00:34:26 +0200 Subject: rust: auxiliary: unregister on parent device unbind Guarantee that an auxiliary driver will be unbound before its parent is unbound; there is no point in operating an auxiliary device whose parent has been unbound. In practice, this guarantee allows us to assume that for a bound auxiliary device, also the parent device is bound. This is useful when an auxiliary driver calls into its parent, since it allows the parent to directly access device resources and its device private data due to the guaranteed bound device context. Reviewed-by: Alice Ryhl Reviewed-by: Greg Kroah-Hartman Signed-off-by: Danilo Krummrich --- drivers/gpu/nova-core/driver.rs | 8 ++-- rust/kernel/auxiliary.rs | 89 ++++++++++++++++++++--------------- samples/rust/rust_driver_auxiliary.rs | 17 ++++--- 3 files changed, 66 insertions(+), 48 deletions(-) (limited to 'samples/rust/rust_driver_auxiliary.rs') diff --git a/drivers/gpu/nova-core/driver.rs b/drivers/gpu/nova-core/driver.rs index a83b86199182..ca0d5f8ad54b 100644 --- a/drivers/gpu/nova-core/driver.rs +++ b/drivers/gpu/nova-core/driver.rs @@ -3,6 +3,7 @@ use kernel::{ auxiliary, c_str, device::Core, + devres::Devres, pci, pci::{Class, ClassMask, Vendor}, prelude::*, @@ -16,7 +17,8 @@ use crate::gpu::Gpu; pub(crate) struct NovaCore { #[pin] pub(crate) gpu: Gpu, - _reg: auxiliary::Registration, + #[pin] + _reg: Devres, } const BAR0_SIZE: usize = SZ_16M; @@ -65,12 +67,12 @@ impl pci::Driver for NovaCore { Ok(try_pin_init!(Self { gpu <- Gpu::new(pdev, bar.clone(), bar.access(pdev.as_ref())?), - _reg: auxiliary::Registration::new( + _reg <- auxiliary::Registration::new( pdev.as_ref(), c_str!("nova-drm"), 0, // TODO[XARR]: Once it lands, use XArray; for now we don't use the ID. crate::MODULE_NAME - )?, + ), })) }) } diff --git a/rust/kernel/auxiliary.rs b/rust/kernel/auxiliary.rs index e5bddb738d58..8c0a2472c26a 100644 --- a/rust/kernel/auxiliary.rs +++ b/rust/kernel/auxiliary.rs @@ -7,6 +7,7 @@ use crate::{ bindings, container_of, device, device_id::{RawDeviceId, RawDeviceIdIndex}, + devres::Devres, driver, error::{from_result, to_result, Result}, prelude::*, @@ -279,8 +280,8 @@ unsafe impl Sync for Device {} /// The registration of an auxiliary device. /// -/// This type represents the registration of a [`struct auxiliary_device`]. When an instance of this -/// type is dropped, its respective auxiliary device will be unregistered from the system. +/// This type represents the registration of a [`struct auxiliary_device`]. When its parent device +/// is unbound, the corresponding auxiliary device will be unregistered from the system. /// /// # Invariants /// @@ -290,44 +291,56 @@ pub struct Registration(NonNull); impl Registration { /// Create and register a new auxiliary device. - pub fn new(parent: &device::Device, name: &CStr, id: u32, modname: &CStr) -> Result { - let boxed = KBox::new(Opaque::::zeroed(), GFP_KERNEL)?; - let adev = boxed.get(); + pub fn new<'a>( + parent: &'a device::Device, + name: &'a CStr, + id: u32, + modname: &'a CStr, + ) -> impl PinInit, Error> + 'a { + pin_init::pin_init_scope(move || { + let boxed = KBox::new(Opaque::::zeroed(), GFP_KERNEL)?; + let adev = boxed.get(); + + // SAFETY: It's safe to set the fields of `struct auxiliary_device` on initialization. + unsafe { + (*adev).dev.parent = parent.as_raw(); + (*adev).dev.release = Some(Device::release); + (*adev).name = name.as_char_ptr(); + (*adev).id = id; + } - // SAFETY: It's safe to set the fields of `struct auxiliary_device` on initialization. - unsafe { - (*adev).dev.parent = parent.as_raw(); - (*adev).dev.release = Some(Device::release); - (*adev).name = name.as_char_ptr(); - (*adev).id = id; - } - - // SAFETY: `adev` is guaranteed to be a valid pointer to a `struct auxiliary_device`, - // which has not been initialized yet. - unsafe { bindings::auxiliary_device_init(adev) }; - - // Now that `adev` is initialized, leak the `Box`; the corresponding memory will be freed - // by `Device::release` when the last reference to the `struct auxiliary_device` is dropped. - let _ = KBox::into_raw(boxed); - - // SAFETY: - // - `adev` is guaranteed to be a valid pointer to a `struct auxiliary_device`, which has - // been initialialized, - // - `modname.as_char_ptr()` is a NULL terminated string. - let ret = unsafe { bindings::__auxiliary_device_add(adev, modname.as_char_ptr()) }; - if ret != 0 { // SAFETY: `adev` is guaranteed to be a valid pointer to a `struct auxiliary_device`, - // which has been initialialized. - unsafe { bindings::auxiliary_device_uninit(adev) }; - - return Err(Error::from_errno(ret)); - } - - // SAFETY: `adev` is guaranteed to be non-null, since the `KBox` was allocated successfully. - // - // INVARIANT: The device will remain registered until `auxiliary_device_delete()` is called, - // which happens in `Self::drop()`. - Ok(Self(unsafe { NonNull::new_unchecked(adev) })) + // which has not been initialized yet. + unsafe { bindings::auxiliary_device_init(adev) }; + + // Now that `adev` is initialized, leak the `Box`; the corresponding memory will be + // freed by `Device::release` when the last reference to the `struct auxiliary_device` + // is dropped. + let _ = KBox::into_raw(boxed); + + // SAFETY: + // - `adev` is guaranteed to be a valid pointer to a `struct auxiliary_device`, which + // has been initialized, + // - `modname.as_char_ptr()` is a NULL terminated string. + let ret = unsafe { bindings::__auxiliary_device_add(adev, modname.as_char_ptr()) }; + if ret != 0 { + // SAFETY: `adev` is guaranteed to be a valid pointer to a + // `struct auxiliary_device`, which has been initialized. + unsafe { bindings::auxiliary_device_uninit(adev) }; + + return Err(Error::from_errno(ret)); + } + + // SAFETY: `adev` is guaranteed to be non-null, since the `KBox` was allocated + // successfully. + // + // INVARIANT: The device will remain registered until `auxiliary_device_delete()` is + // called, which happens in `Self::drop()`. + Ok(Devres::new( + parent, + Self(unsafe { NonNull::new_unchecked(adev) }), + )) + }) } } diff --git a/samples/rust/rust_driver_auxiliary.rs b/samples/rust/rust_driver_auxiliary.rs index 2e9afeb83d4f..95c552ee9489 100644 --- a/samples/rust/rust_driver_auxiliary.rs +++ b/samples/rust/rust_driver_auxiliary.rs @@ -5,7 +5,8 @@ //! To make this driver probe, QEMU must be run with `-device pci-testdev`. use kernel::{ - auxiliary, c_str, device::Core, driver, error::Error, pci, prelude::*, InPlaceModule, + auxiliary, c_str, device::Core, devres::Devres, driver, error::Error, pci, prelude::*, + InPlaceModule, }; use pin_init::PinInit; @@ -40,8 +41,12 @@ impl auxiliary::Driver for AuxiliaryDriver { } } +#[pin_data] struct ParentDriver { - _reg: [auxiliary::Registration; 2], + #[pin] + _reg0: Devres, + #[pin] + _reg1: Devres, } kernel::pci_device_table!( @@ -57,11 +62,9 @@ impl pci::Driver for ParentDriver { const ID_TABLE: pci::IdTable = &PCI_TABLE; fn probe(pdev: &pci::Device, _info: &Self::IdInfo) -> impl PinInit { - Ok(Self { - _reg: [ - auxiliary::Registration::new(pdev.as_ref(), AUXILIARY_NAME, 0, MODULE_NAME)?, - auxiliary::Registration::new(pdev.as_ref(), AUXILIARY_NAME, 1, MODULE_NAME)?, - ], + try_pin_init!(Self { + _reg0 <- auxiliary::Registration::new(pdev.as_ref(), AUXILIARY_NAME, 0, MODULE_NAME), + _reg1 <- auxiliary::Registration::new(pdev.as_ref(), AUXILIARY_NAME, 1, MODULE_NAME), }) } } -- cgit v1.2.3 From 710ac546883c2cae6e8e7b5dcf7757b8a49d75a1 Mon Sep 17 00:00:00 2001 From: Danilo Krummrich Date: Tue, 21 Oct 2025 00:34:29 +0200 Subject: samples: rust: auxiliary: misc cleanup of ParentDriver::connect() In ParentDriver::connect() rename parent to dev, use it for the dev_info!() call, call pdev.vendor_() directly in the print statement and remove the unnecessary generic type of Result. Reviewed-by: Alice Ryhl Reviewed-by: Greg Kroah-Hartman Signed-off-by: Danilo Krummrich --- samples/rust/rust_driver_auxiliary.rs | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) (limited to 'samples/rust/rust_driver_auxiliary.rs') diff --git a/samples/rust/rust_driver_auxiliary.rs b/samples/rust/rust_driver_auxiliary.rs index 95c552ee9489..a5d67d4d9e83 100644 --- a/samples/rust/rust_driver_auxiliary.rs +++ b/samples/rust/rust_driver_auxiliary.rs @@ -70,16 +70,15 @@ impl pci::Driver for ParentDriver { } impl ParentDriver { - fn connect(adev: &auxiliary::Device) -> Result<()> { - let parent = adev.parent(); - let pdev: &pci::Device = parent.try_into()?; + fn connect(adev: &auxiliary::Device) -> Result { + let dev = adev.parent(); + let pdev: &pci::Device = dev.try_into()?; - let vendor = pdev.vendor_id(); dev_info!( - adev.as_ref(), + dev, "Connect auxiliary {} with parent: VendorID={}, DeviceID={:#x}\n", adev.id(), - vendor, + pdev.vendor_id(), pdev.device_id() ); -- cgit v1.2.3 From b0b7301b004301afe920b3d08caa6171dd3f4011 Mon Sep 17 00:00:00 2001 From: Danilo Krummrich Date: Tue, 21 Oct 2025 00:34:30 +0200 Subject: samples: rust: auxiliary: illustrate driver interaction Illustrate how a parent driver of an auxiliary driver can take advantage of the device context guarantees given by the auxiliary bus and subsequently safely derive its device private data. Reviewed-by: Alice Ryhl Reviewed-by: Greg Kroah-Hartman Signed-off-by: Danilo Krummrich --- samples/rust/rust_driver_auxiliary.rs | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) (limited to 'samples/rust/rust_driver_auxiliary.rs') diff --git a/samples/rust/rust_driver_auxiliary.rs b/samples/rust/rust_driver_auxiliary.rs index a5d67d4d9e83..5761ea314f44 100644 --- a/samples/rust/rust_driver_auxiliary.rs +++ b/samples/rust/rust_driver_auxiliary.rs @@ -5,10 +5,17 @@ //! To make this driver probe, QEMU must be run with `-device pci-testdev`. use kernel::{ - auxiliary, c_str, device::Core, devres::Devres, driver, error::Error, pci, prelude::*, + auxiliary, c_str, + device::{Bound, Core}, + devres::Devres, + driver, + error::Error, + pci, + prelude::*, InPlaceModule, }; +use core::any::TypeId; use pin_init::PinInit; const MODULE_NAME: &CStr = ::NAME; @@ -43,6 +50,7 @@ impl auxiliary::Driver for AuxiliaryDriver { #[pin_data] struct ParentDriver { + private: TypeId, #[pin] _reg0: Devres, #[pin] @@ -63,6 +71,7 @@ impl pci::Driver for ParentDriver { fn probe(pdev: &pci::Device, _info: &Self::IdInfo) -> impl PinInit { try_pin_init!(Self { + private: TypeId::of::(), _reg0 <- auxiliary::Registration::new(pdev.as_ref(), AUXILIARY_NAME, 0, MODULE_NAME), _reg1 <- auxiliary::Registration::new(pdev.as_ref(), AUXILIARY_NAME, 1, MODULE_NAME), }) @@ -70,9 +79,10 @@ impl pci::Driver for ParentDriver { } impl ParentDriver { - fn connect(adev: &auxiliary::Device) -> Result { + fn connect(adev: &auxiliary::Device) -> Result { let dev = adev.parent(); - let pdev: &pci::Device = dev.try_into()?; + let pdev: &pci::Device = dev.try_into()?; + let drvdata = dev.drvdata::()?; dev_info!( dev, @@ -82,6 +92,12 @@ impl ParentDriver { pdev.device_id() ); + dev_info!( + dev, + "We have access to the private data of {:?}.\n", + drvdata.private + ); + Ok(()) } } -- cgit v1.2.3