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)
> > + }
> > +}
signature.asc
Description: This is a digitally signed message part
