On Sun, 31 May 2026 09:58:34 +0300 Onur Özkan <[email protected]> wrote:
> On Sun, 31 May 2026 01:51:08 +0200 > Danilo Krummrich <[email protected]> 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. > > Thanks, > Onur > > [1]: > https://lore.kernel.org/rust-for-linux/[email protected] > I pasted the wrong link, sorry. It should have been this instead: https://lore.kernel.org/all/[email protected] > > > + > > > + 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) > > > + } > > > +}
