[PATCH v5 0/4] rust: serdev: Refactor

[PATCH v5 0/4] rust: serdev: Refactor

[Markus Probst]

The following has been changed:
- introduce mutex to C serdev tty port controller
- 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

Signed-off-by: Markus Probst <markus.probst@posteo.de>
---
Changes in v5:
- fix issues reported by Sashiko:
  - fix access to serport->tty outside of locked mutex
- only lock receive_lock in ttyport_open for set_bit
- Link to v4: https://patch.msgid.link/20261016-rust_serdev_probe_refactor-v4-0-46832e034757@posteo.de

Changes in v4:
- fix issues reported by Sashiko:
  - add receive_lock to prevent deadlocks
- Link to v3: https://patch.msgid.link/20260930-rust_serdev_probe_refactor-v3-0-d1b6e77135e0@posteo.de

Changes in v3:
- add mutex
- Link to v2: https://patch.msgid.link/20260920-rust_serdev_probe_refactor-v2-0-43b855162f5d@posteo.de

Changes in v2:
- fix missing memory barriers in serdev apis
- use `tty_buffer_restart_work` instead of `tty_flip_buffer_push`
- use `tty_buffer_flush_work` instead of exclusively locking the mutex
- add `#[inline]` to `PrivateData::driver_data`
- remove patches for mutable references
- Link to v1: https://patch.msgid.link/20260906-rust_serdev_probe_refactor-v1-0-69cdae0074ec@posteo.de

---
Markus Probst (4):
      tty: serdev: Add mutex lock
      tty: serdev: Export functions to pause receive_buf callback calls
      rust: serdev: Replace `active` mutex with receive pause
      rust: serdev: Simplify callbacks

 drivers/tty/serdev/core.c           |  54 +++++++++++-
 drivers/tty/serdev/serdev-ttyport.c |  80 +++++++++++++++--
 include/linux/serdev.h              |   8 +-
 rust/kernel/serdev.rs               | 167 ++++++++++++------------------------
 4 files changed, 184 insertions(+), 125 deletions(-)
---
base-commit: 091b06388b117fefdcb63336ac09956a4914b961
change-id: 20260905-rust_serdev_probe_refactor-0e2b044354a7

[PATCH v5 1/4] tty: serdev: Add mutex lock

[Markus Probst]

Besides more predictable behaviour, this allows for several hardened
behaviour changes:

Return -EALREADY in `serdev_device_open` if the device is already open
instead of causing undefined behaviour.

Allow calling `serdev_device_close`, even if the device is already
closed instead of causing a null pointer dereference.

If the device is left open by the driver after remove, close it and warn
instead of leaving it in a invalid state.

Signed-off-by: Markus Probst <markus.probst@posteo.de>
---
 drivers/tty/serdev/core.c           | 10 ++++++---
 drivers/tty/serdev/serdev-ttyport.c | 44 +++++++++++++++++++++++++++++++------
 include/linux/serdev.h              |  2 +-
 3 files changed, 45 insertions(+), 11 deletions(-)

diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
index 7500efcdfc21..77e8e1d4d2a6 100644
--- a/drivers/tty/serdev/core.c
+++ b/drivers/tty/serdev/core.c
@@ -142,6 +142,11 @@ void serdev_device_remove(struct serdev_device *serdev)
 	struct serdev_controller *ctrl = serdev->ctrl;
 
 	device_unregister(&serdev->dev);
+
+	/* Warn if driver did not close the serial device. */
+	if (ctrl->ops->close && WARN_ON(ctrl->ops->close(ctrl)))
+		pm_runtime_put(&ctrl->dev);
+
 	ctrl->serdev = NULL;
 }
 EXPORT_SYMBOL_GPL(serdev_device_remove);
@@ -181,9 +186,8 @@ void serdev_device_close(struct serdev_device *serdev)
 	if (!ctrl || !ctrl->ops->close)
 		return;
 
-	pm_runtime_put(&ctrl->dev);
-
-	ctrl->ops->close(ctrl);
+	if (ctrl->ops->close(ctrl))
+		pm_runtime_put(&ctrl->dev);
 }
 EXPORT_SYMBOL_GPL(serdev_device_close);
 
