* Re: [PATCH RFC 2/4] rust: add basic serial device bus abstractions
2025-12-20 18:44 ` [PATCH RFC 2/4] rust: add basic serial device bus abstractions Markus Probst
@ 2025-12-21 9:19 ` Dirk Behme
2025-12-21 12:41 ` Markus Probst
2025-12-25 15:13 ` Kari Argillander
2025-12-26 15:09 ` Kari Argillander
2 siblings, 1 reply; 22+ messages in thread
From: Dirk Behme @ 2025-12-21 9:19 UTC (permalink / raw)
To: Markus Probst, Rob Herring, Greg Kroah-Hartman, Jiri Slaby,
Miguel Ojeda, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross,
Danilo Krummrich, Kari Argillander
Cc: linux-serial, linux-kernel, rust-for-linux
Hi Markus,
On 20.12.25 19:44, 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`.
>
> Co-developed-by: Kari Argillander <kari.argillander@gmail.com>
> Signed-off-by: Kari Argillander <kari.argillander@gmail.com>
> Signed-off-by: Markus Probst <markus.probst@posteo.de>
> ---
> 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 | 815 ++++++++++++++++++++++++++++++++++++++++
> 5 files changed, 841 insertions(+)
>
...
> diff --git a/rust/kernel/serdev.rs b/rust/kernel/serdev.rs
> new file mode 100644
> index 000000000000..0f5ef325a054
> --- /dev/null
> +++ b/rust/kernel/serdev.rs
....
> + /// 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 if interrupted).
Should it be "less than data.len"? Instead of just "data"? Same in the
comment for `write()`below.
> + /// [`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(),
> + )
> + };
> + if ret < 0 {
> + // CAST: negative return values are guaranteed to be between `-MAX_ERRNO` and `-1`,
> + // which always fit into a `i32`.
> + Err(Error::from_errno(ret as i32))
> + } else {
> + Ok(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 if interrupted).
> + /// [`kernel::error::code::ETIMEDOUT`] or [`kernel::error::code::ERESTARTSYS`] if interrupted
> + /// before any bytes were written.
> + 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()) };
> + if ret < 0 {
> + Err(Error::from_errno(ret))
> + } else {
> + Ok(ret.unsigned_abs())
> + }
> + }
Best regards
Dirk
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH RFC 2/4] rust: add basic serial device bus abstractions
2025-12-21 9:19 ` Dirk Behme
@ 2025-12-21 12:41 ` Markus Probst
0 siblings, 0 replies; 22+ messages in thread
From: Markus Probst @ 2025-12-21 12:41 UTC (permalink / raw)
To: Dirk Behme, Rob Herring, Greg Kroah-Hartman, Jiri Slaby,
Miguel Ojeda, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross,
Danilo Krummrich, Kari Argillander
Cc: linux-serial, linux-kernel, rust-for-linux
[-- Attachment #1: Type: text/plain, Size: 4397 bytes --]
Hi Dirk,
On Sun, 2025-12-21 at 10:19 +0100, Dirk Behme wrote:
> Hi Markus,
>
> On 20.12.25 19:44, 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`.
> >
> > Co-developed-by: Kari Argillander <kari.argillander@gmail.com>
> > Signed-off-by: Kari Argillander <kari.argillander@gmail.com>
> > Signed-off-by: Markus Probst <markus.probst@posteo.de>
> > ---
> > 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 | 815 ++++++++++++++++++++++++++++++++++++++++
> > 5 files changed, 841 insertions(+)
> >
> ...
> > diff --git a/rust/kernel/serdev.rs b/rust/kernel/serdev.rs
> > new file mode 100644
> > index 000000000000..0f5ef325a054
> > --- /dev/null
> > +++ b/rust/kernel/serdev.rs
> ....
> > + /// 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 if interrupted).
>
> Should it be "less than data.len"? Instead of just "data"? Same in the
> comment for `write()`below.
Yes.
>
>
> > + /// [`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(),
> > + )
> > + };
> > + if ret < 0 {
> > + // CAST: negative return values are guaranteed to be between `-MAX_ERRNO` and `-1`,
> > + // which always fit into a `i32`.
> > + Err(Error::from_errno(ret as i32))
> > + } else {
> > + Ok(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 if interrupted).
> > + /// [`kernel::error::code::ETIMEDOUT`] or [`kernel::error::code::ERESTARTSYS`] if interrupted
> > + /// before any bytes were written.
> > + 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()) };
> > + if ret < 0 {
> > + Err(Error::from_errno(ret))
> > + } else {
> > + Ok(ret.unsigned_abs())
> > + }
> > + }
>
> Best regards
>
> Dirk
Thanks
- Markus Probst
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 870 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH RFC 2/4] rust: add basic serial device bus abstractions
2025-12-20 18:44 ` [PATCH RFC 2/4] rust: add basic serial device bus abstractions Markus Probst
2025-12-21 9:19 ` Dirk Behme
@ 2025-12-25 15:13 ` Kari Argillander
2026-01-13 16:15 ` Markus Probst
2025-12-26 15:09 ` Kari Argillander
2 siblings, 1 reply; 22+ messages in thread
From: Kari Argillander @ 2025-12-25 15:13 UTC (permalink / raw)
To: Markus Probst
Cc: Rob Herring, Greg Kroah-Hartman, Jiri Slaby, Miguel Ojeda,
Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Alice Ryhl, Trevor Gross, Danilo Krummrich,
linux-serial, linux-kernel, rust-for-linux
On 20.12.2025 klo 20.44 Markus Probst (markus.probst@posteo.de) kirjoitti:
>
> 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`.
>
> Co-developed-by: Kari Argillander <kari.argillander@gmail.com>
> Signed-off-by: Kari Argillander <kari.argillander@gmail.com>
> Signed-off-by: Markus Probst <markus.probst@posteo.de>
> ---
> 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 | 815 ++++++++++++++++++++++++++++++++++++++++
> 5 files changed, 841 insertions(+)
>
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index a067038b4b42..bec6c6d0913a 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -79,6 +79,7 @@
> #include <linux/regulator/consumer.h>
> #include <linux/sched.h>
> #include <linux/security.h>
> +#include <linux/serdev.h>
> #include <linux/slab.h>
> #include <linux/task_work.h>
> #include <linux/tracepoint.h>
> diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
> index 79c72762ad9c..834e9fbb897d 100644
> --- a/rust/helpers/helpers.c
> +++ b/rust/helpers/helpers.c
> @@ -50,6 +50,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 f812cf120042..cc71195466b6 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -136,6 +136,8 @@
> pub mod scatterlist;
> pub mod security;
> pub mod seq_file;
> +#[cfg(CONFIG_SERIAL_DEV_BUS)]
> +pub mod serdev;
> pub mod sizes;
> pub mod slice;
> mod static_assert;
> diff --git a/rust/kernel/serdev.rs b/rust/kernel/serdev.rs
> new file mode 100644
> index 000000000000..0f5ef325a054
> --- /dev/null
> +++ b/rust/kernel/serdev.rs
> @@ -0,0 +1,815 @@
> +// 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,
> + container_of,
> + device,
> + driver,
> + error::{
> + from_result,
> + to_result,
> + VTABLE_DEFAULT_ERROR, //
> + },
> + of,
> + prelude::*,
> + time::Jiffies,
> + types::{AlwaysRefCounted, Opaque}, //
> +};
> +
> +use core::{
> + cell::UnsafeCell,
> + marker::PhantomData,
> + mem::offset_of,
> + num::NonZero,
> + ptr::{
> + addr_of_mut, //
> + 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`].
> + Value(NonZero<Jiffies>),
> + /// Wait as long as possible.
> + ///
> + /// This is equivalent to [`kernel::task::MAX_SCHEDULE_TIMEOUT`].
> + MaxScheduleTimeout,
> +}
> +
> +impl Timeout {
> + fn into_jiffies(self) -> isize {
> + match self {
> + Self::Value(value) => value.get().try_into().unwrap_or_default(),
> + Self::MaxScheduleTimeout => 0,
> + }
> + }
> +}
> +
> +/// An adapter for the registration of serial device bus device drivers.
> +pub struct Adapter<T: Driver>(T);
> +
> +// SAFETY: A call to `unregister` for a given instance of `RegType` is guaranteed to be valid if
> +// a preceding call to `register` has been successful.
> +unsafe impl<T: Driver + 'static> driver::RegistrationOps for Adapter<T> {
> + type RegType = bindings::serdev_device_driver;
> +
> + unsafe fn register(
> + sdrv: &Opaque<Self::RegType>,
> + 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 `RegType`.
> + to_result(unsafe { bindings::__serdev_device_driver_register(sdrv.get(), module.0) })
> + }
> +
> + unsafe fn unregister(sdrv: &Opaque<Self::RegType>) {
> + // SAFETY: `sdrv` is guaranteed to be a valid `RegType`.
> + unsafe { bindings::serdev_device_driver_unregister(sdrv.get()) };
> + }
> +}
> +
> +#[pin_data]
> +struct Drvdata<T: Driver + 'static> {
> + #[pin]
> + driver: T,
> + initial_data: UnsafeCell<Option<T::InitialData>>,
> + late_probe_data: UnsafeCell<Option<Pin<KBox<T::LateProbeData>>>>,
> +}
> +
> +impl<T: Driver + 'static> 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: if T::HAS_WRITE_WAKEUP {
> + Some(Self::write_wakeup_callback)
> + } else {
> + Some(bindings::serdev_device_write_wakeup)
> + },
> + };
> + const INITIAL_OPS: &'static bindings::serdev_device_ops = &bindings::serdev_device_ops {
> + receive_buf: Some(Self::initial_receive_buf_callback),
> + write_wakeup: if T::HAS_WRITE_WAKEUP_INITIAL {
> + Some(Self::initial_write_wakeup_callback)
> + } else {
> + Some(bindings::serdev_device_write_wakeup)
> + },
> + };
> + const NO_OPS: &'static bindings::serdev_device_ops = &bindings::serdev_device_ops {
> + receive_buf: 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(|| {
> + let data = try_pin_init!(Drvdata {
> + driver <- T::probe(sdev, info),
> + initial_data: Some(Default::default()).into(),
> + late_probe_data: None.into(),
> + });
> +
> + sdev.as_ref().set_drvdata(data)?;
> +
> + // SAFETY: We just set drvdata to `Drvdata<T>`.
> + let data = unsafe { sdev.as_ref().drvdata_borrow::<Drvdata<T>>() };
> +
> + // SAFETY: `sdev.as_raw()` is guaranteed to be a valid pointer to `serdev_device`.
> + unsafe { bindings::serdev_device_set_client_ops(sdev.as_raw(), Self::INITIAL_OPS) };
> +
> + // SAFETY: The serial device bus only ever calls the probe callback with a valid pointer
> + // to a `struct serdev_device`.
> + to_result(unsafe {
> + bindings::devm_serdev_device_open(sdev.as_ref().as_raw(), sdev.as_raw())
> + })?;
> +
> + // SAFETY: `&data.driver` is guaranteed to be pinned.
> + T::configure(sdev, unsafe { Pin::new_unchecked(&data.driver) }, info)?;
> +
> + if !T::HAS_RECEIVE_INITIAL {
> + // SAFETY:
> + // - It is guaranteed that we have exclusive access to `data.initial_data` and
> + // `data.late_probe_data`.
> + // - We just initialized `data.initial_data` with Some.
> + unsafe { Self::do_late_probe(sdev, data)? };
> + }
> +
> + Ok(0)
> + })
> + }
> +
> + /// # Safety
> + ///
> + /// The caller must guarantee, that we have exclusive access to `data.initial_data` and
> + /// `data.late_probe_data`. `data.initial_data` must be Some.
> + /// (i. e. `late_probe` has not been called yet).
> + unsafe fn do_late_probe(sdev: &Device<device::CoreInternal>, data: Pin<&Drvdata<T>>) -> Result {
> + // SAFETY: `&data.driver` is guaranteed to be pinned.
> + let data_driver = unsafe { Pin::new_unchecked(&data.driver) };
> +
> + // SAFETY: The function contract guarantees that we have exclusive access to
> + // `data.initial_data`.
> + let initial_data = unsafe { &mut *data.initial_data.get() };
> +
> + // SAFETY: The function contract guarantees that we have exclusive access to
> + // `data.late_probe_data`.
> + let late_probe_data = unsafe { &mut *data.late_probe_data.get() };
> +
> + *late_probe_data = Some(KBox::pin_init(
> + T::late_probe(
> + sdev,
> + data_driver,
> + // SAFETY: The function contract guarantees that `data.initial_data` is Some.
> + unsafe { initial_data.take().unwrap_unchecked() },
> + ),
> + GFP_KERNEL,
> + )?);
> + // 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) };
> + Ok(())
> + }
> +
> + 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<T>>`.
> + let data = unsafe { sdev.as_ref().drvdata_obtain::<Drvdata<T>>() };
> +
> + // SAFETY: `&data.driver` is guaranteed to be pinned.
> + let data_driver = unsafe { Pin::new_unchecked(&data.driver) };
> +
> + // SAFETY:
> + // - `data.late_probe_data` is guaranteed to be pinned.
> + // - It is guaranteed that we have exclusive access to `data.late_probe_data`.
> + let late_probe_data = unsafe {
> + (*data.late_probe_data.get())
> + .as_deref()
> + .map(|data| Pin::new_unchecked(data))
> + };
> +
> + T::unbind(sdev, data_driver, late_probe_data);
> + }
> +
> + 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<Drvdata<T>>>`.
> + let data = unsafe { sdev.as_ref().drvdata_borrow::<Drvdata<T>>() };
> +
> + // SAFETY: `buf` is guaranteed to be non-null and has the size of `length`.
> + let buf = unsafe { core::slice::from_raw_parts(buf, length) };
> +
> + // SAFETY: `&data.driver` is guaranteed to be pinned.
> + let data_driver = unsafe { Pin::new_unchecked(&data.driver) };
> +
> + // SAFETY:
> + // - `data.late_probe_data` is guaranteed to be Some.
> + // - `data.late_probe_data` is guaranteed to be pinned.
> + let late_probe_data = unsafe {
> + Pin::new_unchecked((*data.late_probe_data.get()).as_deref().unwrap_unchecked())
> + };
> +
> + T::receive(sdev, data_driver, late_probe_data, buf)
> + }
> +
> + extern "C" fn write_wakeup_callback(sdev: *mut bindings::serdev_device) {
> + // SAFETY: The serial device bus only ever calls the write wakeup callback with a valid
> + // pointer to a `struct serdev_device`.
> + //
> + // INVARIANT: `sdev` is valid for the duration of `write_wakeup_callback()`.
> + let sdev = unsafe { &*sdev.cast::<Device<device::CoreInternal>>() };
> +
> + // SAFETY: `write_wakeup_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<Drvdata<T>>>`.
> + let data = unsafe { sdev.as_ref().drvdata_borrow::<Drvdata<T>>() };
> +
> + // SAFETY: `sdev.as_raw()` is guaranteed to be a pointer to a valid `serdev_device`.
> + unsafe { bindings::serdev_device_write_wakeup(sdev.as_raw()) };
> +
> + // SAFETY: `&data.driver` is guaranteed to be pinned.
> + let data_driver = unsafe { Pin::new_unchecked(&data.driver) };
> +
> + // SAFETY:
> + // - `data.late_probe_data` is guaranteed to be Some.
> + // - `data.late_probe_data` is guaranteed to be pinned.
> + let late_probe_data = unsafe {
> + Pin::new_unchecked((*data.late_probe_data.get()).as_deref().unwrap_unchecked())
> + };
> +
> + // SAFETY: As long as the driver implementation meets the safety requirements, this call
> + // is safe.
> + unsafe { T::write_wakeup(sdev, data_driver, late_probe_data) };
> + }
> +
> + extern "C" fn initial_receive_buf_callback(
> + sdev: *mut bindings::serdev_device,
> + buf: *const u8,
> + length: usize,
> + ) -> usize {
> + if !T::HAS_RECEIVE_INITIAL {
> + return 0;
> + }
> +
> + // 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: `buf` is guaranteed to be non-null and has the size of `length`.
> + let buf = unsafe { core::slice::from_raw_parts(buf, length) };
> +
> + // SAFETY: `initial_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<Drvdata<T>>>`.
> + let data = unsafe { sdev.as_ref().drvdata_borrow::<Drvdata<T>>() };
> +
> + // SAFETY: `&data.driver` is guaranteed to be pinned.
> + let driver_data = unsafe { Pin::new_unchecked(&data.driver) };
> +
> + // SAFETY:
> + // - `data.initial_data` is always Some until `InitialReceiveResult::Ready` is
> + // returned below.
> + // - It is guaranteed that we have exclusive access to `data.initial_data`.
> + let initial_data = unsafe { (*data.initial_data.get()).as_mut().unwrap_unchecked() };
> +
> + match T::receive_initial(sdev, driver_data, initial_data, buf) {
> + Ok(InitialReceiveResult::Pending(size)) => size,
> + Ok(InitialReceiveResult::Ready(size)) => {
> + // SAFETY:
> + // - It is guaranteed that we have exclusive access to `data.initial_data` and
> + // `data.late_probe_data`.
> + // - We just initialized `data.initial_data` with Some.
> + if let Err(err) = unsafe { Self::do_late_probe(sdev, data) } {
> + dev_err!(sdev.as_ref(), "Unable to late probe device: {err:?}\n");
> + // 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::NO_OPS) };
> + return length;
> + }
> + size
> + }
> + Err(err) => {
> + dev_err!(
> + sdev.as_ref(),
> + "Unable to receive initial data for probe: {err:?}.\n"
> + );
> + // 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::NO_OPS) };
> + length
> + }
> + }
> + }
> +
> + extern "C" fn initial_write_wakeup_callback(sdev: *mut bindings::serdev_device) {
> + // SAFETY: The serial device bus only ever calls the write wakeup callback with a valid
> + // pointer to a `struct serdev_device`.
> + //
> + // INVARIANT: `sdev` is valid for the duration of `write_wakeup_callback()`.
> + let sdev = unsafe { &*sdev.cast::<Device<device::CoreInternal>>() };
> +
> + // SAFETY: `initial_write_wakeup_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<Drvdata<T>>>`.
> + let data = unsafe { sdev.as_ref().drvdata_borrow::<Drvdata<T>>() };
> +
> + // SAFETY: `sdev.as_raw()` is guaranteed to be a pointer to a valid `serdev_device`.
> + unsafe { bindings::serdev_device_write_wakeup(sdev.as_raw()) };
> +
> + // SAFETY: `&data.driver` is guaranteed to be pinned.
> + let data_driver = unsafe { Pin::new_unchecked(&data.driver) };
> +
> + // SAFETY: As long as the driver implementation meets the safety requirements, this call
> + // is safe.
> + unsafe { T::write_wakeup_initial(sdev, data_driver) };
> + }
> +}
> +
> +impl<T: Driver + 'static> 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)* });
> + };
> +}
> +
> +/// Result for `receive_initial` in [`Driver`].
> +pub enum InitialReceiveResult {
> + /// More data is required.
> + ///
> + /// The inner data is the number of bytes accepted.
> + Pending(usize),
> + /// Ready for late probe.
> + ///
> + /// The inner data is the number of bytes accepted.
> + Ready(usize),
> +}
> +
> +/// 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 LateProbeData = ();
> +/// type InitialData = ();
> +/// 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(
> +/// _sdev: &serdev::Device,
> +/// _id_info: Option<&Self::IdInfo>,
> +/// ) -> impl PinInit<Self, Error> {
> +/// Err(ENODEV)
> +/// }
> +///
> +/// fn configure(
> +/// dev: &serdev::Device<Core>,
> +/// _this: Pin<&Self>,
> +/// _id_info: Option<&Self::IdInfo>,
> +/// ) -> Result {
> +/// dev.set_baudrate(115200);
> +/// Ok(())
> +/// }
> +///
> +/// fn late_probe(
> +/// _dev: &serdev::Device<Bound>,
> +/// _this: Pin<&Self>,
> +/// _initial_data: Self::InitialData,
> +/// ) -> impl PinInit<Self::LateProbeData, Error> {
> +/// Ok(())
> +/// }
> +/// }
> +///```
> +#[vtable]
> +pub trait Driver: Send {
> + /// 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 LateProbeData: Send + 'static = ();
> + // type InitialData: Default + Send + 'static = ();
> + // ```
> + type IdInfo: 'static;
> +
> + /// Data returned in `late_probe` function.
> + type LateProbeData: Send + 'static;
> + /// Data used for initial data.
> + type InitialData: Default + Send + 'static;
> +
> + /// 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(sdev: &Device, id_info: Option<&Self::IdInfo>) -> impl PinInit<Self, Error>;
> +
> + /// Serial device bus device driver configure.
> + ///
> + /// Called directly after the serial device bus device has been opened.
> + /// This should be used for setting up the communication (i. e. baudrate, flow control, parity)
> + /// and sending an initial hello if required.
> + fn configure(
> + sdev: &Device<device::Core>,
> + this: Pin<&Self>,
> + id_info: Option<&Self::IdInfo>,
> + ) -> Result;
> +
> + /// Serial device bus device data receive callback (initial).
> + ///
> + /// Called when data got received from device, before `late_probe` has been called.
> + /// This should be used, to get information about the device for `late_probe`.
> + ///
> + /// Returns `InitialReceiveResult::Pending` if more data is still required for `late_probe`.
> + /// Otherwise `InitialReceiveResult::Ready`. The inner data is the number of bytes accepted.
> + fn receive_initial(
> + sdev: &Device<device::Bound>,
> + this: Pin<&Self>,
> + initial_data: &mut Self::InitialData,
> + data: &[u8],
> + ) -> Result<InitialReceiveResult> {
> + let _ = (sdev, this, initial_data, data);
> + build_error!(VTABLE_DEFAULT_ERROR)
> + }
> +
> + /// Serial device bus device write wakeup callback (initial).
> + ///
> + /// Called when ready to transmit more data, before `late_probe` has been called.
> + ///
> + /// # Safety
> + ///
> + /// This function must not sleep.
> + unsafe fn write_wakeup_initial(sdev: &Device<device::Core>, this: Pin<&Self>) {
> + let _ = (sdev, this);
> + build_error!(VTABLE_DEFAULT_ERROR)
> + }
> +
> + /// Serial device bus device late probe.
> + ///
> + /// Called after the initial data is available.
> + /// If `receive_initial` isn't implemented, this will be called directly after configure.
> + fn late_probe(
> + sdev: &Device<device::Bound>,
> + this: Pin<&Self>,
> + initial_data: Self::InitialData,
> + ) -> impl PinInit<Self::LateProbeData, Error>;
> +
> + /// 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 `Self::drop`.
> + fn unbind(
> + sdev: &Device<device::Core>,
> + this: Pin<&Self>,
> + late_probe_this: Option<Pin<&Self::LateProbeData>>,
> + ) {
> + let _ = (sdev, this, late_probe_this);
> + }
> +
> + /// Serial device bus device data receive callback.
> + ///
> + /// Called when data got received from device.
> + ///
> + /// Returns the number of bytes accepted.
> + fn receive(
> + sdev: &Device<device::Bound>,
> + this: Pin<&Self>,
> + late_probe_this: Pin<&Self::LateProbeData>,
> + data: &[u8],
> + ) -> usize {
> + let _ = (sdev, this, late_probe_this, data);
> + build_error!(VTABLE_DEFAULT_ERROR)
> + }
> +
> + /// Serial device bus device write wakeup callback.
> + ///
> + /// Called when ready to transmit more data.
> + ///
> + /// # Safety
> + ///
> + /// This function must not sleep.
> + unsafe fn write_wakeup(
> + sdev: &Device<device::Bound>,
> + this: Pin<&Self>,
> + late_probe_this: Pin<&Self::LateProbeData>,
> + ) {
> + let _ = (sdev, this, late_probe_this);
> + 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) -> u32 {
> + // SAFETY: `self.as_raw()` is guaranteed to be a pointer to a valid `serdev_device`.
> + unsafe { bindings::serdev_device_set_baudrate(self.as_raw(), speed) }
> + }
> +
> + /// 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 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(),
> + )
> + };
> + if ret < 0 {
> + // CAST: negative return values are guaranteed to be between `-MAX_ERRNO` and `-1`,
> + // which always fit into a `i32`.
> + Err(Error::from_errno(ret as i32))
> + } else {
> + Ok(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 if interrupted).
> + /// [`kernel::error::code::ETIMEDOUT`] or [`kernel::error::code::ERESTARTSYS`] if interrupted
> + /// before any bytes were written.
> + 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()) };
> + if ret < 0 {
> + Err(Error::from_errno(ret))
> + } else {
> + Ok(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 { addr_of_mut!((*self.as_raw()).dev) };
> +
> + // SAFETY: `dev` points to a valid `struct device`.
> + unsafe { device::Device::from_raw(dev) }
> + }
> +}
> +
> +impl<Ctx: device::DeviceContext> TryFrom<&device::Device<Ctx>> for &Device<Ctx> {
> + type Error = kernel::error::Error;
> +
> + fn try_from(dev: &device::Device<Ctx>) -> Result<Self, Self::Error> {
> + // SAFETY: By the type invariant of `Device`, `dev.as_raw()` is a valid pointer to a
> + // `struct device`.
> + if !unsafe { bindings::is_serdev_device(dev.as_raw()) } {
> + return Err(EINVAL);
> + }
> +
> + // SAFETY: We've just verified that the device_type of `dev` is
> + // `serdev_device_type`, hence `dev` must be embedded in a valid `struct
> + // serdev_device_driver` as guaranteed by the corresponding C code.
> + let sdev = unsafe { container_of!(dev.as_raw(), bindings::serdev_device, dev) };
> +
> + // SAFETY: `sdev` is a valid pointer to a `struct serdev_device_driver`.
> + Ok(unsafe { &*sdev.cast() })
> + }
> +}
> +
> +// 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 {}
>
> --
> 2.51.2
>
>
Currently I'm not a huge fan that we need to have configure(),
receive_initial(), write_wakeup_initial() and late_probe(). Also I do not
like that we hide hide open() completely from user. Maybe user want's to do
something if there is error. This does not feel very ergonomic or standard
kernel way. So after thinking a bit more about this idea I have come up with
a different approach.
We will have just probe() as usual. In probe() user will be forced to do open()
and finnish() on serdev device. Between open() and finnish() user can
configure the device and do initial reads / writes as needed. After probe is
finished we will switch to runtime ops.
This is not production ready code and I have just tested the idea locally
without kernel. Here is example how probe() could look like:
```rust
fn probe(sdev: &serdev::Device<Core>, _id_info:
Option<&Self::IdInfo>)-> impl PinInit<(Self, ProbeEndToken), Error> {
let sdev= sdev.open()?;
// After open we can configure and do initial reads / writes.
sdev.set_baudrate(9600);
// Here we can also write if needed.
for _ in 0..8 {
// For initial reads we will have iterator. This cannot be
done after finish().
let _ = sdev.next_byte(Duration::from_millis(300));
}
// We do finnish so user cannot store "initial sdev" anywhere. Also get
// end_token so probe cannot return unless open() and finnish() are called.
let (sdev, end_token) = sdev.finish();
Ok(try_pin_init!((Self {sdev}, end_token)))
}
```
then our receive_initial will look something like this. (Not runnable
on kernel).
Idea is that we have event for next_byte(). So initial read is little bit slower
but that should not be used so much that it matters in practice.
```rust
extern "C" fn initial_receive_buf(dev: *mut serdev_device, buf: *const
u8, count: usize) -> usize {
let abs = unsafe { &mut *((*dev).client_data as *mut Abstraction) };
let mut consumed = 0usize;
loop {
// Wait until we can either switch or deliver one byte (slot
must be empty).
let mut st = abs.st.lock().unwrap();
while !st.probe_finished && !(st.want_byte && st.slot.is_none()) {
st = abs.ev.wait(st).unwrap();
}
if st.probe_finished {
// TODO: Do we lock write lock so it is really safe to
change OPS here?
unsafe { bindings::serdev_device_set_client_ops(dev,
&RUNTIME_OPS) };
// Forward the *remaining* bytes that we have NOT taken
out of this buf.
let rem_ptr = unsafe { buf.add(consumed) };
let rem = count - consumed;
return consumed + Self::runtime_receive_buf(dev, rem_ptr, rem);
}
// Deliver exactly one byte to iterator (slot is empty here).
let b = unsafe { *buf.add(consumed) };
st.slot = Some(b);
// wake next_byte()
abs.ev.notify_all();
// Now WAIT until next_byte really consumes it (slot becomes None),
// or probe finishes (in which case we’ll flush it to runtime).
while !st.probe_finished && st.slot.is_some() {
st = abs.ev.wait(st).unwrap();
}
if st.probe_finished {
continue;
}
// Now it is truly consumed by next_byte().
consumed += 1;
if consumed == count {
return count;
}
}
}
```
Does anyone have opinions which is better or is there some other way
for this which
we have not yet discovered?
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH RFC 2/4] rust: add basic serial device bus abstractions
2025-12-25 15:13 ` Kari Argillander
@ 2026-01-13 16:15 ` Markus Probst
2026-01-13 17:37 ` Danilo Krummrich
0 siblings, 1 reply; 22+ messages in thread
From: Markus Probst @ 2026-01-13 16:15 UTC (permalink / raw)
To: Kari Argillander
Cc: Rob Herring, Greg Kroah-Hartman, Jiri Slaby, Miguel Ojeda,
Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Alice Ryhl, Trevor Gross, Danilo Krummrich,
linux-serial, linux-kernel, rust-for-linux
[-- Attachment #1: Type: text/plain, Size: 43638 bytes --]
On Thu, 2025-12-25 at 17:13 +0200, Kari Argillander wrote:
> On 20.12.2025 klo 20.44 Markus Probst (markus.probst@posteo.de) kirjoitti:
> >
> > 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`.
> >
> > Co-developed-by: Kari Argillander <kari.argillander@gmail.com>
> > Signed-off-by: Kari Argillander <kari.argillander@gmail.com>
> > Signed-off-by: Markus Probst <markus.probst@posteo.de>
> > ---
> > 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 | 815 ++++++++++++++++++++++++++++++++++++++++
> > 5 files changed, 841 insertions(+)
> >
> > diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> > index a067038b4b42..bec6c6d0913a 100644
> > --- a/rust/bindings/bindings_helper.h
> > +++ b/rust/bindings/bindings_helper.h
> > @@ -79,6 +79,7 @@
> > #include <linux/regulator/consumer.h>
> > #include <linux/sched.h>
> > #include <linux/security.h>
> > +#include <linux/serdev.h>
> > #include <linux/slab.h>
> > #include <linux/task_work.h>
> > #include <linux/tracepoint.h>
> > diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
> > index 79c72762ad9c..834e9fbb897d 100644
> > --- a/rust/helpers/helpers.c
> > +++ b/rust/helpers/helpers.c
> > @@ -50,6 +50,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 f812cf120042..cc71195466b6 100644
> > --- a/rust/kernel/lib.rs
> > +++ b/rust/kernel/lib.rs
> > @@ -136,6 +136,8 @@
> > pub mod scatterlist;
> > pub mod security;
> > pub mod seq_file;
> > +#[cfg(CONFIG_SERIAL_DEV_BUS)]
> > +pub mod serdev;
> > pub mod sizes;
> > pub mod slice;
> > mod static_assert;
> > diff --git a/rust/kernel/serdev.rs b/rust/kernel/serdev.rs
> > new file mode 100644
> > index 000000000000..0f5ef325a054
> > --- /dev/null
> > +++ b/rust/kernel/serdev.rs
> > @@ -0,0 +1,815 @@
> > +// 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,
> > + container_of,
> > + device,
> > + driver,
> > + error::{
> > + from_result,
> > + to_result,
> > + VTABLE_DEFAULT_ERROR, //
> > + },
> > + of,
> > + prelude::*,
> > + time::Jiffies,
> > + types::{AlwaysRefCounted, Opaque}, //
> > +};
> > +
> > +use core::{
> > + cell::UnsafeCell,
> > + marker::PhantomData,
> > + mem::offset_of,
> > + num::NonZero,
> > + ptr::{
> > + addr_of_mut, //
> > + 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`].
> > + Value(NonZero<Jiffies>),
> > + /// Wait as long as possible.
> > + ///
> > + /// This is equivalent to [`kernel::task::MAX_SCHEDULE_TIMEOUT`].
> > + MaxScheduleTimeout,
> > +}
> > +
> > +impl Timeout {
> > + fn into_jiffies(self) -> isize {
> > + match self {
> > + Self::Value(value) => value.get().try_into().unwrap_or_default(),
> > + Self::MaxScheduleTimeout => 0,
> > + }
> > + }
> > +}
> > +
> > +/// An adapter for the registration of serial device bus device drivers.
> > +pub struct Adapter<T: Driver>(T);
> > +
> > +// SAFETY: A call to `unregister` for a given instance of `RegType` is guaranteed to be valid if
> > +// a preceding call to `register` has been successful.
> > +unsafe impl<T: Driver + 'static> driver::RegistrationOps for Adapter<T> {
> > + type RegType = bindings::serdev_device_driver;
> > +
> > + unsafe fn register(
> > + sdrv: &Opaque<Self::RegType>,
> > + 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 `RegType`.
> > + to_result(unsafe { bindings::__serdev_device_driver_register(sdrv.get(), module.0) })
> > + }
> > +
> > + unsafe fn unregister(sdrv: &Opaque<Self::RegType>) {
> > + // SAFETY: `sdrv` is guaranteed to be a valid `RegType`.
> > + unsafe { bindings::serdev_device_driver_unregister(sdrv.get()) };
> > + }
> > +}
> > +
> > +#[pin_data]
> > +struct Drvdata<T: Driver + 'static> {
> > + #[pin]
> > + driver: T,
> > + initial_data: UnsafeCell<Option<T::InitialData>>,
> > + late_probe_data: UnsafeCell<Option<Pin<KBox<T::LateProbeData>>>>,
> > +}
> > +
> > +impl<T: Driver + 'static> 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: if T::HAS_WRITE_WAKEUP {
> > + Some(Self::write_wakeup_callback)
> > + } else {
> > + Some(bindings::serdev_device_write_wakeup)
> > + },
> > + };
> > + const INITIAL_OPS: &'static bindings::serdev_device_ops = &bindings::serdev_device_ops {
> > + receive_buf: Some(Self::initial_receive_buf_callback),
> > + write_wakeup: if T::HAS_WRITE_WAKEUP_INITIAL {
> > + Some(Self::initial_write_wakeup_callback)
> > + } else {
> > + Some(bindings::serdev_device_write_wakeup)
> > + },
> > + };
> > + const NO_OPS: &'static bindings::serdev_device_ops = &bindings::serdev_device_ops {
> > + receive_buf: 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(|| {
> > + let data = try_pin_init!(Drvdata {
> > + driver <- T::probe(sdev, info),
> > + initial_data: Some(Default::default()).into(),
> > + late_probe_data: None.into(),
> > + });
> > +
> > + sdev.as_ref().set_drvdata(data)?;
> > +
> > + // SAFETY: We just set drvdata to `Drvdata<T>`.
> > + let data = unsafe { sdev.as_ref().drvdata_borrow::<Drvdata<T>>() };
> > +
> > + // SAFETY: `sdev.as_raw()` is guaranteed to be a valid pointer to `serdev_device`.
> > + unsafe { bindings::serdev_device_set_client_ops(sdev.as_raw(), Self::INITIAL_OPS) };
> > +
> > + // SAFETY: The serial device bus only ever calls the probe callback with a valid pointer
> > + // to a `struct serdev_device`.
> > + to_result(unsafe {
> > + bindings::devm_serdev_device_open(sdev.as_ref().as_raw(), sdev.as_raw())
> > + })?;
> > +
> > + // SAFETY: `&data.driver` is guaranteed to be pinned.
> > + T::configure(sdev, unsafe { Pin::new_unchecked(&data.driver) }, info)?;
> > +
> > + if !T::HAS_RECEIVE_INITIAL {
> > + // SAFETY:
> > + // - It is guaranteed that we have exclusive access to `data.initial_data` and
> > + // `data.late_probe_data`.
> > + // - We just initialized `data.initial_data` with Some.
> > + unsafe { Self::do_late_probe(sdev, data)? };
> > + }
> > +
> > + Ok(0)
> > + })
> > + }
> > +
> > + /// # Safety
> > + ///
> > + /// The caller must guarantee, that we have exclusive access to `data.initial_data` and
> > + /// `data.late_probe_data`. `data.initial_data` must be Some.
> > + /// (i. e. `late_probe` has not been called yet).
> > + unsafe fn do_late_probe(sdev: &Device<device::CoreInternal>, data: Pin<&Drvdata<T>>) -> Result {
> > + // SAFETY: `&data.driver` is guaranteed to be pinned.
> > + let data_driver = unsafe { Pin::new_unchecked(&data.driver) };
> > +
> > + // SAFETY: The function contract guarantees that we have exclusive access to
> > + // `data.initial_data`.
> > + let initial_data = unsafe { &mut *data.initial_data.get() };
> > +
> > + // SAFETY: The function contract guarantees that we have exclusive access to
> > + // `data.late_probe_data`.
> > + let late_probe_data = unsafe { &mut *data.late_probe_data.get() };
> > +
> > + *late_probe_data = Some(KBox::pin_init(
> > + T::late_probe(
> > + sdev,
> > + data_driver,
> > + // SAFETY: The function contract guarantees that `data.initial_data` is Some.
> > + unsafe { initial_data.take().unwrap_unchecked() },
> > + ),
> > + GFP_KERNEL,
> > + )?);
> > + // 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) };
> > + Ok(())
> > + }
> > +
> > + 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<T>>`.
> > + let data = unsafe { sdev.as_ref().drvdata_obtain::<Drvdata<T>>() };
> > +
> > + // SAFETY: `&data.driver` is guaranteed to be pinned.
> > + let data_driver = unsafe { Pin::new_unchecked(&data.driver) };
> > +
> > + // SAFETY:
> > + // - `data.late_probe_data` is guaranteed to be pinned.
> > + // - It is guaranteed that we have exclusive access to `data.late_probe_data`.
> > + let late_probe_data = unsafe {
> > + (*data.late_probe_data.get())
> > + .as_deref()
> > + .map(|data| Pin::new_unchecked(data))
> > + };
> > +
> > + T::unbind(sdev, data_driver, late_probe_data);
> > + }
> > +
> > + 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<Drvdata<T>>>`.
> > + let data = unsafe { sdev.as_ref().drvdata_borrow::<Drvdata<T>>() };
> > +
> > + // SAFETY: `buf` is guaranteed to be non-null and has the size of `length`.
> > + let buf = unsafe { core::slice::from_raw_parts(buf, length) };
> > +
> > + // SAFETY: `&data.driver` is guaranteed to be pinned.
> > + let data_driver = unsafe { Pin::new_unchecked(&data.driver) };
> > +
> > + // SAFETY:
> > + // - `data.late_probe_data` is guaranteed to be Some.
> > + // - `data.late_probe_data` is guaranteed to be pinned.
> > + let late_probe_data = unsafe {
> > + Pin::new_unchecked((*data.late_probe_data.get()).as_deref().unwrap_unchecked())
> > + };
> > +
> > + T::receive(sdev, data_driver, late_probe_data, buf)
> > + }
> > +
> > + extern "C" fn write_wakeup_callback(sdev: *mut bindings::serdev_device) {
> > + // SAFETY: The serial device bus only ever calls the write wakeup callback with a valid
> > + // pointer to a `struct serdev_device`.
> > + //
> > + // INVARIANT: `sdev` is valid for the duration of `write_wakeup_callback()`.
> > + let sdev = unsafe { &*sdev.cast::<Device<device::CoreInternal>>() };
> > +
> > + // SAFETY: `write_wakeup_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<Drvdata<T>>>`.
> > + let data = unsafe { sdev.as_ref().drvdata_borrow::<Drvdata<T>>() };
> > +
> > + // SAFETY: `sdev.as_raw()` is guaranteed to be a pointer to a valid `serdev_device`.
> > + unsafe { bindings::serdev_device_write_wakeup(sdev.as_raw()) };
> > +
> > + // SAFETY: `&data.driver` is guaranteed to be pinned.
> > + let data_driver = unsafe { Pin::new_unchecked(&data.driver) };
> > +
> > + // SAFETY:
> > + // - `data.late_probe_data` is guaranteed to be Some.
> > + // - `data.late_probe_data` is guaranteed to be pinned.
> > + let late_probe_data = unsafe {
> > + Pin::new_unchecked((*data.late_probe_data.get()).as_deref().unwrap_unchecked())
> > + };
> > +
> > + // SAFETY: As long as the driver implementation meets the safety requirements, this call
> > + // is safe.
> > + unsafe { T::write_wakeup(sdev, data_driver, late_probe_data) };
> > + }
> > +
> > + extern "C" fn initial_receive_buf_callback(
> > + sdev: *mut bindings::serdev_device,
> > + buf: *const u8,
> > + length: usize,
> > + ) -> usize {
> > + if !T::HAS_RECEIVE_INITIAL {
> > + return 0;
> > + }
> > +
> > + // 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: `buf` is guaranteed to be non-null and has the size of `length`.
> > + let buf = unsafe { core::slice::from_raw_parts(buf, length) };
> > +
> > + // SAFETY: `initial_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<Drvdata<T>>>`.
> > + let data = unsafe { sdev.as_ref().drvdata_borrow::<Drvdata<T>>() };
> > +
> > + // SAFETY: `&data.driver` is guaranteed to be pinned.
> > + let driver_data = unsafe { Pin::new_unchecked(&data.driver) };
> > +
> > + // SAFETY:
> > + // - `data.initial_data` is always Some until `InitialReceiveResult::Ready` is
> > + // returned below.
> > + // - It is guaranteed that we have exclusive access to `data.initial_data`.
> > + let initial_data = unsafe { (*data.initial_data.get()).as_mut().unwrap_unchecked() };
> > +
> > + match T::receive_initial(sdev, driver_data, initial_data, buf) {
> > + Ok(InitialReceiveResult::Pending(size)) => size,
> > + Ok(InitialReceiveResult::Ready(size)) => {
> > + // SAFETY:
> > + // - It is guaranteed that we have exclusive access to `data.initial_data` and
> > + // `data.late_probe_data`.
> > + // - We just initialized `data.initial_data` with Some.
> > + if let Err(err) = unsafe { Self::do_late_probe(sdev, data) } {
> > + dev_err!(sdev.as_ref(), "Unable to late probe device: {err:?}\n");
> > + // 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::NO_OPS) };
> > + return length;
> > + }
> > + size
> > + }
> > + Err(err) => {
> > + dev_err!(
> > + sdev.as_ref(),
> > + "Unable to receive initial data for probe: {err:?}.\n"
> > + );
> > + // 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::NO_OPS) };
> > + length
> > + }
> > + }
> > + }
> > +
> > + extern "C" fn initial_write_wakeup_callback(sdev: *mut bindings::serdev_device) {
> > + // SAFETY: The serial device bus only ever calls the write wakeup callback with a valid
> > + // pointer to a `struct serdev_device`.
> > + //
> > + // INVARIANT: `sdev` is valid for the duration of `write_wakeup_callback()`.
> > + let sdev = unsafe { &*sdev.cast::<Device<device::CoreInternal>>() };
> > +
> > + // SAFETY: `initial_write_wakeup_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<Drvdata<T>>>`.
> > + let data = unsafe { sdev.as_ref().drvdata_borrow::<Drvdata<T>>() };
> > +
> > + // SAFETY: `sdev.as_raw()` is guaranteed to be a pointer to a valid `serdev_device`.
> > + unsafe { bindings::serdev_device_write_wakeup(sdev.as_raw()) };
> > +
> > + // SAFETY: `&data.driver` is guaranteed to be pinned.
> > + let data_driver = unsafe { Pin::new_unchecked(&data.driver) };
> > +
> > + // SAFETY: As long as the driver implementation meets the safety requirements, this call
> > + // is safe.
> > + unsafe { T::write_wakeup_initial(sdev, data_driver) };
> > + }
> > +}
> > +
> > +impl<T: Driver + 'static> 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)* });
> > + };
> > +}
> > +
> > +/// Result for `receive_initial` in [`Driver`].
> > +pub enum InitialReceiveResult {
> > + /// More data is required.
> > + ///
> > + /// The inner data is the number of bytes accepted.
> > + Pending(usize),
> > + /// Ready for late probe.
> > + ///
> > + /// The inner data is the number of bytes accepted.
> > + Ready(usize),
> > +}
> > +
> > +/// 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 LateProbeData = ();
> > +/// type InitialData = ();
> > +/// 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(
> > +/// _sdev: &serdev::Device,
> > +/// _id_info: Option<&Self::IdInfo>,
> > +/// ) -> impl PinInit<Self, Error> {
> > +/// Err(ENODEV)
> > +/// }
> > +///
> > +/// fn configure(
> > +/// dev: &serdev::Device<Core>,
> > +/// _this: Pin<&Self>,
> > +/// _id_info: Option<&Self::IdInfo>,
> > +/// ) -> Result {
> > +/// dev.set_baudrate(115200);
> > +/// Ok(())
> > +/// }
> > +///
> > +/// fn late_probe(
> > +/// _dev: &serdev::Device<Bound>,
> > +/// _this: Pin<&Self>,
> > +/// _initial_data: Self::InitialData,
> > +/// ) -> impl PinInit<Self::LateProbeData, Error> {
> > +/// Ok(())
> > +/// }
> > +/// }
> > +///```
> > +#[vtable]
> > +pub trait Driver: Send {
> > + /// 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 LateProbeData: Send + 'static = ();
> > + // type InitialData: Default + Send + 'static = ();
> > + // ```
> > + type IdInfo: 'static;
> > +
> > + /// Data returned in `late_probe` function.
> > + type LateProbeData: Send + 'static;
> > + /// Data used for initial data.
> > + type InitialData: Default + Send + 'static;
> > +
> > + /// 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(sdev: &Device, id_info: Option<&Self::IdInfo>) -> impl PinInit<Self, Error>;
> > +
> > + /// Serial device bus device driver configure.
> > + ///
> > + /// Called directly after the serial device bus device has been opened.
> > + /// This should be used for setting up the communication (i. e. baudrate, flow control, parity)
> > + /// and sending an initial hello if required.
> > + fn configure(
> > + sdev: &Device<device::Core>,
> > + this: Pin<&Self>,
> > + id_info: Option<&Self::IdInfo>,
> > + ) -> Result;
> > +
> > + /// Serial device bus device data receive callback (initial).
> > + ///
> > + /// Called when data got received from device, before `late_probe` has been called.
> > + /// This should be used, to get information about the device for `late_probe`.
> > + ///
> > + /// Returns `InitialReceiveResult::Pending` if more data is still required for `late_probe`.
> > + /// Otherwise `InitialReceiveResult::Ready`. The inner data is the number of bytes accepted.
> > + fn receive_initial(
> > + sdev: &Device<device::Bound>,
> > + this: Pin<&Self>,
> > + initial_data: &mut Self::InitialData,
> > + data: &[u8],
> > + ) -> Result<InitialReceiveResult> {
> > + let _ = (sdev, this, initial_data, data);
> > + build_error!(VTABLE_DEFAULT_ERROR)
> > + }
> > +
> > + /// Serial device bus device write wakeup callback (initial).
> > + ///
> > + /// Called when ready to transmit more data, before `late_probe` has been called.
> > + ///
> > + /// # Safety
> > + ///
> > + /// This function must not sleep.
> > + unsafe fn write_wakeup_initial(sdev: &Device<device::Core>, this: Pin<&Self>) {
> > + let _ = (sdev, this);
> > + build_error!(VTABLE_DEFAULT_ERROR)
> > + }
> > +
> > + /// Serial device bus device late probe.
> > + ///
> > + /// Called after the initial data is available.
> > + /// If `receive_initial` isn't implemented, this will be called directly after configure.
> > + fn late_probe(
> > + sdev: &Device<device::Bound>,
> > + this: Pin<&Self>,
> > + initial_data: Self::InitialData,
> > + ) -> impl PinInit<Self::LateProbeData, Error>;
> > +
> > + /// 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 `Self::drop`.
> > + fn unbind(
> > + sdev: &Device<device::Core>,
> > + this: Pin<&Self>,
> > + late_probe_this: Option<Pin<&Self::LateProbeData>>,
> > + ) {
> > + let _ = (sdev, this, late_probe_this);
> > + }
> > +
> > + /// Serial device bus device data receive callback.
> > + ///
> > + /// Called when data got received from device.
> > + ///
> > + /// Returns the number of bytes accepted.
> > + fn receive(
> > + sdev: &Device<device::Bound>,
> > + this: Pin<&Self>,
> > + late_probe_this: Pin<&Self::LateProbeData>,
> > + data: &[u8],
> > + ) -> usize {
> > + let _ = (sdev, this, late_probe_this, data);
> > + build_error!(VTABLE_DEFAULT_ERROR)
> > + }
> > +
> > + /// Serial device bus device write wakeup callback.
> > + ///
> > + /// Called when ready to transmit more data.
> > + ///
> > + /// # Safety
> > + ///
> > + /// This function must not sleep.
> > + unsafe fn write_wakeup(
> > + sdev: &Device<device::Bound>,
> > + this: Pin<&Self>,
> > + late_probe_this: Pin<&Self::LateProbeData>,
> > + ) {
> > + let _ = (sdev, this, late_probe_this);
> > + 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) -> u32 {
> > + // SAFETY: `self.as_raw()` is guaranteed to be a pointer to a valid `serdev_device`.
> > + unsafe { bindings::serdev_device_set_baudrate(self.as_raw(), speed) }
> > + }
> > +
> > + /// 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 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(),
> > + )
> > + };
> > + if ret < 0 {
> > + // CAST: negative return values are guaranteed to be between `-MAX_ERRNO` and `-1`,
> > + // which always fit into a `i32`.
> > + Err(Error::from_errno(ret as i32))
> > + } else {
> > + Ok(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 if interrupted).
> > + /// [`kernel::error::code::ETIMEDOUT`] or [`kernel::error::code::ERESTARTSYS`] if interrupted
> > + /// before any bytes were written.
> > + 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()) };
> > + if ret < 0 {
> > + Err(Error::from_errno(ret))
> > + } else {
> > + Ok(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 { addr_of_mut!((*self.as_raw()).dev) };
> > +
> > + // SAFETY: `dev` points to a valid `struct device`.
> > + unsafe { device::Device::from_raw(dev) }
> > + }
> > +}
> > +
> > +impl<Ctx: device::DeviceContext> TryFrom<&device::Device<Ctx>> for &Device<Ctx> {
> > + type Error = kernel::error::Error;
> > +
> > + fn try_from(dev: &device::Device<Ctx>) -> Result<Self, Self::Error> {
> > + // SAFETY: By the type invariant of `Device`, `dev.as_raw()` is a valid pointer to a
> > + // `struct device`.
> > + if !unsafe { bindings::is_serdev_device(dev.as_raw()) } {
> > + return Err(EINVAL);
> > + }
> > +
> > + // SAFETY: We've just verified that the device_type of `dev` is
> > + // `serdev_device_type`, hence `dev` must be embedded in a valid `struct
> > + // serdev_device_driver` as guaranteed by the corresponding C code.
> > + let sdev = unsafe { container_of!(dev.as_raw(), bindings::serdev_device, dev) };
> > +
> > + // SAFETY: `sdev` is a valid pointer to a `struct serdev_device_driver`.
> > + Ok(unsafe { &*sdev.cast() })
> > + }
> > +}
> > +
> > +// 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 {}
> >
> > --
> > 2.51.2
> >
> >
>
> Currently I'm not a huge fan that we need to have configure(),
> receive_initial(), write_wakeup_initial() and late_probe(). Also I do not
> like that we hide hide open() completely from user. Maybe user want's to do
> something if there is error. This does not feel very ergonomic or standard
> kernel way. So after thinking a bit more about this idea I have come up with
> a different approach.
>
> We will have just probe() as usual. In probe() user will be forced to do open()
> and finnish() on serdev device. Between open() and finnish() user can
> configure the device and do initial reads / writes as needed. After probe is
> finished we will switch to runtime ops.
>
> This is not production ready code and I have just tested the idea locally
> without kernel. Here is example how probe() could look like:
>
> ```rust
> fn probe(sdev: &serdev::Device<Core>, _id_info:
> Option<&Self::IdInfo>)-> impl PinInit<(Self, ProbeEndToken), Error> {
> let sdev= sdev.open()?;
> // After open we can configure and do initial reads / writes.
> sdev.set_baudrate(9600);
>
> // Here we can also write if needed.
>
> for _ in 0..8 {
> // For initial reads we will have iterator. This cannot be
> done after finish().
> let _ = sdev.next_byte(Duration::from_millis(300));
> }
>
> // We do finnish so user cannot store "initial sdev" anywhere. Also get
> // end_token so probe cannot return unless open() and finnish() are called.
> let (sdev, end_token) = sdev.finish();
> Ok(try_pin_init!((Self {sdev}, end_token)))
> }
> ```
>
> then our receive_initial will look something like this. (Not runnable
> on kernel).
> Idea is that we have event for next_byte(). So initial read is little bit slower
> but that should not be used so much that it matters in practice.
>
> ```rust
> extern "C" fn initial_receive_buf(dev: *mut serdev_device, buf: *const
> u8, count: usize) -> usize {
> let abs = unsafe { &mut *((*dev).client_data as *mut Abstraction) };
>
> let mut consumed = 0usize;
>
> loop {
> // Wait until we can either switch or deliver one byte (slot
> must be empty).
> let mut st = abs.st.lock().unwrap();
> while !st.probe_finished && !(st.want_byte && st.slot.is_none()) {
> st = abs.ev.wait(st).unwrap();
> }
>
> if st.probe_finished {
> // TODO: Do we lock write lock so it is really safe to
> change OPS here?
> unsafe { bindings::serdev_device_set_client_ops(dev,
> &RUNTIME_OPS) };
>
> // Forward the *remaining* bytes that we have NOT taken
> out of this buf.
> let rem_ptr = unsafe { buf.add(consumed) };
> let rem = count - consumed;
> return consumed + Self::runtime_receive_buf(dev, rem_ptr, rem);
> }
>
> // Deliver exactly one byte to iterator (slot is empty here).
> let b = unsafe { *buf.add(consumed) };
> st.slot = Some(b);
> // wake next_byte()
> abs.ev.notify_all();
>
> // Now WAIT until next_byte really consumes it (slot becomes None),
> // or probe finishes (in which case we’ll flush it to runtime).
> while !st.probe_finished && st.slot.is_some() {
> st = abs.ev.wait(st).unwrap();
> }
> if st.probe_finished {
> continue;
> }
>
> // Now it is truly consumed by next_byte().
> consumed += 1;
> if consumed == count {
> return count;
> }
> }
> }
> ```
>
> Does anyone have opinions which is better or is there some other way
> for this which
> we have not yet discovered?
I am not sure if using this approach is intended by the serdev
subsystem. Otherwise I assume there already would be a C function in
the subsystem for reading and waiting for data on probe.
There currently is only one serial device bus driver in the kernel,
which needs initial data:
drivers/bluetooth/hci_uart.h
drivers/bluetooth/hci_ldisc.c
drivers/bluetooth/hci_serdev.c
This driver retrieves this initial data after probe (not in the probe)
and then initializes it with a workqueue. Given it is part of the
kernel, I assume this is the intended behaviour.
@Danilo: Can you share your opinion on this?
Thanks
- Markus Probst
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 870 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH RFC 2/4] rust: add basic serial device bus abstractions
2026-01-13 16:15 ` Markus Probst
@ 2026-01-13 17:37 ` Danilo Krummrich
2026-01-13 17:59 ` Markus Probst
2026-02-08 14:30 ` Markus Probst
0 siblings, 2 replies; 22+ messages in thread
From: Danilo Krummrich @ 2026-01-13 17:37 UTC (permalink / raw)
To: Markus Probst
Cc: Kari Argillander, Rob Herring, Greg Kroah-Hartman, Jiri Slaby,
Miguel Ojeda, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross,
linux-serial, linux-kernel, rust-for-linux
On Tue Jan 13, 2026 at 5:15 PM CET, Markus Probst wrote:
>> > +impl<T: Driver + 'static> 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: if T::HAS_WRITE_WAKEUP {
>> > + Some(Self::write_wakeup_callback)
>> > + } else {
>> > + Some(bindings::serdev_device_write_wakeup)
>> > + },
>> > + };
>> > + const INITIAL_OPS: &'static bindings::serdev_device_ops = &bindings::serdev_device_ops {
>> > + receive_buf: Some(Self::initial_receive_buf_callback),
>> > + write_wakeup: if T::HAS_WRITE_WAKEUP_INITIAL {
>> > + Some(Self::initial_write_wakeup_callback)
>> > + } else {
>> > + Some(bindings::serdev_device_write_wakeup)
>> > + },
>> > + };
>> > + const NO_OPS: &'static bindings::serdev_device_ops = &bindings::serdev_device_ops {
>> > + receive_buf: 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(|| {
>> > + let data = try_pin_init!(Drvdata {
>> > + driver <- T::probe(sdev, info),
>> > + initial_data: Some(Default::default()).into(),
>> > + late_probe_data: None.into(),
>> > + });
>> > +
>> > + sdev.as_ref().set_drvdata(data)?;
This does not work, a driver can obtain its device private data with
Device::<Bound>::drvdata() [1].
For this the driver must assert the correct type, but since you use a private
type instead of the type given by the driver, i.e. T, Device::<Bound>::drvdata()
will always fail for the driver.
[1] https://rust.docs.kernel.org/kernel/device/struct.Device.html#method.drvdata
>> > +
>> > + // SAFETY: We just set drvdata to `Drvdata<T>`.
>> > + let data = unsafe { sdev.as_ref().drvdata_borrow::<Drvdata<T>>() };
>> > +
>> > + // SAFETY: `sdev.as_raw()` is guaranteed to be a valid pointer to `serdev_device`.
>> > + unsafe { bindings::serdev_device_set_client_ops(sdev.as_raw(), Self::INITIAL_OPS) };
>> > +
>> > + // SAFETY: The serial device bus only ever calls the probe callback with a valid pointer
>> > + // to a `struct serdev_device`.
>> > + to_result(unsafe {
>> > + bindings::devm_serdev_device_open(sdev.as_ref().as_raw(), sdev.as_raw())
>> > + })?;
I don't think it is a good idea to hardcode this into the probe() callback and
split it up in multiple stages, we can always solve things like ordering with
new types, type states and guards.
>> > +
>> > + // SAFETY: `&data.driver` is guaranteed to be pinned.
>> > + T::configure(sdev, unsafe { Pin::new_unchecked(&data.driver) }, info)?;
>> > +
>> > + if !T::HAS_RECEIVE_INITIAL {
>> > + // SAFETY:
>> > + // - It is guaranteed that we have exclusive access to `data.initial_data` and
>> > + // `data.late_probe_data`.
How is it ensured that this does not run concurrently with
initial_receive_buf_callback(), etc.?
>> > + // - We just initialized `data.initial_data` with Some.
>> > + unsafe { Self::do_late_probe(sdev, data)? };
>> > + }
It is a bit unclear to me what you try to achieve here.
Do you want to synchronize an initial data transfer? Then something along the
lines of what Kari proposes seems reasonable.
Or is the intention that this will run entirely async? But then distinct
initialization stages as they appear above won't work.
>> > +
>> > + Ok(0)
>> > + })
>> > + }
>> > +
>> > + /// # Safety
>> > + ///
>> > + /// The caller must guarantee, that we have exclusive access to `data.initial_data` and
>> > + /// `data.late_probe_data`. `data.initial_data` must be Some.
>> > + /// (i. e. `late_probe` has not been called yet).
>> > + unsafe fn do_late_probe(sdev: &Device<device::CoreInternal>, data: Pin<&Drvdata<T>>) -> Result {
>> > + // SAFETY: `&data.driver` is guaranteed to be pinned.
>> > + let data_driver = unsafe { Pin::new_unchecked(&data.driver) };
>> > +
>> > + // SAFETY: The function contract guarantees that we have exclusive access to
>> > + // `data.initial_data`.
>> > + let initial_data = unsafe { &mut *data.initial_data.get() };
>> > +
>> > + // SAFETY: The function contract guarantees that we have exclusive access to
>> > + // `data.late_probe_data`.
>> > + let late_probe_data = unsafe { &mut *data.late_probe_data.get() };
>> > +
>> > + *late_probe_data = Some(KBox::pin_init(
>> > + T::late_probe(
>> > + sdev,
>> > + data_driver,
>> > + // SAFETY: The function contract guarantees that `data.initial_data` is Some.
>> > + unsafe { initial_data.take().unwrap_unchecked() },
>> > + ),
>> > + GFP_KERNEL,
>> > + )?);
>> > + // 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) };
>> > + Ok(())
>> > + }
<snip>
>> > + extern "C" fn initial_receive_buf_callback(
>> > + sdev: *mut bindings::serdev_device,
>> > + buf: *const u8,
>> > + length: usize,
>> > + ) -> usize {
>> > + if !T::HAS_RECEIVE_INITIAL {
>> > + return 0;
>> > + }
>> > +
>> > + // 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: `buf` is guaranteed to be non-null and has the size of `length`.
>> > + let buf = unsafe { core::slice::from_raw_parts(buf, length) };
>> > +
>> > + // SAFETY: `initial_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<Drvdata<T>>>`.
>> > + let data = unsafe { sdev.as_ref().drvdata_borrow::<Drvdata<T>>() };
>> > +
>> > + // SAFETY: `&data.driver` is guaranteed to be pinned.
>> > + let driver_data = unsafe { Pin::new_unchecked(&data.driver) };
>> > +
>> > + // SAFETY:
>> > + // - `data.initial_data` is always Some until `InitialReceiveResult::Ready` is
>> > + // returned below.
>> > + // - It is guaranteed that we have exclusive access to `data.initial_data`.
>> > + let initial_data = unsafe { (*data.initial_data.get()).as_mut().unwrap_unchecked() };
>> > +
>> > + match T::receive_initial(sdev, driver_data, initial_data, buf) {
>> > + Ok(InitialReceiveResult::Pending(size)) => size,
>> > + Ok(InitialReceiveResult::Ready(size)) => {
>> > + // SAFETY:
>> > + // - It is guaranteed that we have exclusive access to `data.initial_data` and
>> > + // `data.late_probe_data`.
>> > + // - We just initialized `data.initial_data` with Some.
>> > + if let Err(err) = unsafe { Self::do_late_probe(sdev, data) } {
We are calling late_probe() again from initial_receive_buf_callback()? Why?
>> > + dev_err!(sdev.as_ref(), "Unable to late probe device: {err:?}\n");
>> > + // 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::NO_OPS) };
>> > + return length;
>> > + }
>> > + size
>> > + }
>> > + Err(err) => {
>> > + dev_err!(
>> > + sdev.as_ref(),
>> > + "Unable to receive initial data for probe: {err:?}.\n"
>> > + );
>> > + // 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::NO_OPS) };
>> > + length
>> > + }
>> > + }
>> > + }
<snip>
> There currently is only one serial device bus driver in the kernel,
> which needs initial data:
> drivers/bluetooth/hci_uart.h
> drivers/bluetooth/hci_ldisc.c
> drivers/bluetooth/hci_serdev.c
>
> This driver retrieves this initial data after probe (not in the probe)
> and then initializes it with a workqueue. Given it is part of the
> kernel, I assume this is the intended behaviour.
In this case I assume the driver has a lock protected buffer in its private
data? Which would be entirely different than what you implement above, no?
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH RFC 2/4] rust: add basic serial device bus abstractions
2026-01-13 17:37 ` Danilo Krummrich
@ 2026-01-13 17:59 ` Markus Probst
2026-01-13 19:10 ` Danilo Krummrich
2026-02-08 14:30 ` Markus Probst
1 sibling, 1 reply; 22+ messages in thread
From: Markus Probst @ 2026-01-13 17:59 UTC (permalink / raw)
To: Danilo Krummrich
Cc: Kari Argillander, Rob Herring, Greg Kroah-Hartman, Jiri Slaby,
Miguel Ojeda, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross,
linux-serial, linux-kernel, rust-for-linux
On Tue, 2026-01-13 at 18:37 +0100, Danilo Krummrich wrote:
> On Tue Jan 13, 2026 at 5:15 PM CET, Markus Probst wrote:
> > > > +impl<T: Driver + 'static> 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: if T::HAS_WRITE_WAKEUP {
> > > > + Some(Self::write_wakeup_callback)
> > > > + } else {
> > > > + Some(bindings::serdev_device_write_wakeup)
> > > > + },
> > > > + };
> > > > + const INITIAL_OPS: &'static bindings::serdev_device_ops = &bindings::serdev_device_ops {
> > > > + receive_buf: Some(Self::initial_receive_buf_callback),
> > > > + write_wakeup: if T::HAS_WRITE_WAKEUP_INITIAL {
> > > > + Some(Self::initial_write_wakeup_callback)
> > > > + } else {
> > > > + Some(bindings::serdev_device_write_wakeup)
> > > > + },
> > > > + };
> > > > + const NO_OPS: &'static bindings::serdev_device_ops = &bindings::serdev_device_ops {
> > > > + receive_buf: 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(|| {
> > > > + let data = try_pin_init!(Drvdata {
> > > > + driver <- T::probe(sdev, info),
> > > > + initial_data: Some(Default::default()).into(),
> > > > + late_probe_data: None.into(),
> > > > + });
> > > > +
> > > > + sdev.as_ref().set_drvdata(data)?;
>
> This does not work, a driver can obtain its device private data with
> Device::<Bound>::drvdata() [1].
>
> For this the driver must assert the correct type, but since you use a private
> type instead of the type given by the driver, i.e. T, Device::<Bound>::drvdata()
> will always fail for the driver.
>
> [1] https://rust.docs.kernel.org/kernel/device/struct.Device.html#method.drvdata
I need to fix that.
>
> > > > +
> > > > + // SAFETY: We just set drvdata to `Drvdata<T>`.
> > > > + let data = unsafe { sdev.as_ref().drvdata_borrow::<Drvdata<T>>() };
> > > > +
> > > > + // SAFETY: `sdev.as_raw()` is guaranteed to be a valid pointer to `serdev_device`.
> > > > + unsafe { bindings::serdev_device_set_client_ops(sdev.as_raw(), Self::INITIAL_OPS) };
> > > > +
> > > > + // SAFETY: The serial device bus only ever calls the probe callback with a valid pointer
> > > > + // to a `struct serdev_device`.
> > > > + to_result(unsafe {
> > > > + bindings::devm_serdev_device_open(sdev.as_ref().as_raw(), sdev.as_raw())
> > > > + })?;
>
> I don't think it is a good idea to hardcode this into the probe() callback and
> split it up in multiple stages, we can always solve things like ordering with
> new types, type states and guards.
>
> > > > +
> > > > + // SAFETY: `&data.driver` is guaranteed to be pinned.
> > > > + T::configure(sdev, unsafe { Pin::new_unchecked(&data.driver) }, info)?;
> > > > +
> > > > + if !T::HAS_RECEIVE_INITIAL {
> > > > + // SAFETY:
> > > > + // - It is guaranteed that we have exclusive access to `data.initial_data` and
> > > > + // `data.late_probe_data`.
>
> How is it ensured that this does not run concurrently with
> initial_receive_buf_callback(), etc.?
```
if !T::HAS_RECEIVE_INITIAL {
return 0;
}
```
at the beginning of the function does.
>
> > > > + // - We just initialized `data.initial_data` with Some.
> > > > + unsafe { Self::do_late_probe(sdev, data)? };
> > > > + }
>
> It is a bit unclear to me what you try to achieve here.
>
> Do you want to synchronize an initial data transfer? Then something along the
> lines of what Kari proposes seems reasonable.
>
> Or is the intention that this will run entirely async? But then distinct
> initialization stages as they appear above won't work.
Async.
The driver gets probed.
late_probe will be called, after all the necessary data for the
initialization is there.
Can you explain what doesn't work? The code has been tested with a
prototype driver before submission.
Kari his approach is synchronize inside the probe function, until all
the necessary data is there.
Which one do you think should be used for the abstraction?
>
> > > > +
> > > > + Ok(0)
> > > > + })
> > > > + }
> > > > +
> > > > + /// # Safety
> > > > + ///
> > > > + /// The caller must guarantee, that we have exclusive access to `data.initial_data` and
> > > > + /// `data.late_probe_data`. `data.initial_data` must be Some.
> > > > + /// (i. e. `late_probe` has not been called yet).
> > > > + unsafe fn do_late_probe(sdev: &Device<device::CoreInternal>, data: Pin<&Drvdata<T>>) -> Result {
> > > > + // SAFETY: `&data.driver` is guaranteed to be pinned.
> > > > + let data_driver = unsafe { Pin::new_unchecked(&data.driver) };
> > > > +
> > > > + // SAFETY: The function contract guarantees that we have exclusive access to
> > > > + // `data.initial_data`.
> > > > + let initial_data = unsafe { &mut *data.initial_data.get() };
> > > > +
> > > > + // SAFETY: The function contract guarantees that we have exclusive access to
> > > > + // `data.late_probe_data`.
> > > > + let late_probe_data = unsafe { &mut *data.late_probe_data.get() };
> > > > +
> > > > + *late_probe_data = Some(KBox::pin_init(
> > > > + T::late_probe(
> > > > + sdev,
> > > > + data_driver,
> > > > + // SAFETY: The function contract guarantees that `data.initial_data` is Some.
> > > > + unsafe { initial_data.take().unwrap_unchecked() },
> > > > + ),
> > > > + GFP_KERNEL,
> > > > + )?);
> > > > + // 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) };
> > > > + Ok(())
> > > > + }
>
> <snip>
>
> > > > + extern "C" fn initial_receive_buf_callback(
> > > > + sdev: *mut bindings::serdev_device,
> > > > + buf: *const u8,
> > > > + length: usize,
> > > > + ) -> usize {
> > > > + if !T::HAS_RECEIVE_INITIAL {
> > > > + return 0;
> > > > + }
> > > > +
> > > > + // 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: `buf` is guaranteed to be non-null and has the size of `length`.
> > > > + let buf = unsafe { core::slice::from_raw_parts(buf, length) };
> > > > +
> > > > + // SAFETY: `initial_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<Drvdata<T>>>`.
> > > > + let data = unsafe { sdev.as_ref().drvdata_borrow::<Drvdata<T>>() };
> > > > +
> > > > + // SAFETY: `&data.driver` is guaranteed to be pinned.
> > > > + let driver_data = unsafe { Pin::new_unchecked(&data.driver) };
> > > > +
> > > > + // SAFETY:
> > > > + // - `data.initial_data` is always Some until `InitialReceiveResult::Ready` is
> > > > + // returned below.
> > > > + // - It is guaranteed that we have exclusive access to `data.initial_data`.
> > > > + let initial_data = unsafe { (*data.initial_data.get()).as_mut().unwrap_unchecked() };
> > > > +
> > > > + match T::receive_initial(sdev, driver_data, initial_data, buf) {
> > > > + Ok(InitialReceiveResult::Pending(size)) => size,
> > > > + Ok(InitialReceiveResult::Ready(size)) => {
> > > > + // SAFETY:
> > > > + // - It is guaranteed that we have exclusive access to `data.initial_data` and
> > > > + // `data.late_probe_data`.
> > > > + // - We just initialized `data.initial_data` with Some.
> > > > + if let Err(err) = unsafe { Self::do_late_probe(sdev, data) } {
>
> We are calling late_probe() again from initial_receive_buf_callback()? Why?
It only gets called once.
This is gated behind "if T::HAS_RECEIVE_INITIAL". And in probe its
gated behind "if T::HAS_RECEIVE_INITIAL".
If "receive_initial" does not get implemented by the driver, late probe
will be run immediately after the regular probe.
>
> > > > + dev_err!(sdev.as_ref(), "Unable to late probe device: {err:?}\n");
> > > > + // 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::NO_OPS) };
> > > > + return length;
> > > > + }
> > > > + size
> > > > + }
> > > > + Err(err) => {
> > > > + dev_err!(
> > > > + sdev.as_ref(),
> > > > + "Unable to receive initial data for probe: {err:?}.\n"
> > > > + );
> > > > + // 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::NO_OPS) };
> > > > + length
> > > > + }
> > > > + }
> > > > + }
>
> <snip>
>
> > There currently is only one serial device bus driver in the kernel,
> > which needs initial data:
> > drivers/bluetooth/hci_uart.h
> > drivers/bluetooth/hci_ldisc.c
> > drivers/bluetooth/hci_serdev.c
> >
> > This driver retrieves this initial data after probe (not in the probe)
> > and then initializes it with a workqueue. Given it is part of the
> > kernel, I assume this is the intended behaviour.
>
> In this case I assume the driver has a lock protected buffer in its private
> data? Which would be entirely different than what you implement above, no?
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH RFC 2/4] rust: add basic serial device bus abstractions
2026-01-13 17:59 ` Markus Probst
@ 2026-01-13 19:10 ` Danilo Krummrich
0 siblings, 0 replies; 22+ messages in thread
From: Danilo Krummrich @ 2026-01-13 19:10 UTC (permalink / raw)
To: Markus Probst
Cc: Kari Argillander, Rob Herring, Greg Kroah-Hartman, Jiri Slaby,
Miguel Ojeda, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross,
linux-serial, linux-kernel, rust-for-linux
On Tue Jan 13, 2026 at 6:59 PM CET, Markus Probst wrote:
> This is gated behind "if T::HAS_RECEIVE_INITIAL". And in probe its
> gated behind "if T::HAS_RECEIVE_INITIAL".
I guess you mean `if !T::HAS_RECEIVE_INITIAL` in probe().
Anyways, I now get what you are trying to do. (I got confused by late_probe(),
which I think is a misleading name for what it actually is and in which context
it might run in. I think a better name would be initial_xfer() or something
along those lines.)
So, what you really want is additional private data that is valid between some
inital xfer happening at some point of time after probe() and unbind(), and
hence is valid to access in all subsequent transfers callbacks.
There are three options:
(1) What you already implement, but with less callbacks.
I think the only additional callback you need is initial_xfer(), where you
return the init private data.
For storing this data you probably want to add void *init_data to
struct serdev_device.
(2) A synchronous inital transfer in probe() as proposed by Kari.
This is much simpler than (1), but still leaves the driver without an
Option for the init data in its device private data.
(3) Leave it to the driver.
The driver can store an Option in its device private data, which is
initialized in the first callback.
> Which one do you think should be used for the abstraction?
I don't know, it depends on the requirements of serdev drivers.
But since you already mentioned that there is only a single driver that needs
this initial xfer data, I'd say don't consider it in the abstraction at all
until we are sure it is a common pattern needed by drivers. We can always add it
later on if needed.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH RFC 2/4] rust: add basic serial device bus abstractions
2026-01-13 17:37 ` Danilo Krummrich
2026-01-13 17:59 ` Markus Probst
@ 2026-02-08 14:30 ` Markus Probst
1 sibling, 0 replies; 22+ messages in thread
From: Markus Probst @ 2026-02-08 14:30 UTC (permalink / raw)
To: Danilo Krummrich
Cc: Kari Argillander, Rob Herring, Greg Kroah-Hartman, Jiri Slaby,
Miguel Ojeda, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross,
linux-serial, linux-kernel, rust-for-linux
[-- Attachment #1: Type: text/plain, Size: 2861 bytes --]
On Tue, 2026-01-13 at 18:37 +0100, Danilo Krummrich wrote:
> On Tue Jan 13, 2026 at 5:15 PM CET, Markus Probst wrote:
> > > > +impl<T: Driver + 'static> 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: if T::HAS_WRITE_WAKEUP {
> > > > + Some(Self::write_wakeup_callback)
> > > > + } else {
> > > > + Some(bindings::serdev_device_write_wakeup)
> > > > + },
> > > > + };
> > > > + const INITIAL_OPS: &'static bindings::serdev_device_ops = &bindings::serdev_device_ops {
> > > > + receive_buf: Some(Self::initial_receive_buf_callback),
> > > > + write_wakeup: if T::HAS_WRITE_WAKEUP_INITIAL {
> > > > + Some(Self::initial_write_wakeup_callback)
> > > > + } else {
> > > > + Some(bindings::serdev_device_write_wakeup)
> > > > + },
> > > > + };
> > > > + const NO_OPS: &'static bindings::serdev_device_ops = &bindings::serdev_device_ops {
> > > > + receive_buf: 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(|| {
> > > > + let data = try_pin_init!(Drvdata {
> > > > + driver <- T::probe(sdev, info),
> > > > + initial_data: Some(Default::default()).into(),
> > > > + late_probe_data: None.into(),
> > > > + });
> > > > +
> > > > + sdev.as_ref().set_drvdata(data)?;
>
> This does not work, a driver can obtain its device private data with
> Device::<Bound>::drvdata() [1].
>
> For this the driver must assert the correct type, but since you use a private
> type instead of the type given by the driver, i.e. T, Device::<Bound>::drvdata()
> will always fail for the driver.
>
> [1] https://rust.docs.kernel.org/kernel/device/struct.Device.html#method.drvdata
>
What do you suggest as alternative? Besides from the "initial data"
debate, I still need to store at least one `Completion` to ensure
safety.
Thanks
- Markus Probst
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 870 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH RFC 2/4] rust: add basic serial device bus abstractions
2025-12-20 18:44 ` [PATCH RFC 2/4] rust: add basic serial device bus abstractions Markus Probst
2025-12-21 9:19 ` Dirk Behme
2025-12-25 15:13 ` Kari Argillander
@ 2025-12-26 15:09 ` Kari Argillander
2 siblings, 0 replies; 22+ messages in thread
From: Kari Argillander @ 2025-12-26 15:09 UTC (permalink / raw)
To: Markus Probst
Cc: Rob Herring, Greg Kroah-Hartman, Jiri Slaby, Miguel Ojeda,
Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Alice Ryhl, Trevor Gross, Danilo Krummrich,
linux-serial, linux-kernel, rust-for-linux
Here are some review comments. Overall it seems that this will be quite hard to
get right, safe and easy to use. Example many null pointer dereferences can
happen with C API if things are not done in right order. That currently force
this abstraction to do those things in right order (like you have done and kudos
for that). That however makes this probably too rigid for many use cases. Maybe
right thing to do is to fix C API so it is safer to use. That way we can call
things in wrong order and that is still safe but just logic bug in driver.
On 20.12.2025 20.44 +0200 Markus Probst (markus.probst@posteo.de) 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`.
Maybe add more info about design decisions here.
> Co-developed-by: Kari Argillander <kari.argillander@gmail.com>
> Signed-off-by: Kari Argillander <kari.argillander@gmail.com>
> Signed-off-by: Markus Probst <markus.probst@posteo.de>
> ---
> 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 | 815 ++++++++++++++++++++++++++++++++++++++++
> 5 files changed, 841 insertions(+)
>
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index a067038b4b42..bec6c6d0913a 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -79,6 +79,7 @@
> #include <linux/regulator/consumer.h>
> #include <linux/sched.h>
> #include <linux/security.h>
> +#include <linux/serdev.h>
> #include <linux/slab.h>
> #include <linux/task_work.h>
> #include <linux/tracepoint.h>
> diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
> index 79c72762ad9c..834e9fbb897d 100644
> --- a/rust/helpers/helpers.c
> +++ b/rust/helpers/helpers.c
> @@ -50,6 +50,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 f812cf120042..cc71195466b6 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -136,6 +136,8 @@
> pub mod scatterlist;
> pub mod security;
> pub mod seq_file;
> +#[cfg(CONFIG_SERIAL_DEV_BUS)]
> +pub mod serdev;
> pub mod sizes;
> pub mod slice;
> mod static_assert;
> diff --git a/rust/kernel/serdev.rs b/rust/kernel/serdev.rs
> new file mode 100644
> index 000000000000..0f5ef325a054
> --- /dev/null
> +++ b/rust/kernel/serdev.rs
> @@ -0,0 +1,815 @@
> +// 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,
> + container_of,
> + device,
> + driver,
> + error::{
> + from_result,
> + to_result,
> + VTABLE_DEFAULT_ERROR, //
> + },
> + of,
> + prelude::*,
> + time::Jiffies,
> + types::{AlwaysRefCounted, Opaque}, //
> +};
> +
> +use core::{
> + cell::UnsafeCell,
> + marker::PhantomData,
> + mem::offset_of,
> + num::NonZero,
> + ptr::{
> + addr_of_mut, //
> + 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`].
> + Value(NonZero<Jiffies>),
Maybe better name this Jiffies than value. As Jiffies is not yet real unit. Also
many C drivers use msecs_to_jiffies() maybe also have Msecs variant that takes
NonZero<Msecs>. Then into_jiffies() can call msecs_to_jiffies() for that. That
would make construction easier. Also that way people can have const
Timeout::Msecs which is quite nice. Probably people usually will define timeouts
as consts and then use those consts.
> + /// Wait as long as possible.
> + ///
> + /// This is equivalent to [`kernel::task::MAX_SCHEDULE_TIMEOUT`].
> + MaxScheduleTimeout,
As enum is already Timeout maybe Max or MaxSchedule is better name than
MaxScheduleTimeout.
> +}
> +
> +impl Timeout {
> + fn into_jiffies(self) -> isize {
> + match self {
> + Self::Value(value) => value.get().try_into().unwrap_or_default(),
> + Self::MaxScheduleTimeout => 0,
> + }
> + }
> +}
> +
> +/// An adapter for the registration of serial device bus device drivers.
> +pub struct Adapter<T: Driver>(T);
> +
> +// SAFETY: A call to `unregister` for a given instance of `RegType` is guaranteed to be valid if
> +// a preceding call to `register` has been successful.
> +unsafe impl<T: Driver + 'static> driver::RegistrationOps for Adapter<T> {
> + type RegType = bindings::serdev_device_driver;
> +
> + unsafe fn register(
> + sdrv: &Opaque<Self::RegType>,
> + 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 `RegType`.
> + to_result(unsafe { bindings::__serdev_device_driver_register(sdrv.get(), module.0) })
> + }
> +
> + unsafe fn unregister(sdrv: &Opaque<Self::RegType>) {
> + // SAFETY: `sdrv` is guaranteed to be a valid `RegType`.
> + unsafe { bindings::serdev_device_driver_unregister(sdrv.get()) };
> + }
> +}
> +
> +#[pin_data]
> +struct Drvdata<T: Driver + 'static> {
> + #[pin]
> + driver: T,
> + initial_data: UnsafeCell<Option<T::InitialData>>,
Maybe also box initial_data as that can be destroyed in after late_probe.
> + late_probe_data: UnsafeCell<Option<Pin<KBox<T::LateProbeData>>>>,
> +}
> +
> +impl<T: Driver + 'static> 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: if T::HAS_WRITE_WAKEUP {
> + Some(Self::write_wakeup_callback)
> + } else {
> + Some(bindings::serdev_device_write_wakeup)
> + },
> + };
> + const INITIAL_OPS: &'static bindings::serdev_device_ops = &bindings::serdev_device_ops {
> + receive_buf: Some(Self::initial_receive_buf_callback),
> + write_wakeup: if T::HAS_WRITE_WAKEUP_INITIAL {
> + Some(Self::initial_write_wakeup_callback)
> + } else {
> + Some(bindings::serdev_device_write_wakeup)
> + },
> + };
> + const NO_OPS: &'static bindings::serdev_device_ops = &bindings::serdev_device_ops {
> + receive_buf: 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(|| {
> + let data = try_pin_init!(Drvdata {
> + driver <- T::probe(sdev, info),
> + initial_data: Some(Default::default()).into(),
> + late_probe_data: None.into(),
> + });
> +
> + sdev.as_ref().set_drvdata(data)?;
> +
> + // SAFETY: We just set drvdata to `Drvdata<T>`.
> + let data = unsafe { sdev.as_ref().drvdata_borrow::<Drvdata<T>>() };
> +
> + // SAFETY: `sdev.as_raw()` is guaranteed to be a valid pointer to `serdev_device`.
> + unsafe { bindings::serdev_device_set_client_ops(sdev.as_raw(), Self::INITIAL_OPS) };
> +
> + // SAFETY: The serial device bus only ever calls the probe callback with a valid pointer
> + // to a `struct serdev_device`.
> + to_result(unsafe {
> + bindings::devm_serdev_device_open(sdev.as_ref().as_raw(), sdev.as_raw())
> + })?;
> +
> + // SAFETY: `&data.driver` is guaranteed to be pinned.
> + T::configure(sdev, unsafe { Pin::new_unchecked(&data.driver) }, info)?;
> +
> + if !T::HAS_RECEIVE_INITIAL {
> + // SAFETY:
> + // - It is guaranteed that we have exclusive access to `data.initial_data` and
> + // `data.late_probe_data`.
> + // - We just initialized `data.initial_data` with Some.
> + unsafe { Self::do_late_probe(sdev, data)? };
> + }
> +
> + Ok(0)
> + })
> + }
> +
> + /// # Safety
> + ///
> + /// The caller must guarantee, that we have exclusive access to `data.initial_data` and
> + /// `data.late_probe_data`. `data.initial_data` must be Some.
> + /// (i. e. `late_probe` has not been called yet).
> + unsafe fn do_late_probe(sdev: &Device<device::CoreInternal>, data: Pin<&Drvdata<T>>) -> Result {
> + // SAFETY: `&data.driver` is guaranteed to be pinned.
> + let data_driver = unsafe { Pin::new_unchecked(&data.driver) };
> +
> + // SAFETY: The function contract guarantees that we have exclusive access to
> + // `data.initial_data`.
> + let initial_data = unsafe { &mut *data.initial_data.get() };
> +
> + // SAFETY: The function contract guarantees that we have exclusive access to
> + // `data.late_probe_data`.
> + let late_probe_data = unsafe { &mut *data.late_probe_data.get() };
> +
> + *late_probe_data = Some(KBox::pin_init(
> + T::late_probe(
> + sdev,
> + data_driver,
> + // SAFETY: The function contract guarantees that `data.initial_data` is Some.
> + unsafe { initial_data.take().unwrap_unchecked() },
> + ),
> + GFP_KERNEL,
> + )?);
> + // 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) };
How can this be safe? What if serdev core is accessing the ops while we are
changing them? SAFETY comment should address also concurrency issues in this
case.
> + Ok(())
> + }
> +
> + 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<T>>`.
> + let data = unsafe { sdev.as_ref().drvdata_obtain::<Drvdata<T>>() };
> +
> + // SAFETY: `&data.driver` is guaranteed to be pinned.
> + let data_driver = unsafe { Pin::new_unchecked(&data.driver) };
> +
> + // SAFETY:
> + // - `data.late_probe_data` is guaranteed to be pinned.
> + // - It is guaranteed that we have exclusive access to `data.late_probe_data`.
> + let late_probe_data = unsafe {
> + (*data.late_probe_data.get())
> + .as_deref()
> + .map(|data| Pin::new_unchecked(data))
> + };
> +
> + T::unbind(sdev, data_driver, late_probe_data);
> + }
> +
> + 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<Drvdata<T>>>`.
> + let data = unsafe { sdev.as_ref().drvdata_borrow::<Drvdata<T>>() };
> +
> + // SAFETY: `buf` is guaranteed to be non-null and has the size of `length`.
> + let buf = unsafe { core::slice::from_raw_parts(buf, length) };
> +
> + // SAFETY: `&data.driver` is guaranteed to be pinned.
> + let data_driver = unsafe { Pin::new_unchecked(&data.driver) };
> +
> + // SAFETY:
> + // - `data.late_probe_data` is guaranteed to be Some.
> + // - `data.late_probe_data` is guaranteed to be pinned.
> + let late_probe_data = unsafe {
> + Pin::new_unchecked((*data.late_probe_data.get()).as_deref().unwrap_unchecked())
> + };
> +
> + T::receive(sdev, data_driver, late_probe_data, buf)
> + }
> +
> + extern "C" fn write_wakeup_callback(sdev: *mut bindings::serdev_device) {
> + // SAFETY: The serial device bus only ever calls the write wakeup callback with a valid
> + // pointer to a `struct serdev_device`.
> + //
> + // INVARIANT: `sdev` is valid for the duration of `write_wakeup_callback()`.
> + let sdev = unsafe { &*sdev.cast::<Device<device::CoreInternal>>() };
> +
> + // SAFETY: `write_wakeup_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<Drvdata<T>>>`.
> + let data = unsafe { sdev.as_ref().drvdata_borrow::<Drvdata<T>>() };
> +
> + // SAFETY: `sdev.as_raw()` is guaranteed to be a pointer to a valid `serdev_device`.
> + unsafe { bindings::serdev_device_write_wakeup(sdev.as_raw()) };
> +
> + // SAFETY: `&data.driver` is guaranteed to be pinned.
> + let data_driver = unsafe { Pin::new_unchecked(&data.driver) };
> +
> + // SAFETY:
> + // - `data.late_probe_data` is guaranteed to be Some.
> + // - `data.late_probe_data` is guaranteed to be pinned.
> + let late_probe_data = unsafe {
> + Pin::new_unchecked((*data.late_probe_data.get()).as_deref().unwrap_unchecked())
> + };
> +
> + // SAFETY: As long as the driver implementation meets the safety requirements, this call
> + // is safe.
> + unsafe { T::write_wakeup(sdev, data_driver, late_probe_data) };
> + }
> +
> + extern "C" fn initial_receive_buf_callback(
> + sdev: *mut bindings::serdev_device,
> + buf: *const u8,
> + length: usize,
> + ) -> usize {
> + if !T::HAS_RECEIVE_INITIAL {
> + return 0;
> + }
> +
> + // 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: `buf` is guaranteed to be non-null and has the size of `length`.
> + let buf = unsafe { core::slice::from_raw_parts(buf, length) };
> +
> + // SAFETY: `initial_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<Drvdata<T>>>`.
> + let data = unsafe { sdev.as_ref().drvdata_borrow::<Drvdata<T>>() };
> +
> + // SAFETY: `&data.driver` is guaranteed to be pinned.
> + let driver_data = unsafe { Pin::new_unchecked(&data.driver) };
> +
> + // SAFETY:
> + // - `data.initial_data` is always Some until `InitialReceiveResult::Ready` is
> + // returned below.
> + // - It is guaranteed that we have exclusive access to `data.initial_data`.
> + let initial_data = unsafe { (*data.initial_data.get()).as_mut().unwrap_unchecked() };
> +
> + match T::receive_initial(sdev, driver_data, initial_data, buf) {
> + Ok(InitialReceiveResult::Pending(size)) => size,
> + Ok(InitialReceiveResult::Ready(size)) => {
> + // SAFETY:
> + // - It is guaranteed that we have exclusive access to `data.initial_data` and
> + // `data.late_probe_data`.
> + // - We just initialized `data.initial_data` with Some.
> + if let Err(err) = unsafe { Self::do_late_probe(sdev, data) } {
> + dev_err!(sdev.as_ref(), "Unable to late probe device: {err:?}\n");
Not an abstraction job to log this.
> + // 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::NO_OPS) };
What if write_wakeup is accessed by serdev core while we are changing the ops? I
checked and even taking serdev write lock does not prevent that as checking
write_wakeup ops is done outside the lock. We could probably change that however
if necessary.
> + return length;
> + }
> + size
> + }
> + Err(err) => {
> + dev_err!(
> + sdev.as_ref(),
> + "Unable to receive initial data for probe: {err:?}.\n"
> + );
Same here. Not an abstraction job to log this.
> + // 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::NO_OPS) };
> + length
> + }
> + }
> + }
> +
> + extern "C" fn initial_write_wakeup_callback(sdev: *mut bindings::serdev_device) {
> + // SAFETY: The serial device bus only ever calls the write wakeup callback with a valid
> + // pointer to a `struct serdev_device`.
> + //
> + // INVARIANT: `sdev` is valid for the duration of `write_wakeup_callback()`.
initial_write_wakeup_callback
> + let sdev = unsafe { &*sdev.cast::<Device<device::CoreInternal>>() };
> +
> + // SAFETY: `initial_write_wakeup_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<Drvdata<T>>>`.
> + let data = unsafe { sdev.as_ref().drvdata_borrow::<Drvdata<T>>() };
> +
> + // SAFETY: `sdev.as_raw()` is guaranteed to be a pointer to a valid `serdev_device`.
> + unsafe { bindings::serdev_device_write_wakeup(sdev.as_raw()) };
> +
> + // SAFETY: `&data.driver` is guaranteed to be pinned.
> + let data_driver = unsafe { Pin::new_unchecked(&data.driver) };
> +
> + // SAFETY: As long as the driver implementation meets the safety requirements, this call
> + // is safe.
> + unsafe { T::write_wakeup_initial(sdev, data_driver) };
> + }
> +}
> +
> +impl<T: Driver + 'static> 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 {
To be consistent with other bus module macros this should be named
module_serdev_driver. Also change in example.
> + ($($f:tt)*) => {
> + $crate::module_driver!(<T>, $crate::serdev::Adapter<T>, { $($f)* });
> + };
> +}
> +
> +/// Result for `receive_initial` in [`Driver`].
> +pub enum InitialReceiveResult {
> + /// More data is required.
> + ///
> + /// The inner data is the number of bytes accepted.
> + Pending(usize),
> + /// Ready for late probe.
> + ///
> + /// The inner data is the number of bytes accepted.
> + Ready(usize),
> +}
> +
> +/// 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 LateProbeData = ();
> +/// type InitialData = ();
> +/// 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(
> +/// _sdev: &serdev::Device,
> +/// _id_info: Option<&Self::IdInfo>,
> +/// ) -> impl PinInit<Self, Error> {
> +/// Err(ENODEV)
> +/// }
> +///
> +/// fn configure(
> +/// dev: &serdev::Device<Core>,
> +/// _this: Pin<&Self>,
> +/// _id_info: Option<&Self::IdInfo>,
> +/// ) -> Result {
> +/// dev.set_baudrate(115200);
> +/// Ok(())
> +/// }
> +///
> +/// fn late_probe(
> +/// _dev: &serdev::Device<Bound>,
> +/// _this: Pin<&Self>,
> +/// _initial_data: Self::InitialData,
> +/// ) -> impl PinInit<Self::LateProbeData, Error> {
> +/// Ok(())
> +/// }
> +/// }
> +///```
> +#[vtable]
> +pub trait Driver: Send {
> + /// 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 LateProbeData: Send + 'static = ();
> + // type InitialData: Default + Send + 'static = ();
> + // ```
> + type IdInfo: 'static;
> +
> + /// Data returned in `late_probe` function.
> + type LateProbeData: Send + 'static;
> + /// Data used for initial data.
> + type InitialData: Default + Send + 'static;
> +
> + /// 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(sdev: &Device, id_info: Option<&Self::IdInfo>) -> impl PinInit<Self, Error>;
As probe has serdev::Device. This means we cannot example enable let's say
regulators as those needs device::Device<Bound>. So we could enable those just
in configure(). That means open() is called before we can do preparations. This
seems to be a problem to me. It might not be so someone else can comment if this
is actually a problem.
If we cannot do any preparation before open() we might as well call open()
before probe() and have probe() receive Device<Core>. So something does not seem
quite right here.
Also like I said in another thread I think user should be able to control when
open() is called.
> +
> + /// Serial device bus device driver configure.
> + ///
> + /// Called directly after the serial device bus device has been opened.
> + /// This should be used for setting up the communication (i. e. baudrate, flow control, parity)
> + /// and sending an initial hello if required.
> + fn configure(
> + sdev: &Device<device::Core>,
> + this: Pin<&Self>,
> + id_info: Option<&Self::IdInfo>,
> + ) -> Result;
> +
> + /// Serial device bus device data receive callback (initial).
> + ///
> + /// Called when data got received from device, before `late_probe` has been called.
> + /// This should be used, to get information about the device for `late_probe`.
> + ///
> + /// Returns `InitialReceiveResult::Pending` if more data is still required for `late_probe`.
> + /// Otherwise `InitialReceiveResult::Ready`. The inner data is the number of bytes accepted.
> + fn receive_initial(
> + sdev: &Device<device::Bound>,
> + this: Pin<&Self>,
> + initial_data: &mut Self::InitialData,
> + data: &[u8],
> + ) -> Result<InitialReceiveResult> {
> + let _ = (sdev, this, initial_data, data);
> + build_error!(VTABLE_DEFAULT_ERROR)
> + }
Maybe just let implementors could just write to `this` in receive_initial and
then we do not need initial_data at all. That would simplify things a bit. That
would mean some "locking" from implementors is required though. But I think that
is acceptable. Maybe same for late_probe data.
> + /// Serial device bus device write wakeup callback (initial).
> + ///
> + /// Called when ready to transmit more data, before `late_probe` has been called.
> + ///
> + /// # Safety
> + ///
> + /// This function must not sleep.
> + unsafe fn write_wakeup_initial(sdev: &Device<device::Core>, this: Pin<&Self>) {
> + let _ = (sdev, this);
> + build_error!(VTABLE_DEFAULT_ERROR)
> + }
This seems useless as during probe we can just do sequential writes as nothing
else is happening. After late_probe we can use the normal write_wakeup.
> +
> + /// Serial device bus device late probe.
> + ///
> + /// Called after the initial data is available.
> + /// If `receive_initial` isn't implemented, this will be called directly after configure.
> + fn late_probe(
> + sdev: &Device<device::Bound>,
> + this: Pin<&Self>,
> + initial_data: Self::InitialData,
> + ) -> impl PinInit<Self::LateProbeData, Error>;
> +
> + /// 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 `Self::drop`.
> + fn unbind(
> + sdev: &Device<device::Core>,
> + this: Pin<&Self>,
> + late_probe_this: Option<Pin<&Self::LateProbeData>>,
> + ) {
> + let _ = (sdev, this, late_probe_this);
> + }
> +
> + /// Serial device bus device data receive callback.
> + ///
> + /// Called when data got received from device.
> + ///
> + /// Returns the number of bytes accepted.
> + fn receive(
> + sdev: &Device<device::Bound>,
> + this: Pin<&Self>,
> + late_probe_this: Pin<&Self::LateProbeData>,
> + data: &[u8],
> + ) -> usize {
> + let _ = (sdev, this, late_probe_this, data);
> + build_error!(VTABLE_DEFAULT_ERROR)
> + }
> +
> + /// Serial device bus device write wakeup callback.
> + ///
> + /// Called when ready to transmit more data.
> + ///
> + /// # Safety
> + ///
> + /// This function must not sleep.
Maybe add hint here that implementors should enable DEBUG_ATOMIC_SLEEP when
developing to catch accidental sleeps.
> + unsafe fn write_wakeup(
> + sdev: &Device<device::Bound>,
> + this: Pin<&Self>,
> + late_probe_this: Pin<&Self::LateProbeData>,
> + ) {
> + let _ = (sdev, this, late_probe_this);
> + build_error!(VTABLE_DEFAULT_ERROR)
> + }
I do not see how writing can actually be done. Only place we can actually write
after late probe is receive(). This is because all write functions are in
serdev::Device<Bound>. Also we cannot use those in write_wakeup as that is
atomic context. We should schedule work there but if we schedule work there is
no way to actually call any write function in there as we only have
Aref<serdev::Device> and that has no write functions. So this seems like a very
big problem.
Also I fear that as we do OPS swap we lost first write wakeup call. That means
as this usually schedules work we might never schedule that work if we miss the
first call. I'm not sure of if this is a real problem or not.
> +}
> +
> +/// 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) -> u32 {
> + // SAFETY: `self.as_raw()` is guaranteed to be a pointer to a valid `serdev_device`.
> + unsafe { bindings::serdev_device_set_baudrate(self.as_raw(), speed) }
> + }
Here I'm wondering two things. Could we just have Baudare enum instead. That
would mean that return value is not needed as it will always succeed.
Another thing is if we do not use enum is that should we do something like
```rust
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 {
Err(ret)
}
Ok(())
}
```
I'm still not quite convinced for this idea. So I still think maybe enum is good
idea. But then again there must be reason why C API does not use enum?
It might be
historical reason only and we should not make same thing again.
Another idea is to have enum which have all bauderates but last one is
Custom(u32). But having this kind of work done during this abstraction layer
seems a bit off so I'm totally ok if we just use u32 here.
> + /// 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 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(),
> + )
> + };
Use to_result()
> + if ret < 0 {
> + // CAST: negative return values are guaranteed to be between `-MAX_ERRNO` and `-1`,
> + // which always fit into a `i32`.
> + Err(Error::from_errno(ret as i32))
> + } else {
> + Ok(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 if interrupted).
> + /// [`kernel::error::code::ETIMEDOUT`] or [`kernel::error::code::ERESTARTSYS`] if interrupted
> + /// before any bytes were written.
> + 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()) };
Use to_result()
> + if ret < 0 {
> + Err(Error::from_errno(ret))
> + } else {
> + Ok(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 { addr_of_mut!((*self.as_raw()).dev) };
Now days `&raw mut` is preferred instead of `addr_of_mut!`.
> + // SAFETY: `dev` points to a valid `struct device`.
> + unsafe { device::Device::from_raw(dev) }
> + }
> +}
> +
> +impl<Ctx: device::DeviceContext> TryFrom<&device::Device<Ctx>> for &Device<Ctx> {
> + type Error = kernel::error::Error;
> +
> + fn try_from(dev: &device::Device<Ctx>) -> Result<Self, Self::Error> {
> + // SAFETY: By the type invariant of `Device`, `dev.as_raw()` is a valid pointer to a
> + // `struct device`.
> + if !unsafe { bindings::is_serdev_device(dev.as_raw()) } {
> + return Err(EINVAL);
> + }
> +
> + // SAFETY: We've just verified that the device_type of `dev` is
> + // `serdev_device_type`, hence `dev` must be embedded in a valid `struct
> + // serdev_device_driver` as guaranteed by the corresponding C code.
> + let sdev = unsafe { container_of!(dev.as_raw(), bindings::serdev_device, dev) };
> +
> + // SAFETY: `sdev` is a valid pointer to a `struct serdev_device_driver`.
> + Ok(unsafe { &*sdev.cast() })
> + }
> +}
> +
> +// 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 {}
^ permalink raw reply [flat|nested] 22+ messages in thread