Claude review: samples: rust: add Rust serial device bus sample device driver

Claude review: samples: rust: add Rust serial device bus sample device driver

[Claude Code Review Bot]

Patch Review

The sample is clean and demonstrates the abstraction well. A few notes:

**`set_baudrate` error handling is odd due to the API design:**

```rust
if sdev
    .set_baudrate(
        dev.fwnode()
            .and_then(|fwnode| fwnode.property_read(c"baudrate").optional())
            .unwrap_or(115200),
    )
    .is_err()
{
    return Err(EINVAL);
}
```

This discards the actual baudrate returned in the error. If `set_baudrate` returned `u32` instead of `Result<(), u32>`, this would be simpler and more informative.

**Echo server with `Timeout::Max` would hang:** Due to the `Timeout::Max` bug (returning 0), `write_all` in receive would return immediately or with ETIMEDOUT rather than actually waiting. This makes the sample non-functional in practice.

**The sample stores an `ARef<serdev::Device>` but never uses it** (except in Drop for a debug message). This is fine for a sample but worth noting.

---
Generated by Claude Code Patch Reviewer

Claude review: samples: rust: add Rust serial device bus sample device driver

[Claude Code Review Bot]

Patch Review

The sample is clean and demonstrates the API well.

**Missing dependency in Kconfig:**

```
+config SAMPLE_RUST_DRIVER_SERDEV
+	tristate "Serial Device Bus Device Driver"
+	depends on SERIAL_DEV_BUS
```

This should also `depends on RUST_SERIAL_DEV_BUS_ABSTRACTIONS` (or `select` it), since the sample uses `kernel::serdev` which is gated behind that config option.

**The sample stores an `ARef<serdev::Device>` but never uses it functionally:**

```rust
+struct SampleDriver {
+    sdev: ARef<serdev::Device>,
+}
```

It's only used in `Drop` for a debug message. This is fine for a sample, but the reference counting should be correct — `sdev.into()` in `probe` calls `impl_device_context_into_aref` which should increment the refcount.

**Minor: `"GPL v2"` vs `"GPL"`:**

```rust
+    license: "GPL v2",
```

Modern kernel modules typically use just `"GPL"`. `"GPL v2"` is also accepted but `"GPL"` is the recommended form.

---
Generated by Claude Code Patch Reviewer

Claude review: samples: rust: add Rust serial device bus sample device driver

[Claude Code Review Bot]

Patch Review

A clean sample that demonstrates probe, receive (echo/loopback), and cleanup. A few observations:

**Error in receive is swallowed**:

```rust
+    fn receive(sdev: &serdev::Device<Bound>, _this: Pin<&Self>, data: &[u8]) -> usize {
+        let _ = sdev.write_all(data, serdev::Timeout::Max);
+        data.len()
+    }
```

The `write_all` error is discarded with `let _ = ...`. For a sample this is acceptable, but a comment like `// Ignore write errors for simplicity` would be helpful.

**Strict baudrate matching**:

```rust
+        if sdev
+            .set_baudrate(...)
+            .is_err()
+        {
+            return Err(EINVAL);
+        }
```

This fails probe if the exact baudrate isn't matched. As noted above, many serial controllers approximate the baudrate. For a sample this is fine, but it could be confusing if someone tries to run this on real hardware.

**License string**: `"GPL v2"` is used, which is the correct form for `MODULE_LICENSE`.

**Overall the sample is clean and well-structured**, demonstrating the key abstractions (OF/ACPI ID tables, probe, receive, device reference management via `ARef`, Drop-based cleanup).

---
Generated by Claude Code Patch Reviewer

Claude review: samples: rust: add Rust serial device bus sample device driver

[Claude Code Review Bot]

Patch Review

Clean and functional sample driver. A few observations:

**Echoing data in receive callback:**
```rust
+    fn receive(sdev: &serdev::Device<Bound>, _this: Pin<&Self>, data: &[u8]) -> usize {
+        let _ = sdev.write_all(data, serdev::Timeout::Max);
+        data.len()
+    }
```
The `write_all` with `Timeout::Max` could block indefinitely if the device can't accept data. For a sample this is acceptable, but a brief comment noting this is intentional would be helpful. Also, `let _ =` silently discards write errors, which is fine for a sample.

**Baudrate handling:**
```rust
+        if sdev
+            .set_baudrate(
+                dev.fwnode()
+                    .and_then(|fwnode| fwnode.property_read(c"baudrate").optional())
+                    .unwrap_or(115200),
+            )
+            .is_err()
+        {
+            return Err(EINVAL);
+        }
```
Reading a `baudrate` property from firmware and falling back to 115200 is a reasonable pattern. The `set_baudrate` error (which contains the actual baudrate set) is discarded and replaced with `EINVAL`, which is appropriate.

**Kconfig entry properly uses `select`:**
```
+	select RUST_SERIAL_DEV_BUS_ABSTRACTIONS
```
This ensures the abstraction is compiled when the sample is enabled. Correct.

The sample effectively demonstrates OF and ACPI device table setup, probe/remove lifecycle, baudrate/flow-control/parity configuration, and the receive callback -- good coverage for a sample driver.

---
Generated by Claude Code Patch Reviewer

Claude review: samples: rust: add Rust serial device bus sample device driver

[Claude Code Review Bot]

Patch Review

The sample is clean and demonstrates the key API surface well.

**One issue with the sample probe:**
```rust
fn probe(
    sdev: &serdev::Device<Core>,
    _info: Option<&Self::IdInfo>,
) -> impl PinInit<Self, Error> {
    ...
    if sdev.set_baudrate(...).is_err() {
        return Err(EINVAL);
    }
```
The `set_baudrate` method is on `Device<Bound>`, but `sdev` is `&Device<Core>`. This works via the deref coercion chain, which is correct but could be surprising to readers. A brief comment in the abstraction's module-level docs explaining the device context hierarchy and auto-deref would help (though this is a broader Rust-for-Linux documentation issue, not specific to this patch).

**The receive callback echoes data back:**
```rust
fn receive(sdev: &serdev::Device<Bound>, _this: Pin<&Self>, data: &[u8]) -> usize {
    let _ = sdev.write_all(data, serdev::Timeout::Max);
    data.len()
}
```
This is fine for a sample. The `let _ =` discards the write result, which is acceptable for demonstration purposes. Note that this always returns `data.len()` (claiming all data was consumed) regardless of whether `write_all` succeeded, which is the right behavior for a simple echo.

**The `SampleDriver` stores an `ARef<serdev::Device>` but only uses it in `Drop`** for a debug message. This is fine as a sample demonstrating how to keep a reference to the device.

No blocking issues with this patch.

---
Generated by Claude Code Patch Reviewer

Claude review: samples: rust: add Rust serial device bus sample device driver

[Claude Code Review Bot]

Patch Review

Clean, well-structured sample driver. It demonstrates:
- OF and ACPI device table registration
- Probe with optional baudrate property from firmware
- Receive callback that echoes data back
- Drop for cleanup logging

```rust
+    fn receive(sdev: &serdev::Device<Bound>, _this: Pin<&Self>, data: &[u8]) -> usize {
+        let _ = sdev.write_all(data, serdev::Timeout::Max);
+        data.len()
+    }
```

The `let _ =` silently discards write errors during echo, which is fine for a sample. The function always returns `data.len()` claiming all bytes were consumed regardless of write success — acceptable for a demo.

**Minor:** The Kconfig entry doesn't have a `depends on SAMPLES` guard, but that's consistent with other sample drivers in the same file.

No significant issues with this patch.

---
Generated by Claude Code Patch Reviewer

[PATCH v11 0/3] rust: add basic serial device bus abstractions

[Markus Probst via B4 Relay]

This patch series adds the serdev device bus rust abstraction into the
kernel.

This abstraction will be used by a driver,
which targets the MCU devices in Synology devices.

Kari Argillander also messaged me, stating that he wants to write a
watchdog driver with this abstraction (needing initial device data).

This series depends on [1] and [2].

[1]
https://lore.kernel.org/rust-for-linux/20260525202921.124698-1-dakr@kernel.org/
[2] https://lore.kernel.org/rust-for-linux/20260530132736.3298549-1-dakr@kernel.org/

Signed-off-by: Markus Probst <markus.probst@posteo.de>
---
Changes in v11:
- redo changes from v9 with the following fixes:
  - fix memory leak on probe failure
  - fix possible access of dropped drvdata from receive_buf
- Link to v10: https://patch.msgid.link/20260530-rust_serdev-v10-0-65d1d5db876c@posteo.de

Changes in v10:
- revert everything from v9, except BoundInternal
- use mutex to ensure receive_buf won't be called on a dropped drvdata.
- Link to v9: https://patch.msgid.link/20260530-rust_serdev-v9-0-f8b5fccb49c3@posteo.de

Changes in v9:
- make use of BoundInternal
- use PrivateData wrapper and drop rust_private_data field
- use non-devm version of serdev_device_open
- Link to v8: https://patch.msgid.link/20260530-rust_serdev-v8-0-2a95f1da22a7@posteo.de

Changes in v8:
- adapted to driver-lifetime v5 patch series
- add MAINTAINERS file patch
- Link to v7: https://patch.msgid.link/20260429-rust_serdev-v7-0-0d89c791b5c8@posteo.de

Changes in v7:
- adapted to driver-lifetime patch series
- Link to v6: https://patch.msgid.link/20260427-rust_serdev-v6-0-173798d5e1a3@posteo.de

Changes in v6:
- rebased onto v7.1-rc1
- Link to v5: https://patch.msgid.link/20260420-rust_serdev-v5-0-57e8ba0519f3@posteo.de

Changes in v5:
- fix typo in documentation
- Link to v4: https://lore.kernel.org/r/20260411-rust_serdev-v4-0-845e960c6627@posteo.de