diff --git a/drivers/tty/serdev/serdev-ttyport.c b/drivers/tty/serdev/serdev-ttyport.c
index bab1b143b8a6..7d2d50d7e1a4 100644
--- a/drivers/tty/serdev/serdev-ttyport.c
+++ b/drivers/tty/serdev/serdev-ttyport.c
@@ -16,6 +16,8 @@ struct serport {
 	struct tty_driver *tty_drv;
 	int tty_idx;
 	unsigned long flags;
+	struct mutex lock; /* lock preventing modification of flags */
+	struct mutex receive_lock; /* lock preventing execution of serdev_controller_receive_buf */
 };
 
 /*
@@ -29,6 +31,8 @@ static size_t ttyport_receive_buf(struct tty_port *port, const u8 *cp,
 	struct serport *serport = serdev_controller_get_drvdata(ctrl);
 	size_t ret;
 
+	guard(mutex)(&serport->receive_lock);
+
 	if (!test_bit(SERPORT_ACTIVE, &serport->flags))
 		return 0;
 
@@ -99,14 +103,23 @@ static int ttyport_open(struct serdev_controller *ctrl)
 	struct ktermios ktermios;
 	int ret;
 
+	mutex_lock(&serport->lock);
+
+	if (test_bit(SERPORT_ACTIVE, &serport->flags)) {
+		ret = -EALREADY;
+		goto err_flags_unlock;
+	}
+
 	tty = tty_init_dev(serport->tty_drv, serport->tty_idx);
-	if (IS_ERR(tty))
-		return PTR_ERR(tty);
+	if (IS_ERR(tty)) {
+		ret = PTR_ERR(tty);
+		goto err_flags_unlock;
+	}
 	serport->tty = tty;
 
 	if (!tty->ops->open || !tty->ops->close) {
 		ret = -ENODEV;
-		goto err_unlock;
+		goto err_tty_unlock;
 	}
 
 	ret = tty->ops->open(serport->tty, NULL);
@@ -128,25 +141,37 @@ static int ttyport_open(struct serdev_controller *ctrl)
 	ktermios.c_cflag |= CLOCAL;
 	tty_set_termios(tty, &ktermios);
 
+	mutex_lock(&serport->receive_lock);
 	set_bit(SERPORT_ACTIVE, &serport->flags);
+	mutex_unlock(&serport->receive_lock);
+
+	mutex_unlock(&serport->lock);
 
 	return 0;
 
 err_close:
 	tty->ops->close(tty, NULL);
-err_unlock:
+err_tty_unlock:
 	tty_unlock(tty);
 	tty_release_struct(tty, serport->tty_idx);
+err_flags_unlock:
+	mutex_unlock(&serport->lock);
 
 	return ret;
 }
 
-static void ttyport_close(struct serdev_controller *ctrl)
+static bool ttyport_close(struct serdev_controller *ctrl)
 {
 	struct serport *serport = serdev_controller_get_drvdata(ctrl);
-	struct tty_struct *tty = serport->tty;
+	struct tty_struct *tty;
+
+	guard(mutex)(&serport->lock);
 
-	clear_bit(SERPORT_ACTIVE, &serport->flags);
+	tty = serport->tty;
+
+	scoped_guard(mutex, &serport->receive_lock)
+		if (!__test_and_clear_bit(SERPORT_ACTIVE, &serport->flags))
+			return false;
 
 	tty_lock(tty);
 	if (tty->ops->close)
@@ -154,6 +179,8 @@ static void ttyport_close(struct serdev_controller *ctrl)
 	tty_unlock(tty);
 
 	tty_release_struct(tty, serport->tty_idx);
+
+	return true;
 }
 
 static unsigned int ttyport_set_baudrate(struct serdev_controller *ctrl, unsigned int speed)
@@ -288,6 +315,9 @@ struct device *serdev_tty_port_register(struct tty_port *port,
 	port->client_ops = &client_ops;
 	port->client_data = ctrl;
 
+	mutex_init(&serport->lock);
+	mutex_init(&serport->receive_lock);
+
 	ret = serdev_controller_add(ctrl);
 	if (ret)
 		goto err_reset_data;
diff --git a/include/linux/serdev.h b/include/linux/serdev.h
index b6c3d957ec15..0f4e81c0950d 100644
--- a/include/linux/serdev.h
+++ b/include/linux/serdev.h
@@ -81,7 +81,7 @@ struct serdev_controller_ops {
 	ssize_t (*write_buf)(struct serdev_controller *, const u8 *, size_t);
 	void (*write_flush)(struct serdev_controller *);
 	int (*open)(struct serdev_controller *);
-	void (*close)(struct serdev_controller *);
+	bool (*close)(struct serdev_controller *);
 	void (*set_flow_control)(struct serdev_controller *, bool);
 	int (*set_parity)(struct serdev_controller *, enum serdev_parity);
 	unsigned int (*set_baudrate)(struct serdev_controller *, unsigned int);

-- 
2.55.0

[PATCH v5 2/4] 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           | 44 ++++++++++++++++++++++++++++++++++++-
 drivers/tty/serdev/serdev-ttyport.c | 36 ++++++++++++++++++++++++++++++
 include/linux/serdev.h              |  6 +++++
 3 files changed, 85 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
index 77e8e1d4d2a6..af1470e33620 100644
--- a/drivers/tty/serdev/core.c
+++ b/drivers/tty/serdev/core.c
@@ -191,6 +191,45 @@ 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.
+ *
+ * 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.
+ *
+ * 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);
@@ -402,6 +441,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 |
@@ -409,7 +449,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 7d2d50d7e1a4..8fce7daa619f 100644
--- a/drivers/tty/serdev/serdev-ttyport.c
+++ b/drivers/tty/serdev/serdev-ttyport.c
@@ -7,8 +7,10 @@
 #include <linux/tty.h>
 #include <linux/tty_driver.h>
 #include <linux/poll.h>
+#include "../tty.h"
 
 #define SERPORT_ACTIVE		1
+#define SERPORT_PAUSE_RX	2
 
 struct serport {
 	struct tty_port *port;
@@ -36,6 +38,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,
@@ -183,6 +188,35 @@ static bool ttyport_close(struct serdev_controller *ctrl)
 	return true;
 }
 
+static void ttyport_pause_rx(struct serdev_controller *ctrl)
+{
+	struct serport *serport = serdev_controller_get_drvdata(ctrl);
+
+	guard(mutex)(&serport->lock);
+
+	scoped_guard(mutex, &serport->receive_lock)
+		set_bit(SERPORT_PAUSE_RX, &serport->flags);
+
+	if (test_bit(SERPORT_ACTIVE, &serport->flags))
+		tty_buffer_cancel_work(serport->tty->port);
+}
+
+static void ttyport_resume_rx(struct serdev_controller *ctrl)
+{
+	struct serport *serport = serdev_controller_get_drvdata(ctrl);
+
+	guard(mutex)(&serport->lock);
+
+	if (!test_bit(SERPORT_PAUSE_RX, &serport->flags))
+		return;
+
+	scoped_guard(mutex, &serport->receive_lock)
+		clear_bit(SERPORT_PAUSE_RX, &serport->flags);
+
+	if (test_bit(SERPORT_ACTIVE, &serport->flags))
+		tty_buffer_restart_work(serport->tty->port);
+}
+
 static unsigned int ttyport_set_baudrate(struct serdev_controller *ctrl, unsigned int speed)
 {
 	struct serport *serport = serdev_controller_get_drvdata(ctrl);
@@ -287,6 +321,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 0f4e81c0950d..02773554e759 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 v5 3/4] 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 v5 4/4] 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 | 131 ++++++++++++++++++++------------------------------
 1 file changed, 52 insertions(+), 79 deletions(-)

diff --git a/rust/kernel/serdev.rs b/rust/kernel/serdev.rs
index c16d6593a8d2..3f9165e16776 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,36 @@ 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> {
+    #[inline]
+    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 +142,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 +181,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 +192,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 +206,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

Re: [PATCH v5 2/4] tty: serdev: Export functions to pause receive_buf callback calls

[sashiko-bot]

> 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>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022-rust_serdev_probe_refactor-v5-0-a27d862a415a@posteo.de?part=2

Re: [PATCH v5 4/4] rust: serdev: Simplify callbacks

[sashiko-bot]

> Initialize the driver's private data directly on `PrivateData`.
> 
> Introduce `OpenGuard` for resource cleanup.
> 
> Signed-off-by: Markus Probst <markus.probst@posteo.de>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022-rust_serdev_probe_refactor-v5-0-a27d862a415a@posteo.de?part=4

Re: [PATCH v5 3/4] rust: serdev: Replace `active` mutex with receive pause

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] rust: serdev: mutable borrow of shared ref in remove_callback()
--

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

rust: serdev: Replace active mutex with receive pause

This commit replaces the active mutex in the Rust serdev abstraction with
serdev_device_pause_rx() and serdev_device_resume_rx() to fix race
conditions during probe failure and driver unbind.

> diff --git a/rust/kernel/serdev.rs b/rust/kernel/serdev.rs
> index 17ca504b7f8d6..c16d6593a8d2d 100644
> --- a/rust/kernel/serdev.rs
> +++ b/rust/kernel/serdev.rs
[ ... ]
> @@ -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() };
>      }

[Severity: Medium]
Does Adapter::remove_callback() in rust/kernel/serdev.rs fail to compile
when building this commit with CONFIG_RUST_SERIAL_DEV_BUS_ABSTRACTIONS=y?

Earlier in Adapter::remove_callback(), data is bound as an immutable
shared reference of type &MaybeUninit<T::Data<'_>>:

    let data = unsafe { &*private_data.driver.get() };
    let data_pinned = unsafe { Pin::new_unchecked(data.assume_init_ref()) };

    T::unbind(sdev, data_pinned);

Because MaybeUninit::assume_init_drop(&mut self) requires a mutable
reference, calling data.assume_init_drop() fails with rustc error E0596
(cannot borrow *data as mutable, as it is behind a & reference).

Since receive_buf_callback() can still run concurrently during T::unbind()
and create a shared reference to *private_data.driver.get() before
serdev_device_pause_rx() is called, data cannot be changed to a mutable
reference at the start of remove_callback().

Although a later commit in this series ("rust: serdev: Simplify callbacks")
refactors PrivateData to store T::Data directly and removes the
assume_init_drop() call, should this commit dereference the UnsafeCell
pointer directly after serdev_device_pause_rx() establishes exclusive
access:

    unsafe { (*private_data.driver.get()).assume_init_drop() };

so that git bisection and standalone backporting remain unbroken?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022-rust_serdev_probe_refactor-v5-0-a27d862a415a@posteo.de?part=3

Re: [PATCH v5 3/4] rust: serdev: Replace `active` mutex with receive pause

[Markus Probst]

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

On Thu, 2026-10-22 at 12:04 +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] rust: serdev: mutable borrow of shared ref in remove_callback()
> --
> 
> commit 7873e32e25475629867563ba84c28fd279d7f25d
> Author: Markus Probst <markus.probst@posteo.de>
> 
> rust: serdev: Replace active mutex with receive pause
> 
> This commit replaces the active mutex in the Rust serdev abstraction with
> serdev_device_pause_rx() and serdev_device_resume_rx() to fix race
> conditions during probe failure and driver unbind.
> 
> > diff --git a/rust/kernel/serdev.rs b/rust/kernel/serdev.rs
> > index 17ca504b7f8d6..c16d6593a8d2d 100644
> > --- a/rust/kernel/serdev.rs
> > +++ b/rust/kernel/serdev.rs
> [ ... ]
> > @@ -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() };
> >      }
> 
> [Severity: Medium]
> Does Adapter::remove_callback() in rust/kernel/serdev.rs fail to compile
> when building this commit with CONFIG_RUST_SERIAL_DEV_BUS_ABSTRACTIONS=y?
> 
> Earlier in Adapter::remove_callback(), data is bound as an immutable
> shared reference of type &MaybeUninit<T::Data<'_>>:
> 
>     let data = unsafe { &*private_data.driver.get() };
>     let data_pinned = unsafe { Pin::new_unchecked(data.assume_init_ref()) };
> 
>     T::unbind(sdev, data_pinned);
> 
> Because MaybeUninit::assume_init_drop(&mut self) requires a mutable
> reference, calling data.assume_init_drop() fails with rustc error E0596
> (cannot borrow *data as mutable, as it is behind a & reference).
> 
> Since receive_buf_callback() can still run concurrently during T::unbind()
> and create a shared reference to *private_data.driver.get() before
> serdev_device_pause_rx() is called, data cannot be changed to a mutable
> reference at the start of remove_callback().
> 
> Although a later commit in this series ("rust: serdev: Simplify callbacks")
> refactors PrivateData to store T::Data directly and removes the
> assume_init_drop() call, should this commit dereference the UnsafeCell
> pointer directly after serdev_device_pause_rx() establishes exclusive
> access:
> 
>     unsafe { (*private_data.driver.get()).assume_init_drop() };
> 
> so that git bisection and standalone backporting remain unbroken?
Thats true.

```
// SAFETY: We have exclusive access to `private_data.driver`.
let data = unsafe { &mut *private_data.driver.get() };
```
needs to be added after `serdev_device_pause_rx`.

Thanks
- Markus Probst


> 
> [ ... ]

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

Re: [PATCH v5 1/4] tty: serdev: Add mutex lock

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] tty: serdev: returning -EALREADY in ttyport_open() breaks hci_mrvl probe
- [High] tty: serdev: late close in serdev_device_remove() and missed driver unbind
--

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

tty: serdev: Add mutex lock

This commit adds mutexes to serialize serport state changes and receive
buffer handling in serdev-ttyport. It also rejects repeated opens with
-EALREADY, ignores redundant closes, and warns if a device is left open on
removal.

> diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
> index 7500efcdfc218..77e8e1d4d2a66 100644
> --- a/drivers/tty/serdev/core.c
> +++ b/drivers/tty/serdev/core.c
> @@ -142,6 +142,11 @@ void serdev_device_remove(struct serdev_device *serdev)
>  	struct serdev_controller *ctrl = serdev->ctrl;
>  
>  	device_unregister(&serdev->dev);
> +
> +	/* Warn if driver did not close the serial device. */
> +	if (ctrl->ops->close && WARN_ON(ctrl->ops->close(ctrl)))
> +		pm_runtime_put(&ctrl->dev);
> +
>  	ctrl->serdev = NULL;
>  }
>  EXPORT_SYMBOL_GPL(serdev_device_remove);

