Re: [PATCH] rust: serdev: Synchronize receive callback before calling unbind

[Markus Probst <markus.probst@posteo.de>]

[-- Attachment #1: Type: text/plain, Size: 7867 bytes --]

On Thu, 2026-09-03 at 22:03 +0000, Markus Probst wrote:
> The receive callback and unbind callback now have exclusive access to
> the drivers private data. Provide mutable references in callbacks to
> avoid the need for locks in the private data. Remove the Sync
> requirement.
> 
> Signed-off-by: Markus Probst <markus.probst@posteo.de>
> ---
> This patch avoids the need for a SpinLock in the patch series
> https://lore.kernel.org/rust-for-linux/20260827-gb-uart-transport-v2-7-a03bb1f5fbd1@beagleboard.org/
> .
> ---
>  rust/kernel/serdev.rs              | 51 ++++++++++++++++++++++----------------
>  samples/rust/rust_driver_serdev.rs |  2 +-
>  2 files changed, 31 insertions(+), 22 deletions(-)
> 
> diff --git a/rust/kernel/serdev.rs b/rust/kernel/serdev.rs
> index 17ca504b7f8d..44f029ed93fd 100644
> --- a/rust/kernel/serdev.rs
> +++ b/rust/kernel/serdev.rs
> @@ -106,7 +106,7 @@ pub struct PrivateData<'bound, T: Driver> {
>      /// Whether `receive_buf_callback` is allowed to call `Driver::receive`.
>      ///
>      /// If locked, the receive_buf_callback will be blocked on data reception.
> -    /// This is the case while the driver is being probed or while [`PrivateData`] is being dropped.
> +    /// This is the case while the driver is being probed or removed.
>      /// This is necessary, because we need to open the serdev device before the driver has been
>      /// probed in order to allow it to be configured, which allows `receive_buf_callback` to be
>      /// called. Thus we need to block data until probe completes and the driver data becomes
> @@ -127,16 +127,6 @@ pub struct PrivateData<'bound, T: Driver> {
>  #[pinned_drop]
>  impl<T: Driver> PinnedDrop for PrivateData<'_, T> {
>      fn drop(self: Pin<&mut Self>) {
> -        let mut active = self.active.lock();
> -        if *active {
> -            // SAFETY:
> -            // - We have exclusive access to `self.driver`.
> -            // - `self.driver` is guaranteed to be initialized.
> -            unsafe { (*self.driver.get()).assume_init_drop() };
> -            *active = false;
> -        }
> -        drop(active);
> -
>          // SAFETY: We have exclusive access to `self.open`.
>          if unsafe { *self.open.get() } {
>              // SAFETY: `self.sdev.as_raw()` is guaranteed to be a pointer to a valid
> @@ -176,7 +166,20 @@ extern "C" fn probe_callback(sdev: *mut bindings::serdev_device) -> kernel::ffi:
>              let private_data = unsafe { sdev.as_ref().drvdata_borrow::<PrivateData<'_, T>>() };
>              let private_data = ScopeGuard::new_with_data(private_data, |_| {
>                  // SAFETY: We just set drvdata to `PrivateData<'_, T>`.
> -                drop(unsafe { sdev.as_ref().drvdata_obtain::<PrivateData<'_, T>>() });
> +                let private_data = unsafe {
> +                    sdev.as_ref()
> +                        .drvdata_obtain::<PrivateData<'_, T>>()
> +                        .unwrap_unchecked()
> +                };
> +
> +                let mut active = private_data.active.lock();
> +                if *active {
> +                    // SAFETY:
> +                    // - We have exclusive access to `private_data.driver`.
> +                    // - `private_data.driver` is guaranteed to be initialized.
> +                    unsafe { (*private_data.driver.get()).assume_init_drop() };
> +                    *active = false;
> +                }
Ignore this hunk.

As Sashiko correctly noticed, this introduces dead code.

Thanks
- Markus Probst

>              });
>              let mut active = private_data.active.lock();
>  
> @@ -222,15 +225,21 @@ extern "C" fn remove_callback(sdev: *mut bindings::serdev_device) {
>          // and stored a `Pin<KBox<PrivateData<'_, T>>>`.
>          let private_data = unsafe { sdev.as_ref().drvdata_borrow::<PrivateData<'_, T>>() };
>  
> -        // SAFETY: No one has exclusive access to `private_data.driver`.
> -        let data = unsafe { &*private_data.driver.get() };
> +        let mut active = private_data.active.lock();
> +
> +        // SAFETY: We have exclusive access to `private_data.driver`.
> +        let data = unsafe { &mut *private_data.driver.get() };
>          // SAFETY:
>          // - `private_data.driver` is pinned.
>          // - `remove_callback` is only ever called after a successful call to `probe_callback`,
>          //   hence it's guaranteed that `private_data.driver` was initialized.
> -        let data_pinned = unsafe { Pin::new_unchecked(data.assume_init_ref()) };
> +        let data_pinned = unsafe { Pin::new_unchecked(data.assume_init_mut()) };
>  
>          T::unbind(sdev, data_pinned);
> +
> +        // SAFETY: We already established that `data` is guaranteed to be initialized.
> +        unsafe { data.assume_init_drop() };
> +        *active = false;
>      }
>  
>      extern "C" fn receive_buf_callback(
> @@ -254,13 +263,13 @@ extern "C" fn receive_buf_callback(
>              return length;
>          }
>  
> -        // SAFETY: No one has exclusive access to `private_data.driver`.
> -        let data = unsafe { &*private_data.driver.get() };
> +        // SAFETY: We have exclusive access to `private_data.driver`.
> +        let data = unsafe { &mut *private_data.driver.get() };
>          // SAFETY:
>          // - `private_data.driver` is pinned.
>          // - `receive_buf_callback` is only ever called after a successful call to `probe_callback`,
>          //   hence it's guaranteed that `private_data.driver` was initialized.
> -        let data_pinned = unsafe { Pin::new_unchecked(data.assume_init_ref()) };
> +        let data_pinned = unsafe { Pin::new_unchecked(data.assume_init_mut()) };
>  
>          // SAFETY: `buf` is guaranteed to be non-null and has the size of `length`.
>          let buf = unsafe { core::slice::from_raw_parts(buf, length) };
> @@ -365,7 +374,7 @@ pub trait Driver {
>      type IdInfo: 'static;
>  
>      /// The type of the driver's bus device private data.
> -    type Data<'bound>: Send + Sync + 'bound;
> +    type Data<'bound>: Send + 'bound;
>  
>      /// The table of OF device ids supported by the driver.
>      const OF_ID_TABLE: Option<of::IdTable<Self::IdInfo>> = None;
> @@ -391,7 +400,7 @@ fn probe<'bound>(
>      /// `&Device<Core>` or `&Device<Bound>` reference. For instance.
>      ///
>      /// Otherwise, release operations for driver resources should be performed in `Drop`.
> -    fn unbind<'bound>(sdev: &'bound Device<device::Core<'_>>, this: Pin<&Self::Data<'bound>>) {
> +    fn unbind<'bound>(sdev: &'bound Device<device::Core<'_>>, this: Pin<&mut Self::Data<'bound>>) {
>          let _ = (sdev, this);
>      }
>  
> @@ -402,7 +411,7 @@ fn unbind<'bound>(sdev: &'bound Device<device::Core<'_>>, this: Pin<&Self::Data<
>      /// Returns the number of bytes accepted.
>      fn receive<'bound>(
>          sdev: &'bound Device<device::Bound>,
> -        this: Pin<&Self::Data<'bound>>,
> +        this: Pin<&mut Self::Data<'bound>>,
>          data: &[u8],
>      ) -> usize {
>          let _ = (sdev, this, data);
> diff --git a/samples/rust/rust_driver_serdev.rs b/samples/rust/rust_driver_serdev.rs
> index 51b4898cd855..d00d547234c8 100644
> --- a/samples/rust/rust_driver_serdev.rs
> +++ b/samples/rust/rust_driver_serdev.rs
> @@ -63,7 +63,7 @@ fn probe<'bound>(
>  
>      fn receive<'bound>(
>          sdev: &'bound serdev::Device<Bound>,
> -        _this: Pin<&Self>,
> +        _this: Pin<&mut Self>,
>          data: &[u8],
>      ) -> usize {
>          sdev.write(data).unwrap_or_default() as usize
> 
> ---
> base-commit: e5e04726cdd043e309677071ab1b65a4b18f422b
> change-id: 20260903-rust_serdev_ref_mut-4d2285776ae1

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 870 bytes --]