Re: [PATCH 2/3] rust: devres: implement Devres::access_with()

Re: [PATCH 2/3] rust: devres: implement Devres::access_with()

[Benno Lossin]

On Sat Apr 26, 2025 at 3:30 PM CEST, Danilo Krummrich wrote:
> Implement a direct accessor for the data stored within the Devres for
> cases where we can proof that we own a reference to a Device<Bound>
> (i.e. a bound device) of the same device that was used to create the
> corresponding Devres container.
>
> Usually, when accessing the data stored within a Devres container, it is
> not clear whether the data has been revoked already due to the device
> being unbound and, hence, we have to try whether the access is possible
> and subsequently keep holding the RCU read lock for the duration of the
> access.
>
> However, when we can proof that we hold a reference to Device<Bound>
> matching the device the Devres container has been created with, we can
> guarantee that the device is not unbound for the duration of the
> lifetime of the Device<Bound> reference and, hence, it is not possible
> for the data within the Devres container to be revoked.
>
> Therefore, in this case, we can bypass the atomic check and the RCU read
> lock, which is a great optimization and simplification for drivers.
>
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> ---
>  rust/kernel/devres.rs | 35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
>
> diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
> index 1e58f5d22044..ec2cd9cdda8b 100644
> --- a/rust/kernel/devres.rs
> +++ b/rust/kernel/devres.rs
> @@ -181,6 +181,41 @@ pub fn new_foreign_owned(dev: &Device<Bound>, data: T, flags: Flags) -> Result {
>  
>          Ok(())
>      }
> +
> +    /// Obtain `&'a T`, bypassing the [`Revocable`].
> +    ///
> +    /// This method allows to directly obtain a `&'a T`, bypassing the [`Revocable`], by presenting
> +    /// a `&'a Device<Bound>` of the same [`Device`] this [`Devres`] instance has been created with.
> +    ///
> +    /// An error is returned if `dev` does not match the same [`Device`] this [`Devres`] instance
> +    /// has been created with.
> +    ///
> +    /// # Example
> +    ///
> +    /// ```no_run

The `no_run` is not necessary, as you don't run any code, you only
define a function.

> +    /// # use kernel::{device::Core, devres::Devres, pci};
> +    ///
> +    /// fn from_core(dev: &pci::Device<Core>, devres: Devres<pci::Bar<0x4>>) -> Result<()> {
> +    ///     let bar = devres.access_with(dev.as_ref())?;
> +    ///
> +    ///     let _ = bar.read32(0x0);
> +    ///
> +    ///     // might_sleep()
> +    ///
> +    ///     bar.write32(0x42, 0x0);
> +    ///
> +    ///     Ok(())
> +    /// }

Missing '```'?

