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

[Markus Probst]

On Sun, 2026-05-31 at 09:58 +0300, Onur Özkan wrote:
> On Sun, 31 May 2026 01:51:08 +0200
> Danilo Krummrich <dakr@xxxxxxxxxx> 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.
> > 
> 
> Jfyi v9 is on the list [1] and I would say we are pretty close on taking that.
How close? Should I rebase this patch on top of the srcu patch series?

Thanks
- Markus Probst

> 
> Thanks,
> Onur
> 
> [1]: https://lore.kernel.org/rust-for-linux/DIWEXUCNLURG.136XXDBHSBNVR@xxxxxxxxxx
> 
> > > +
> > > +        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: signature.asc
Description: This is a digitally signed message part