Re: [PATCH RFC 2/4] rust: add basic serial device bus abstractions

["Danilo Krummrich" <dakr@kernel.org>]

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?