Re: [PATCH v11 1/3] rust: add basic serial device bus abstractions

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

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

On Sun, 2026-05-31 at 01:51 +0200, Danilo Krummrich wrote:
> On Sun May 31, 2026 at 12:51 AM CEST, Markus Probst via B4 Relay wrote:
> > +#[pinned_drop]
> > +impl<T: Driver> PinnedDrop for PrivateData<'_, T> {
> > +    fn drop(self: Pin<&mut Self>) {
> > +        let mut active = self.active.lock();
> > +        if *active {
> > +            // SAFETY:
> > +            // - We have exclusive access to `self.driver`.
> > +            // - `self.driver` is guaranteed to be initialized.
> > +            unsafe { (*self.driver.get()).assume_init_drop() };
> > +            *active = false;
> > +        }
> > +
> > +        // SAFETY: We have exclusive access to `self.open`.
> > +        if unsafe { *self.open.get() } {
> > +            // SAFETY: `self.sdev.as_raw()` is guaranteed to be a pointer to a valid
> > +            // `struct serdev_device`.
> > +            unsafe { bindings::serdev_device_close(self.sdev.as_raw()) };
> > +        }
> > +    }
> > +}
> 
> Just in case it came across otherwise, I did not mean to push for you to go for
> this approach. We just kept discussing it because it let to the (to me more
> interesting) discussion around the LED class device abstraction.
> 
> While this approach gets us rid of an extra allocation and the rust_private_data
> pointer in struct serdev_device it also turns out a bit more convoluted.
> 
> That said, both are correct, and I won't object either one; up to you and the
> serdev / tty maintainers.
> 
> Please wait a bit before resending, so other people can comment on this as well.
> 
> > +    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 snuck back in, should be BoundInternal.
> 
> > +
> > +        // 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<PrivateData<'_, T>>>`.
> > +        let private_data = unsafe { sdev.as_ref().drvdata_borrow::<PrivateData<'_, T>>() };
> > +        let active = private_data.active.lock();
> 
> I think SRCU would be a much better fit, but the code didn't land yet, so the
> mutex seems fine for now. But I'd probably add a TODO.
I just noticed, is it even possible to use SRCU here? Currently the
mutex not only ensures that no drvdata access happens after drvdata
drop, but also that the receive_buf waits for the probe to complete, as
the drvdata hasn't been initialized yet.

(drvdata refers to PrivateData.driver in this context, not the whole
PrivateData)

Thanks
- Markus Probst

> 
> > +
> > +        if !*active {
> > +            return length;
> > +        }
> > +
> > +        // SAFETY: No one has exclusive access to `private_data.driver`.
> > +        let data = unsafe { &*private_data.driver.get() };
> > +        // SAFETY:
> > +        // - `private_data.driver` is pinned.
> > +        // - `receive_buf_callback` is only ever called after a successful call to `probe_callback`,
> > +        //   hence it's guaranteed that `private_data.driver` was initialized.
> > +        let data_pinned = unsafe { Pin::new_unchecked(data.assume_init_ref()) };
> > +
> > +        // 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_pinned, buf)
> > +    }
> > +}

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 870 bytes --]