Changes in v4:
- fixed not selecting rust serdev abstraction in sample
- Link to v3: https://lore.kernel.org/r/20260313-rust_serdev-v3-0-c9a3af214f7f@posteo.de

Changes in v3:
- fix vertical import style
- add Kconfig entry for the rust abstraction
- fix documentation in include/linux/serdev.h
- rename private_data to rust_private_data
- fix `complete_all` <-> `wait_for_completion` typo
- move drvdata_borrow call after the completion
- Link to v2: https://lore.kernel.org/r/20260306-rust_serdev-v2-0-e9b23b42b255@posteo.de

Changes in v2:
- fix documentation in `serdev::Driver::write` and
  `serdev::Driver::write_all`
- remove use of `dev_info` in probe from the sample
- remove `properties_parse` from the sample
- add optional `baudrate` property to the sample
- remove 1. patch
- remove `TryFrom<&device::Device<Ctx>> for &serdev::Device<Ctx>`
  implementation
- fix import style
- add patch to return reference in `devres::register` to fix safety
  issue
- add patch to add private data to serdev_device, to fix
  `Device.drvdata()` from failing
- simplify abstraction by removing ability to receive the initial
  transmission. It may be added later in a separate patch series if
  needed.
- Link to v1: https://lore.kernel.org/r/20251220-rust_serdev-v1-0-e44645767621@posteo.de

---
Markus Probst (3):
      rust: add basic serial device bus abstractions
      samples: rust: add Rust serial device bus sample device driver
      MAINTAINERS: serdev: Add self for serdev

 MAINTAINERS                        |   5 +-
 drivers/tty/serdev/Kconfig         |   7 +
 rust/bindings/bindings_helper.h    |   1 +
 rust/helpers/helpers.c             |   1 +
 rust/helpers/serdev.c              |  22 ++
 rust/kernel/lib.rs                 |   2 +
 rust/kernel/serdev.rs              | 590 +++++++++++++++++++++++++++++++++++++
 samples/rust/Kconfig               |  11 +
 samples/rust/Makefile              |   1 +
 samples/rust/rust_driver_serdev.rs |  91 ++++++
 10 files changed, 730 insertions(+), 1 deletion(-)
---
base-commit: 9e171fc1d7d7ab847a750c03571c87ac3c17bd84
change-id: 20251217-rust_serdev-ee5481e9085c
prerequisite-message-id: 20260505152400.3905096-1-dakr@kernel.org
prerequisite-patch-id: d2aebf69b153c039bbed1d0ed26906708fd22534
prerequisite-patch-id: 84b28da2f5de20fc1785095c647b2ffc35d969a5
prerequisite-patch-id: 67318671a5eed5fb4ad23a450f1cf0e442bf8ca2
prerequisite-message-id: 20260525202921.124698-1-dakr@kernel.org
prerequisite-patch-id: b84db329d4372a175cb8d49e4e88c3eecf7eb228
prerequisite-patch-id: 2c30303f409cc8288cc87e241920219f5ddd8390
prerequisite-patch-id: 4e4f0ad370d763ad00b0f75b91fa216f2cc95953
prerequisite-patch-id: 5bcd6b37f3498feebda275dfef78136eba34004e
prerequisite-patch-id: 872b0982f3e5e7d1698d9df3b325e4cd27b27789
prerequisite-patch-id: 3a3c7749e017d9335f58497404d1350e96caf471
prerequisite-patch-id: 3526c9154f581497a11465b936d83ef61a875454
prerequisite-patch-id: 65d8c757b52475c2acc7d22ddc92cd3f0152b55d
prerequisite-patch-id: 4bd31f1414d5248dc080884caadf5f21684a8427
prerequisite-patch-id: 7beadbb0da3e589ed86d12f512d1c83427dd82b4
prerequisite-patch-id: 12cd0f67ffd27347f90c065db491945908206b7f
prerequisite-patch-id: 4642e31f66331f6c3b579377111ea733dbb3a11c
prerequisite-patch-id: 52d67b40b4396c741e2222d6a5bc7927abcb77aa
prerequisite-patch-id: 74ca82ff26cf9c7a993757c87db8be62006e820f
prerequisite-patch-id: 466fb9fa7febbffd8ef51b311c7d9893c11fc0f0
prerequisite-patch-id: e515cd98b06e26721cbbe6a4fbacd251d0073b63
prerequisite-patch-id: 8dc8e75d9f6499a554ef7e474bbacdbf3660a9f2
prerequisite-patch-id: 5fdb9f71dca2f44dd293760a60db125b770f1f55
prerequisite-patch-id: c766a24c2d5064f5ec09daada0b8e8fba862d3aa
prerequisite-patch-id: b768f6456d35fa7a80c015e34bbdba6082dbd593
prerequisite-patch-id: 6a8b17234f12f7084e6e2ce843a7031b0a891ce4
prerequisite-patch-id: 98b2deb9e60c1f28f90c5ee34fd608aaa9fd9420
prerequisite-patch-id: 774b29be66e641ee50cedb4704cf49d8b9fabf50
prerequisite-patch-id: cf95dc936cfc4b3a7a363435a51a48d9009645b3
prerequisite-message-id: 20260530132736.3298549-1-dakr@kernel.org
prerequisite-patch-id: 310c6bee038ca3909a8e5e58ec12b74f7189b869
prerequisite-patch-id: 92812c3d42b29504838e6dc66c307ff5c035bb5e

[PATCH v11 1/3] rust: add basic serial device bus abstractions

[Markus Probst via B4 Relay]

From: Markus Probst <markus.probst@posteo.de>

Implement the basic serial device bus abstractions required to write a
serial device bus device driver with or without the need for initial device
data. This includes the following data structures:

The `serdev::Driver` trait represents the interface to the driver.

The `serdev::Device` abstraction represents a `struct serdev_device`.

In order to provide the Serdev specific parts to a generic
`driver::Registration` the `driver::RegistrationOps` trait is
implemented by `serdev::Adapter`.

Signed-off-by: Markus Probst <markus.probst@posteo.de>
---
 MAINTAINERS                     |   2 +
 drivers/tty/serdev/Kconfig      |   7 +
 rust/bindings/bindings_helper.h |   1 +
 rust/helpers/helpers.c          |   1 +
 rust/helpers/serdev.c           |  22 ++
 rust/kernel/lib.rs              |   2 +
 rust/kernel/serdev.rs           | 590 ++++++++++++++++++++++++++++++++++++++++
 7 files changed, 625 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 4e118f704699..d2f608ff8ca0 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -24276,6 +24276,8 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/serial/serial.yaml
 F:	drivers/tty/serdev/
 F:	include/linux/serdev.h
+F:	rust/helpers/serdev.c
+F:	rust/kernel/serdev.rs
 
 SERIAL IR RECEIVER
 M:	Sean Young <sean@mess.org>
diff --git a/drivers/tty/serdev/Kconfig b/drivers/tty/serdev/Kconfig
index 46ae732bfc68..e6dfe949ad01 100644
--- a/drivers/tty/serdev/Kconfig
+++ b/drivers/tty/serdev/Kconfig
@@ -9,6 +9,13 @@ menuconfig SERIAL_DEV_BUS
 
 	  Note that you typically also want to enable TTY port controller support.
 
+config RUST_SERIAL_DEV_BUS_ABSTRACTIONS
+	bool "Rust Serial device bus abstractions"
+	depends on RUST
+	select SERIAL_DEV_BUS
+	help
+	  This enables the Rust abstraction for the serial device bus API.
+
 if SERIAL_DEV_BUS
 
 config SERIAL_DEV_CTRL_TTYPORT
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 446dbeaf0866..4e42635b8607 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -84,6 +84,7 @@
 #include <linux/regulator/consumer.h>
 #include <linux/sched.h>
 #include <linux/security.h>
+#include <linux/serdev.h>
 #include <linux/slab.h>
 #include <linux/sys_soc.h>
 #include <linux/task_work.h>
diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
index 38b34518eff1..2fb8506a748a 100644
--- a/rust/helpers/helpers.c
+++ b/rust/helpers/helpers.c
@@ -86,6 +86,7 @@
 #include "regulator.c"
 #include "scatterlist.c"
 #include "security.c"
+#include "serdev.c"
 #include "signal.c"
 #include "slab.c"
 #include "spinlock.c"
