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

["Danilo Krummrich" <dakr@kernel.org>]

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.

> +
> +        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)
> +    }
> +}