[Severity: High]
Is it safe to call ctrl->ops->close(ctrl) after device_unregister(&serdev->dev)
has already completed?

In serdev_device_remove(), device_unregister(&serdev->dev) calls device_del(),
which unbinds the driver via serdev_drv_remove() and runs
device_unbind_cleanup() (releasing devres resources and clearing drvdata),
followed by put_device(&serdev->dev), which invokes serdev_device_release()
and frees serdev:

drivers/tty/serdev/core.c:serdev_device_release() {
	struct serdev_device *serdev = to_serdev_device(dev);
	kfree(serdev);
}

When a driver leaves the port open across remove, SERPORT_ACTIVE remains set
and ctrl->serdev points to the freed serdev_device throughout
device_unbind_cleanup() and put_device() until ctrl->ops->close(ctrl) clears
SERPORT_ACTIVE. Could a concurrent ttyport_receive_buf() or
ttyport_write_wakeup() during this window dereference the freed ctrl->serdev
or invoke serdev->ops callbacks with NULL or freed drvdata?

Also, serdev_device_remove() is only called when unregistering the
serdev_device itself, not when unbinding a driver via serdev_drv_remove():

drivers/tty/serdev/core.c:serdev_drv_remove() {
	const struct serdev_device_driver *sdrv = to_serdev_device_driver(dev->driver);
	if (sdrv->remove)
		sdrv->remove(to_serdev_device(dev));
}