diff --git a/rust/helpers/serdev.c b/rust/helpers/serdev.c
new file mode 100644
index 000000000000..c52b78ca3fc7
--- /dev/null
+++ b/rust/helpers/serdev.c
@@ -0,0 +1,22 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/serdev.h>
+
+__rust_helper
+void rust_helper_serdev_device_driver_unregister(struct serdev_device_driver *sdrv)
+{
+	serdev_device_driver_unregister(sdrv);
+}
+
+__rust_helper
+void rust_helper_serdev_device_put(struct serdev_device *serdev)
+{
+	serdev_device_put(serdev);
+}
+
+__rust_helper
+void rust_helper_serdev_device_set_client_ops(struct serdev_device *serdev,
+					      const struct serdev_device_ops *ops)
+{
+	serdev_device_set_client_ops(serdev, ops);
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index b72b2fbe046d..83bc2c312241 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -118,6 +118,8 @@
 pub mod scatterlist;
 pub mod security;
 pub mod seq_file;
+#[cfg(CONFIG_RUST_SERIAL_DEV_BUS_ABSTRACTIONS)]
+pub mod serdev;
 pub mod sizes;
 #[cfg(CONFIG_SOC_BUS)]
 pub mod soc;
diff --git a/rust/kernel/serdev.rs b/rust/kernel/serdev.rs
new file mode 100644
index 000000000000..c6a7f9683d06
--- /dev/null
+++ b/rust/kernel/serdev.rs
@@ -0,0 +1,590 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Abstractions for the serial device bus.
+//!
+//! C header: [`include/linux/serdev.h`](srctree/include/linux/serdev.h)
+
+use crate::{
+    acpi,
+    device,
+    driver,
+    error::{
+        from_result,
+        to_result,
+        VTABLE_DEFAULT_ERROR, //
+    },
+    new_mutex,
+    of,
+    prelude::*,
+    sync::{
+        aref::AlwaysRefCounted,
+        Mutex, //
+    },
+    time::{
+        msecs_to_jiffies,
+        Jiffies,
+        Msecs, //
+    },
+    types::{
+        Opaque,
+        ScopeGuard, //
+    }, //
+};
+
+use core::{
+    cell::UnsafeCell,
+    marker::PhantomData,
+    mem::{offset_of, MaybeUninit},
+    num::NonZero,
+    ptr::NonNull, //
+};
+
+/// Parity bit to use with a serial device.
+#[repr(u32)]
+pub enum Parity {
+    /// No parity bit.
+    None = bindings::serdev_parity_SERDEV_PARITY_NONE,
+    /// Even partiy.
+    Even = bindings::serdev_parity_SERDEV_PARITY_EVEN,
+    /// Odd parity.
+    Odd = bindings::serdev_parity_SERDEV_PARITY_ODD,
+}
+
+/// Timeout in Jiffies.
+pub enum Timeout {
+    /// Wait for a specific amount of [`Jiffies`].
+    Jiffies(NonZero<Jiffies>),
+    /// Wait for a specific amount of [`Msecs`].
+    Milliseconds(NonZero<Msecs>),
+    /// Wait as long as possible.
+    ///
+    /// This is equivalent to [`kernel::task::MAX_SCHEDULE_TIMEOUT`].
+    Max,
+}
+
+impl Timeout {
+    fn into_jiffies(self) -> isize {
+        match self {
+            Self::Jiffies(value) => value.get().try_into().unwrap_or_default(),
+            Self::Milliseconds(value) => {
+                msecs_to_jiffies(value.get()).try_into().unwrap_or_default()
+            }
+            Self::Max => 0,
+        }
+    }
+}
+
+/// An adapter for the registration of serial device bus device drivers.
+pub struct Adapter<T: Driver>(T);
+
+// SAFETY:
+// - `bindings::serdev_device_driver` is a C type declared as `repr(C)`.
+// - `PrivateData<'bound, T>` is the type of the driver's device private data.
+// - `struct serdev_device_driver` embeds a `struct device_driver`.
+// - `DEVICE_DRIVER_OFFSET` is the correct byte offset to the embedded `struct device_driver`.
+unsafe impl<T: Driver> driver::DriverLayout for Adapter<T> {
+    type DriverType = bindings::serdev_device_driver;
+    type DriverData<'bound> = PrivateData<'bound, T>;
+    const DEVICE_DRIVER_OFFSET: usize = core::mem::offset_of!(Self::DriverType, driver);
+}
+
+// SAFETY: A call to `unregister` for a given instance of `DriverType` is guaranteed to be valid if
+// a preceding call to `register` has been successful.
+unsafe impl<T: Driver> driver::RegistrationOps for Adapter<T> {
+    unsafe fn register(
+        sdrv: &Opaque<Self::DriverType>,
+        name: &'static CStr,
+        module: &'static ThisModule,
+    ) -> Result {
+        let of_table = match T::OF_ID_TABLE {
+            Some(table) => table.as_ptr(),
+            None => core::ptr::null(),
+        };
+
+        let acpi_table = match T::ACPI_ID_TABLE {
+            Some(table) => table.as_ptr(),
+            None => core::ptr::null(),
+        };
+
+        // SAFETY: It's safe to set the fields of `struct serdev_device_driver` on initialization.
+        unsafe {
+            (*sdrv.get()).driver.name = name.as_char_ptr();
+            (*sdrv.get()).probe = Some(Self::probe_callback);
+            (*sdrv.get()).remove = Some(Self::remove_callback);
+            (*sdrv.get()).driver.of_match_table = of_table;
+            (*sdrv.get()).driver.acpi_match_table = acpi_table;
+        }
+
+        // SAFETY: `sdrv` is guaranteed to be a valid `DriverType`.
+        to_result(unsafe { bindings::__serdev_device_driver_register(sdrv.get(), module.0) })
+    }
+
+    unsafe fn unregister(sdrv: &Opaque<Self::DriverType>) {
+        // SAFETY: `sdrv` is guaranteed to be a valid `DriverType`.
+        unsafe { bindings::serdev_device_driver_unregister(sdrv.get()) };
+    }
+}
+
+#[doc(hidden)]
+#[pin_data(PinnedDrop)]
+pub struct PrivateData<'bound, T: Driver> {
+    sdev: &'bound Device<device::Bound>,
+    #[pin]
+    driver: UnsafeCell<MaybeUninit<T::Data<'bound>>>,
+    open: UnsafeCell<bool>,
+    #[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;
+        }
+
+        // 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<T: Driver> Adapter<T> {
+    const OPS: &'static bindings::serdev_device_ops = &bindings::serdev_device_ops {
+        receive_buf: if T::HAS_RECEIVE {
+            Some(Self::receive_buf_callback)
+        } else {
+            None
+        },
+        write_wakeup: Some(bindings::serdev_device_write_wakeup),
+    };
+
+    extern "C" fn probe_callback(sdev: *mut bindings::serdev_device) -> kernel::ffi::c_int {
+        // SAFETY: The serial device bus only ever calls the probe callback with a valid pointer to
+        // a `struct serdev_device`.
+        //
+        // INVARIANT: `sdev` is valid for the duration of `probe_callback()`.
+        let sdev = unsafe { &*sdev.cast::<Device<device::CoreInternal<'_>>>() };
+        let info = <Self as driver::Adapter>::id_info(sdev.as_ref());
+
+        from_result(|| {
+            sdev.as_ref().set_drvdata(try_pin_init!(PrivateData::<T> {
+                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>>() };
+            let private_data = ScopeGuard::new_with_data(private_data, |_| {
+                // SAFETY: We just set drvdata to `PrivateData<'_, T>`.
+                drop(unsafe { sdev.as_ref().drvdata_obtain::<PrivateData<'_, T>>() });
+            });
+            let 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: 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);
+
+            // 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 { data.__pinned_init(driver.as_mut_ptr()) };
+
+            *active = result.is_ok();
+
+            drop(active);
+
+            result.map(|()| {
+                private_data.dismiss();
+                0
+            })
+        })
+    }
+
+    extern "C" fn remove_callback(sdev: *mut bindings::serdev_device) {
+        // SAFETY: The serial device bus only ever calls the remove callback with a valid pointer
+        // to a `struct 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>>() };
+
+        // 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);
+    }
+
+    extern "C" fn receive_buf_callback(
+        sdev: *mut bindings::serdev_device,
+        buf: *const u8,
+        length: usize,
+    ) -> usize {
+        // SAFETY: The serial device bus only ever calls the receive buf callback with a valid
+        // pointer to a `struct serdev_device`.
+        //
+        // INVARIANT: `sdev` is valid for the duration of `receive_buf_callback()`.
+        let sdev = unsafe { &*sdev.cast::<Device<device::CoreInternal<'_>>>() };
+
+        // 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>>() };
+        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() };
+        // 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)
+    }
+}
+
+impl<T: Driver> driver::Adapter for Adapter<T> {
+    type IdInfo = T::IdInfo;
+
+    fn of_id_table() -> Option<of::IdTable<Self::IdInfo>> {
+        T::OF_ID_TABLE
+    }
+
+    fn acpi_id_table() -> Option<acpi::IdTable<Self::IdInfo>> {
+        T::ACPI_ID_TABLE
+    }
+}
+
+/// Declares a kernel module that exposes a single serial device bus device driver.
+///
+/// # Examples
+///
+/// ```ignore
+/// kernel::module_serdev_device_driver! {
+///     type: MyDriver,
+///     name: "Module name",
+///     authors: ["Author name"],
+///     description: "Description",
+///     license: "GPL v2",
+/// }
+/// ```
+#[macro_export]
+macro_rules! module_serdev_device_driver {
+    ($($f:tt)*) => {
+        $crate::module_driver!(<T>, $crate::serdev::Adapter<T>, { $($f)* });
+    };
+}
+
+/// The serial device bus device driver trait.
+///
+/// Drivers must implement this trait in order to get a serial device bus device driver registered.
+///
+/// # Examples
+///
+///```
+/// # use kernel::{
+///     acpi,
+///     bindings,
+///     device::{
+///         Bound,
+///         Core, //
+///     },
+///     of,
+///     serdev, //
+/// };
+///
+/// struct MyDriver;
+///
+/// kernel::of_device_table!(
+///     OF_TABLE,
+///     MODULE_OF_TABLE,
+///     <MyDriver as serdev::Driver>::IdInfo,
+///     [
+///         (of::DeviceId::new(c"test,device"), ())
+///     ]
+/// );
+///
+/// kernel::acpi_device_table!(
+///     ACPI_TABLE,
+///     MODULE_ACPI_TABLE,
+///     <MyDriver as serdev::Driver>::IdInfo,
+///     [
+///         (acpi::DeviceId::new(c"LNUXBEEF"), ())
+///     ]
+/// );
+///
+/// #[vtable]
+/// impl serdev::Driver for MyDriver {
+///     type IdInfo = ();
+///     type Data<'bound> = Self;
+///     const OF_ID_TABLE: Option<of::IdTable<Self::IdInfo>> = Some(&OF_TABLE);
+///     const ACPI_ID_TABLE: Option<acpi::IdTable<Self::IdInfo>> = Some(&ACPI_TABLE);
+///
+///     fn probe<'bound>(
+///         sdev: &'bound serdev::Device<Core<'_>>,
+///         _id_info: Option<&'bound Self::IdInfo>,
+///     ) -> impl PinInit<Self::Data<'bound>, Error> + 'bound {
+///         sdev.set_baudrate(115200);
+///         sdev.write_all(b"Hello\n", serdev::Timeout::Max)?;
+///         Ok(MyDriver)
+///     }
+/// }
+///```
+#[vtable]
+pub trait Driver {
+    /// The type holding driver private data about each device id supported by the driver.
+    // TODO: Use associated_type_defaults once stabilized:
+    //
+    // ```
+    // type IdInfo: 'static = ();
+    // ```
+    type IdInfo: 'static;
+
+    /// The type of the driver's bus device private data.
+    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;
+
+    /// The table of ACPI device ids supported by the driver.
+    const ACPI_ID_TABLE: Option<acpi::IdTable<Self::IdInfo>> = None;
+
+    /// Serial device bus device driver probe.
+    ///
+    /// Called when a new serial device bus device is added or discovered.
+    /// Implementers should attempt to initialize the device here.
+    fn probe<'bound>(
+        sdev: &'bound Device<device::Core<'_>>,
+        id_info: Option<&'bound Self::IdInfo>,
+    ) -> impl PinInit<Self::Data<'bound>, Error> + 'bound;
+
+    /// Serial device bus device driver unbind.
+    ///
+    /// Called when a [`Device`] is unbound from its bound [`Driver`]. Implementing this callback
+    /// is optional.
+    ///
+    /// This callback serves as a place for drivers to perform teardown operations that require a
+    /// `&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>>) {
+        let _ = (sdev, this);
+    }
+
+    /// Serial device bus device data receive callback.
+    ///
+    /// Called when data got received from device.
+    ///
+    /// Returns the number of bytes accepted.
+    fn receive<'bound>(
+        sdev: &'bound Device<device::Bound>,
+        this: Pin<&Self::Data<'bound>>,
+        data: &[u8],
+    ) -> usize {
+        let _ = (sdev, this, data);
+        build_error!(VTABLE_DEFAULT_ERROR)
+    }
+}
+
+/// The serial device bus device representation.
+///
+/// This structure represents the Rust abstraction for a C `struct serdev_device`. The
+/// implementation abstracts the usage of an already existing C `struct serdev_device` within Rust
+/// code that we get passed from the C side.
+///
+/// # Invariants
+///
+/// A [`Device`] instance represents a valid `struct serdev_device` created by the C portion of
+/// the kernel.
+#[repr(transparent)]
+pub struct Device<Ctx: device::DeviceContext = device::Normal>(
+    Opaque<bindings::serdev_device>,
+    PhantomData<Ctx>,
+);
+
+impl<Ctx: device::DeviceContext> Device<Ctx> {
+    fn as_raw(&self) -> *mut bindings::serdev_device {
+        self.0.get()
+    }
+}
+
+impl Device<device::Bound> {
+    /// Set the baudrate in bits per second.
+    ///
+    /// Common baudrates are 115200, 9600, 19200, 57600, 4800.
+    ///
+    /// Use [`Device::write_flush`] before calling this if you have written data prior to this call.
+    pub fn set_baudrate(&self, speed: u32) -> Result<(), u32> {
+        // SAFETY: `self.as_raw()` is guaranteed to be a pointer to a valid `serdev_device`.
+        let ret = unsafe { bindings::serdev_device_set_baudrate(self.as_raw(), speed) };
+        if ret == speed {
+            Ok(())
+        } else {
+            Err(ret)
+        }
+    }
+
+    /// Set if flow control should be enabled.
+    ///
+    /// Use [`Device::write_flush`] before calling this if you have written data prior to this call.
+    pub fn set_flow_control(&self, enable: bool) {
+        // SAFETY: `self.as_raw()` is guaranteed to be a pointer to a valid `serdev_device`.
+        unsafe { bindings::serdev_device_set_flow_control(self.as_raw(), enable) };
+    }
+
+    /// Set parity to use.
+    ///
+    /// Use [`Device::write_flush`] before calling this if you have written data prior to this call.
+    pub fn set_parity(&self, parity: Parity) -> Result {
+        // SAFETY: `self.as_raw()` is guaranteed to be a pointer to a valid `serdev_device`.
+        to_result(unsafe { bindings::serdev_device_set_parity(self.as_raw(), parity as u32) })
+    }
+
+    /// Write data to the serial device until the controller has accepted all the data or has
+    /// been interrupted by a timeout or signal.
+    ///
+    /// Note that any accepted data has only been buffered by the controller. Use
+    /// [`Device::wait_until_sent`] to make sure the controller write buffer has actually been
+    /// emptied.
+    ///
+    /// Returns the number of bytes written (less than `data.len()` if interrupted).
+    /// [`kernel::error::code::ETIMEDOUT`] or [`kernel::error::code::ERESTARTSYS`] if interrupted
+    /// before any bytes were written.
+    pub fn write_all(&self, data: &[u8], timeout: Timeout) -> Result<usize> {
+        // SAFETY:
+        // - `self.as_raw()` is guaranteed to be a pointer to a valid `serdev_device`.
+        // - `data.as_ptr()` is guaranteed to be a valid array pointer with the size of
+        //   `data.len()`.
+        let ret = unsafe {
+            bindings::serdev_device_write(
+                self.as_raw(),
+                data.as_ptr(),
+                data.len(),
+                timeout.into_jiffies(),
+            )
+        };
+        // CAST: negative return values are guaranteed to be between `-MAX_ERRNO` and `-1`,
+        // which always fit into a `i32`.
+        to_result(ret as i32).map(|()| ret.unsigned_abs())
+    }
+
+    /// Write data to the serial device.
+    ///
+    /// If you want to write until the controller has accepted all the data, use
+    /// [`Device::write_all`].
+    ///
+    /// Note that any accepted data has only been buffered by the controller. Use
+    /// [ Device::wait_until_sent`] to make sure the controller write buffer has actually been
+    /// emptied.
+    ///
+    /// Returns the number of bytes written (less than `data.len()` if not enough room in the
+    /// write buffer).
+    pub fn write(&self, data: &[u8]) -> Result<u32> {
+        // SAFETY:
+        // - `self.as_raw()` is guaranteed to be a pointer to a valid `serdev_device`.
+        // - `data.as_ptr()` is guaranteed to be a valid array pointer with the size of
+        //   `data.len()`.
+        let ret =
+            unsafe { bindings::serdev_device_write_buf(self.as_raw(), data.as_ptr(), data.len()) };
+
+        to_result(ret as i32).map(|()| ret.unsigned_abs())
+    }
+
+    /// Send data to the serial device immediately.
+    ///
+    /// Note that this doesn't guarantee that the data has been transmitted.
+    /// Use [`Device::wait_until_sent`] for this purpose.
+    pub fn write_flush(&self) {
+        // SAFETY: `self.as_raw()` is guaranteed to be a pointer to a valid `serdev_device`.
+        unsafe { bindings::serdev_device_write_flush(self.as_raw()) };
+    }
+
+    /// Wait for the data to be sent.
+    ///
+    /// After this function, the write buffer of the controller should be empty.
+    pub fn wait_until_sent(&self, timeout: Timeout) {
+        // SAFETY: `self.as_raw()` is guaranteed to be a pointer to a valid `serdev_device`.
+        unsafe { bindings::serdev_device_wait_until_sent(self.as_raw(), timeout.into_jiffies()) };
+    }
+}
+
+// SAFETY: `serdev::Device` is a transparent wrapper of `struct serdev_device`.
+// The offset is guaranteed to point to a valid device field inside `serdev::Device`.
+unsafe impl<Ctx: device::DeviceContext> device::AsBusDevice<Ctx> for Device<Ctx> {
+    const OFFSET: usize = offset_of!(bindings::serdev_device, dev);
+}
+
+// SAFETY: `Device` is a transparent wrapper of a type that doesn't depend on `Device`'s generic
+// argument.
+kernel::impl_device_context_deref!(unsafe { Device });
+kernel::impl_device_context_into_aref!(Device);
+
+// SAFETY: Instances of `Device` are always reference-counted.
+unsafe impl AlwaysRefCounted for Device {
+    fn inc_ref(&self) {
+        self.as_ref().inc_ref();
+    }
+
+    unsafe fn dec_ref(obj: NonNull<Self>) {
+        // SAFETY: The safety requirements guarantee that the refcount is non-zero.
+        unsafe { bindings::serdev_device_put(obj.cast().as_ptr()) }
+    }
+}
+
+impl<Ctx: device::DeviceContext> AsRef<device::Device<Ctx>> for Device<Ctx> {
+    fn as_ref(&self) -> &device::Device<Ctx> {
+        // SAFETY: By the type invariant of `Self`, `self.as_raw()` is a pointer to a valid
+        // `struct serdev_device`.
+        let dev = unsafe { &raw mut (*self.as_raw()).dev };
+
+        // SAFETY: `dev` points to a valid `struct device`.
+        unsafe { device::Device::from_raw(dev) }
+    }
+}
+
+// SAFETY: A `Device` is always reference-counted and can be released from any thread.
+unsafe impl Send for Device {}
+
+// SAFETY: `Device` can be shared among threads because all methods of `Device`
+// (i.e. `Device<Normal>) are thread safe.
+unsafe impl Sync for Device {}
+
+// SAFETY: Same as `Device<Normal>` -- the underlying `struct serdev_device` is the same;
+// `Bound` is a zero-sized type-state marker that does not affect thread safety.
+unsafe impl Sync for Device<device::Bound> {}

