Re: [PATCH v8 3/5] rust: add basic serial device bus abstractions

[Danilo Krummrich]

On Sat May 30, 2026 at 3:13 AM CEST, Markus Probst via B4 Relay wrote:
> +    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 private_data = devres::register(
> +                sdev.as_ref(),
> +                try_pin_init!(PrivateData {
> +                    probe_complete <- Completion::new(),
> +                    error: false.into(),
> +                }),
> +                GFP_KERNEL,
> +            )?;
> +
> +            // SAFETY: `sdev.as_raw()` is guaranteed to be a valid pointer to `serdev_device`.
> +            unsafe {
> +                (*sdev.as_raw()).rust_private_data =
> +                    (&raw const *private_data).cast::<c_void>().cast_mut()
> +            };
> +
> +            // SAFETY: `sdev.as_raw()` is guaranteed to be a valid pointer to `serdev_device`.
> +            unsafe { bindings::serdev_device_set_client_ops(sdev.as_raw(), Self::OPS) };
> +
> +            // SAFETY: The serial device bus only ever calls the probe callback with a valid pointer
> +            // to a `serdev_device`.
> +            to_result(unsafe {
> +                bindings::devm_serdev_device_open(sdev.as_ref().as_raw(), sdev.as_raw())
> +            })?;

The bus device private data drops before devres callbacks run, thus you can't
use the devm helper here. I suggest to use serdev_device_open() and call
serdev_device_close() from remove_callback() instead.

You may also want to consider whether you potentially want T::unbind() to allow
to have concurrent transmits in flight regardless. IOW, you may want to call
serdev_device_close() before T::unbind() anyway.

> +
> +            let data = T::probe(sdev, info);
> +            let result = sdev.as_ref().set_drvdata(data);
> +
> +            // SAFETY: We have exclusive access to `private_data.error`.
> +            unsafe { *private_data.error.get() = result.is_err() };
> +
> +            private_data.probe_complete.complete_all();
> +
> +            result.map(|()| 0)
> +        })
> +    }
> +
> +    extern "C" fn remove_callback(sdev: *mut bindings::serdev_device) {
> +        // SAFETY: The serial device bus only ever calls the remove callback with a valid pointer
> +        // to a `struct serdev_device`.
> +        //
> +        // INVARIANT: `sdev` is valid for the duration of `remove_callback()`.
> +        let sdev = unsafe { &*sdev.cast::<Device<device::CoreInternal<'_>>>() };
> +
> +        // SAFETY: `remove_callback` is only ever called after a successful call to
> +        // `probe_callback`, hence it's guaranteed that `Device::set_drvdata()` has been called
> +        // and stored a `Pin<KBox<T::Data<'_>>>`.
> +        let data = unsafe { sdev.as_ref().drvdata_borrow::<T::Data<'_>>() };
> +
> +        T::unbind(sdev, 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<'_>>>() };

CoreInternal dereferences to Core, which is technically unsound within this
callback.

I think you want CoreInternal for drvdata_borrow(), so we should add a
BoundInternal type state for this; I will send a patch for this.

With this and the above fixed,

Reviewed-by: Danilo Krummrich <dakr@xxxxxxxxxx>

from the driver-core side of things.

> +        // SAFETY:
> +        // - The serial device bus only ever calls the receive buf callback with a valid pointer to
> +        //   a `struct serdev_device`.
> +        // - `receive_buf_callback` is only ever called after a successful call to
> +        //   `probe_callback`, hence it's guaranteed that `sdev.private_data` is a pointer
> +        //   to a valid `PrivateData`.
> +        let private_data = unsafe { &*(*sdev.as_raw()).rust_private_data.cast::<PrivateData>() };
> +
> +        private_data.probe_complete.wait_for_completion();
> +
> +        // SAFETY: No one has exclusive access to `private_data.error`.
> +        if unsafe { *private_data.error.get() } {
> +            return length;
> +        }
> +
> +        // 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<T::Data<'_>>>`.
> +        let data = unsafe { sdev.as_ref().drvdata_borrow::<T::Data<'_>>() };
> +
> +        // SAFETY: `buf` is guaranteed to be non-null and has the size of `length`.
> +        let buf = unsafe { core::slice::from_raw_parts(buf, length) };
> +
> +        T::receive(sdev, data, buf)
> +    }
> +}