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

[Markus Probst]

On Sat, 2026-05-30 at 15:10 +0200, Danilo Krummrich wrote:
> 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.
`serdev::Device<Core>` will be passed as argument to unbind, i.e.
set_baudrate, set_flow_control etc. calls are possible. If we close the
serdev device before unbind, calling those would result in a null
pointer dereference. Maybe a driver even wants to send a message here,
so it should stay open.

If I think about it, I can't even close it inside remove_callback after
`T::unbind`. With the driver lifetimes, the driver could store the
`serdev::Device<Bound>` pointer in its DriverData and could still make
calls to the device.

Any suggestions?

> 
> > +
> > +            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)
> > +    }
> > +}

Thanks
- Markus Probst

Attachment: signature.asc
Description: This is a digitally signed message part