-- 
2.53.0

[PATCH v11 2/3] samples: rust: add Rust serial device bus sample device driver

[Markus Probst via B4 Relay]

From: Markus Probst <markus.probst@posteo.de>

Add a sample Rust serial device bus device driver illustrating the usage
of the serial device bus abstractions.

This drivers probes through either a match of device / driver name or a
match within the OF ID table.

Signed-off-by: Markus Probst <markus.probst@posteo.de>
---
 MAINTAINERS                        |  1 +
 samples/rust/Kconfig               | 11 +++++
 samples/rust/Makefile              |  1 +
 samples/rust/rust_driver_serdev.rs | 91 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 104 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index d2f608ff8ca0..fa3f20f0cc4f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -24278,6 +24278,7 @@ F:	drivers/tty/serdev/
 F:	include/linux/serdev.h
 F:	rust/helpers/serdev.c
 F:	rust/kernel/serdev.rs
+F:	samples/rust/rust_driver_serdev.rs
 
 SERIAL IR RECEIVER
 M:	Sean Young <sean@mess.org>
diff --git a/samples/rust/Kconfig b/samples/rust/Kconfig
index c49ab9106345..31d62533ef25 100644
--- a/samples/rust/Kconfig
+++ b/samples/rust/Kconfig
@@ -161,6 +161,17 @@ config SAMPLE_RUST_DRIVER_AUXILIARY
 
 	  If unsure, say N.
 
