linux-kernel - Re: [PATCH 4/4] rust: devres: implement register_foreign

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <DATC5A2JBHZQ.33AFQQP6V6L1A@kernel.org>
Date: Sun, 22 Jun 2025 22:14:00 +0200
From: "Benno Lossin" <lossin@...nel.org>
To: "Danilo Krummrich" <dakr@...nel.org>
Cc: <gregkh@...uxfoundation.org>, <rafael@...nel.org>, <ojeda@...nel.org>,
 <alex.gaynor@...il.com>, <boqun.feng@...il.com>, <gary@...yguo.net>,
 <bjorn3_gh@...tonmail.com>, <benno.lossin@...ton.me>,
 <a.hindborg@...nel.org>, <aliceryhl@...gle.com>, <tmgross@...ch.edu>,
 <rust-for-linux@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 4/4] rust: devres: implement register_foreign_release()

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