If a driver leaves the device open on unbind (or after a failed probe),
wouldn't SERPORT_ACTIVE stay set while drvdata is cleared, leaving the port
open and causing any subsequent re-probe to fail in serdev_device_open() with
-EALREADY?

[ ... ]

> diff --git a/drivers/tty/serdev/serdev-ttyport.c b/drivers/tty/serdev/serdev-ttyport.c
> index bab1b143b8a68..7d2d50d7e1a48 100644
> --- a/drivers/tty/serdev/serdev-ttyport.c
> +++ b/drivers/tty/serdev/serdev-ttyport.c

[ ... ]

> @@ -99,14 +103,23 @@ static int ttyport_open(struct serdev_controller *ctrl)
>  	struct ktermios ktermios;
>  	int ret;
>  
> +	mutex_lock(&serport->lock);
> +
> +	if (test_bit(SERPORT_ACTIVE, &serport->flags)) {
> +		ret = -EALREADY;
> +		goto err_flags_unlock;
> +	}

[Severity: High]
Won't returning -EALREADY here cause a regression when probing Marvell
Bluetooth serdev devices in drivers/bluetooth/hci_mrvl.c?

During mrvl_serdev_probe(), hci_uart_register_device() calls
hci_uart_register_device_priv(), which opens hu->serdev before invoking the
protocol open callback p->open(hu):

drivers/bluetooth/hci_serdev.c:hci_uart_register_device_priv() {
	...
	err = serdev_device_open(hu->serdev);
	if (err)
		goto err_rwsem;

	err = p->open(hu);
	if (err)
		goto err_open;
	...
}

In drivers/bluetooth/hci_mrvl.c, mrvl_open() then calls
serdev_device_open(hu->serdev) a second time:

drivers/bluetooth/hci_mrvl.c:mrvl_open() {
	...
	if (hu->serdev) {
		ret = serdev_device_open(hu->serdev);
		if (ret)
			goto err;
	}
	...
}

With ttyport_open() returning -EALREADY when SERPORT_ACTIVE is already set,
won't mrvl_open() fail unconditionally and abort mrvl_serdev_probe()?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022-rust_serdev_probe_refactor-v5-0-a27d862a415a@posteo.de?part=1