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

["Benno Lossin" <lossin@kernel.org>]

On Sun Jun 22, 2025 at 2:46 PM CEST, Danilo Krummrich wrote:
> On Sun, Jun 22, 2025 at 09:26:33AM +0200, Benno Lossin wrote:
>> On Thu Jun 12, 2025 at 4:51 PM CEST, Danilo Krummrich wrote:
>> > +pub trait Release {
>> > +    /// Called once the [`Device`] given to [`register_foreign_release`] is unbound.
>> > +    fn release(&self);
>> 
>> Would it make sense to also supply the `Device` that this is attached
>> to? In case you have one object in multiple `register_foreign_release`
>> calls with different devices, or is that something that doesn't happen?
>
> No, doing that wouldn't make any sense. A resource should only be bound to the
> lifetime of a single device.

But the API doesn't prevent it... Does it make more sense to instead ask
the user to provide a closure?

>> > +}
>> > +
>> > +impl<T: Release> Release for crate::sync::ArcBorrow<'_, T> {
>> > +    fn release(&self) {
>> > +        self.deref().release();
>> > +    }
>> > +}
>> > +
>> > +impl<T: Release> Release for Pin<&'_ T> {
>> > +    fn release(&self) {
>> > +        self.deref().release();
>> > +    }
>> > +}
>> 
>> We should also implement it for `&T`, since that is `Box`'s `Borrowed`.
>
> That should implicitly be the case when T: Release, where T is P<T>.

I don't understand? When `P` is set to `Box<T>` in the
`register_foreign_release` below, then `P::Borrow<'_> == &'_ T` and the
bound of `&T: Release` is not satisfied.

>> > +
>> > +/// Consume the `data`, [`Release::release`] and [`Drop::drop`] `data` once `dev` is unbound.
>> > +///
>> > +/// # Examples
>> > +///
>> > +/// ```no_run
>> > +/// use kernel::{device::{Bound, Device}, devres, devres::Release, sync::Arc};
>> > +///
>> > +/// struct Registration<T> {
>> 
>> Maybe add some explanation above/below this example. It looks like a new
>> bus registration?
>
> *class device registration, see Registration::new() below. But I can also add a
> brief comment to the struct. It's indeed a bit subtly this way. :)

I'd just add a small paragraph explaining what it does :)

---
Cheers,
Benno