+config SAMPLE_RUST_DRIVER_SERDEV
+	tristate "Serial Device Bus Device Driver"
+	select RUST_SERIAL_DEV_BUS_ABSTRACTIONS
+	help
+	  This option builds the Rust serial device bus driver sample.
+
+	  To compile this as a module, choose M here:
+	  the module will be called rust_driver_serdev.
+
+	  If unsure, say N.
+
 config SAMPLE_RUST_SOC
 	tristate "SoC Driver"
 	select SOC_BUS
diff --git a/samples/rust/Makefile b/samples/rust/Makefile
index 6c0aaa58cccc..b986b681cde5 100644
--- a/samples/rust/Makefile
+++ b/samples/rust/Makefile
@@ -14,6 +14,7 @@ obj-$(CONFIG_SAMPLE_RUST_DRIVER_PLATFORM)	+= rust_driver_platform.o
 obj-$(CONFIG_SAMPLE_RUST_DRIVER_USB)		+= rust_driver_usb.o
 obj-$(CONFIG_SAMPLE_RUST_DRIVER_FAUX)		+= rust_driver_faux.o
 obj-$(CONFIG_SAMPLE_RUST_DRIVER_AUXILIARY)	+= rust_driver_auxiliary.o
+obj-$(CONFIG_SAMPLE_RUST_DRIVER_SERDEV)		+= rust_driver_serdev.o
 obj-$(CONFIG_SAMPLE_RUST_CONFIGFS)		+= rust_configfs.o
 obj-$(CONFIG_SAMPLE_RUST_SOC)			+= rust_soc.o
 
diff --git a/samples/rust/rust_driver_serdev.rs b/samples/rust/rust_driver_serdev.rs
new file mode 100644
index 000000000000..824affbf6593
--- /dev/null
+++ b/samples/rust/rust_driver_serdev.rs
@@ -0,0 +1,91 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Rust Serial device bus device driver sample.
+
+use kernel::{
+    acpi,
+    device::{
+        Bound,
+        Core, //
+    },
+    of,
+    prelude::*,
+    serdev,
+    sync::aref::ARef, //
+};
+
+struct SampleDriver {
+    sdev: ARef<serdev::Device>,
+}
+
+kernel::of_device_table!(
+    OF_TABLE,
+    MODULE_OF_TABLE,
+    <SampleDriver as serdev::Driver>::IdInfo,
+    [(of::DeviceId::new(c"test,rust_driver_serdev"), ())]
+);
+
+kernel::acpi_device_table!(
+    ACPI_TABLE,
+    MODULE_ACPI_TABLE,
+    <SampleDriver as serdev::Driver>::IdInfo,
+    [(acpi::DeviceId::new(c"LNUXBEEF"), ())]
+);
+
+#[vtable]
+impl serdev::Driver for SampleDriver {
+    type IdInfo = ();
+    type Data<'bound> = Self;
+    const OF_ID_TABLE: Option<of::IdTable<Self::IdInfo>> = Some(&OF_TABLE);
+    const ACPI_ID_TABLE: Option<acpi::IdTable<Self::IdInfo>> = Some(&ACPI_TABLE);
+
+    fn probe<'bound>(
+        sdev: &'bound serdev::Device<Core<'_>>,
+        _info: Option<&'bound Self::IdInfo>,
+    ) -> impl PinInit<Self, Error> + 'bound {
+        let dev = sdev.as_ref();
+
+        dev_dbg!(dev, "Probe Rust Serial device bus device driver sample.\n");
+
+        if sdev
+            .set_baudrate(
+                dev.fwnode()
+                    .and_then(|fwnode| fwnode.property_read(c"baudrate").optional())
+                    .unwrap_or(115200),
+            )
+            .is_err()
+        {
+            return Err(EINVAL);
+        }
+        sdev.set_flow_control(false);
+        sdev.set_parity(serdev::Parity::None)?;
+
+        Ok(Self { sdev: sdev.into() })
+    }
+
+    fn receive<'bound>(
+        sdev: &'bound serdev::Device<Bound>,
+        _this: Pin<&Self>,
+        data: &[u8],
+    ) -> usize {
+        let _ = sdev.write_all(data, serdev::Timeout::Max);
+        data.len()
+    }
+}
+
+impl Drop for SampleDriver {
+    fn drop(&mut self) {
+        dev_dbg!(
+            self.sdev.as_ref(),
+            "Remove Rust Serial device bus device driver sample.\n"
+        );
+    }
+}
+
+kernel::module_serdev_device_driver! {
+    type: SampleDriver,
+    name: "rust_driver_serdev",
+    authors: ["Markus Probst"],
+    description: "Rust Serial device bus device driver",
+    license: "GPL v2",
+}

-- 
2.53.0

[PATCH v11 3/3] MAINTAINERS: serdev: Add self for serdev

[Markus Probst via B4 Relay]

From: Markus Probst <markus.probst@posteo.de>

Rob mentioned he needs to find someone else to maintain serdev.

Link: https://lore.kernel.org/rust-for-linux/20260430195858.GA1650658-robh@kernel.org/
Link: https://lore.kernel.org/rust-for-linux/da85ceb81f51079d4a8248a1ffde6a27d2ef24ad.camel@posteo.de/
Signed-off-by: Markus Probst <markus.probst@posteo.de>
---
 MAINTAINERS | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index fa3f20f0cc4f..58db6aedf45a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -24270,7 +24270,7 @@ F:	drivers/iio/chemical/sps30_i2c.c
 F:	drivers/iio/chemical/sps30_serial.c
 
 SERIAL DEVICE BUS
-M:	Rob Herring <robh@kernel.org>
+M:	Markus Probst <markus.probst@posteo.de>
 L:	linux-serial@vger.kernel.org
 S:	Maintained
 F:	Documentation/devicetree/bindings/serial/serial.yaml

-- 
2.53.0

Re: [PATCH v11 1/3] rust: add basic serial device bus abstractions

[Danilo Krummrich]

On Sun May 31, 2026 at 12:51 AM CEST, Markus Probst via B4 Relay wrote:
> +#[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;
> +        }
> +
> +        // 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()) };
> +        }
> +    }
> +}

Just in case it came across otherwise, I did not mean to push for you to go for
this approach. We just kept discussing it because it let to the (to me more
interesting) discussion around the LED class device abstraction.

While this approach gets us rid of an extra allocation and the rust_private_data
pointer in struct serdev_device it also turns out a bit more convoluted.

That said, both are correct, and I won't object either one; up to you and the
serdev / tty maintainers.

Please wait a bit before resending, so other people can comment on this as well.

> +    extern "C" fn receive_buf_callback(
> +        sdev: *mut bindings::serdev_device,
> +        buf: *const u8,
> +        length: usize,
> +    ) -> usize {
> +        // SAFETY: The serial device bus only ever calls the receive buf callback with a valid
> +        // pointer to a `struct serdev_device`.
> +        //
> +        // INVARIANT: `sdev` is valid for the duration of `receive_buf_callback()`.
> +        let sdev = unsafe { &*sdev.cast::<Device<device::CoreInternal<'_>>>() };

CoreInternal snuck back in, should be 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>>() };
> +        let active = private_data.active.lock();

I think SRCU would be a much better fit, but the code didn't land yet, so the
mutex seems fine for now. But I'd probably add a TODO.

> +
> +        if !*active {
> +            return length;
> +        }
> +
> +        // 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)
> +    }
> +}

Re: [PATCH v11 1/3] rust: add basic serial device bus abstractions

[Onur Özkan]

On Sun, 31 May 2026 01:51:08 +0200
Danilo Krummrich <dakr@kernel.org> wrote:

> On Sun May 31, 2026 at 12:51 AM CEST, Markus Probst via B4 Relay wrote:
> > +#[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;
> > +        }
> > +
> > +        // 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()) };
> > +        }
> > +    }
> > +}
> 
> Just in case it came across otherwise, I did not mean to push for you to go for
> this approach. We just kept discussing it because it let to the (to me more
> interesting) discussion around the LED class device abstraction.
> 
> While this approach gets us rid of an extra allocation and the rust_private_data
> pointer in struct serdev_device it also turns out a bit more convoluted.
> 
> That said, both are correct, and I won't object either one; up to you and the
> serdev / tty maintainers.
> 
> Please wait a bit before resending, so other people can comment on this as well.
> 
> > +    extern "C" fn receive_buf_callback(
> > +        sdev: *mut bindings::serdev_device,
> > +        buf: *const u8,
> > +        length: usize,
> > +    ) -> usize {
> > +        // SAFETY: The serial device bus only ever calls the receive buf callback with a valid
> > +        // pointer to a `struct serdev_device`.
> > +        //
> > +        // INVARIANT: `sdev` is valid for the duration of `receive_buf_callback()`.
> > +        let sdev = unsafe { &*sdev.cast::<Device<device::CoreInternal<'_>>>() };
> 
> CoreInternal snuck back in, should be 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>>() };
> > +        let active = private_data.active.lock();
> 
> I think SRCU would be a much better fit, but the code didn't land yet, so the
> mutex seems fine for now. But I'd probably add a TODO.
> 

Jfyi v9 is on the list [1] and I would say we are pretty close on taking that.

Thanks,
Onur

[1]: https://lore.kernel.org/rust-for-linux/DIWEXUCNLURG.136XXDBHSBNVR@kernel.org

> > +
> > +        if !*active {
> > +            return length;
> > +        }
> > +
> > +        // 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)
> > +    }
> > +}

