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 <aliceryhl@google.com>
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Danilo Krummrich <dakr@kernel.org>

diff --git a/drivers/gpu/nova-core/driver.rs b/drivers/gpu/nova-core/driver.rs
index a83b8619918214494a19efa51833c450accdc9c7..ca0d5f8ad54bb74e7b31d79525e3af64abebd7eb 100644 (file)
--- 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<auxiliary::Registration>,
 }
 
 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 e5bddb738d587fa32242cc7403ac99dede0586ac..8c0a2472c26adb6233fb144519cb0f5fddefc49f 100644 (file)
--- 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<bindings::auxiliary_device>);
 
 impl Registration {
     /// Create and register a new auxiliary device.
-    pub fn new(parent: &device::Device, name: &CStr, id: u32, modname: &CStr) -> Result<Self> {
-        let boxed = KBox::new(Opaque::<bindings::auxiliary_device>::zeroed(), GFP_KERNEL)?;
-        let adev = boxed.get();
+    pub fn new<'a>(
+        parent: &'a device::Device<device::Bound>,
+        name: &'a CStr,
+        id: u32,
+        modname: &'a CStr,
+    ) -> impl PinInit<Devres<Self>, Error> + 'a {
+        pin_init::pin_init_scope(move || {
+            let boxed = KBox::new(Opaque::<bindings::auxiliary_device>::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 2e9afeb83d4fa95ee7a783c2abb7ea6cc349c5ca..95c552ee948996c1633f21ee36c3123bc8dcb956 100644 (file)
--- 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<auxiliary::Registration>,
+    #[pin]
+    _reg1: Devres<auxiliary::Registration>,
 }
 
 kernel::pci_device_table!(
@@ -57,11 +62,9 @@ impl pci::Driver for ParentDriver {
     const ID_TABLE: pci::IdTable<Self::IdInfo> = &PCI_TABLE;
 
     fn probe(pdev: &pci::Device<Core>, _info: &Self::IdInfo) -> impl PinInit<Self, Error> {
-        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),
         })
     }
 }