Re: [PATCH v3 4/4] rust: devres: implement register_release()

["Benno Lossin" <lossin@xxxxxxxxxx>]

On Tue Jun 24, 2025 at 11:54 PM CEST, Danilo Krummrich wrote:
> +pub fn register_release<P>(dev: &Device<Bound>, data: P) -> Result
> +where
> +    P: ForeignOwnable + 'static,
> +    for<'a> P::Borrowed<'a>: Release,
> +{
> +    let ptr = data.into_foreign();
> +
> +    #[allow(clippy::missing_safety_doc)]
> +    unsafe extern "C" fn callback<P>(ptr: *mut kernel::ffi::c_void)
> +    where
> +        P: ForeignOwnable,
> +        for<'a> P::Borrowed<'a>: Release,
> +    {
> +        // SAFETY: `ptr` is the pointer to the `ForeignOwnable` leaked above and hence valid.
> +        unsafe { P::borrow(ptr.cast()) }.release();
> +
> +        // SAFETY: `ptr` is the pointer to the `ForeignOwnable` leaked above and hence valid.
> +        drop(unsafe { P::from_foreign(ptr.cast()) });

Maybe this function should just be:

    let p = unsafe { P::from_foreign(ptr.cast()) };
    p.release();

And we require `P: ForeignOwnable + Release + 'static`?

We then need these impls instead:

    impl<T: Release, A: Allocator> Release for Pin<Box<T, A>>;
    impl<T: Release, A: Allocator> Release for Box<T, A>;
    impl<T: Release> Release for Arc<T>;

Or, we could change `Release` to be:

    pub trait Release {
        type Ptr: ForeignOwnable;

        fn release(this: Self::Ptr);
    }

and then `register_release` is:

    pub fn register_release<T: Release>(dev: &Device<Bound>, data: T::Ptr) -> Result

This way, one can store a `Box<T>` and get access to the `T` at the end.
Or if they store the value in an `Arc<T>`, they have the option to clone
it and give it to somewhere else.

Related questions:

* should we implement `ForeignOwnable` for `&'static T`?
* should we require `'static` in `ForeignOwnable`? At the moment we only
  have those kinds supported and it only makes sense, a foreign owned
  object can be owned for any amount of time (so it must stay valid
  indefinitely).

---
Cheers,
Benno

> +    }
> +
> +    // SAFETY:
> +    // - `dev.as_raw()` is a pointer to a valid and bound device.
> +    // - `ptr` is a valid pointer the `ForeignOwnable` devres takes ownership of.
> +    to_result(unsafe {
> +        // `devm_add_action_or_reset()` also calls `callback` on failure, such that the
> +        // `ForeignOwnable` is released eventually.
> +        bindings::devm_add_action_or_reset(dev.as_raw(), Some(callback::<P>), ptr.cast())
> +    })
> +}