Re: [PATCH v11 1/3] rust: add basic serial device bus abstractions

[Onur Özkan]

On Sun, 31 May 2026 09:58:34 +0300
Onur Özkan <work@onurozkan.dev> wrote:

> On Sun, 31 May 2026 01:51:08 +0200
> Danilo Krummrich <dakr@kernel.org> wrote:
> 
> > On Sun May 31, 2026 at 12:51 AM CEST, Markus Probst via B4 Relay wrote:
> > > +#[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;
> > > +        }
> > > +
> > > +        // 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()) };
> > > +        }
> > > +    }
> > > +}
> > 
> > Just in case it came across otherwise, I did not mean to push for you to go for
> > this approach. We just kept discussing it because it let to the (to me more
> > interesting) discussion around the LED class device abstraction.
> > 
> > While this approach gets us rid of an extra allocation and the rust_private_data
> > pointer in struct serdev_device it also turns out a bit more convoluted.
> > 
> > That said, both are correct, and I won't object either one; up to you and the
> > serdev / tty maintainers.
> > 
> > Please wait a bit before resending, so other people can comment on this as well.
> > 
> > > +    extern "C" fn receive_buf_callback(
> > > +        sdev: *mut bindings::serdev_device,
> > > +        buf: *const u8,
> > > +        length: usize,
> > > +    ) -> usize {
> > > +        // SAFETY: The serial device bus only ever calls the receive buf callback with a valid
> > > +        // pointer to a `struct serdev_device`.
> > > +        //
> > > +        // INVARIANT: `sdev` is valid for the duration of `receive_buf_callback()`.
> > > +        let sdev = unsafe { &*sdev.cast::<Device<device::CoreInternal<'_>>>() };
> > 
> > CoreInternal snuck back in, should be 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>>() };
> > > +        let active = private_data.active.lock();
> > 
> > I think SRCU would be a much better fit, but the code didn't land yet, so the
> > mutex seems fine for now. But I'd probably add a TODO.
> > 
> 
> Jfyi v9 is on the list [1] and I would say we are pretty close on taking that.
> 
> Thanks,
> Onur
> 
> [1]: https://lore.kernel.org/rust-for-linux/DIWEXUCNLURG.136XXDBHSBNVR@kernel.org
> 

I pasted the wrong link, sorry.

It should have been this instead:

  https://lore.kernel.org/all/20260529134004.396743-1-work@onurozkan.dev

> > > +
> > > +        if !*active {
> > > +            return length;
> > > +        }
> > > +
> > > +        // 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)
> > > +    }
> > > +}

Re: [PATCH v11 1/3] rust: add basic serial device bus abstractions

[Markus Probst]

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

On Sun, 2026-05-31 at 09:58 +0300, Onur Özkan wrote:
> On Sun, 31 May 2026 01:51:08 +0200
> Danilo Krummrich <dakr@kernel.org> wrote:
> 
> > On Sun May 31, 2026 at 12:51 AM CEST, Markus Probst via B4 Relay wrote:
> > > +#[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;
> > > +        }
> > > +
> > > +        // 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()) };
> > > +        }
> > > +    }
> > > +}
> > 
> > Just in case it came across otherwise, I did not mean to push for you to go for
> > this approach. We just kept discussing it because it let to the (to me more
> > interesting) discussion around the LED class device abstraction.
> > 
> > While this approach gets us rid of an extra allocation and the rust_private_data
> > pointer in struct serdev_device it also turns out a bit more convoluted.
> > 
> > That said, both are correct, and I won't object either one; up to you and the
> > serdev / tty maintainers.
> > 
> > Please wait a bit before resending, so other people can comment on this as well.
> > 
> > > +    extern "C" fn receive_buf_callback(
> > > +        sdev: *mut bindings::serdev_device,
> > > +        buf: *const u8,
> > > +        length: usize,
> > > +    ) -> usize {
> > > +        // SAFETY: The serial device bus only ever calls the receive buf callback with a valid
> > > +        // pointer to a `struct serdev_device`.
> > > +        //
> > > +        // INVARIANT: `sdev` is valid for the duration of `receive_buf_callback()`.
> > > +        let sdev = unsafe { &*sdev.cast::<Device<device::CoreInternal<'_>>>() };
> > 
> > CoreInternal snuck back in, should be 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>>() };
> > > +        let active = private_data.active.lock();
> > 
> > I think SRCU would be a much better fit, but the code didn't land yet, so the
> > mutex seems fine for now. But I'd probably add a TODO.
> > 
> 
> Jfyi v9 is on the list [1] and I would say we are pretty close on taking that.
How close? Should I rebase this patch on top of the srcu patch series?

Thanks
- Markus Probst

> 
> Thanks,
> Onur
> 
> [1]: https://lore.kernel.org/rust-for-linux/DIWEXUCNLURG.136XXDBHSBNVR@kernel.org
> 
> > > +
> > > +        if !*active {
> > > +            return length;
> > > +        }
> > > +
> > > +        // 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)
> > > +    }
> > > +}

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

Re: [PATCH v11 1/3] rust: add basic serial device bus abstractions

[Danilo Krummrich]

On Sun May 31, 2026 at 6:37 PM CEST, Markus Probst wrote:
> How close? Should I rebase this patch on top of the srcu patch series?

Since it isn't blocking you, it would only make sense if SRCU makes it this
cycle, but this series does not.

Re: [PATCH v11 1/3] rust: add basic serial device bus abstractions

[Markus Probst]

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

On Sun, 2026-05-31 at 01:51 +0200, Danilo Krummrich wrote:
> On Sun May 31, 2026 at 12:51 AM CEST, Markus Probst via B4 Relay wrote:
> > +#[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;
> > +        }
> > +
> > +        // 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()) };
> > +        }
> > +    }
> > +}
> 
> Just in case it came across otherwise, I did not mean to push for you to go for
> this approach. We just kept discussing it because it let to the (to me more
> interesting) discussion around the LED class device abstraction.
> 
> While this approach gets us rid of an extra allocation and the rust_private_data
> pointer in struct serdev_device it also turns out a bit more convoluted.
> 
> That said, both are correct, and I won't object either one; up to you and the
> serdev / tty maintainers.
> 
> Please wait a bit before resending, so other people can comment on this as well.
> 
> > +    extern "C" fn receive_buf_callback(
> > +        sdev: *mut bindings::serdev_device,
> > +        buf: *const u8,
> > +        length: usize,
> > +    ) -> usize {
> > +        // SAFETY: The serial device bus only ever calls the receive buf callback with a valid
> > +        // pointer to a `struct serdev_device`.
> > +        //
> > +        // INVARIANT: `sdev` is valid for the duration of `receive_buf_callback()`.
> > +        let sdev = unsafe { &*sdev.cast::<Device<device::CoreInternal<'_>>>() };
> 
> CoreInternal snuck back in, should be 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>>() };
> > +        let active = private_data.active.lock();
> 
> I think SRCU would be a much better fit, but the code didn't land yet, so the
> mutex seems fine for now. But I'd probably add a TODO.
I just noticed, is it even possible to use SRCU here? Currently the
mutex not only ensures that no drvdata access happens after drvdata
drop, but also that the receive_buf waits for the probe to complete, as
the drvdata hasn't been initialized yet.

(drvdata refers to PrivateData.driver in this context, not the whole
PrivateData)

Thanks
- Markus Probst

> 
> > +
> > +        if !*active {
> > +            return length;
> > +        }
> > +
> > +        // 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)
> > +    }
> > +}

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

Re: [PATCH v11 1/3] rust: add basic serial device bus abstractions

[Danilo Krummrich]

On Sun May 31, 2026 at 9:42 PM CEST, Markus Probst wrote:
> I just noticed, is it even possible to use SRCU here? Currently the mutex not
> only ensures that no drvdata access happens after drvdata drop, but also that
> the receive_buf waits for the probe to complete, as the drvdata hasn't been
> initialized yet.

Yeah, if you drop the completion, you need the mutex.

(In case it wasn't discussed in previous versions already, there is also the
option to just attach separate private data to the receive callback, which would
avoid this synchonization problem in the first place.

You could have serdev::Device<Core>::open(), which takes its own private data
and a corresponding close(), this way you'd allow drivers to control whether
they want the serial line "open" or not. You just need to make sure it is closed
eventually.)

That said, I don't know what turns out to be the better approach. And maybe it
simply isn't something this initial series has to tackle? I think your driver
does not implement the receive callback?

Re: [PATCH v11 1/3] rust: add basic serial device bus abstractions

[Markus Probst]

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

On Sun, 2026-05-31 at 23:49 +0200, Danilo Krummrich wrote:
> On Sun May 31, 2026 at 9:42 PM CEST, Markus Probst wrote:
> > I just noticed, is it even possible to use SRCU here? Currently the mutex not
> > only ensures that no drvdata access happens after drvdata drop, but also that
> > the receive_buf waits for the probe to complete, as the drvdata hasn't been
> > initialized yet.
> 
> Yeah, if you drop the completion, you need the mutex.
Is the performance impact on an mutex or on srcu + completion higher?

