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 <[email protected]>
>
> 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
signature.asc
Description: This is a digitally signed message part
