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

[Markus Probst <markus.probst@posteo.de>]

[-- Attachment #1: Type: text/plain, Size: 7051 bytes --]

On Sat, 2026-05-30 at 15:37 +0200, Markus Probst wrote:
> 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?
I could keep calling devm, but close it before unbind and reopen it
without the receive ops. The devm will close it then again.

This way it stays open until the last devres callback has run, but
doesn't call the receive_buf_callback while the drvdata has been
dropped.

I will test it and see if it works.

Thanks
- Markus Probst

> 
> > 
> > > +
> > > +            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@kernel.org>
> > 
> > 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 #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 870 bytes --]