> 
> (In case it wasn't discussed in previous versions already, there is also the
> option to just attach separate private data to the receive callback, which would
> avoid this synchonization problem in the first place.
> 
> You could have serdev::Device<Core>::open(), which takes its own private data
> and a corresponding close(), this way you'd allow drivers to control whether
> they want the serial line "open" or not. You just need to make sure it is closed
> eventually.)
This would add complexity with types, as we need to ensure that write,
set_baudrate, set_parity etc. does not run when closed.
> 
> That said, I don't know what turns out to be the better approach. And maybe it
> simply isn't something this initial series has to tackle? I think your driver
> does not implement the receive callback?
The initial driver with only leds: no.

That changes once the driver also takes care of hwmon (ADC sensor and
fan failure) and input (power button and possibly other buttons).

Thanks
- Markus Probst

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

Re: [PATCH v11 1/3] rust: add basic serial device bus abstractions

[Danilo Krummrich]

On Mon Jun 1, 2026 at 12:00 AM CEST, Markus Probst wrote:
> On Sun, 2026-05-31 at 23:49 +0200, Danilo Krummrich wrote:
>> On Sun May 31, 2026 at 9:42 PM CEST, Markus Probst wrote:
>> > I just noticed, is it even possible to use SRCU here? Currently the mutex not
>> > only ensures that no drvdata access happens after drvdata drop, but also that
>> > the receive_buf waits for the probe to complete, as the drvdata hasn't been
>> > initialized yet.
>> 
>> Yeah, if you drop the completion, you need the mutex.
> Is the performance impact on an mutex or on srcu + completion higher?

Weighing in the completion, the mutex probably wins as it will always be
uncontested under normal operation.

>> 
>> (In case it wasn't discussed in previous versions already, there is also the
>> option to just attach separate private data to the receive callback, which would
>> avoid this synchonization problem in the first place.
>> 
>> You could have serdev::Device<Core>::open(), which takes its own private data
>> and a corresponding close(), this way you'd allow drivers to control whether
>> they want the serial line "open" or not. You just need to make sure it is closed
>> eventually.)
> This would add complexity with types, as we need to ensure that write,
> set_baudrate, set_parity etc. does not run when closed.

Right, but looking at a few serdev drivers, there seem to be a few cases where
drivers need to close and re-open.

>> That said, I don't know what turns out to be the better approach. And maybe it
>> simply isn't something this initial series has to tackle? I think your driver
>> does not implement the receive callback?
> The initial driver with only leds: no.
>
> That changes once the driver also takes care of hwmon (ADC sensor and
> fan failure) and input (power button and possibly other buttons).

Maybe drop it from this initial series then and revisit with the first user?

Re: [PATCH v11 1/3] rust: add basic serial device bus abstractions

[Markus Probst]

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

On Mon, 2026-06-01 at 00:32 +0200, Danilo Krummrich wrote:
> On Mon Jun 1, 2026 at 12:00 AM CEST, Markus Probst wrote:
> > On Sun, 2026-05-31 at 23:49 +0200, Danilo Krummrich wrote:
> > > On Sun May 31, 2026 at 9:42 PM CEST, Markus Probst wrote:
> > > > I just noticed, is it even possible to use SRCU here? Currently the mutex not
> > > > only ensures that no drvdata access happens after drvdata drop, but also that
> > > > the receive_buf waits for the probe to complete, as the drvdata hasn't been
> > > > initialized yet.
> > > 
> > > Yeah, if you drop the completion, you need the mutex.
> > Is the performance impact on an mutex or on srcu + completion higher?
> 
> Weighing in the completion, the mutex probably wins as it will always be
> uncontested under normal operation.
> 
> > > 
> > > (In case it wasn't discussed in previous versions already, there is also the
> > > option to just attach separate private data to the receive callback, which would
> > > avoid this synchonization problem in the first place.
> > > 
> > > You could have serdev::Device<Core>::open(), which takes its own private data
> > > and a corresponding close(), this way you'd allow drivers to control whether
> > > they want the serial line "open" or not. You just need to make sure it is closed
> > > eventually.)
> > This would add complexity with types, as we need to ensure that write,
> > set_baudrate, set_parity etc. does not run when closed.
> 
> Right, but looking at a few serdev drivers, there seem to be a few cases where
> drivers need to close and re-open.
That is something we could consider once there is a user.

I am certain it would make it more complex, not less.
> 
> > > That said, I don't know what turns out to be the better approach. And maybe it
> > > simply isn't something this initial series has to tackle? I think your driver
> > > does not implement the receive callback?
> > The initial driver with only leds: no.
> > 
> > That changes once the driver also takes care of hwmon (ADC sensor and
> > fan failure) and input (power button and possibly other buttons).
> 
> Maybe drop it from this initial series then and revisit with the first user?
I would prefer it with this series.

Except for probe and remove there should be no overhead if receive_buf
is not implemented.

As it will be needed eventually and it is basic serdev functionality, I
don't see a reason to not include it now.

Thanks
- Markus Probst


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

Claude review: samples: rust: add Rust serial device bus sample device driver

[Claude Code Review Bot]

Patch Review

This is a clean, minimal sample driver that demonstrates the serdev abstraction.

**1. Probe error handling — discards actual baudrate on error (lines 220–229):**

```rust
if sdev
    .set_baudrate(
        dev.fwnode()
            .and_then(|fwnode| fwnode.property_read(c"baudrate").optional())
            .unwrap_or(115200),
    )
    .is_err()
{
    return Err(EINVAL);
}
```

`set_baudrate` returns `Result<(), u32>` where the `Err(u32)` contains the actual baudrate that was set. The sample discards this. Fine for a sample, but a real driver might want to log or use the actual rate.

**2. `receive` callback echoes data — good illustrative choice:**

```rust
fn receive<'bound>(
    sdev: &'bound serdev::Device<Bound>,
    _this: Pin<&Self>,
    data: &[u8],
) -> usize {
    let _ = sdev.write_all(data, serdev::Timeout::Max);
    data.len()
}
```

The echo pattern is a good sample. Ignoring the `write_all` result with `let _ = ...` is acceptable here since there's no meaningful recovery in a sample echo driver.

**3. Kconfig correctly selects `RUST_SERIAL_DEV_BUS_ABSTRACTIONS`.**

No issues.

---

---
Generated by Claude Code Patch Reviewer

Claude review: rust: add basic serial device bus abstractions

[Claude Code Review Bot]

Overall Series Review

Subject: rust: add basic serial device bus abstractions
Author: Markus Probst via B4 Relay <devnull+markus.probst.posteo.de@kernel.org>
Patches: 14
Reviewed: 2026-06-04T15:07:54.612360

---

This is v11 of a 3-patch series adding Rust serial device bus (serdev) abstractions to the Linux kernel, authored by Markus Probst. The series provides:

1. Core serdev abstraction (`rust/kernel/serdev.rs`) with driver registration, device operations (baudrate, flow control, parity, read/write), and lifecycle management
2. A sample driver demonstrating usage
3. MAINTAINERS update adding Markus as serdev maintainer

**Overall assessment: Good quality, well-structured.** The abstraction follows established patterns from other Rust bus driver abstractions in the kernel. The safety comments are thorough. The use of a `Mutex<bool>` to guard against `receive_buf` accessing dropped driver data (the key fix in v10/v11) is a sound approach.

There are a few issues worth addressing, mostly around a typo, documentation correctness, an inconsistency in the sample's error handling, and a potential soundness concern in `write()`.

---

---
Generated by Claude Code Patch Reviewer

Claude review: rust: add basic serial device bus abstractions

[Claude Code Review Bot]

Patch Review

**Typo in `Parity` enum doc comment:**

```rust
/// Even partiy.
Even = bindings::serdev_parity_SERDEV_PARITY_EVEN,
```

Should be "Even parity." (typo: "partiy" → "parity").

**`Timeout::into_jiffies` returns `isize` but C functions take `long`:**

The `into_jiffies` method returns `isize`, which matches Rust's `c_long` on LP64 platforms. However, the `unwrap_or_default()` fallback for the `Jiffies` and `Milliseconds` variants returns 0 on overflow, which silently maps to `MAX_SCHEDULE_TIMEOUT` (wait forever). This is unlikely in practice but surprising — a caller providing a specific timeout would get an infinite wait if the conversion overflowed. Worth a comment or documenting the behavior.

**`write()` return type is `Result<u32>` but `write_buf` returns `int`:**

```rust
pub fn write(&self, data: &[u8]) -> Result<u32> {
    let ret =
        unsafe { bindings::serdev_device_write_buf(self.as_raw(), data.as_ptr(), data.len()) };
    to_result(ret as i32).map(|()| ret.unsigned_abs())
}
```

The C function `serdev_device_write_buf()` returns `int`. The `ret` here would be `c_int` (i32). Calling `ret.unsigned_abs()` on a `c_int` returns `u32`, which is fine. But `to_result(ret as i32)` followed by `ret.unsigned_abs()` — on success, `ret` could be 0 (zero bytes written), and `to_result(0)` returns `Ok(())`, so `unsigned_abs()` would return 0. That's correct. Looks sound.

**`write_all()` return type cast comment says "negative return values":**

```rust
// CAST: negative return values are guaranteed to be between `-MAX_ERRNO` and `-1`,
// which always fit into a `i32`.
to_result(ret as i32).map(|()| ret.unsigned_abs())
```

`serdev_device_write()` returns `ssize_t`. On success it returns a positive `size_t` value cast to `ssize_t`. The `ret as i32` cast is safe for error codes, but for large successful writes (> 2^31 bytes), truncating `ssize_t` to `i32` could wrap to negative and be incorrectly treated as an error. In practice, serial writes are small, but this is technically unsound for very large writes. Since serdev buffer sizes are realistically bounded well below `i32::MAX`, this is acceptable, but a defensive comment would be worthwhile.

