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

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

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

On Sat, 2026-05-30 at 15:53 +0200, Markus Probst wrote:
> 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.
Ok, forget it. Only works safely in theory. `serdev_device_open` could
fail and that would be unsound again.

Thanks
- Markus Probst

> 
> 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 --]