[PATCH 0/5] rust: serdev: Refactor

[PATCH 0/5] rust: serdev: Refactor

[Markus Probst]

The following has been changed:
- introduce a new C serdev API to fix race conditions and simplify rust
  code. Should also be useful on the C side.
- fix race conditions
- simplify rust code
- add rust function to provide mutable references to driver's private
  data
- provide mutable references in callbacks to avoid the need for a
  SpinLock in the greybus patch series [1]

[1]
https://lore.kernel.org/rust-for-linux/20260827-gb-uart-transport-v2-7-a03bb1f5fbd1@beagleboard.org/

Signed-off-by: Markus Probst <markus.probst@posteo.de>
---
Markus Probst (5):
      tty: serdev: Export functions to pause receive_buf callback calls
      rust: serdev: Replace `active` mutex with receive pause
      rust: serdev: Simplify callbacks
      rust: Add `Device::drvdata_borrow_mut`
      rust: serdev: Pause receive callback before calling unbind

 drivers/tty/serdev/core.c           |  50 +++++++++-
 drivers/tty/serdev/serdev-ttyport.c |  32 ++++++
 include/linux/serdev.h              |   6 ++
 rust/kernel/device.rs               |  24 +++++
 rust/kernel/serdev.rs               | 189 +++++++++++++-----------------------
 samples/rust/rust_driver_serdev.rs  |   2 +-
 6 files changed, 180 insertions(+), 123 deletions(-)
---
base-commit: e5e04726cdd043e309677071ab1b65a4b18f422b
change-id: 20260905-rust_serdev_probe_refactor-0e2b044354a7

[PATCH 1/5] tty: serdev: Export functions to pause receive_buf callback calls

[Markus Probst]

These functions will be used to simply the serdev rust abstraction. It
also contributes to the fixing of 2 race conditions in the serdev rust
abstraction.

Signed-off-by: Markus Probst <markus.probst@posteo.de>
---
 drivers/tty/serdev/core.c           | 50 ++++++++++++++++++++++++++++++++++++-
 drivers/tty/serdev/serdev-ttyport.c | 32 ++++++++++++++++++++++++
 include/linux/serdev.h              |  6 +++++
 3 files changed, 87 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
index 7500efcdfc21..7d24f16710cb 100644
--- a/drivers/tty/serdev/core.c
+++ b/drivers/tty/serdev/core.c
@@ -187,6 +187,51 @@ void serdev_device_close(struct serdev_device *serdev)
 }
 EXPORT_SYMBOL_GPL(serdev_device_close);
 