**`Device<Bound>` methods but `probe` uses `Device<Core<'_>>`:**

`set_baudrate`, `set_flow_control`, `set_parity`, `write_all`, `write`, `write_flush`, `wait_until_sent` are all only available on `Device<Bound>`. But `probe` receives `Device<Core<'_>>`, and `Core` derefs to `Bound`, so the sample can call `sdev.set_baudrate(...)` through auto-deref. This is correct and follows the device context pattern.

**`receive_buf_callback` holding mutex across `T::receive`:**

```rust
let active = private_data.active.lock();
if !*active {
    return length;
}
// ...
T::receive(sdev, data_pinned, buf)
```

The lock on `active` is held for the entire duration of `T::receive`. This means `receive_buf` and `PinnedDrop` are mutually exclusive, which is the intended safety property. However, this means `T::receive` runs under a mutex — the C documentation says `receive_buf` "may sleep", so using a `Mutex` (which is sleepable in the kernel) is correct here. Good design.

**When `!*active`, returning `length` (consuming all data):**

```rust
if !*active {
    return length;
}
```

This silently discards all received data when the driver has been deactivated. This is reasonable — there's no meaningful consumer for the data anymore — but worth noting that the C serdev core will consider this data "consumed."

**Missing `[ ]` bracket in doc comment:**

```rust
/// Note that any accepted data has only been buffered by the controller. Use
/// [ Device::wait_until_sent`] to make sure the controller write buffer has actually been
```

The opening bracket is separated from the backtick: `[ Device::wait_until_sent`]` should be `[`Device::wait_until_sent`]`. This will produce a broken rustdoc link.

**`AlwaysRefCounted` `dec_ref` implementation:**

```rust
unsafe fn dec_ref(obj: NonNull<Self>) {
    unsafe { bindings::serdev_device_put(obj.cast().as_ptr()) }
}
```

This casts `NonNull<Device>` to `NonNull<bindings::serdev_device>` via `.cast()`. Since `Device` is `#[repr(transparent)]` wrapping `Opaque<bindings::serdev_device>`, the cast is valid.

**`Sync` for `Device<Bound>` but not for other contexts explicitly:**

```rust
unsafe impl Sync for Device {}
unsafe impl Sync for Device<device::Bound> {}
```

`Device` (i.e., `Device<Normal>`) and `Device<Bound>` both get `Sync`. `Device<Core<'_>>` and `Device<CoreInternal<'_>>` do not have explicit `Sync` impls. Since `Core` and `CoreInternal` are only used during probe/remove (single-threaded contexts), this seems intentional and correct.

**`probe_callback` acquires and dismisses `ScopeGuard` for cleanup:**

The probe error path is well structured:
1. `set_drvdata` installs `PrivateData`
2. `ScopeGuard` is created to clean up via `drvdata_obtain` if probe fails
3. If `T::probe` (the `__pinned_init` call) succeeds, `dismiss()` prevents cleanup
4. `*active = true` is set only on success

This correctly handles the memory leak fix mentioned in the v11 changelog.

---
Generated by Claude Code Patch Reviewer

Claude review: samples: rust: add Rust serial device bus sample device driver

[Claude Code Review Bot]

Patch Review

**Inconsistent error handling for `set_baudrate`:**

```rust
if sdev
    .set_baudrate(
        dev.fwnode()
            .and_then(|fwnode| fwnode.property_read(c"baudrate").optional())
            .unwrap_or(115200),
    )
    .is_err()
{
    return Err(EINVAL);
}
sdev.set_flow_control(false);
sdev.set_parity(serdev::Parity::None)?;
```

`set_baudrate` uses an explicit `if .is_err()` check and returns `Err(EINVAL)`, while `set_parity` uses `?` to propagate the error. This inconsistency is because `set_baudrate` returns `Result<(), u32>` (where the error value is the actual baudrate set, which can't use `?` with `Error`), but it's worth noting in a comment that the asymmetry is intentional. The `set_baudrate` API returns the actual rate set on failure rather than an error code — this is a reasonable Rust wrapper for the C semantics where `serdev_device_set_baudrate` returns the actual baudrate (0 if not set).

**The `receive` callback echoes all data back:**

```rust
fn receive<'bound>(
    sdev: &'bound serdev::Device<Bound>,
    _this: Pin<&Self>,
    data: &[u8],
) -> usize {
    let _ = sdev.write_all(data, serdev::Timeout::Max);
    data.len()
}
```

This is an echo server — fine for a sample. Ignoring the `write_all` error with `let _` is acceptable in a sample, though it always claims to have consumed all bytes regardless of whether the write succeeded. This is a reasonable simplification for demonstration purposes.

**`SampleDriver` stores `ARef<serdev::Device>`:**

```rust
struct SampleDriver {
    sdev: ARef<serdev::Device>,
}
```

This takes a reference-counted reference to the device in `probe`:
```rust
Ok(Self { sdev: sdev.into() })
```

This is fine — it stores a `Device<Normal>` (via the `impl_device_context_into_aref!` conversion) for use in `Drop` to print a debug message.

---
Generated by Claude Code Patch Reviewer

Claude review: MAINTAINERS: serdev: Add self for serdev

[Claude Code Review Bot]

Patch Review

Straightforward MAINTAINERS change replacing Rob Herring with Markus Probst as the serdev maintainer. Rob indicated he needs someone else to maintain serdev, and the linked lore messages confirm Markus volunteered. No issues.

**Summary:** The series is well-constructed with good attention to safety invariants. The key fixes are: (1) the typo "partiy" → "parity", and (2) the broken rustdoc link `[ Device::wait_until_sent`]`. The probe error-path handling looks correct after the v11 fixes. The mutex-guarded `active` flag is a clean solution for the receive-buf-after-drop race.

---
Generated by Claude Code Patch Reviewer

Claude review: samples: rust: add Rust serial device bus sample device driver

[Claude Code Review Bot]

Patch Review

The sample is clean and demonstrates the abstraction well.

```rust
+    fn receive<'bound>(
+        sdev: &'bound serdev::Device<Bound>,
+        _this: Pin<&Self>,
+        data: &[u8],
+    ) -> usize {
+        let _ = sdev.write_all(data, serdev::Timeout::Max);
+        data.len()
+    }
```

The echo-back in `receive` silently discards write errors with `let _ =`. For a sample this is acceptable, but it might be worth a brief comment or `dev_warn!` for educational purposes.

The `set_baudrate` error handling:

```rust
+        if sdev
+            .set_baudrate(...)
+            .is_err()
+        {
+            return Err(EINVAL);
+        }
```

This discards the actual baudrate returned in the `Err` variant. A more idiomatic approach would be to use `map_err` or log the actual baudrate.

---
Generated by Claude Code Patch Reviewer

Claude review: samples: rust: add Rust serial device bus sample device driver

[Claude Code Review Bot]

Patch Review

A clean sample driver demonstrating OF and ACPI device table matching, baudrate configuration from firmware properties, parity/flow control setup, and an echo-back receive callback.

**Probe error handling for `set_baudrate`:**
```rust
if sdev
    .set_baudrate(
        dev.fwnode()
            .and_then(|fwnode| fwnode.property_read(c"baudrate").optional())
            .unwrap_or(115200),
    )
    .is_err()
{
    return Err(EINVAL);
}
```
`set_baudrate` returns `Err(actual_baudrate)` when the hardware can't match the requested rate. Treating any mismatch as a hard error is fine for a sample, though real drivers may want to accept similar rates. The `if .is_err() { return Err(EINVAL) }` pattern could be simplified, but again, fine for illustration purposes.

**Receive callback silently discards write errors:**
```rust
fn receive<'bound>(
    sdev: &'bound serdev::Device<Bound>,
    _this: Pin<&Self>,
    data: &[u8],
) -> usize {
    let _ = sdev.write_all(data, serdev::Timeout::Max);
    data.len()
}
```
The `let _ =` discarding the `Result` is acceptable for a sample echo driver but would be a concern in production code. Worth a brief inline comment to note the intentional discard, since `let _ =` on a `Result` sometimes triggers lints.

**Kconfig entry:** Clean, correctly uses `select RUST_SERIAL_DEV_BUS_ABSTRACTIONS` to pull in the dependency.

---
Generated by Claude Code Patch Reviewer

Claude review: samples: rust: add Rust serial device bus sample device driver

[Claude Code Review Bot]

Patch Review

The sample is a simple echo driver — it reads a baudrate from device tree (defaulting to 115200), configures the port, and echoes back received data.

```rust
+    fn receive<'bound>(
+        sdev: &'bound serdev::Device<Bound>,
+        _this: Pin<&Self>,
+        data: &[u8],
+    ) -> usize {
+        let _ = sdev.write_all(data, serdev::Timeout::Max);
+        data.len()
+    }
```

The `let _ = ...` explicitly discards the write result, which is acceptable for a sample. The function always returns `data.len()` claiming all data was consumed regardless of write success — fine for demonstration purposes.

**Nit:** The `set_baudrate` error handling:
```rust
+        if sdev
+            .set_baudrate(
+                dev.fwnode()
+                    .and_then(|fwnode| fwnode.property_read(c"baudrate").optional())
+                    .unwrap_or(115200),
+            )
+            .is_err()
+        {
+            return Err(EINVAL);
```
The `Err(u32)` from `set_baudrate` (containing the actual baudrate that was set) is discarded. A `dev_warn!` logging the mismatch would be helpful for debugging, but not required for a sample.

The `// ` trailing comments on some import lines are unusual formatting artifacts (likely from rustfmt vertical formatting). Not a functional issue.

---

---
Generated by Claude Code Patch Reviewer