On Tue, Jun 24, 2025 at 11:54:01PM +0200, Danilo Krummrich wrote:
[...]
> +#[pin_data(PinnedDrop)]
> +pub struct Devres<T> {
It makes me realize: I think we need to make `T` being `Send`? Because
the devm callback can happen on a different thread other than
`Devres::new()` and the callback may drop `T` because of revoke(), so we
are essientially sending `T`. Alternatively we can make `Devres::new()`
and its friend require `T` being `Send`.
If it's true, we need a separate patch that "Fixes" this.
(Imagine a Devres<MutexGuard>)
> + dev: ARef<Device>,
> + /// Pointer to [`Self::devres_callback`].
> + ///
> + /// Has to be stored, since Rust does not guarantee to always return the same address for a
> + /// function. However, the C API uses the address as a key.
> + callback: unsafe extern "C" fn(*mut c_void),
> + /// Contains all the fields shared with [`Self::callback`].
> + // TODO: Replace with `UnsafePinned`, once available.
nit: Maybe also reference the `drop_in_place()` in Devres::drop() as
well, because once we use `UnsafePinned`, we don't need that
`drop_in_place()`. But not a big deal, just trying to help the people
who would handle that "TODO" ;-)
> + #[pin]
> + inner: Opaque<Inner<T>>,
> +}
> +
[...]
> +// SAFETY: `Devres` can be send to any task, if `T: Send`.
> +unsafe impl<T: Send> Send for Devres<T> {}
> +
> +// SAFETY: `Devres` can be shared with any task, if `T: Sync`.
> +unsafe impl<T: Sync> Sync for Devres<T> {}
`T` also need to be `Send` for `Devres<T>` to be `Sync` because that's
what `Revocable<T>` requires. (Unless we want `T` always being `Send`
because of the issue I mentioned above)
The rest looks good to me.
Regards,
Boqun
> +
> +#[pinned_drop]
> +impl<T> PinnedDrop for Devres<T> {
> + fn drop(self: Pin<&mut Self>) {
> // SAFETY: When `drop` runs, it is guaranteed that nobody is accessing the revocable data
> // anymore, hence it is safe not to wait for the grace period to finish.
> - if unsafe { self.0.data.revoke_nosync() } {
> - // We revoked `self.0.data` before the devres action did, hence try to remove it.
> - if !DevresInner::remove_action(&self.0) {
> + if unsafe { self.data().revoke_nosync() } {
> + // We revoked `self.data` before the devres action did, hence try to remove it.
> + if !self.remove_action() {
> // We could not remove the devres action, which means that it now runs concurrently,
> - // hence signal that `self.0.data` has been revoked successfully.
> - self.0.revoke.complete_all();
> + // hence signal that `self.data` has been revoked by us successfully.
> + self.inner().revoke.complete_all();
> +
> + // Wait for `Self::devres_callback` to be done using this object.
> + self.inner().devm.wait_for_completion();
> }
> + } else {
> + // `Self::devres_callback` revokes `self.data` for us, hence wait for it to be done
> + // using this object.
> + self.inner().devm.wait_for_completion();
> }
> +
> + // INVARIANT: At this point it is guaranteed that `inner` can't be accessed any more.
> + //
> + // SAFETY: `inner` is valid for dropping.
> + unsafe { core::ptr::drop_in_place(self.inner.get()) };
> }
> }
>
[...]