+/**
+ * serdev_device_pause_rx() - pause data receive
+ * @serdev:	serdev device
+ *
+ * Pause calls to receive_buf.
+ *
+ * The caller must guarantee that this does not run concurrently with
+ * `serdev_device_open` or `serdev_device_close`.
+ *
+ * Note that if a call to receive_buf is currently executed, the function will
+ * sleep until it has finished.
+ */
+void serdev_device_pause_rx(struct serdev_device *serdev)
+{
+	struct serdev_controller *ctrl = serdev->ctrl;
+
+	if (!ctrl || !ctrl->ops->pause_rx)
+		return;
+
+	ctrl->ops->pause_rx(ctrl);
+}
+EXPORT_SYMBOL_GPL(serdev_device_pause_rx);
+
+/**
+ * serdev_device_resume_rx() - resume data receive
+ * @serdev:	serdev device
+ *
+ * Resume calls to receive_buf.
+ *
+ * The caller must guarantee that this does not run concurrently with
+ * `serdev_device_open` or `serdev_device_close`.
+ *
+ * This can be called even if not paused to ensure data receive is active.
+ */
+void serdev_device_resume_rx(struct serdev_device *serdev)
+{
+	struct serdev_controller *ctrl = serdev->ctrl;
+
+	if (!ctrl || !ctrl->ops->resume_rx)
+		return;
+
+	ctrl->ops->resume_rx(ctrl);
+}
+EXPORT_SYMBOL_GPL(serdev_device_resume_rx);
+
 static void devm_serdev_device_close(void *serdev)
 {
 	serdev_device_close(serdev);
@@ -398,6 +443,7 @@ EXPORT_SYMBOL_GPL(serdev_device_break_ctl);
 static int serdev_drv_probe(struct device *dev)
 {
 	const struct serdev_device_driver *sdrv = to_serdev_device_driver(dev->driver);
+	struct serdev_device *sdev = to_serdev_device(dev);
 	int ret;
 
 	ret = dev_pm_domain_attach(dev, PD_FLAG_ATTACH_POWER_ON |
@@ -405,7 +451,9 @@ static int serdev_drv_probe(struct device *dev)
 	if (ret)
 		return ret;
 
-	return sdrv->probe(to_serdev_device(dev));
+	serdev_device_resume_rx(sdev);
+
+	return sdrv->probe(sdev);
 }
 
 static void serdev_drv_remove(struct device *dev)
diff --git a/drivers/tty/serdev/serdev-ttyport.c b/drivers/tty/serdev/serdev-ttyport.c
index bab1b143b8a6..85ab454c2f13 100644
--- a/drivers/tty/serdev/serdev-ttyport.c
+++ b/drivers/tty/serdev/serdev-ttyport.c
@@ -6,9 +6,11 @@
 #include <linux/serdev.h>
 #include <linux/tty.h>
 #include <linux/tty_driver.h>
+#include <linux/tty_flip.h>
 #include <linux/poll.h>
 
 #define SERPORT_ACTIVE		1
+#define SERPORT_PAUSE_RX	2
 
 struct serport {
 	struct tty_port *port;
@@ -32,6 +34,9 @@ static size_t ttyport_receive_buf(struct tty_port *port, const u8 *cp,
 	if (!test_bit(SERPORT_ACTIVE, &serport->flags))
 		return 0;
 
+	if (test_bit(SERPORT_PAUSE_RX, &serport->flags))
+		return 0;
+
 	ret = serdev_controller_receive_buf(ctrl, cp, count);
 
 	dev_WARN_ONCE(&ctrl->dev, ret > count,
@@ -156,6 +161,31 @@ static void ttyport_close(struct serdev_controller *ctrl)
 	tty_release_struct(tty, serport->tty_idx);
 }
 
+static void ttyport_pause_rx(struct serdev_controller *ctrl)
+{
+	struct serport *serport = serdev_controller_get_drvdata(ctrl);
+	struct tty_struct *tty = serport->tty;
+
+	if (test_bit(SERPORT_ACTIVE, &serport->flags))
+		tty_buffer_lock_exclusive(tty->port);
+
+	set_bit(SERPORT_PAUSE_RX, &serport->flags);
+
+	if (test_bit(SERPORT_ACTIVE, &serport->flags))
+		tty_buffer_unlock_exclusive(tty->port);
+}
+
+static void ttyport_resume_rx(struct serdev_controller *ctrl)
+{
+	struct serport *serport = serdev_controller_get_drvdata(ctrl);
+	struct tty_struct *tty = serport->tty;
+
+	clear_bit(SERPORT_PAUSE_RX, &serport->flags);
+
+	if (test_bit(SERPORT_ACTIVE, &serport->flags))
+		tty_flip_buffer_push(tty->port);
+}
+
 static unsigned int ttyport_set_baudrate(struct serdev_controller *ctrl, unsigned int speed)
 {
 	struct serport *serport = serdev_controller_get_drvdata(ctrl);
@@ -260,6 +290,8 @@ static const struct serdev_controller_ops ctrl_ops = {
 	.get_tiocm = ttyport_get_tiocm,
 	.set_tiocm = ttyport_set_tiocm,
 	.break_ctl = ttyport_break_ctl,
+	.pause_rx = ttyport_pause_rx,
+	.resume_rx = ttyport_resume_rx,
 };
 
 struct device *serdev_tty_port_register(struct tty_port *port,
diff --git a/include/linux/serdev.h b/include/linux/serdev.h
index b6c3d957ec15..5cf05df17ddf 100644
--- a/include/linux/serdev.h
+++ b/include/linux/serdev.h
@@ -89,6 +89,8 @@ struct serdev_controller_ops {
 	int (*get_tiocm)(struct serdev_controller *);
 	int (*set_tiocm)(struct serdev_controller *, unsigned int, unsigned int);
 	int (*break_ctl)(struct serdev_controller *ctrl, unsigned int break_state);
+	void (*pause_rx)(struct serdev_controller *ctrl);
+	void (*resume_rx)(struct serdev_controller *ctrl);
 };
 
 /**
@@ -194,6 +196,8 @@ static inline size_t serdev_controller_receive_buf(struct serdev_controller *ctr
 int serdev_device_open(struct serdev_device *);
 void serdev_device_close(struct serdev_device *);
 int devm_serdev_device_open(struct device *, struct serdev_device *);
+void serdev_device_pause_rx(struct serdev_device *serdev);
+void serdev_device_resume_rx(struct serdev_device *serdev);
 unsigned int serdev_device_set_baudrate(struct serdev_device *, unsigned int);
 void serdev_device_set_flow_control(struct serdev_device *, bool);
 int serdev_device_write_buf(struct serdev_device *, const u8 *, size_t);
@@ -233,6 +237,8 @@ static inline int serdev_device_open(struct serdev_device *sdev)
 	return -ENODEV;
 }
 static inline void serdev_device_close(struct serdev_device *sdev) {}
+static inline void serdev_device_pause_rx(struct serdev_device *serdev) {}
+static inline void serdev_device_resume_rx(struct serdev_device *serdev) {}
 static inline unsigned int serdev_device_set_baudrate(struct serdev_device *sdev, unsigned int baudrate)
 {
 	return 0;

-- 
2.55.0

[PATCH 2/5] rust: serdev: Replace `active` mutex with receive pause

[Markus Probst]

There are currently 2 race conditions:
- in probe if `Driver::probe` returns Err
- in unbind
. In those cases the driver data will be set to NULL before the serdev
device was closed. If data is received while the driver data is dropped,
the `receive_buf_callback` might try to access the `active` mutex on a
null pointer.

Removing the need for `receive_buf_callback` to lock the `active` mutex
fixes these.

Fixes: 99f59aa82341 ("rust: add basic serial device bus abstractions")
Reported-by: Sashiko Bot <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/linux-serial/20260905000836.C8FC91F00A3D@smtp.kernel.org/
Closes: https://lore.kernel.org/linux-serial/20260903222159.70A911F000E9@smtp.kernel.org/
Signed-off-by: Markus Probst <markus.probst@posteo.de>
---
 rust/kernel/serdev.rs | 60 ++++++++++++---------------------------------------
 1 file changed, 14 insertions(+), 46 deletions(-)

diff --git a/rust/kernel/serdev.rs b/rust/kernel/serdev.rs
index 17ca504b7f8d..c16d6593a8d2 100644
--- a/rust/kernel/serdev.rs
+++ b/rust/kernel/serdev.rs
@@ -13,13 +13,9 @@
         to_result,
         VTABLE_DEFAULT_ERROR, //
     },
-    new_mutex,
     of,
     prelude::*,
-    sync::{
-        aref::AlwaysRefCounted,
-        Mutex, //
-    },
+    sync::aref::AlwaysRefCounted,
     time::Jiffies,
     types::{
         Opaque,
@@ -103,40 +99,11 @@ pub struct PrivateData<'bound, T: Driver> {
     #[pin]
     driver: UnsafeCell<MaybeUninit<T::Data<'bound>>>,
     open: UnsafeCell<bool>,
-    /// 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 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
-    /// initialized.
-    ///
-    /// If unlocked and true, the receive_buf_callback will forward the data to
-    /// `Driver::receive`. This is the normal state of operation.
-    ///
-    /// If unlocked and false, the receive_buf_callback will throw away the data.
-    /// This is only the case, if the serdev device is open and
-    /// - the driver returned an error in probe
-    /// or
-    /// - the driver data already has been dropped, because it was unbound.
-    #[pin]
-    active: Mutex<bool>,
 }
 
 #[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
@@ -170,7 +137,6 @@ extern "C" fn probe_callback(sdev: *mut bindings::serdev_device) -> kernel::ffi:
                 sdev: &**sdev,
                 driver: MaybeUninit::<T::Data<'_>>::zeroed().into(),
                 open: false.into(),
-                active <- new_mutex!(false),
             }))?;
             // SAFETY: We just set drvdata to `PrivateData<'_, T>`.
             let private_data = unsafe { sdev.as_ref().drvdata_borrow::<PrivateData<'_, T>>() };
@@ -178,11 +144,12 @@ extern "C" fn probe_callback(sdev: *mut bindings::serdev_device) -> kernel::ffi:
                 // SAFETY: We just set drvdata to `PrivateData<'_, T>`.
                 drop(unsafe { sdev.as_ref().drvdata_obtain::<PrivateData<'_, T>>() });
             });
-            let mut active = private_data.active.lock();
-
             // SAFETY: `sdev.as_raw()` is guaranteed to be a valid pointer to `serdev_device`.
             unsafe { bindings::serdev_device_set_client_ops(sdev.as_raw(), Self::OPS) };
 
+            // SAFETY: `sdev.as_raw()` is guaranteed to be a valid pointer to `serdev_device`.
+            unsafe { bindings::serdev_device_pause_rx(sdev.as_raw()) };
+
             // SAFETY: The serial device bus only ever calls the probe callback with a valid pointer
             // to a `serdev_device`.
             to_result(unsafe { bindings::serdev_device_open(sdev.as_raw()) })?;
@@ -199,12 +166,12 @@ extern "C" fn probe_callback(sdev: *mut bindings::serdev_device) -> kernel::ffi:
             // - `private_data.driver` is pinned.
             let result = unsafe { pin_init::raw_try_init(driver.as_mut_ptr(), data) };
 
-            *active = result.is_ok();
-
-            drop(active);
-
             result.map(|()| {
                 private_data.dismiss();
+
+                // SAFETY: `sdev.as_raw()` is guaranteed to be a valid pointer to `serdev_device`.
+                unsafe { bindings::serdev_device_resume_rx(sdev.as_raw()) };
+
                 0
             })
         })
@@ -231,6 +198,12 @@ extern "C" fn remove_callback(sdev: *mut bindings::serdev_device) {
         let data_pinned = unsafe { Pin::new_unchecked(data.assume_init_ref()) };
 
         T::unbind(sdev, data_pinned);
+
+        // SAFETY: `sdev.as_raw()` is guaranteed to be a valid pointer to `serdev_device`.
+        unsafe { bindings::serdev_device_pause_rx(sdev.as_raw()) };
+
+        // SAFETY: We already established that `data` is guaranteed to be initialized.
+        unsafe { data.assume_init_drop() };
     }
 
     extern "C" fn receive_buf_callback(
@@ -248,11 +221,6 @@ extern "C" fn receive_buf_callback(
         // `probe_callback`, hence it's guaranteed that `Device::set_drvdata()` has been called
         // and stored a `Pin<KBox<PrivateData<'_, T>>>`.
         let private_data = unsafe { sdev.as_ref().drvdata_borrow::<PrivateData<'_, T>>() };
-        let active = private_data.active.lock();
-
-        if !*active {
-            return length;
-        }
 
         // SAFETY: No one has exclusive access to `private_data.driver`.
         let data = unsafe { &*private_data.driver.get() };

-- 
2.55.0

[PATCH 3/5] rust: serdev: Simplify callbacks

[Markus Probst]

Initialize the driver's private data directly on `PrivateData`.

Introduce `OpenGuard` for resource cleanup.

Signed-off-by: Markus Probst <markus.probst@posteo.de>
---
 rust/kernel/serdev.rs | 130 ++++++++++++++++++++------------------------------
 1 file changed, 51 insertions(+), 79 deletions(-)

diff --git a/rust/kernel/serdev.rs b/rust/kernel/serdev.rs
index c16d6593a8d2..66543108ec2f 100644
--- a/rust/kernel/serdev.rs
+++ b/rust/kernel/serdev.rs
@@ -17,16 +17,12 @@
     prelude::*,
     sync::aref::AlwaysRefCounted,
     time::Jiffies,
-    types::{
-        Opaque,
-        ScopeGuard, //
-    }, //
+    types::Opaque, //
 };
 
 use core::{
-    cell::UnsafeCell,
     marker::PhantomData,
-    mem::{offset_of, MaybeUninit},
+    mem::offset_of,
     ptr::NonNull, //
 };
 
@@ -92,24 +88,35 @@ unsafe fn unregister(sdrv: &Opaque<Self::DriverType>) {
     }
 }
 
+struct OpenGuard<'bound> {
+    sdev: &'bound Device<device::Bound>,
+}
+
+impl Drop for OpenGuard<'_> {
+    fn drop(&mut self) {
+        // SAFETY:
+        // - `self.sdev.as_raw()` is guaranteed to be a pointer to a valid
+        //   `struct serdev_device`.
+        // - The existence of self proves that the device is open.
+        unsafe { bindings::serdev_device_close(self.sdev.as_raw()) };
+    }
+}
+
 #[doc(hidden)]
-#[pin_data(PinnedDrop)]
+#[pin_data]
 pub struct PrivateData<'bound, T: Driver> {
-    sdev: &'bound Device<device::Bound>,
     #[pin]
-    driver: UnsafeCell<MaybeUninit<T::Data<'bound>>>,
-    open: UnsafeCell<bool>,
+    driver: T::Data<'bound>,
+    open: OpenGuard<'bound>,
 }
 
-#[pinned_drop]
-impl<T: Driver> PinnedDrop for PrivateData<'_, T> {
-    fn drop(self: Pin<&mut Self>) {
-        // 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
-            // `struct serdev_device`.
-            unsafe { bindings::serdev_device_close(self.sdev.as_raw()) };
-        }
+impl<'bound, T: Driver> PrivateData<'bound, T> {
+    fn driver_data(self: Pin<&Self>) -> Pin<&T::Data<'bound>> {
+        // SAFETY: We treat the result as pinned.
+        let inner = unsafe { Pin::into_inner_unchecked(self) };
+
+        // SAFETY: `self.driver` is pinned.
+        unsafe { Pin::new_unchecked(&inner.driver) }
     }
 }
 
@@ -134,46 +141,30 @@ extern "C" fn probe_callback(sdev: *mut bindings::serdev_device) -> kernel::ffi:
 
         from_result(|| {
             sdev.as_ref().set_drvdata(try_pin_init!(PrivateData::<T> {
-                sdev: &**sdev,
-                driver: MaybeUninit::<T::Data<'_>>::zeroed().into(),
-                open: false.into(),
+                open: {
+                    // SAFETY:
+                    // - `sdev.as_raw()` is guaranteed to be a valid pointer to
+                    //   `serdev_device`.
+                    // - It is safe to call before open.
+                    unsafe { bindings::serdev_device_set_client_ops(sdev.as_raw(), Self::OPS) };
+
+                    // SAFETY: `sdev.as_raw()` is guaranteed to be a valid pointer to
+                    // `serdev_device`.
+                    unsafe { bindings::serdev_device_pause_rx(sdev.as_raw()) };
+
+                    // SAFETY: `sdev.as_raw()` is guaranteed to be a valid pointer to
+                    // `serdev_device`.
+                    to_result(unsafe { bindings::serdev_device_open(sdev.as_raw()) })?;
+
+                    OpenGuard { sdev }
+                },
+                driver <- T::probe(sdev, info),
             }))?;
-            // SAFETY: We just set drvdata to `PrivateData<'_, T>`.
-            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>>() });
-            });
-            // SAFETY: `sdev.as_raw()` is guaranteed to be a valid pointer to `serdev_device`.
-            unsafe { bindings::serdev_device_set_client_ops(sdev.as_raw(), Self::OPS) };
 
             // SAFETY: `sdev.as_raw()` is guaranteed to be a valid pointer to `serdev_device`.
-            unsafe { bindings::serdev_device_pause_rx(sdev.as_raw()) };
-
-            // SAFETY: The serial device bus only ever calls the probe callback with a valid pointer
-            // to a `serdev_device`.
-            to_result(unsafe { bindings::serdev_device_open(sdev.as_raw()) })?;
-
-            // SAFETY: We have exclusive access to `private_data.open`.
-            unsafe { *private_data.open.get() = true };
-
-            let data = T::probe(sdev, info);
+            unsafe { bindings::serdev_device_resume_rx(sdev.as_raw()) };
 
-            // SAFETY: We have exclusive access to `private_data.driver`.
-            let driver = unsafe { &mut *private_data.driver.get() };
-            // SAFETY:
-            // - `driver.as_mut_ptr()` is a valid pointer to uninitialized data.
-            // - `private_data.driver` is pinned.
-            let result = unsafe { pin_init::raw_try_init(driver.as_mut_ptr(), data) };
-
-            result.map(|()| {
-                private_data.dismiss();
-
-                // SAFETY: `sdev.as_raw()` is guaranteed to be a valid pointer to `serdev_device`.
-                unsafe { bindings::serdev_device_resume_rx(sdev.as_raw()) };
-
-                0
-            })
+            Ok(0)
         })
     }
 
@@ -189,21 +180,10 @@ 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() };
-        // 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()) };
-
-        T::unbind(sdev, data_pinned);
+        T::unbind(sdev, private_data.driver_data());
 
         // SAFETY: `sdev.as_raw()` is guaranteed to be a valid pointer to `serdev_device`.
         unsafe { bindings::serdev_device_pause_rx(sdev.as_raw()) };
-
-        // SAFETY: We already established that `data` is guaranteed to be initialized.
-        unsafe { data.assume_init_drop() };
     }
 
     extern "C" fn receive_buf_callback(
@@ -211,6 +191,9 @@ extern "C" fn receive_buf_callback(
         buf: *const u8,
         length: usize,
     ) -> usize {
+        // SAFETY: `buf` is guaranteed to be non-null and has the size of `length`.
+        let buf = unsafe { core::slice::from_raw_parts(buf, length) };
+
         // SAFETY: The serial device bus only ever calls the receive buf callback with a valid
         // pointer to a `struct serdev_device`.
         //
@@ -222,18 +205,7 @@ extern "C" fn receive_buf_callback(
         // 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() };
-        // 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()) };
-
-        // SAFETY: `buf` is guaranteed to be non-null and has the size of `length`.
-        let buf = unsafe { core::slice::from_raw_parts(buf, length) };
-
-        T::receive(sdev, data_pinned, buf)
+        T::receive(sdev, private_data.driver_data(), buf)
     }
 }
 

-- 
2.55.0

[PATCH 4/5] rust: Add `Device::drvdata_borrow_mut`

[Markus Probst]

This function allows the caller to obtain a mutable reference to the
driver's private data if he has exclusive access.

This will be used in serdev to provide mutable references in callbacks.

Signed-off-by: Markus Probst <markus.probst@posteo.de>
---
 rust/kernel/device.rs | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
index 2291d85b6849..aa2d87c6f4d3 100644
--- a/rust/kernel/device.rs
+++ b/rust/kernel/device.rs
@@ -258,6 +258,30 @@ pub unsafe fn drvdata_borrow<T>(&self) -> Pin<&T> {
         //   in `into_foreign()`.
         unsafe { Pin::<KBox<T>>::borrow(ptr.cast()) }
     }
+
+    /// Borrow the driver's private data bound to this [`Device`] mutable.
+    ///
+    /// # Safety
+    ///
+    /// - Must only be called after a preceding call to [`Device::set_drvdata`] and before the
+    ///   device is fully unbound.
+    /// - The type `T` must match the type of the `ForeignOwnable` previously stored by
+    ///   [`Device::set_drvdata`].
+    /// - The caller must have exclusive access to `T`.
+    #[expect(clippy::mut_from_ref)]
+    pub unsafe fn drvdata_borrow_mut<T>(&self) -> Pin<&mut T> {
+        // SAFETY: By the type invariants, `self.as_raw()` is a valid pointer to a `struct device`.
+        let ptr = unsafe { bindings::dev_get_drvdata(self.as_raw()) };
+
+        // SAFETY:
+        // - By the safety requirements of this function, `ptr` comes from a previous call to
+        //   `into_foreign()`.
+        // - `dev_get_drvdata()` guarantees to return the same pointer given to `dev_set_drvdata()`
+        //   in `into_foreign()`.
+        // - By the safety requirements of this function, `borrow` and `borrow_mut` do not overlap
+        //   on the same object.
+        unsafe { Pin::<KBox<T>>::borrow_mut(ptr.cast()) }
+    }
 }
 
 impl<Ctx: DeviceContext> Device<Ctx> {

-- 
2.55.0

[PATCH 5/5] rust: serdev: Pause receive callback before calling unbind

[Markus Probst]

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>
---
 rust/kernel/serdev.rs              | 39 ++++++++++++++++++++++----------------
 samples/rust/rust_driver_serdev.rs |  2 +-
 2 files changed, 24 insertions(+), 17 deletions(-)

diff --git a/rust/kernel/serdev.rs b/rust/kernel/serdev.rs
index 66543108ec2f..7d47d91e3bc3 100644
--- a/rust/kernel/serdev.rs
+++ b/rust/kernel/serdev.rs
@@ -111,12 +111,12 @@ pub struct PrivateData<'bound, T: Driver> {
 }
 
 impl<'bound, T: Driver> PrivateData<'bound, T> {
-    fn driver_data(self: Pin<&Self>) -> Pin<&T::Data<'bound>> {
+    fn driver_data(self: Pin<&mut Self>) -> Pin<&mut T::Data<'bound>> {
         // SAFETY: We treat the result as pinned.
         let inner = unsafe { Pin::into_inner_unchecked(self) };
 
         // SAFETY: `self.driver` is pinned.
-        unsafe { Pin::new_unchecked(&inner.driver) }
+        unsafe { Pin::new_unchecked(&mut inner.driver) }
     }
 }
 
@@ -175,15 +175,18 @@ extern "C" fn remove_callback(sdev: *mut bindings::serdev_device) {
         // INVARIANT: `sdev` is valid for the duration of `remove_callback()`.
         let sdev = unsafe { &*sdev.cast::<Device<device::CoreInternal<'_>>>() };
 
-        // SAFETY: `remove_callback` is only ever called after a successful call to
-        // `probe_callback`, hence it's guaranteed that `Device::set_drvdata()` has been called
-        // and stored a `Pin<KBox<PrivateData<'_, T>>>`.
-        let private_data = unsafe { sdev.as_ref().drvdata_borrow::<PrivateData<'_, T>>() };
-
-        T::unbind(sdev, private_data.driver_data());
-
         // SAFETY: `sdev.as_raw()` is guaranteed to be a valid pointer to `serdev_device`.
         unsafe { bindings::serdev_device_pause_rx(sdev.as_raw()) };
+
+        // SAFETY:
+        // - `remove_callback` is only ever called after a successful call to `probe_callback`,
+        //   hence it's guaranteed that `Device::set_drvdata()` has been called and stored a
+        //   `Pin<KBox<PrivateData<'_, T>>>`.
+        // - The call to `serdev_device_pause_rx` above guarantees that we do not overlap with
+        //   `receive_buf_callback`, thus it is guaranteed that we have exclusive access.
+        let private_data = unsafe { sdev.as_ref().drvdata_borrow_mut::<PrivateData<'_, T>>() };
+
+        T::unbind(sdev, private_data.driver_data());
     }
 
     extern "C" fn receive_buf_callback(
@@ -200,10 +203,14 @@ extern "C" fn receive_buf_callback(
         // INVARIANT: `sdev` is valid for the duration of `receive_buf_callback()`.
         let sdev = unsafe { &*sdev.cast::<Device<device::BoundInternal>>() };
 
-        // SAFETY: `receive_buf_callback` is only ever called after a successful call to
-        // `probe_callback`, hence it's guaranteed that `Device::set_drvdata()` has been called
-        // and stored a `Pin<KBox<PrivateData<'_, T>>>`.
-        let private_data = unsafe { sdev.as_ref().drvdata_borrow::<PrivateData<'_, T>>() };
+        // SAFETY:
+        // - `receive_buf_callback` is only ever called after a successful call to `probe_callback`,
+        //   hence it's guaranteed that `Device::set_drvdata()` has been called and stored a
+        //   `Pin<KBox<PrivateData<'_, T>>>`.
+        // - `unbind_callback` calls `serdev_device_pause_rx` before accessing the driver data,
+        //   which guarantees that this function will not overlap with it. Thus we have exclusive
+        //   access.
+        let private_data = unsafe { sdev.as_ref().drvdata_borrow_mut::<PrivateData<'_, T>>() };
 
         T::receive(sdev, private_data.driver_data(), buf)
     }
@@ -305,7 +312,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;
@@ -331,7 +338,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);
     }
 
@@ -342,7 +349,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

-- 
2.55.0

Re: [PATCH 1/5] tty: serdev: Export functions to pause receive_buf callback calls

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] ttyport_resume_rx calls the producer API tty_flip_buffer_push from the consumer side, racing with active producers and exposing uninitialized memory.
--

commit bcc044ef1e478c63e3d04ef2d46ec10ebc5b78ee
Author: Markus Probst <markus.probst@posteo.de>

tty: serdev: Export functions to pause receive_buf callback calls
    
These functions will be used to simply the serdev rust abstraction. It
also contributes to the fixing of 2 race conditions in the serdev rust
abstraction.

> diff --git a/drivers/tty/serdev/serdev-ttyport.c b/drivers/tty/serdev/serdev-ttyport.c
> index bab1b143b8a68..85ab454c2f13a 100644
> --- a/drivers/tty/serdev/serdev-ttyport.c
> +++ b/drivers/tty/serdev/serdev-ttyport.c
[ ... ]
> @@ -156,6 +161,31 @@ static void ttyport_close(struct serdev_controller *ctrl)
>  	tty_release_struct(tty, serport->tty_idx);
>  }
>  
> +static void ttyport_pause_rx(struct serdev_controller *ctrl)
> +{
[ ... ]
> +}
> +
> +static void ttyport_resume_rx(struct serdev_controller *ctrl)
> +{
> +	struct serport *serport = serdev_controller_get_drvdata(ctrl);
> +	struct tty_struct *tty = serport->tty;
> +
> +	clear_bit(SERPORT_PAUSE_RX, &serport->flags);
> +
> +	if (test_bit(SERPORT_ACTIVE, &serport->flags))
> +		tty_flip_buffer_push(tty->port);

[Severity: High]
Does calling tty_flip_buffer_push() from the consumer context create a data
race on the tail->commit index?

Because tty_flip_buffer_push() commits unwritten buffer space (moving
tail->used to tail->commit), this could cause the consumer worker to read
uninitialized memory and pass garbage data to the receive_buf callback.

For example, if the serdev consumer calls serdev_device_resume_rx(), and
concurrently a UART hardware IRQ receives data and calls
tty_prepare_flip_string() (which increments tb->used).

If this call to tty_flip_buffer_push() executes before the UART driver writes
data to the buffer, it commits the pre-incremented tb->used.

Is there a way for the consumer to wake up the worker without committing
unwritten data, rather than invoking producer APIs directly?

> +}
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260906-rust_serdev_probe_refactor-v1-0-69cdae0074ec@posteo.de?part=1

Re: [PATCH 4/5] rust: Add `Device::drvdata_borrow_mut`

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] Incomplete safety contract for `drvdata_borrow` fails to forbid overlapping with `drvdata_borrow_mut`, leading to an API soundness hole.
- [Low] Missing `#[inline]` attribute on the abstraction function `drvdata_borrow_mut` which forwards to a C binding.
- [Low] Grammatical error in the documentation comment for `drvdata_borrow_mut`.

Pre-existing issues:
- [High] Potential NULL pointer dereference in `receive_buf_callback` due to reliance on an optional C callback (`pause_rx`) to suppress data reception during probe.
--

commit 0556399b0fd78424186d5ad317732507484df8d2
Author: Markus Probst <markus.probst@posteo.de>

rust: Add Device::drvdata_borrow_mut

This patch adds Device::drvdata_borrow_mut to allow callers with exclusive
access to obtain a mutable reference to the driver's private data. This
functionality is intended for use in serdev to provide mutable references
in callbacks.

> diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
> index 2291d85b6849e..aa2d87c6f4d3e 100644
> --- a/rust/kernel/device.rs
> +++ b/rust/kernel/device.rs
> @@ -258,6 +258,30 @@ pub unsafe fn drvdata_borrow<T>(&self) -> Pin<&T> {
>          //   in `into_foreign()`.
>          unsafe { Pin::<KBox<T>>::borrow(ptr.cast()) }
>      }

[Severity: High]
With the introduction of the mutable variant below, should drvdata_borrow
have an explicit safety contract requiring callers to ensure no overlapping
mutable references exist? Since drvdata_borrow_mut is being added, the
missing contract on drvdata_borrow could lead to aliasing mutable and
immutable references.

> +
> +    /// Borrow the driver's private data bound to this [Device] mutable.

[Severity: Low]
This isn't a bug, but there is a minor grammatical error. Should "mutable"
be "mutably" to properly modify the verb "Borrow"?

> +    ///
> +    /// # Safety
> +    ///
> +    /// - Must only be called after a preceding call to [Device::set_drvdata] and before the
> +    ///   device is fully unbound.
> +    /// - The type T must match the type of the ForeignOwnable previously stored by
> +    ///   [Device::set_drvdata].
> +    /// - The caller must have exclusive access to T.
> +    #[expect(clippy::mut_from_ref)]
> +    pub unsafe fn drvdata_borrow_mut<T>(&self) -> Pin<&mut T> {

[Severity: Low]
Should this abstraction function include the #[inline] attribute? According
to the Rust subsystem guidelines, small functions in abstractions that forward
to C bindings should be annotated with #[inline].

> +        // SAFETY: By the type invariants, `self.as_raw()` is a valid pointer to a `struct device`.
> +        let ptr = unsafe { bindings::dev_get_drvdata(self.as_raw()) };
> +
> +        // SAFETY:
> +        // - By the safety requirements of this function, `ptr` comes from a previous call to
> +        //   `into_foreign()`.
> +        // - `dev_get_drvdata()` guarantees to return the same pointer given to `dev_set_drvdata()`
> +        //   in `into_foreign()`.
> +        // - By the safety requirements of this function, `borrow` and `borrow_mut` do not overlap
> +        //   on the same object.
> +        unsafe { Pin::<KBox<T>>::borrow_mut(ptr.cast()) }
> +    }

[Severity: High]
This is a pre-existing issue, but I noticed a potential problem in how
serdev handles data reception during probe, which might be relevant
since this patch is intended for serdev callbacks.

Can receive_buf_callback dereference a NULL pointer if a serdev controller 
does not implement pause_rx?

In rust/kernel/serdev.rs:probe_callback(), the code opens the device and 
relies on pause_rx to suppress data:

    unsafe { bindings::serdev_device_pause_rx(sdev.as_raw()) };
    to_result(unsafe { bindings::serdev_device_open(sdev.as_raw()) })?;

However, the C API treats pause_rx as optional and fails silently:

drivers/tty/serdev/core.c:serdev_device_pause_rx() {
    if (!ctrl || !ctrl->ops->pause_rx)
        return;
    ...
}

If a controller missing pause_rx is probed, serdev_device_open allows 
data to be received. If data arrives asynchronously before probe_callback 
finishes and calls set_drvdata, wouldn't receive_buf_callback unconditionally 
borrow a NULL pointer here:

    let private_data = unsafe { sdev.as_ref().drvdata_borrow::<PrivateData<'_, T>>() };

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260906-rust_serdev_probe_refactor-v1-0-69cdae0074ec@posteo.de?part=4

Re: [PATCH 2/5] rust: serdev: Replace `active` mutex with receive pause

[sashiko-bot]

> There are currently 2 race conditions:
> - in probe if `Driver::probe` returns Err
> - in unbind
> . In those cases the driver data will be set to NULL before the serdev
> device was closed. If data is received while the driver data is dropped,
> the `receive_buf_callback` might try to access the `active` mutex on a
> null pointer.
> 
> Removing the need for `receive_buf_callback` to lock the `active` mutex
> fixes these.
> 
> Fixes: 99f59aa82341 ("rust: add basic serial device bus abstractions")
> Reported-by: Sashiko Bot <sashiko-bot@kernel.org>
> Closes: https://lore.kernel.org/linux-serial/20260905000836.C8FC91F00A3D@smtp.kernel.org/
> Closes: https://lore.kernel.org/linux-serial/20260903222159.70A911F000E9@smtp.kernel.org/
> Signed-off-by: Markus Probst <markus.probst@posteo.de>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260906-rust_serdev_probe_refactor-v1-0-69cdae0074ec@posteo.de?part=2

Re: [PATCH 5/5] rust: serdev: Pause receive callback before calling unbind

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Missing memory barrier between setting drvdata and resuming RX allows concurrent NULL pointer dereference.
--

commit dde2ba583b3f14a65bfcd445c3ea93289575651c
Author: Markus Probst <markus.probst@posteo.de>

rust: serdev: Pause receive callback before calling unbind
    
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.

> diff --git a/rust/kernel/serdev.rs b/rust/kernel/serdev.rs
> index 66543108ec2fa..7d47d91e3bc35 100644
> --- a/rust/kernel/serdev.rs
> +++ b/rust/kernel/serdev.rs
[ ... ]
> @@ -200,10 +203,14 @@ extern "C" fn receive_buf_callback(
>          // INVARIANT: `sdev` is valid for the duration of `receive_buf_callback()`.
>          let sdev = unsafe { &*sdev.cast::<Device<device::BoundInternal>>() };
>  
> -        // SAFETY: `receive_buf_callback` is only ever called after a successful call to
> -        // `probe_callback`, hence it's guaranteed that `Device::set_drvdata()` has been called
> -        // and stored a `Pin<KBox<PrivateData<'_, T>>>`.
> -        let private_data = unsafe { sdev.as_ref().drvdata_borrow::<PrivateData<'_, T>>() };
> +        // SAFETY:
> +        // - `receive_buf_callback` is only ever called after a successful call to `probe_callback`,
> +        //   hence it's guaranteed that `Device::set_drvdata()` has been called and stored a
> +        //   `Pin<KBox<PrivateData<'_, T>>>`.
> +        // - `unbind_callback` calls `serdev_device_pause_rx` before accessing the driver data,
> +        //   which guarantees that this function will not overlap with it. Thus we have exclusive
> +        //   access.
> +        let private_data = unsafe { sdev.as_ref().drvdata_borrow_mut::<PrivateData<'_, T>>() };

[Severity: Critical]
This is a pre-existing issue, but does this code allow a concurrent NULL pointer
dereference?

In probe_callback() in rust/kernel/serdev.rs, set_drvdata() writes the
driver data pointer to memory, followed by serdev_device_resume_rx() which
clears the SERPORT_PAUSE_RX flag using a relaxed clear_bit() operation:

    unsafe { bindings::serdev_device_resume_rx(sdev.as_raw()) };

On weakly ordered architectures like ARM64, the CPU can reorder the plain data
store to occur after the flag is cleared. A concurrent tty receive path
observing the cleared flag will enter receive_buf_callback() and read
dev->driver_data before the store propagates, obtaining a NULL pointer.

Calling drvdata_borrow_mut() on this NULL pointer creates a reference to NULL,
triggering undefined behavior and a kernel panic. Can this be triggered
externally if the serial port receives data from an attached hardware device
during the narrow window of probe_callback()?

>  
>          T::receive(sdev, private_data.driver_data(), buf)
>      }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260906-rust_serdev_probe_refactor-v1-0-69cdae0074ec@posteo.de?part=5

Re: [PATCH 3/5] rust: serdev: Simplify callbacks

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] TOCTOU race condition causing a NULL pointer dereference in `receive_buf_callback` during device probe.
- [Low] Missing `#[inline]` annotations on abstraction functions `PrivateData::driver_data` and `OpenGuard::drop`.
--

commit 4244bd311fa3c091fa7a05ad05f18ce4e08829f6
Author: Markus Probst <markus.probst@posteo.de>
Subject: rust: serdev: Simplify callbacks

This commit simplifies serdev callbacks by initializing private data
directly on PrivateData. It also introduces OpenGuard for resource cleanup.

> diff --git a/rust/kernel/serdev.rs b/rust/kernel/serdev.rs
> index c16d6593a8d2d..66543108ec2fa 100644
> --- a/rust/kernel/serdev.rs
> +++ b/rust/kernel/serdev.rs
[ ... ]
> @@ -92,24 +88,35 @@ unsafe fn unregister(sdrv: &Opaque<Self::DriverType>) {
>      }
>  }
>  
> +struct OpenGuard<'bound> {
> +    sdev: &'bound Device<device::Bound>,
> +}
> +
> +impl Drop for OpenGuard<'_> {
> +    fn drop(&mut self) {
> +        // SAFETY:
> +        // - `self.sdev.as_raw()` is guaranteed to be a pointer to a valid
> +        //   `struct serdev_device`.
> +        // - The existence of self proves that the device is open.
> +        unsafe { bindings::serdev_device_close(self.sdev.as_raw()) };
> +    }
> +}

[Severity: Low]
Does this drop implementation need an #[inline] annotation?

The Rust subsystem guidelines state that in abstractions like the kernel crate,
small functions or functions forwarding to a C binding call should be annotated
with #[inline].

> +
>  #[doc(hidden)]
> -#[pin_data(PinnedDrop)]
> +#[pin_data]
>  pub struct PrivateData<'bound, T: Driver> {
> -    sdev: &'bound Device<device::Bound>,
>      #[pin]
> -    driver: UnsafeCell<MaybeUninit<T::Data<'bound>>>,
> -    open: UnsafeCell<bool>,
> +    driver: T::Data<'bound>,
> +    open: OpenGuard<'bound>,
>  }
>  
> -#[pinned_drop]
> -impl<T: Driver> PinnedDrop for PrivateData<'_, T> {
> -    fn drop(self: Pin<&mut Self>) {
> -        // 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
> -            // `struct serdev_device`.
> -            unsafe { bindings::serdev_device_close(self.sdev.as_raw()) };
> -        }
> +impl<'bound, T: Driver> PrivateData<'bound, T> {
> +    fn driver_data(self: Pin<&Self>) -> Pin<&T::Data<'bound>> {
> +        // SAFETY: We treat the result as pinned.
> +        let inner = unsafe { Pin::into_inner_unchecked(self) };
> +
> +        // SAFETY: `self.driver` is pinned.
> +        unsafe { Pin::new_unchecked(&inner.driver) }
>      }
>  }

[Severity: Low]
Does this helper function also need an #[inline] annotation?

Similar to OpenGuard::drop above, this appears to be a small projection
function in an abstraction crate, which the subsystem guidelines suggest
annotating with #[inline].

> @@ -134,46 +141,30 @@ extern "C" fn probe_callback(sdev: *mut bindings::serdev_device) -> kernel::ffi:
>  
>          from_result(|| {
>              sdev.as_ref().set_drvdata(try_pin_init!(PrivateData::<T> {
> -                sdev: &**sdev,
> -                driver: MaybeUninit::<T::Data<'_>>::zeroed().into(),
> -                open: false.into(),
> +                open: {
> +                    // SAFETY:
> +                    // - `sdev.as_raw()` is guaranteed to be a valid pointer to
> +                    //   `serdev_device`.
> +                    // - It is safe to call before open.
> +                    unsafe { bindings::serdev_device_set_client_ops(sdev.as_raw(), Self::OPS) };
> +
> +                    // SAFETY: `sdev.as_raw()` is guaranteed to be a valid pointer to
> +                    // `serdev_device`.
> +                    unsafe { bindings::serdev_device_pause_rx(sdev.as_raw()) };
> +
> +                    // SAFETY: `sdev.as_raw()` is guaranteed to be a valid pointer to
> +                    // `serdev_device`.
> +                    to_result(unsafe { bindings::serdev_device_open(sdev.as_raw()) })?;
> +
> +                    OpenGuard { sdev }
> +                },
> +                driver <- T::probe(sdev, info),
>              }))?;

[Severity: High]
Could this sequence introduce a race condition that leads to a NULL pointer
dereference in receive_buf_callback() during device probe?

Because bindings::serdev_device_open() is called inside try_pin_init!(), the
device is exposed to hardware interrupts before the initialized struct is
passed to set_drvdata(). This means dev->driver_data remains NULL during this
window.

While bindings::serdev_device_pause_rx() is called before open, and
bindings::serdev_device_resume_rx() is called after set_drvdata() below,
resume_rx() uses clear_bit(SERPORT_PAUSE_RX) which lacks a memory barrier.

On weakly-ordered CPU architectures, can the CPU reorder the store to
dev->driver_data to happen after the atomic clear?

If a concurrent RX worker thread observes SERPORT_PAUSE_RX == 0 while
dev->driver_data is still NULL, it could trigger receive_buf_callback() and
crash when dereferencing the NULL pointer.

> -            // SAFETY: We just set drvdata to `PrivateData<'_, T>`.
> -            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>>() });
> -            });
> -            // SAFETY: `sdev.as_raw()` is guaranteed to be a valid pointer to `serdev_device`.
> -            unsafe { bindings::serdev_device_set_client_ops(sdev.as_raw(), Self::OPS) };
>  
>              // SAFETY: `sdev.as_raw()` is guaranteed to be a valid pointer to `serdev_device`.
> -            unsafe { bindings::serdev_device_pause_rx(sdev.as_raw()) };
> -
> -            // SAFETY: The serial device bus only ever calls the probe callback with a valid pointer
> -            // to a `serdev_device`.
> -            to_result(unsafe { bindings::serdev_device_open(sdev.as_raw()) })?;
> -
> -            // SAFETY: We have exclusive access to `private_data.open`.
> -            unsafe { *private_data.open.get() = true };
> -
> -            let data = T::probe(sdev, info);
> +            unsafe { bindings::serdev_device_resume_rx(sdev.as_raw()) };
>  
> -            // SAFETY: We have exclusive access to `private_data.driver`.
> -            let driver = unsafe { &mut *private_data.driver.get() };
> -            // SAFETY:
> -            // - `driver.as_mut_ptr()` is a valid pointer to uninitialized data.
> -            // - `private_data.driver` is pinned.
> -            let result = unsafe { pin_init::raw_try_init(driver.as_mut_ptr(), data) };
> -
> -            result.map(|()| {
> -                private_data.dismiss();
> -
> -                // SAFETY: `sdev.as_raw()` is guaranteed to be a valid pointer to `serdev_device`.
> -                unsafe { bindings::serdev_device_resume_rx(sdev.as_raw()) };
> -
> -                0
> -            })
> +            Ok(0)
>          })
>      }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260906-rust_serdev_probe_refactor-v1-0-69cdae0074ec@posteo.de?part=3

Re: [PATCH 5/5] rust: serdev: Pause receive callback before calling unbind

[Danilo Krummrich]

On Sun Sep 6, 2026 at 5:55 PM CEST, Markus Probst wrote:
> @@ -200,10 +203,14 @@ extern "C" fn receive_buf_callback(
>          // INVARIANT: `sdev` is valid for the duration of `receive_buf_callback()`.
>          let sdev = unsafe { &*sdev.cast::<Device<device::BoundInternal>>() };
>  
> -        // SAFETY: `receive_buf_callback` is only ever called after a successful call to
> -        // `probe_callback`, hence it's guaranteed that `Device::set_drvdata()` has been called
> -        // and stored a `Pin<KBox<PrivateData<'_, T>>>`.
> -        let private_data = unsafe { sdev.as_ref().drvdata_borrow::<PrivateData<'_, T>>() };
> +        // SAFETY:
> +        // - `receive_buf_callback` is only ever called after a successful call to `probe_callback`,
> +        //   hence it's guaranteed that `Device::set_drvdata()` has been called and stored a
> +        //   `Pin<KBox<PrivateData<'_, T>>>`.
> +        // - `unbind_callback` calls `serdev_device_pause_rx` before accessing the driver data,
> +        //   which guarantees that this function will not overlap with it. Thus we have exclusive
> +        //   access.
> +        let private_data = unsafe { sdev.as_ref().drvdata_borrow_mut::<PrivateData<'_, T>>() };

This would break the driver core's lifetime design. Any kind of registration
(such as class device, auxiliary, IRQ, etc.) may borrow fields from the bus
device private data. The whole design is based on the guarantee that we never
construct a mutable reference of the bus device private data.

Re: [PATCH 5/5] rust: serdev: Pause receive callback before calling unbind

[Markus Probst]

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

On Sun, 2026-09-06 at 18:20 +0200, Danilo Krummrich wrote:
> On Sun Sep 6, 2026 at 5:55 PM CEST, Markus Probst wrote:
> > @@ -200,10 +203,14 @@ extern "C" fn receive_buf_callback(
> >          // INVARIANT: `sdev` is valid for the duration of `receive_buf_callback()`.
> >          let sdev = unsafe { &*sdev.cast::<Device<device::BoundInternal>>() };
> >  
> > -        // SAFETY: `receive_buf_callback` is only ever called after a successful call to
> > -        // `probe_callback`, hence it's guaranteed that `Device::set_drvdata()` has been called
> > -        // and stored a `Pin<KBox<PrivateData<'_, T>>>`.
> > -        let private_data = unsafe { sdev.as_ref().drvdata_borrow::<PrivateData<'_, T>>() };
> > +        // SAFETY:
> > +        // - `receive_buf_callback` is only ever called after a successful call to `probe_callback`,
> > +        //   hence it's guaranteed that `Device::set_drvdata()` has been called and stored a
> > +        //   `Pin<KBox<PrivateData<'_, T>>>`.
> > +        // - `unbind_callback` calls `serdev_device_pause_rx` before accessing the driver data,
> > +        //   which guarantees that this function will not overlap with it. Thus we have exclusive
> > +        //   access.
> > +        let private_data = unsafe { sdev.as_ref().drvdata_borrow_mut::<PrivateData<'_, T>>() };
> 
> This would break the driver core's lifetime design. Any kind of registration
> (such as class device, auxiliary, IRQ, etc.) may borrow fields from the bus
> device private data. The whole design is based on the guarantee that we never
> construct a mutable reference of the bus device private data.
Thanks for the info.

That explains why unbind doesn't provide a mutable reference on
platform drivers either.

I will drop the 2 patches in the next revision.

Thanks
- Markus Probst

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

Re: [PATCH 5/5] rust: serdev: Pause receive callback before calling unbind

[Gary Guo]

On Sun Sep 6, 2026 at 5:20 PM BST, Danilo Krummrich wrote:
> On Sun Sep 6, 2026 at 5:55 PM CEST, Markus Probst wrote:
>> @@ -200,10 +203,14 @@ extern "C" fn receive_buf_callback(
>>          // INVARIANT: `sdev` is valid for the duration of `receive_buf_callback()`.
>>          let sdev = unsafe { &*sdev.cast::<Device<device::BoundInternal>>() };
>>  
>> -        // SAFETY: `receive_buf_callback` is only ever called after a successful call to
>> -        // `probe_callback`, hence it's guaranteed that `Device::set_drvdata()` has been called
>> -        // and stored a `Pin<KBox<PrivateData<'_, T>>>`.
>> -        let private_data = unsafe { sdev.as_ref().drvdata_borrow::<PrivateData<'_, T>>() };
>> +        // SAFETY:
>> +        // - `receive_buf_callback` is only ever called after a successful call to `probe_callback`,
>> +        //   hence it's guaranteed that `Device::set_drvdata()` has been called and stored a
>> +        //   `Pin<KBox<PrivateData<'_, T>>>`.
>> +        // - `unbind_callback` calls `serdev_device_pause_rx` before accessing the driver data,
>> +        //   which guarantees that this function will not overlap with it. Thus we have exclusive
>> +        //   access.
>> +        let private_data = unsafe { sdev.as_ref().drvdata_borrow_mut::<PrivateData<'_, T>>() };
>
> This would break the driver core's lifetime design. Any kind of registration
> (such as class device, auxiliary, IRQ, etc.) may borrow fields from the bus
> device private data. The whole design is based on the guarantee that we never
> construct a mutable reference of the bus device private data.

Mutable references should be fine (of course, provided that the bus actually
serialize callbacks).

It's only problematic now because in absence of pin-init self-reference, the
immutable borrow is the only mechanism that prevent user from having multiple
mutable borrow of the data fields.

Say this code:

    struct MyDeviceData<'a> {
        foo: Resource<'a>,
        bar: Resource<'foo>,
        baz: Resource<'bar>,
    }

pin-init would make `foo` and `bar` be only visible immutably in the projection,
even from `Pin<&mut MyDeviceData<'_>>`, so the design is still sound.

Best,
Gary

Re: [PATCH 5/5] rust: serdev: Pause receive callback before calling unbind

[Markus Probst]

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

On Sun, 2026-09-06 at 21:13 +0100, Gary Guo wrote:
> On Sun Sep 6, 2026 at 5:20 PM BST, Danilo Krummrich wrote:
> > On Sun Sep 6, 2026 at 5:55 PM CEST, Markus Probst wrote:
> > > @@ -200,10 +203,14 @@ extern "C" fn receive_buf_callback(
> > >          // INVARIANT: `sdev` is valid for the duration of `receive_buf_callback()`.
> > >          let sdev = unsafe { &*sdev.cast::<Device<device::BoundInternal>>() };
> > >  
> > > -        // SAFETY: `receive_buf_callback` is only ever called after a successful call to
> > > -        // `probe_callback`, hence it's guaranteed that `Device::set_drvdata()` has been called
> > > -        // and stored a `Pin<KBox<PrivateData<'_, T>>>`.
> > > -        let private_data = unsafe { sdev.as_ref().drvdata_borrow::<PrivateData<'_, T>>() };
> > > +        // SAFETY:
> > > +        // - `receive_buf_callback` is only ever called after a successful call to `probe_callback`,
> > > +        //   hence it's guaranteed that `Device::set_drvdata()` has been called and stored a
> > > +        //   `Pin<KBox<PrivateData<'_, T>>>`.
> > > +        // - `unbind_callback` calls `serdev_device_pause_rx` before accessing the driver data,
> > > +        //   which guarantees that this function will not overlap with it. Thus we have exclusive
> > > +        //   access.
> > > +        let private_data = unsafe { sdev.as_ref().drvdata_borrow_mut::<PrivateData<'_, T>>() };
> > 
> > This would break the driver core's lifetime design. Any kind of registration
> > (such as class device, auxiliary, IRQ, etc.) may borrow fields from the bus
> > device private data. The whole design is based on the guarantee that we never
> > construct a mutable reference of the bus device private data.
> 
> Mutable references should be fine (of course, provided that the bus actually
> serialize callbacks).
The calls do not overlap.

> 
> It's only problematic now because in absence of pin-init self-reference, the
> immutable borrow is the only mechanism that prevent user from having multiple
> mutable borrow of the data fields.
Why is having multiple mutable borrows, assuming they are from
different fields, problematic?

> 
> Say this code:
> 
>     struct MyDeviceData<'a> {
>         foo: Resource<'a>,
>         bar: Resource<'foo>,
>         baz: Resource<'bar>,
>     }
> 
> pin-init would make `foo` and `bar` be only visible immutably in the projection,
> even from `Pin<&mut MyDeviceData<'_>>`, so the design is still sound.
So its only problematic, if the driver data is not pinned?

Thanks
- Markus Probst

> 
> Best,
> Gary

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