> +    pub fn access_with<'s, 'd: 's>(&'s self, dev: &'d Device<Bound>) -> Result<&'s T> {

I don't think that we need the `'d` lifetime here (if not, we should
remove it).

> +        if self.0.dev.as_raw() != dev.as_raw() {
> +            return Err(EINVAL);
> +        }
> +
> +        // SAFETY: `dev` being the same device as the device this `Devres` has been created for
> +        // proofes that `self.0.data` hasn't been revoked and is guaranteed to not be revoked as

s/proofes/proves/

---
Cheers,
Benno

> +        // long as `dev` lives; `dev` lives at least as long as `self`.
> +        Ok(unsafe { self.deref().access() })
> +    }
>  }
>  
>  impl<T> Deref for Devres<T> {

Re: [PATCH 2/3] rust: devres: implement Devres::access_with()

[Danilo Krummrich]

On Sat, Apr 26, 2025 at 08:28:30PM +0000, Benno Lossin wrote:
> On Sat Apr 26, 2025 at 3:30 PM CEST, Danilo Krummrich wrote:
> > Implement a direct accessor for the data stored within the Devres for
> > cases where we can proof that we own a reference to a Device<Bound>
> > (i.e. a bound device) of the same device that was used to create the
> > corresponding Devres container.
> >
> > Usually, when accessing the data stored within a Devres container, it is
> > not clear whether the data has been revoked already due to the device
> > being unbound and, hence, we have to try whether the access is possible
> > and subsequently keep holding the RCU read lock for the duration of the
> > access.
> >
> > However, when we can proof that we hold a reference to Device<Bound>
> > matching the device the Devres container has been created with, we can
> > guarantee that the device is not unbound for the duration of the
> > lifetime of the Device<Bound> reference and, hence, it is not possible
> > for the data within the Devres container to be revoked.
> >
> > Therefore, in this case, we can bypass the atomic check and the RCU read
> > lock, which is a great optimization and simplification for drivers.
> >
> > Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> > ---
> >  rust/kernel/devres.rs | 35 +++++++++++++++++++++++++++++++++++
> >  1 file changed, 35 insertions(+)
> >
> > diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
> > index 1e58f5d22044..ec2cd9cdda8b 100644
> > --- a/rust/kernel/devres.rs
> > +++ b/rust/kernel/devres.rs
> > @@ -181,6 +181,41 @@ pub fn new_foreign_owned(dev: &Device<Bound>, data: T, flags: Flags) -> Result {
> >  
> >          Ok(())
> >      }
> > +
> > +    /// Obtain `&'a T`, bypassing the [`Revocable`].
> > +    ///
> > +    /// This method allows to directly obtain a `&'a T`, bypassing the [`Revocable`], by presenting
> > +    /// a `&'a Device<Bound>` of the same [`Device`] this [`Devres`] instance has been created with.
> > +    ///
> > +    /// An error is returned if `dev` does not match the same [`Device`] this [`Devres`] instance
> > +    /// has been created with.
> > +    ///
> > +    /// # Example
> > +    ///
> > +    /// ```no_run
> 
> The `no_run` is not necessary, as you don't run any code, you only
> define a function.

Yet, I'd like to keep it to make it obvious that this test isn't supposed to
run.

> > +    /// # use kernel::{device::Core, devres::Devres, pci};
> > +    ///
> > +    /// fn from_core(dev: &pci::Device<Core>, devres: Devres<pci::Bar<0x4>>) -> Result<()> {
> > +    ///     let bar = devres.access_with(dev.as_ref())?;
> > +    ///
> > +    ///     let _ = bar.read32(0x0);
> > +    ///
> > +    ///     // might_sleep()
> > +    ///
> > +    ///     bar.write32(0x42, 0x0);
> > +    ///
> > +    ///     Ok(())
> > +    /// }
> 
> Missing '```'?

Good catch -- interestingly the doctest did compile anyways.

> 
> > +    pub fn access_with<'s, 'd: 's>(&'s self, dev: &'d Device<Bound>) -> Result<&'s T> {
> 
> I don't think that we need the `'d` lifetime here (if not, we should
> remove it).

If the returned reference out-lives dev it can become invalid, since it means
that the device could subsequently be unbound. Hence, I think we indeed need to
require that the returned reference cannot out-live dev.

Re: [PATCH 2/3] rust: devres: implement Devres::access_with()

[Benno Lossin]

On Sat Apr 26, 2025 at 11:24 PM CEST, Danilo Krummrich wrote:
> On Sat, Apr 26, 2025 at 08:28:30PM +0000, Benno Lossin wrote:
>> On Sat Apr 26, 2025 at 3:30 PM CEST, Danilo Krummrich wrote:
>> > +    pub fn access_with<'s, 'd: 's>(&'s self, dev: &'d Device<Bound>) -> Result<&'s T> {
>> 
>> I don't think that we need the `'d` lifetime here (if not, we should
>> remove it).
>
> If the returned reference out-lives dev it can become invalid, since it means
> that the device could subsequently be unbound. Hence, I think we indeed need to
> require that the returned reference cannot out-live dev.

I meant the following signature:

    pub fn access_with<'a>(&'a self, dev: &'a Device<Bound>) -> Result<&'a T>

You don't need to specify the additional `'d` one, since lifetimes allow
subtyping [1]. So if I have a `&'s self` and a `&'d Device<Bound>` and
`'d: 's`, then I can supply those arguments to my suggested function and
the compiler will shorten `'d` to be `'s` or whatever is correct in the
context.

[1]: https://doc.rust-lang.org/nomicon/subtyping.html#subtyping

---
Cheers,
Benno

Re: [PATCH 2/3] rust: devres: implement Devres::access_with()

[Danilo Krummrich]

On Sun, Apr 27, 2025 at 08:41:02AM +0000, Benno Lossin wrote:
> On Sat Apr 26, 2025 at 11:24 PM CEST, Danilo Krummrich wrote:
> > On Sat, Apr 26, 2025 at 08:28:30PM +0000, Benno Lossin wrote:
> >> On Sat Apr 26, 2025 at 3:30 PM CEST, Danilo Krummrich wrote:
> >> > +    pub fn access_with<'s, 'd: 's>(&'s self, dev: &'d Device<Bound>) -> Result<&'s T> {
> >> 
> >> I don't think that we need the `'d` lifetime here (if not, we should
> >> remove it).
> >
> > If the returned reference out-lives dev it can become invalid, since it means
> > that the device could subsequently be unbound. Hence, I think we indeed need to
> > require that the returned reference cannot out-live dev.
> 
> I meant the following signature:
> 
>     pub fn access_with<'a>(&'a self, dev: &'a Device<Bound>) -> Result<&'a T>
> 
> You don't need to specify the additional `'d` one, since lifetimes allow
> subtyping [1]. So if I have a `&'s self` and a `&'d Device<Bound>` and
> `'d: 's`, then I can supply those arguments to my suggested function and
> the compiler will shorten `'d` to be `'s` or whatever is correct in the
> context.
> 
> [1]: https://doc.rust-lang.org/nomicon/subtyping.html#subtyping

Makes sense, and I don't mind changing it, but I still think the orignal version
makes the actual requirement more obvious to the reader, i.e. dev must live *at
least* as long as self, but not dev must live *exactly* as long as self.

Re: [PATCH 2/3] rust: devres: implement Devres::access_with()

[Benno Lossin]

On Sun Apr 27, 2025 at 12:10 PM CEST, Danilo Krummrich wrote:
> On Sun, Apr 27, 2025 at 08:41:02AM +0000, Benno Lossin wrote:
>> On Sat Apr 26, 2025 at 11:24 PM CEST, Danilo Krummrich wrote:
>> > On Sat, Apr 26, 2025 at 08:28:30PM +0000, Benno Lossin wrote:
>> >> On Sat Apr 26, 2025 at 3:30 PM CEST, Danilo Krummrich wrote:
>> >> > +    pub fn access_with<'s, 'd: 's>(&'s self, dev: &'d Device<Bound>) -> Result<&'s T> {
>> >> 
>> >> I don't think that we need the `'d` lifetime here (if not, we should
>> >> remove it).
>> >
>> > If the returned reference out-lives dev it can become invalid, since it means
>> > that the device could subsequently be unbound. Hence, I think we indeed need to
>> > require that the returned reference cannot out-live dev.
>> 
>> I meant the following signature:
>> 
>>     pub fn access_with<'a>(&'a self, dev: &'a Device<Bound>) -> Result<&'a T>
>> 
>> You don't need to specify the additional `'d` one, since lifetimes allow
>> subtyping [1]. So if I have a `&'s self` and a `&'d Device<Bound>` and
>> `'d: 's`, then I can supply those arguments to my suggested function and
>> the compiler will shorten `'d` to be `'s` or whatever is correct in the
>> context.
>> 
>> [1]: https://doc.rust-lang.org/nomicon/subtyping.html#subtyping
>
> Makes sense, and I don't mind changing it, but I still think the orignal version
> makes the actual requirement more obvious to the reader, i.e. dev must live *at
> least* as long as self, but not dev must live *exactly* as long as self.

I think it makes the function harder to read, since you have multiple
lifetimes around. Once one gets used to the subtyping rule, it's much
better to reduce the total amount of lifetimes. Otherwise it seems to me
as if it's more complicated.

---
Cheers,
Benno

Re: [PATCH 2/3] rust: devres: implement Devres::access_with()

[Danilo Krummrich]

On Sun, Apr 27, 2025 at 05:11:12PM +0000, Benno Lossin wrote:
> On Sun Apr 27, 2025 at 12:10 PM CEST, Danilo Krummrich wrote:
> > On Sun, Apr 27, 2025 at 08:41:02AM +0000, Benno Lossin wrote:
> >> On Sat Apr 26, 2025 at 11:24 PM CEST, Danilo Krummrich wrote:
> >> > On Sat, Apr 26, 2025 at 08:28:30PM +0000, Benno Lossin wrote:
> >> >> On Sat Apr 26, 2025 at 3:30 PM CEST, Danilo Krummrich wrote:
> >> >> > +    pub fn access_with<'s, 'd: 's>(&'s self, dev: &'d Device<Bound>) -> Result<&'s T> {
> >> >> 
> >> >> I don't think that we need the `'d` lifetime here (if not, we should
> >> >> remove it).
> >> >
> >> > If the returned reference out-lives dev it can become invalid, since it means
> >> > that the device could subsequently be unbound. Hence, I think we indeed need to
> >> > require that the returned reference cannot out-live dev.
> >> 
> >> I meant the following signature:
> >> 
> >>     pub fn access_with<'a>(&'a self, dev: &'a Device<Bound>) -> Result<&'a T>
> >> 
> >> You don't need to specify the additional `'d` one, since lifetimes allow
> >> subtyping [1]. So if I have a `&'s self` and a `&'d Device<Bound>` and
> >> `'d: 's`, then I can supply those arguments to my suggested function and
> >> the compiler will shorten `'d` to be `'s` or whatever is correct in the
> >> context.
> >> 
> >> [1]: https://doc.rust-lang.org/nomicon/subtyping.html#subtyping
> >
> > Makes sense, and I don't mind changing it, but I still think the orignal version
> > makes the actual requirement more obvious to the reader, i.e. dev must live *at
> > least* as long as self, but not dev must live *exactly* as long as self.
> 
> I think it makes the function harder to read, since you have multiple
> lifetimes around. Once one gets used to the subtyping rule, it's much
> better to reduce the total amount of lifetimes. Otherwise it seems to me
> as if it's more complicated.

As mentioned above, I don't mind changing it, so I'll drop the 'd lifetime.

[PATCH 0/3] Devres optimization with bound devices

[Danilo Krummrich]

This patch series implements a direct accessor for the data stored within
a Devres container for cases where we can proof that we own a reference
to a Device<Bound> (i.e. a bound device) of the same device that was used
to create the corresponding Devres container.

Usually, when accessing the data stored within a Devres container, it is
not clear whether the data has been revoked already due to the device
being unbound and, hence, we have to try whether the access is possible
and subsequently keep holding the RCU read lock for the duration of the
access.

However, when we can proof that we hold a reference to Device<Bound>
matching the device the Devres container has been created with, we can
guarantee that the device is not unbound for the duration of the
lifetime of the Device<Bound> reference and, hence, it is not possible
for the data within the Devres container to be revoked.

Therefore, in this case, we can bypass the atomic check and the RCU read
lock, which is a great optimization and simplification for drivers.

The patches of this series are also available in [1].

[1] https://web.git.kernel.org/pub/scm/linux/kernel/git/dakr/linux.git/log/?h=rust/devres

Danilo Krummrich (3):
  rust: revocable: implement Revocable::access()
  rust: devres: implement Devres::access_with()
  samples: rust: pci: take advantage of Devres::access_with()

 rust/kernel/devres.rs           | 35 +++++++++++++++++++++++++++++++++
 rust/kernel/revocable.rs        | 12 +++++++++++
 samples/rust/rust_driver_pci.rs | 12 +++++------
 3 files changed, 53 insertions(+), 6 deletions(-)


base-commit: b5cb47f81a2857d270cabbbb3a9feec0e483caed
-- 
2.49.0

[PATCH 2/3] rust: devres: implement Devres::access_with()

[Danilo Krummrich]

Implement a direct accessor for the data stored within the Devres for
cases where we can proof that we own a reference to a Device<Bound>
(i.e. a bound device) of the same device that was used to create the
corresponding Devres container.

Usually, when accessing the data stored within a Devres container, it is
not clear whether the data has been revoked already due to the device
being unbound and, hence, we have to try whether the access is possible
and subsequently keep holding the RCU read lock for the duration of the
access.

However, when we can proof that we hold a reference to Device<Bound>
matching the device the Devres container has been created with, we can
guarantee that the device is not unbound for the duration of the
lifetime of the Device<Bound> reference and, hence, it is not possible
for the data within the Devres container to be revoked.

Therefore, in this case, we can bypass the atomic check and the RCU read
lock, which is a great optimization and simplification for drivers.

Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 rust/kernel/devres.rs | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
index 1e58f5d22044..ec2cd9cdda8b 100644
--- a/rust/kernel/devres.rs
+++ b/rust/kernel/devres.rs
@@ -181,6 +181,41 @@ pub fn new_foreign_owned(dev: &Device<Bound>, data: T, flags: Flags) -> Result {
 
         Ok(())
     }
+
+    /// Obtain `&'a T`, bypassing the [`Revocable`].
+    ///
+    /// This method allows to directly obtain a `&'a T`, bypassing the [`Revocable`], by presenting
+    /// a `&'a Device<Bound>` of the same [`Device`] this [`Devres`] instance has been created with.
+    ///
+    /// An error is returned if `dev` does not match the same [`Device`] this [`Devres`] instance
+    /// has been created with.
+    ///
+    /// # Example
+    ///
+    /// ```no_run
+    /// # use kernel::{device::Core, devres::Devres, pci};
+    ///
+    /// fn from_core(dev: &pci::Device<Core>, devres: Devres<pci::Bar<0x4>>) -> Result<()> {
+    ///     let bar = devres.access_with(dev.as_ref())?;
+    ///
+    ///     let _ = bar.read32(0x0);
+    ///
+    ///     // might_sleep()
+    ///
+    ///     bar.write32(0x42, 0x0);
+    ///
+    ///     Ok(())
+    /// }
+    pub fn access_with<'s, 'd: 's>(&'s self, dev: &'d Device<Bound>) -> Result<&'s T> {
+        if self.0.dev.as_raw() != dev.as_raw() {
+            return Err(EINVAL);
+        }
+
+        // SAFETY: `dev` being the same device as the device this `Devres` has been created for
+        // proofes that `self.0.data` hasn't been revoked and is guaranteed to not be revoked as
+        // long as `dev` lives; `dev` lives at least as long as `self`.
+        Ok(unsafe { self.deref().access() })
+    }
 }
 
 impl<T> Deref for Devres<T> {
-- 
2.49.0

Re: [PATCH 2/3] rust: devres: implement Devres::access_with()

[Christian Schrefl]

On 26.04.25 3:30 PM, Danilo Krummrich wrote:
> Implement a direct accessor for the data stored within the Devres for
> cases where we can proof that we own a reference to a Device<Bound>
> (i.e. a bound device) of the same device that was used to create the
> corresponding Devres container.
> 
> Usually, when accessing the data stored within a Devres container, it is
> not clear whether the data has been revoked already due to the device
> being unbound and, hence, we have to try whether the access is possible
> and subsequently keep holding the RCU read lock for the duration of the
> access.
> 
> However, when we can proof that we hold a reference to Device<Bound>
> matching the device the Devres container has been created with, we can
> guarantee that the device is not unbound for the duration of the
> lifetime of the Device<Bound> reference and, hence, it is not possible
> for the data within the Devres container to be revoked.
> 
> Therefore, in this case, we can bypass the atomic check and the RCU read
> lock, which is a great optimization and simplification for drivers.
> 
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> ---
>  rust/kernel/devres.rs | 35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
> index 1e58f5d22044..ec2cd9cdda8b 100644
> --- a/rust/kernel/devres.rs
> +++ b/rust/kernel/devres.rs
> @@ -181,6 +181,41 @@ pub fn new_foreign_owned(dev: &Device<Bound>, data: T, flags: Flags) -> Result {
>  
>          Ok(())
>      }
> +
> +    /// Obtain `&'a T`, bypassing the [`Revocable`].
> +    ///
> +    /// This method allows to directly obtain a `&'a T`, bypassing the [`Revocable`], by presenting
> +    /// a `&'a Device<Bound>` of the same [`Device`] this [`Devres`] instance has been created with.
> +    ///
> +    /// An error is returned if `dev` does not match the same [`Device`] this [`Devres`] instance
> +    /// has been created with.

I would prefer this as a `# Errors` section.

Also are there any cases where this is actually wanted as an error?
I'm not very familiar with the `Revocable` infrastructure,
but I would assume a mismatch here to be a bug in almost every case,
so a panic here might be reasonable.
(I would be fine with a reason for using an error here in the 
commit message or documentation/comments)

With that:

Reviewed-by: Christian Schrefl <chrisi.schrefl@gmail.com>

> +    ///
> +    /// # Example
> +    ///
> +    /// ```no_run
> +    /// # use kernel::{device::Core, devres::Devres, pci};
> +    ///
> +    /// fn from_core(dev: &pci::Device<Core>, devres: Devres<pci::Bar<0x4>>) -> Result<()> {
> +    ///     let bar = devres.access_with(dev.as_ref())?;
> +    ///
> +    ///     let _ = bar.read32(0x0);
> +    ///
> +    ///     // might_sleep()
> +    ///
> +    ///     bar.write32(0x42, 0x0);
> +    ///
> +    ///     Ok(())
> +    /// }
> +    pub fn access_with<'s, 'd: 's>(&'s self, dev: &'d Device<Bound>) -> Result<&'s T> {
> +        if self.0.dev.as_raw() != dev.as_raw() {
> +            return Err(EINVAL);
> +        }
> +
> +        // SAFETY: `dev` being the same device as the device this `Devres` has been created for
> +        // proofes that `self.0.data` hasn't been revoked and is guaranteed to not be revoked as
> +        // long as `dev` lives; `dev` lives at least as long as `self`.
> +        Ok(unsafe { self.deref().access() })
> +    }
>  }
>  
>  impl<T> Deref for Devres<T> {

Re: [PATCH 2/3] rust: devres: implement Devres::access_with()

[Danilo Krummrich]

On Sat, Apr 26, 2025 at 06:53:10PM +0200, Christian Schrefl wrote:
> On 26.04.25 3:30 PM, Danilo Krummrich wrote:
> > Implement a direct accessor for the data stored within the Devres for
> > cases where we can proof that we own a reference to a Device<Bound>
> > (i.e. a bound device) of the same device that was used to create the
> > corresponding Devres container.
> > 
> > Usually, when accessing the data stored within a Devres container, it is
> > not clear whether the data has been revoked already due to the device
> > being unbound and, hence, we have to try whether the access is possible
> > and subsequently keep holding the RCU read lock for the duration of the
> > access.
> > 
> > However, when we can proof that we hold a reference to Device<Bound>
> > matching the device the Devres container has been created with, we can
> > guarantee that the device is not unbound for the duration of the
> > lifetime of the Device<Bound> reference and, hence, it is not possible
> > for the data within the Devres container to be revoked.
> > 
> > Therefore, in this case, we can bypass the atomic check and the RCU read
> > lock, which is a great optimization and simplification for drivers.
> > 
> > Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> > ---
> >  rust/kernel/devres.rs | 35 +++++++++++++++++++++++++++++++++++
> >  1 file changed, 35 insertions(+)
> > 
> > diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
> > index 1e58f5d22044..ec2cd9cdda8b 100644
> > --- a/rust/kernel/devres.rs
> > +++ b/rust/kernel/devres.rs
> > @@ -181,6 +181,41 @@ pub fn new_foreign_owned(dev: &Device<Bound>, data: T, flags: Flags) -> Result {
> >  
> >          Ok(())
> >      }
> > +
> > +    /// Obtain `&'a T`, bypassing the [`Revocable`].
> > +    ///
> > +    /// This method allows to directly obtain a `&'a T`, bypassing the [`Revocable`], by presenting
> > +    /// a `&'a Device<Bound>` of the same [`Device`] this [`Devres`] instance has been created with.
> > +    ///
> > +    /// An error is returned if `dev` does not match the same [`Device`] this [`Devres`] instance
> > +    /// has been created with.
> 
> I would prefer this as a `# Errors` section.

I can make this an # Errors section.

> Also are there any cases where this is actually wanted as an error?
> I'm not very familiar with the `Revocable` infrastructure,
> but I would assume a mismatch here to be a bug in almost every case,
> so a panic here might be reasonable.

Passing in a device reference that doesn't match the device the Devres instance
was created with would indeed be a bug, but a panic isn't the solution, since we
can handle this error just fine.

We never panic the whole kernel unless things go so utterly wrong that we can't
can't recover from it; e.g. if otherwise we'd likely corrupt memory, etc.

> (I would be fine with a reason for using an error here in the 
> commit message or documentation/comments)

I don't think we need this in this commit or the method's documentation, it's a
general kernel rule not to panic unless there's really no other way.

> With that:
> 
> Reviewed-by: Christian Schrefl <chrisi.schrefl@gmail.com>
> 
> > +    ///
> > +    /// # Example
> > +    ///
> > +    /// ```no_run
> > +    /// # use kernel::{device::Core, devres::Devres, pci};
> > +    ///
> > +    /// fn from_core(dev: &pci::Device<Core>, devres: Devres<pci::Bar<0x4>>) -> Result<()> {
> > +    ///     let bar = devres.access_with(dev.as_ref())?;
> > +    ///
> > +    ///     let _ = bar.read32(0x0);
> > +    ///
> > +    ///     // might_sleep()
> > +    ///
> > +    ///     bar.write32(0x42, 0x0);
> > +    ///
> > +    ///     Ok(())
> > +    /// }
> > +    pub fn access_with<'s, 'd: 's>(&'s self, dev: &'d Device<Bound>) -> Result<&'s T> {
> > +        if self.0.dev.as_raw() != dev.as_raw() {
> > +            return Err(EINVAL);
> > +        }
> > +
> > +        // SAFETY: `dev` being the same device as the device this `Devres` has been created for
> > +        // proofes that `self.0.data` hasn't been revoked and is guaranteed to not be revoked as
> > +        // long as `dev` lives; `dev` lives at least as long as `self`.
> > +        Ok(unsafe { self.deref().access() })
> > +    }
> >  }
> >  
> >  impl<T> Deref for Devres<T> {
> 
> 

Re: [PATCH 2/3] rust: devres: implement Devres::access_with()

[Christian Schrefl]

On 26.04.25 7:08 PM, Danilo Krummrich wrote:
> On Sat, Apr 26, 2025 at 06:53:10PM +0200, Christian Schrefl wrote:
>> On 26.04.25 3:30 PM, Danilo Krummrich wrote:
>>> Implement a direct accessor for the data stored within the Devres for
>>> cases where we can proof that we own a reference to a Device<Bound>
>>> (i.e. a bound device) of the same device that was used to create the
>>> corresponding Devres container.
>>>
>>> Usually, when accessing the data stored within a Devres container, it is
>>> not clear whether the data has been revoked already due to the device
>>> being unbound and, hence, we have to try whether the access is possible
>>> and subsequently keep holding the RCU read lock for the duration of the
>>> access.
>>>
>>> However, when we can proof that we hold a reference to Device<Bound>
>>> matching the device the Devres container has been created with, we can
>>> guarantee that the device is not unbound for the duration of the
>>> lifetime of the Device<Bound> reference and, hence, it is not possible
>>> for the data within the Devres container to be revoked.
>>>
>>> Therefore, in this case, we can bypass the atomic check and the RCU read
>>> lock, which is a great optimization and simplification for drivers.
>>>
>>> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
>>> ---
>>>  rust/kernel/devres.rs | 35 +++++++++++++++++++++++++++++++++++
>>>  1 file changed, 35 insertions(+)
>>>
>>> diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
>>> index 1e58f5d22044..ec2cd9cdda8b 100644
>>> --- a/rust/kernel/devres.rs
>>> +++ b/rust/kernel/devres.rs
>>> @@ -181,6 +181,41 @@ pub fn new_foreign_owned(dev: &Device<Bound>, data: T, flags: Flags) -> Result {
>>>  
>>>          Ok(())
>>>      }
>>> +
>>> +    /// Obtain `&'a T`, bypassing the [`Revocable`].
>>> +    ///
>>> +    /// This method allows to directly obtain a `&'a T`, bypassing the [`Revocable`], by presenting
>>> +    /// a `&'a Device<Bound>` of the same [`Device`] this [`Devres`] instance has been created with.
>>> +    ///
>>> +    /// An error is returned if `dev` does not match the same [`Device`] this [`Devres`] instance
>>> +    /// has been created with.
>>
>> I would prefer this as a `# Errors` section.
> 
> I can make this an # Errors section.
> 
>> Also are there any cases where this is actually wanted as an error?
>> I'm not very familiar with the `Revocable` infrastructure,
>> but I would assume a mismatch here to be a bug in almost every case,
>> so a panic here might be reasonable.
> 
> Passing in a device reference that doesn't match the device the Devres instance
> was created with would indeed be a bug, but a panic isn't the solution, since we
> can handle this error just fine.
> 
> We never panic the whole kernel unless things go so utterly wrong that we can't
> can't recover from it; e.g. if otherwise we'd likely corrupt memory, etc.
>> (I would be fine with a reason for using an error here in the 
>> commit message or documentation/comments)
> 
> I don't think we need this in this commit or the method's documentation, it's a
> general kernel rule not to panic unless there's really no other way.

Alright I'm fine with it then.

I just don't think that most users of the function would be able to
gracefully recover from that other than failing the probe
or whatever, but it makes sense to allow the caller to deal with this.

Cheers
Christian

Re: [PATCH 2/3] rust: devres: implement Devres::access_with()

[Benno Lossin]

On Sat Apr 26, 2025 at 7:18 PM CEST, Christian Schrefl wrote:
> On 26.04.25 7:08 PM, Danilo Krummrich wrote:
>> On Sat, Apr 26, 2025 at 06:53:10PM +0200, Christian Schrefl wrote:
>>> On 26.04.25 3:30 PM, Danilo Krummrich wrote:
>>>> Implement a direct accessor for the data stored within the Devres for
>>>> cases where we can proof that we own a reference to a Device<Bound>
>>>> (i.e. a bound device) of the same device that was used to create the
>>>> corresponding Devres container.
>>>>
>>>> Usually, when accessing the data stored within a Devres container, it is
>>>> not clear whether the data has been revoked already due to the device
>>>> being unbound and, hence, we have to try whether the access is possible
>>>> and subsequently keep holding the RCU read lock for the duration of the
>>>> access.
>>>>
>>>> However, when we can proof that we hold a reference to Device<Bound>
>>>> matching the device the Devres container has been created with, we can
>>>> guarantee that the device is not unbound for the duration of the
>>>> lifetime of the Device<Bound> reference and, hence, it is not possible
>>>> for the data within the Devres container to be revoked.
>>>>
>>>> Therefore, in this case, we can bypass the atomic check and the RCU read
>>>> lock, which is a great optimization and simplification for drivers.
>>>>
>>>> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
>>>> ---
>>>>  rust/kernel/devres.rs | 35 +++++++++++++++++++++++++++++++++++
>>>>  1 file changed, 35 insertions(+)
>>>>
>>>> diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
>>>> index 1e58f5d22044..ec2cd9cdda8b 100644
>>>> --- a/rust/kernel/devres.rs
>>>> +++ b/rust/kernel/devres.rs
>>>> @@ -181,6 +181,41 @@ pub fn new_foreign_owned(dev: &Device<Bound>, data: T, flags: Flags) -> Result {
>>>>  
>>>>          Ok(())
>>>>      }
>>>> +
>>>> +    /// Obtain `&'a T`, bypassing the [`Revocable`].
>>>> +    ///
>>>> +    /// This method allows to directly obtain a `&'a T`, bypassing the [`Revocable`], by presenting
>>>> +    /// a `&'a Device<Bound>` of the same [`Device`] this [`Devres`] instance has been created with.
>>>> +    ///
>>>> +    /// An error is returned if `dev` does not match the same [`Device`] this [`Devres`] instance
>>>> +    /// has been created with.
>>>
>>> I would prefer this as a `# Errors` section.
>> 
>> I can make this an # Errors section.
>> 
>>> Also are there any cases where this is actually wanted as an error?
>>> I'm not very familiar with the `Revocable` infrastructure,
>>> but I would assume a mismatch here to be a bug in almost every case,
>>> so a panic here might be reasonable.
>> 
>> Passing in a device reference that doesn't match the device the Devres instance
>> was created with would indeed be a bug, but a panic isn't the solution, since we
>> can handle this error just fine.
>> 
>> We never panic the whole kernel unless things go so utterly wrong that we can't
>> can't recover from it; e.g. if otherwise we'd likely corrupt memory, etc.
>>> (I would be fine with a reason for using an error here in the 
>>> commit message or documentation/comments)
>> 
>> I don't think we need this in this commit or the method's documentation, it's a
>> general kernel rule not to panic unless there's really no other way.
>
> Alright I'm fine with it then.
>
> I just don't think that most users of the function would be able to
> gracefully recover from that other than failing the probe
> or whatever, but it makes sense to allow the caller to deal with this.

Failing the probe *is* "gracefully" handling the error. As Danilo said,
a panic is the last resort when the whole world is on fire and you want
to avoid doing more damage to the system.

---
Cheers,
Benno

Re: [PATCH 2/3] rust: devres: implement Devres::access_with()

[Alexandre Courbot]

On Sat Apr 26, 2025 at 10:30 PM JST, Danilo Krummrich wrote:
> Implement a direct accessor for the data stored within the Devres for
> cases where we can proof that we own a reference to a Device<Bound>
> (i.e. a bound device) of the same device that was used to create the
> corresponding Devres container.
>
> Usually, when accessing the data stored within a Devres container, it is
> not clear whether the data has been revoked already due to the device
> being unbound and, hence, we have to try whether the access is possible
> and subsequently keep holding the RCU read lock for the duration of the
> access.
>
> However, when we can proof that we hold a reference to Device<Bound>
> matching the device the Devres container has been created with, we can
> guarantee that the device is not unbound for the duration of the
> lifetime of the Device<Bound> reference and, hence, it is not possible
> for the data within the Devres container to be revoked.
>
> Therefore, in this case, we can bypass the atomic check and the RCU read
> lock, which is a great optimization and simplification for drivers.
>
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> ---
>  rust/kernel/devres.rs | 35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
>
> diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
> index 1e58f5d22044..ec2cd9cdda8b 100644
> --- a/rust/kernel/devres.rs
> +++ b/rust/kernel/devres.rs
> @@ -181,6 +181,41 @@ pub fn new_foreign_owned(dev: &Device<Bound>, data: T, flags: Flags) -> Result {
>  
>          Ok(())
>      }
> +
> +    /// Obtain `&'a T`, bypassing the [`Revocable`].
> +    ///
> +    /// This method allows to directly obtain a `&'a T`, bypassing the [`Revocable`], by presenting
> +    /// a `&'a Device<Bound>` of the same [`Device`] this [`Devres`] instance has been created with.
> +    ///
> +    /// An error is returned if `dev` does not match the same [`Device`] this [`Devres`] instance
> +    /// has been created with.
> +    ///
> +    /// # Example
> +    ///
> +    /// ```no_run
> +    /// # use kernel::{device::Core, devres::Devres, pci};
> +    ///
> +    /// fn from_core(dev: &pci::Device<Core>, devres: Devres<pci::Bar<0x4>>) -> Result<()> {
> +    ///     let bar = devres.access_with(dev.as_ref())?;
> +    ///
> +    ///     let _ = bar.read32(0x0);
> +    ///
> +    ///     // might_sleep()
> +    ///
> +    ///     bar.write32(0x42, 0x0);
> +    ///
> +    ///     Ok(())
> +    /// }
> +    pub fn access_with<'s, 'd: 's>(&'s self, dev: &'d Device<Bound>) -> Result<&'s T> {

Coming from `Revocable::try_access_with` (and the standard library in
general), the name of this method made me think that it would take a
closure to run while the resource is held. Maybe we should differenciate
the names a bit more? Maybe just `access` is fine, or
`access_with_device`?

Re: [PATCH 2/3] rust: devres: implement Devres::access_with()

[Danilo Krummrich]

On Sun, Apr 27, 2025 at 10:15:17PM +0900, Alexandre Courbot wrote:
> On Sat Apr 26, 2025 at 10:30 PM JST, Danilo Krummrich wrote:
> > Implement a direct accessor for the data stored within the Devres for
> > cases where we can proof that we own a reference to a Device<Bound>
> > (i.e. a bound device) of the same device that was used to create the
> > corresponding Devres container.
> >
> > Usually, when accessing the data stored within a Devres container, it is
> > not clear whether the data has been revoked already due to the device
> > being unbound and, hence, we have to try whether the access is possible
> > and subsequently keep holding the RCU read lock for the duration of the
> > access.
> >
> > However, when we can proof that we hold a reference to Device<Bound>
> > matching the device the Devres container has been created with, we can
> > guarantee that the device is not unbound for the duration of the
> > lifetime of the Device<Bound> reference and, hence, it is not possible
> > for the data within the Devres container to be revoked.
> >
> > Therefore, in this case, we can bypass the atomic check and the RCU read
> > lock, which is a great optimization and simplification for drivers.
> >
> > Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> > ---
> >  rust/kernel/devres.rs | 35 +++++++++++++++++++++++++++++++++++
> >  1 file changed, 35 insertions(+)
> >
> > diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
> > index 1e58f5d22044..ec2cd9cdda8b 100644
> > --- a/rust/kernel/devres.rs
> > +++ b/rust/kernel/devres.rs
> > @@ -181,6 +181,41 @@ pub fn new_foreign_owned(dev: &Device<Bound>, data: T, flags: Flags) -> Result {
> >  
> >          Ok(())
> >      }
> > +
> > +    /// Obtain `&'a T`, bypassing the [`Revocable`].
> > +    ///
> > +    /// This method allows to directly obtain a `&'a T`, bypassing the [`Revocable`], by presenting
> > +    /// a `&'a Device<Bound>` of the same [`Device`] this [`Devres`] instance has been created with.
> > +    ///
> > +    /// An error is returned if `dev` does not match the same [`Device`] this [`Devres`] instance
> > +    /// has been created with.
> > +    ///
> > +    /// # Example
> > +    ///
> > +    /// ```no_run
> > +    /// # use kernel::{device::Core, devres::Devres, pci};
> > +    ///
> > +    /// fn from_core(dev: &pci::Device<Core>, devres: Devres<pci::Bar<0x4>>) -> Result<()> {
> > +    ///     let bar = devres.access_with(dev.as_ref())?;
> > +    ///
> > +    ///     let _ = bar.read32(0x0);
> > +    ///
> > +    ///     // might_sleep()
> > +    ///
> > +    ///     bar.write32(0x42, 0x0);
> > +    ///
> > +    ///     Ok(())
> > +    /// }
> > +    pub fn access_with<'s, 'd: 's>(&'s self, dev: &'d Device<Bound>) -> Result<&'s T> {
> 
> Coming from `Revocable::try_access_with` (and the standard library in
> general), the name of this method made me think that it would take a
> closure to run while the resource is held. Maybe we should differenciate
> the names a bit more? Maybe just `access` is fine, or
> `access_with_device`?

Seems reasonable -- access() it is then.