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

[Gary Guo]

On Wed Jul 15, 2026 at 10:00 PM BST, Markus Probst wrote:
> 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@xxxxxxxxx>
> ---
>  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           | 599 ++++++++++++++++++++++++++++++++++++++++
>  7 files changed, 634 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 806bd2d80d15..cd735d227892 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -24568,6 +24568,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@xxxxxxxx>
> 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 1124785e210b..fe7c505da236 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -85,6 +85,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 998e31052e66..6921a5ecb78a 100644
> --- a/rust/helpers/helpers.c
> +++ b/rust/helpers/helpers.c
> @@ -87,6 +87,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 9512af7156df..d5ca17336739 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -119,6 +119,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..0ffcef1849d2
> --- /dev/null
> +++ b/rust/kernel/serdev.rs
> @@ -0,0 +1,599 @@
> +// 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,
> +        }
> +    }
> +}

FWIW this shouldn't be part of serdev abstraction. See
https://lore.kernel.org/rust-for-linux/20260717042247.3634961-1-tomo@xxxxxxxxxxxx

> +
> +/// 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;
> +        }
> +        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
> +            // `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();

What's this for? This gives me a strong vibe that something is off. Either the
serdev subsystem is not having sufficient synchronization or there is an
abstraction issue.

Either way, you really should have more comments about what this active mutex is
for and why it is needed.

> +
> +            // 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::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();
> +
> +        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 + Sync + '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()
> +    }

This and many other functions should be `#[inline]`. I am surprised that Sashiko
didn't point out.

Best,
Gary