[PATCH 0/3] Devres optimization with bound devices

[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 1/3] rust: revocable: implement Revocable::access()

[Danilo Krummrich]

Implement an unsafe direct accessor for the data stored within the
Revocable.

This is useful for cases where we can proof that the data stored within
the Revocable is not and cannot be revoked for the duration of the
lifetime of the returned reference.

Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
The explicit lifetimes in access() probably don't serve a practical
purpose, but I found them to be useful for documentation purposes.
---
 rust/kernel/revocable.rs | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
index 971d0dc38d83..33535de141ce 100644
--- a/rust/kernel/revocable.rs
+++ b/rust/kernel/revocable.rs
@@ -139,6 +139,18 @@ pub fn try_access_with<R, F: FnOnce(&T) -> R>(&self, f: F) -> Option<R> {
         self.try_access().map(|t| f(&*t))
     }
 
+    /// Directly access the revocable wrapped object.
+    ///
+    /// # Safety
+    ///
+    /// The caller must ensure this [`Revocable`] instance hasn't been revoked and won't be revoked
+    /// for the duration of `'a`.
+    pub unsafe fn access<'a, 's: 'a>(&'s self) -> &'a T {
+        // SAFETY: By the safety requirement of this function it is guaranteed that
+        // `self.data.get()` is a valid pointer to an instance of `T`.
+        unsafe { &*self.data.get() }
+    }
+
     /// # Safety
     ///
     /// Callers must ensure that there are no more concurrent users of the revocable object.
-- 
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

[PATCH 3/3] samples: rust: pci: take advantage of Devres::access_with()

[Danilo Krummrich]

For the I/O operations executed from the probe() method, take advantage
of Devres::access_with(), avoiding the atomic check and RCU read lock
required otherwise entirely.

Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 samples/rust/rust_driver_pci.rs | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/samples/rust/rust_driver_pci.rs b/samples/rust/rust_driver_pci.rs
index 9ce3a7323a16..3e1569e5096e 100644
--- a/samples/rust/rust_driver_pci.rs
+++ b/samples/rust/rust_driver_pci.rs
@@ -83,12 +83,12 @@ fn probe(pdev: &pci::Device<Core>, info: &Self::IdInfo) -> Result<Pin<KBox<Self>
             GFP_KERNEL,
         )?;
 
-        let res = drvdata
-            .bar
-            .try_access_with(|b| Self::testdev(info, b))
-            .ok_or(ENXIO)??;
-
-        dev_info!(pdev.as_ref(), "pci-testdev data-match count: {}\n", res);
+        let bar = drvdata.bar.access_with(pdev.as_ref())?;
+        dev_info!(
+            pdev.as_ref(),
+            "pci-testdev data-match count: {}\n",
+            Self::testdev(info, bar)?
+        );
 
         Ok(drvdata.into())
     }
-- 
2.49.0

Re: [PATCH 1/3] rust: revocable: implement Revocable::access()

[Christian Schrefl]

On 26.04.25 3:30 PM, Danilo Krummrich wrote:
> Implement an unsafe direct accessor for the data stored within the
> Revocable.
> 
> This is useful for cases where we can proof that the data stored within
> the Revocable is not and cannot be revoked for the duration of the
> lifetime of the returned reference.
> 
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> ---
> The explicit lifetimes in access() probably don't serve a practical
> purpose, but I found them to be useful for documentation purposes.
> --->  rust/kernel/revocable.rs | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
> index 971d0dc38d83..33535de141ce 100644
> --- a/rust/kernel/revocable.rs
> +++ b/rust/kernel/revocable.rs
> @@ -139,6 +139,18 @@ pub fn try_access_with<R, F: FnOnce(&T) -> R>(&self, f: F) -> Option<R> {
>          self.try_access().map(|t| f(&*t))
>      }
>  
> +    /// Directly access the revocable wrapped object.
> +    ///
> +    /// # Safety
> +    ///
> +    /// The caller must ensure this [`Revocable`] instance hasn't been revoked and won't be revoked
> +    /// for the duration of `'a`.
> +    pub unsafe fn access<'a, 's: 'a>(&'s self) -> &'a T {
I'm not sure if the `'s` lifetime really carries much meaning here.
I find just (explicit) `'a` on both parameter and return value is clearer to me,
but I'm not sure what others (particularly those not very familiar with rust)
think of this.

Either way:

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

> +        // SAFETY: By the safety requirement of this function it is guaranteed that
> +        // `self.data.get()` is a valid pointer to an instance of `T`.
> +        unsafe { &*self.data.get() }
> +    }
> +
>      /// # Safety
>      ///
>      /// Callers must ensure that there are no more concurrent users of the revocable object.

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 1/3] rust: revocable: implement Revocable::access()

[Boqun Feng]

On Sat, Apr 26, 2025 at 06:44:03PM +0200, Christian Schrefl wrote:
> On 26.04.25 3:30 PM, Danilo Krummrich wrote:
> > Implement an unsafe direct accessor for the data stored within the
> > Revocable.
> > 
> > This is useful for cases where we can proof that the data stored within
> > the Revocable is not and cannot be revoked for the duration of the
> > lifetime of the returned reference.
> > 
> > Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> > ---
> > The explicit lifetimes in access() probably don't serve a practical
> > purpose, but I found them to be useful for documentation purposes.
> > --->  rust/kernel/revocable.rs | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
> > index 971d0dc38d83..33535de141ce 100644
> > --- a/rust/kernel/revocable.rs
> > +++ b/rust/kernel/revocable.rs
> > @@ -139,6 +139,18 @@ pub fn try_access_with<R, F: FnOnce(&T) -> R>(&self, f: F) -> Option<R> {
> >          self.try_access().map(|t| f(&*t))
> >      }
> >  
> > +    /// Directly access the revocable wrapped object.
> > +    ///
> > +    /// # Safety
> > +    ///
> > +    /// The caller must ensure this [`Revocable`] instance hasn't been revoked and won't be revoked
> > +    /// for the duration of `'a`.
> > +    pub unsafe fn access<'a, 's: 'a>(&'s self) -> &'a T {
> I'm not sure if the `'s` lifetime really carries much meaning here.
> I find just (explicit) `'a` on both parameter and return value is clearer to me,
> but I'm not sure what others (particularly those not very familiar with rust)
> think of this.

Yeah, I don't think we need two lifetimes here, the following version
should be fine (with implicit lifetime):

	pub unsafe fn access(&self) -> &T { ... }

, because if you do:

	let revocable: &'1 Revocable = ...;
	...
	let t: &'2 T = unsafe { revocable.access() };

'1 should already outlive '2 (i.e. '1: '2).

Regards,
Boqun

> 
> Either way:
> 
> Reviewed-by: Christian Schrefl <chrisi.schrefl@gmail.com>
> 
> > +        // SAFETY: By the safety requirement of this function it is guaranteed that
> > +        // `self.data.get()` is a valid pointer to an instance of `T`.
> > +        unsafe { &*self.data.get() }
> > +    }
> > +
> >      /// # Safety
> >      ///
> >      /// Callers must ensure that there are no more concurrent users of the revocable object.
> 

Re: [PATCH 1/3] rust: revocable: implement Revocable::access()

[Danilo Krummrich]

On Sat, Apr 26, 2025 at 09:54:58AM -0700, Boqun Feng wrote:
> On Sat, Apr 26, 2025 at 06:44:03PM +0200, Christian Schrefl wrote:
> > On 26.04.25 3:30 PM, Danilo Krummrich wrote:
> > > Implement an unsafe direct accessor for the data stored within the
> > > Revocable.
> > > 
> > > This is useful for cases where we can proof that the data stored within
> > > the Revocable is not and cannot be revoked for the duration of the
> > > lifetime of the returned reference.
> > > 
> > > Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> > > ---
> > > The explicit lifetimes in access() probably don't serve a practical
> > > purpose, but I found them to be useful for documentation purposes.
> > > --->  rust/kernel/revocable.rs | 12 ++++++++++++
> > >  1 file changed, 12 insertions(+)
> > > 
> > > diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
> > > index 971d0dc38d83..33535de141ce 100644
> > > --- a/rust/kernel/revocable.rs
> > > +++ b/rust/kernel/revocable.rs
> > > @@ -139,6 +139,18 @@ pub fn try_access_with<R, F: FnOnce(&T) -> R>(&self, f: F) -> Option<R> {
> > >          self.try_access().map(|t| f(&*t))
> > >      }
> > >  
> > > +    /// Directly access the revocable wrapped object.
> > > +    ///
> > > +    /// # Safety
> > > +    ///
> > > +    /// The caller must ensure this [`Revocable`] instance hasn't been revoked and won't be revoked
> > > +    /// for the duration of `'a`.
> > > +    pub unsafe fn access<'a, 's: 'a>(&'s self) -> &'a T {
> > I'm not sure if the `'s` lifetime really carries much meaning here.
> > I find just (explicit) `'a` on both parameter and return value is clearer to me,
> > but I'm not sure what others (particularly those not very familiar with rust)
> > think of this.
> 
> Yeah, I don't think we need two lifetimes here, the following version
> should be fine (with implicit lifetime):
> 
> 	pub unsafe fn access(&self) -> &T { ... }
> 
> , because if you do:
> 
> 	let revocable: &'1 Revocable = ...;
> 	...
> 	let t: &'2 T = unsafe { revocable.access() };
> 
> '1 should already outlive '2 (i.e. '1: '2).

Yes, this is indeed sufficient, that's why I wrote

	"The explicit lifetimes in access() probably don't serve a practical
	purpose, but I found them to be useful for documentation purposes."

below the commit message. :)

Any opinions in terms of documentation purposes?

> > 
> > Either way:
> > 
> > Reviewed-by: Christian Schrefl <chrisi.schrefl@gmail.com>
> > 
> > > +        // SAFETY: By the safety requirement of this function it is guaranteed that
> > > +        // `self.data.get()` is a valid pointer to an instance of `T`.
> > > +        unsafe { &*self.data.get() }
> > > +    }
> > > +
> > >      /// # Safety
> > >      ///
> > >      /// Callers must ensure that there are no more concurrent users of the revocable object.
> > 

Re: [PATCH 1/3] rust: revocable: implement Revocable::access()

[Christian Schrefl]

On 26.04.25 6:54 PM, Boqun Feng wrote:
> On Sat, Apr 26, 2025 at 06:44:03PM +0200, Christian Schrefl wrote:
>> On 26.04.25 3:30 PM, Danilo Krummrich wrote:
>>> Implement an unsafe direct accessor for the data stored within the
>>> Revocable.
>>>
>>> This is useful for cases where we can proof that the data stored within
>>> the Revocable is not and cannot be revoked for the duration of the
>>> lifetime of the returned reference.
>>>
>>> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
>>> ---
>>> The explicit lifetimes in access() probably don't serve a practical
>>> purpose, but I found them to be useful for documentation purposes.
>>> --->  rust/kernel/revocable.rs | 12 ++++++++++++
>>>  1 file changed, 12 insertions(+)
>>>
>>> diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
>>> index 971d0dc38d83..33535de141ce 100644
>>> --- a/rust/kernel/revocable.rs
>>> +++ b/rust/kernel/revocable.rs
>>> @@ -139,6 +139,18 @@ pub fn try_access_with<R, F: FnOnce(&T) -> R>(&self, f: F) -> Option<R> {
>>>          self.try_access().map(|t| f(&*t))
>>>      }
>>>  
>>> +    /// Directly access the revocable wrapped object.
>>> +    ///
>>> +    /// # Safety
>>> +    ///
>>> +    /// The caller must ensure this [`Revocable`] instance hasn't been revoked and won't be revoked
>>> +    /// for the duration of `'a`.
>>> +    pub unsafe fn access<'a, 's: 'a>(&'s self) -> &'a T {
>> I'm not sure if the `'s` lifetime really carries much meaning here.
>> I find just (explicit) `'a` on both parameter and return value is clearer to me,
>> but I'm not sure what others (particularly those not very familiar with rust)
>> think of this.
> 
> Yeah, I don't think we need two lifetimes here, the following version
> should be fine (with implicit lifetime):
> 
> 	pub unsafe fn access(&self) -> &T { ... }
> 
> , because if you do:
> 
> 	let revocable: &'1 Revocable = ...;
> 	...
> 	let t: &'2 T = unsafe { revocable.access() };
> 
> '1 should already outlive '2 (i.e. '1: '2).

I understand that implicit lifetimes desugars to 
effectively the same code, I just think that keeping
a explicit 'a makes it a bit more obvious that the
lifetimes need to be considered here.

But I'm also fine with just implicit lifetimes here.

Cheers
Christian

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 0/3] Devres optimization with bound devices

[Boqun Feng]

On Sat, Apr 26, 2025 at 03:30:38PM +0200, Danilo Krummrich wrote:
> 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.
> 

Nice! However, IIUC, if the users use Devres::new() to create a `Devres`
, they will have a `Devres` they can revoke anytime, which means you can
still revoke the `Devres` even if the device is bound.

Also if a `Devres` belongs to device A, but someone passes device B's
bound reference to `access_with()`, the compiler won't check for that,
and the `Devres` can be being revoked as the same, no? If so the
function is not safe.

Regards,
Boqun

> 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
> 

Re: [PATCH 1/3] rust: revocable: implement Revocable::access()

[Christian Schrefl]

On 26.04.25 7:01 PM, Danilo Krummrich wrote:
> On Sat, Apr 26, 2025 at 09:54:58AM -0700, Boqun Feng wrote:
>> On Sat, Apr 26, 2025 at 06:44:03PM +0200, Christian Schrefl wrote:
>>> On 26.04.25 3:30 PM, Danilo Krummrich wrote:
>>>> Implement an unsafe direct accessor for the data stored within the
>>>> Revocable.
>>>>
>>>> This is useful for cases where we can proof that the data stored within
>>>> the Revocable is not and cannot be revoked for the duration of the
>>>> lifetime of the returned reference.
>>>>
>>>> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
>>>> ---
>>>> The explicit lifetimes in access() probably don't serve a practical
>>>> purpose, but I found them to be useful for documentation purposes.
>>>> --->  rust/kernel/revocable.rs | 12 ++++++++++++
>>>>  1 file changed, 12 insertions(+)
>>>>
>>>> diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
>>>> index 971d0dc38d83..33535de141ce 100644
>>>> --- a/rust/kernel/revocable.rs
>>>> +++ b/rust/kernel/revocable.rs
>>>> @@ -139,6 +139,18 @@ pub fn try_access_with<R, F: FnOnce(&T) -> R>(&self, f: F) -> Option<R> {
>>>>          self.try_access().map(|t| f(&*t))
>>>>      }
>>>>  
>>>> +    /// Directly access the revocable wrapped object.
>>>> +    ///
>>>> +    /// # Safety
>>>> +    ///
>>>> +    /// The caller must ensure this [`Revocable`] instance hasn't been revoked and won't be revoked
>>>> +    /// for the duration of `'a`.
>>>> +    pub unsafe fn access<'a, 's: 'a>(&'s self) -> &'a T {
>>> I'm not sure if the `'s` lifetime really carries much meaning here.
>>> I find just (explicit) `'a` on both parameter and return value is clearer to me,
>>> but I'm not sure what others (particularly those not very familiar with rust)
>>> think of this.
>>
>> Yeah, I don't think we need two lifetimes here, the following version
>> should be fine (with implicit lifetime):
>>
>> 	pub unsafe fn access(&self) -> &T { ... }
>>
>> , because if you do:
>>
>> 	let revocable: &'1 Revocable = ...;
>> 	...
>> 	let t: &'2 T = unsafe { revocable.access() };
>>
>> '1 should already outlive '2 (i.e. '1: '2).
> 
> Yes, this is indeed sufficient, that's why I wrote
> 
> 	"The explicit lifetimes in access() probably don't serve a practical
> 	purpose, but I found them to be useful for documentation purposes."
> 
> below the commit message. :)
> 
> Any opinions in terms of documentation purposes?

I would prefer just one explicit lifetime, but I'm
not sure about others.
But I think either way is fine.

Maybe I should have written it more clearly that I 
only meant that the second lifetime makes it 
IMO unnecessarily complicated when reading it.

Cheers
Christian

Re: [PATCH 0/3] Devres optimization with bound devices

[Danilo Krummrich]

On Sat, Apr 26, 2025 at 10:09:39AM -0700, Boqun Feng wrote:
> On Sat, Apr 26, 2025 at 03:30:38PM +0200, Danilo Krummrich wrote:
> > 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.
> > 
> 
> Nice! However, IIUC, if the users use Devres::new() to create a `Devres`
> , they will have a `Devres` they can revoke anytime, which means you can
> still revoke the `Devres` even if the device is bound.

No, a user of Devres can't revoke the inner Revocable itself. A user can only
drop the Devres instance, in which case the user also wouldn't be able to call
access_with() anymore.

> Also if a `Devres` belongs to device A, but someone passes device B's
> bound reference to `access_with()`, the compiler won't check for that,
> and the `Devres` can be being revoked as the same, no? If so the
> function is not safe.

Devres::access_with() compares the Device<Bound> parameter with its inner
ARef<Device>, and just fails if they don't match.

Re: [PATCH 0/3] Devres optimization with bound devices

[Boqun Feng]

On Sat, Apr 26, 2025 at 07:14:54PM +0200, Danilo Krummrich wrote:
> On Sat, Apr 26, 2025 at 10:09:39AM -0700, Boqun Feng wrote:
> > On Sat, Apr 26, 2025 at 03:30:38PM +0200, Danilo Krummrich wrote:
> > > 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.
> > > 
> > 
> > Nice! However, IIUC, if the users use Devres::new() to create a `Devres`
> > , they will have a `Devres` they can revoke anytime, which means you can
> > still revoke the `Devres` even if the device is bound.
> 
> No, a user of Devres can't revoke the inner Revocable itself. A user can only
> drop the Devres instance, in which case the user also wouldn't be able to call
> access_with() anymore.
> 

Oh, right, because it's a `Devres` not `Revocable` in general.

> > Also if a `Devres` belongs to device A, but someone passes device B's
> > bound reference to `access_with()`, the compiler won't check for that,
> > and the `Devres` can be being revoked as the same, no? If so the
> > function is not safe.
> 
> Devres::access_with() compares the Device<Bound> parameter with its inner
> ARef<Device>, and just fails if they don't match.

I see, I missed that. Thanks!

Regards,
Boqun

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 1/3] rust: revocable: implement Revocable::access()

[Boqun Feng]

On Sat, Apr 26, 2025 at 07:01:52PM +0200, Danilo Krummrich wrote:
> On Sat, Apr 26, 2025 at 09:54:58AM -0700, Boqun Feng wrote:
> > On Sat, Apr 26, 2025 at 06:44:03PM +0200, Christian Schrefl wrote:
> > > On 26.04.25 3:30 PM, Danilo Krummrich wrote:
> > > > Implement an unsafe direct accessor for the data stored within the
> > > > Revocable.
> > > > 
> > > > This is useful for cases where we can proof that the data stored within
> > > > the Revocable is not and cannot be revoked for the duration of the
> > > > lifetime of the returned reference.
> > > > 
> > > > Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> > > > ---
> > > > The explicit lifetimes in access() probably don't serve a practical
> > > > purpose, but I found them to be useful for documentation purposes.
> > > > --->  rust/kernel/revocable.rs | 12 ++++++++++++
> > > >  1 file changed, 12 insertions(+)
> > > > 
> > > > diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
> > > > index 971d0dc38d83..33535de141ce 100644
> > > > --- a/rust/kernel/revocable.rs
> > > > +++ b/rust/kernel/revocable.rs
> > > > @@ -139,6 +139,18 @@ pub fn try_access_with<R, F: FnOnce(&T) -> R>(&self, f: F) -> Option<R> {
> > > >          self.try_access().map(|t| f(&*t))
> > > >      }
> > > >  
> > > > +    /// Directly access the revocable wrapped object.
> > > > +    ///
> > > > +    /// # Safety
> > > > +    ///
> > > > +    /// The caller must ensure this [`Revocable`] instance hasn't been revoked and won't be revoked
> > > > +    /// for the duration of `'a`.
> > > > +    pub unsafe fn access<'a, 's: 'a>(&'s self) -> &'a T {
> > > I'm not sure if the `'s` lifetime really carries much meaning here.
> > > I find just (explicit) `'a` on both parameter and return value is clearer to me,
> > > but I'm not sure what others (particularly those not very familiar with rust)
> > > think of this.
> > 
> > Yeah, I don't think we need two lifetimes here, the following version
> > should be fine (with implicit lifetime):
> > 
> > 	pub unsafe fn access(&self) -> &T { ... }
> > 
> > , because if you do:
> > 
> > 	let revocable: &'1 Revocable = ...;
> > 	...
> > 	let t: &'2 T = unsafe { revocable.access() };
> > 
> > '1 should already outlive '2 (i.e. '1: '2).
> 
> Yes, this is indeed sufficient, that's why I wrote
> 
> 	"The explicit lifetimes in access() probably don't serve a practical
> 	purpose, but I found them to be useful for documentation purposes."
> 
> below the commit message. :)
> 

Sorry, I overlooked.

> Any opinions in terms of documentation purposes?
> 

I think for access() the explicit lifetimes is unnecessary, because it's
a one-lifetime case, the two explicit lifetimes would make a simple case
looking complicated.

For access_with(), that's needed and a good idea.

Just my two cents.

Regards,
Boqun

> > > 
> > > Either way:
> > > 
> > > Reviewed-by: Christian Schrefl <chrisi.schrefl@gmail.com>
> > > 
> > > > +        // SAFETY: By the safety requirement of this function it is guaranteed that
> > > > +        // `self.data.get()` is a valid pointer to an instance of `T`.
> > > > +        unsafe { &*self.data.get() }
> > > > +    }
> > > > +
> > > >      /// # Safety
> > > >      ///
> > > >      /// Callers must ensure that there are no more concurrent users of the revocable object.
> > > 

Re: [PATCH 1/3] rust: revocable: implement Revocable::access()

[Benno Lossin]

On Sat Apr 26, 2025 at 7:03 PM CEST, Christian Schrefl wrote:
> On 26.04.25 6:54 PM, Boqun Feng wrote:
>> On Sat, Apr 26, 2025 at 06:44:03PM +0200, Christian Schrefl wrote:
>>> On 26.04.25 3:30 PM, Danilo Krummrich wrote:
>>>> Implement an unsafe direct accessor for the data stored within the
>>>> Revocable.
>>>>
>>>> This is useful for cases where we can proof that the data stored within
>>>> the Revocable is not and cannot be revoked for the duration of the
>>>> lifetime of the returned reference.
>>>>
>>>> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
>>>> ---
>>>> The explicit lifetimes in access() probably don't serve a practical
>>>> purpose, but I found them to be useful for documentation purposes.
>>>> --->  rust/kernel/revocable.rs | 12 ++++++++++++
>>>>  1 file changed, 12 insertions(+)
>>>>
>>>> diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
>>>> index 971d0dc38d83..33535de141ce 100644
>>>> --- a/rust/kernel/revocable.rs
>>>> +++ b/rust/kernel/revocable.rs
>>>> @@ -139,6 +139,18 @@ pub fn try_access_with<R, F: FnOnce(&T) -> R>(&self, f: F) -> Option<R> {
>>>>          self.try_access().map(|t| f(&*t))
>>>>      }
>>>>  
>>>> +    /// Directly access the revocable wrapped object.
>>>> +    ///
>>>> +    /// # Safety
>>>> +    ///
>>>> +    /// The caller must ensure this [`Revocable`] instance hasn't been revoked and won't be revoked
>>>> +    /// for the duration of `'a`.
>>>> +    pub unsafe fn access<'a, 's: 'a>(&'s self) -> &'a T {
>>> I'm not sure if the `'s` lifetime really carries much meaning here.
>>> I find just (explicit) `'a` on both parameter and return value is clearer to me,
>>> but I'm not sure what others (particularly those not very familiar with rust)
>>> think of this.
>> 
>> Yeah, I don't think we need two lifetimes here, the following version
>> should be fine (with implicit lifetime):
>> 
>> 	pub unsafe fn access(&self) -> &T { ... }
>> 
>> , because if you do:
>> 
>> 	let revocable: &'1 Revocable = ...;
>> 	...
>> 	let t: &'2 T = unsafe { revocable.access() };
>> 
>> '1 should already outlive '2 (i.e. '1: '2).
>
> I understand that implicit lifetimes desugars to 
> effectively the same code, I just think that keeping
> a explicit 'a makes it a bit more obvious that the
> lifetimes need to be considered here.
>
> But I'm also fine with just implicit lifetimes here.

We elide lifetimes all over the place especially for methods taking
`&self` and returning `&T`. I don't think that it serves a purpose here.

---
Cheers,
Benno

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 1/3] rust: revocable: implement Revocable::access()

[Benno Lossin]

On Sat Apr 26, 2025 at 3:30 PM CEST, Danilo Krummrich wrote:
> Implement an unsafe direct accessor for the data stored within the
> Revocable.
>
> This is useful for cases where we can proof that the data stored within
> the Revocable is not and cannot be revoked for the duration of the
> lifetime of the returned reference.
>
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> ---
> The explicit lifetimes in access() probably don't serve a practical
> purpose, but I found them to be useful for documentation purposes.
> ---
>  rust/kernel/revocable.rs | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
> index 971d0dc38d83..33535de141ce 100644
> --- a/rust/kernel/revocable.rs
> +++ b/rust/kernel/revocable.rs
> @@ -139,6 +139,18 @@ pub fn try_access_with<R, F: FnOnce(&T) -> R>(&self, f: F) -> Option<R> {
>          self.try_access().map(|t| f(&*t))
>      }
>  
> +    /// Directly access the revocable wrapped object.
> +    ///
> +    /// # Safety
> +    ///
> +    /// The caller must ensure this [`Revocable`] instance hasn't been revoked and won't be revoked
> +    /// for the duration of `'a`.

Ah I missed this in my other email, in case you want to directly refer
to the lifetime, you should keep it defined. I would still remove the
`'s` lifetime though.
> +    pub unsafe fn access<'a, 's: 'a>(&'s self) -> &'a T {
> +        // SAFETY: By the safety requirement of this function it is guaranteed that
> +        // `self.data.get()` is a valid pointer to an instance of `T`.

I don't see how the "not-being revoked" state makes the `data` ptr be
valid. Is that an invariant of `Revocable`? (it's not documented to have
any invariants)

---
Cheers,
Benno

> +        unsafe { &*self.data.get() }
> +    }
> +
>      /// # Safety
>      ///
>      /// Callers must ensure that there are no more concurrent users of the revocable object.

Re: [PATCH 3/3] samples: rust: pci: take advantage of Devres::access_with()

[Benno Lossin]

On Sat Apr 26, 2025 at 3:30 PM CEST, Danilo Krummrich wrote:
> For the I/O operations executed from the probe() method, take advantage
> of Devres::access_with(), avoiding the atomic check and RCU read lock
> required otherwise entirely.
>
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> ---
>  samples/rust/rust_driver_pci.rs | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/samples/rust/rust_driver_pci.rs b/samples/rust/rust_driver_pci.rs
> index 9ce3a7323a16..3e1569e5096e 100644
> --- a/samples/rust/rust_driver_pci.rs
> +++ b/samples/rust/rust_driver_pci.rs
> @@ -83,12 +83,12 @@ fn probe(pdev: &pci::Device<Core>, info: &Self::IdInfo) -> Result<Pin<KBox<Self>
>              GFP_KERNEL,
>          )?;
>  
> -        let res = drvdata
> -            .bar
> -            .try_access_with(|b| Self::testdev(info, b))
> -            .ok_or(ENXIO)??;
> -
> -        dev_info!(pdev.as_ref(), "pci-testdev data-match count: {}\n", res);
> +        let bar = drvdata.bar.access_with(pdev.as_ref())?;

Since this code might inspire other code, I don't think that we should
return `EINVAL` here (bubbled up from `access_with`). Not sure what the
correct thing here would be though...

---
Cheers,
Benno

> +        dev_info!(
> +            pdev.as_ref(),
> +            "pci-testdev data-match count: {}\n",
> +            Self::testdev(info, bar)?
> +        );
>  
>          Ok(drvdata.into())
>      }

Re: [PATCH 1/3] rust: revocable: implement Revocable::access()

[Danilo Krummrich]

On Sat, Apr 26, 2025 at 08:24:14PM +0000, Benno Lossin wrote:
> On Sat Apr 26, 2025 at 3:30 PM CEST, Danilo Krummrich wrote:
> > Implement an unsafe direct accessor for the data stored within the
> > Revocable.
> >
> > This is useful for cases where we can proof that the data stored within
> > the Revocable is not and cannot be revoked for the duration of the
> > lifetime of the returned reference.
> >
> > Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> > ---
> > The explicit lifetimes in access() probably don't serve a practical
> > purpose, but I found them to be useful for documentation purposes.
> > ---
> >  rust/kernel/revocable.rs | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
> > index 971d0dc38d83..33535de141ce 100644
> > --- a/rust/kernel/revocable.rs
> > +++ b/rust/kernel/revocable.rs
> > @@ -139,6 +139,18 @@ pub fn try_access_with<R, F: FnOnce(&T) -> R>(&self, f: F) -> Option<R> {
> >          self.try_access().map(|t| f(&*t))
> >      }
> >  
> > +    /// Directly access the revocable wrapped object.
> > +    ///
> > +    /// # Safety
> > +    ///
> > +    /// The caller must ensure this [`Revocable`] instance hasn't been revoked and won't be revoked
> > +    /// for the duration of `'a`.
> 
> Ah I missed this in my other email, in case you want to directly refer
> to the lifetime, you should keep it defined. I would still remove the
> `'s` lifetime though.
> > +    pub unsafe fn access<'a, 's: 'a>(&'s self) -> &'a T {
> > +        // SAFETY: By the safety requirement of this function it is guaranteed that
> > +        // `self.data.get()` is a valid pointer to an instance of `T`.
> 
> I don't see how the "not-being revoked" state makes the `data` ptr be
> valid. Is that an invariant of `Revocable`? (it's not documented to have
> any invariants)

What else makes it valid?

AFAICS, try_access() and try_access_with_guard() argue the exact same way,
except that the reason for not being revoked is the atomic check and the RCU
read lock.

Re: [PATCH 3/3] samples: rust: pci: take advantage of Devres::access_with()

[Danilo Krummrich]

On Sat, Apr 26, 2025 at 08:30:39PM +0000, Benno Lossin wrote:
> On Sat Apr 26, 2025 at 3:30 PM CEST, Danilo Krummrich wrote:
> > For the I/O operations executed from the probe() method, take advantage
> > of Devres::access_with(), avoiding the atomic check and RCU read lock
> > required otherwise entirely.
> >
> > Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> > ---
> >  samples/rust/rust_driver_pci.rs | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/samples/rust/rust_driver_pci.rs b/samples/rust/rust_driver_pci.rs
> > index 9ce3a7323a16..3e1569e5096e 100644
> > --- a/samples/rust/rust_driver_pci.rs
> > +++ b/samples/rust/rust_driver_pci.rs
> > @@ -83,12 +83,12 @@ fn probe(pdev: &pci::Device<Core>, info: &Self::IdInfo) -> Result<Pin<KBox<Self>
> >              GFP_KERNEL,
> >          )?;
> >  
> > -        let res = drvdata
> > -            .bar
> > -            .try_access_with(|b| Self::testdev(info, b))
> > -            .ok_or(ENXIO)??;
> > -
> > -        dev_info!(pdev.as_ref(), "pci-testdev data-match count: {}\n", res);
> > +        let bar = drvdata.bar.access_with(pdev.as_ref())?;
> 
> Since this code might inspire other code, I don't think that we should
> return `EINVAL` here (bubbled up from `access_with`). Not sure what the
> correct thing here would be though...

I can't think of any other error code that would match better, EINVAL seems to
be the correct thing. Maybe one could argue for ENODEV, but I still think EINVAL
fits better.

Re: [PATCH 1/3] rust: revocable: implement Revocable::access()

[Benno Lossin]

On Sat Apr 26, 2025 at 11:18 PM CEST, Danilo Krummrich wrote:
> On Sat, Apr 26, 2025 at 08:24:14PM +0000, Benno Lossin wrote:
>> On Sat Apr 26, 2025 at 3:30 PM CEST, Danilo Krummrich wrote:
>> > Implement an unsafe direct accessor for the data stored within the
>> > Revocable.
>> >
>> > This is useful for cases where we can proof that the data stored within
>> > the Revocable is not and cannot be revoked for the duration of the
>> > lifetime of the returned reference.
>> >
>> > Signed-off-by: Danilo Krummrich <dakr@kernel.org>
>> > ---
>> > The explicit lifetimes in access() probably don't serve a practical
>> > purpose, but I found them to be useful for documentation purposes.
>> > ---
>> >  rust/kernel/revocable.rs | 12 ++++++++++++
>> >  1 file changed, 12 insertions(+)
>> >
>> > diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
>> > index 971d0dc38d83..33535de141ce 100644
>> > --- a/rust/kernel/revocable.rs
>> > +++ b/rust/kernel/revocable.rs
>> > @@ -139,6 +139,18 @@ pub fn try_access_with<R, F: FnOnce(&T) -> R>(&self, f: F) -> Option<R> {
>> >          self.try_access().map(|t| f(&*t))
>> >      }
>> >  
>> > +    /// Directly access the revocable wrapped object.
>> > +    ///
>> > +    /// # Safety
>> > +    ///
>> > +    /// The caller must ensure this [`Revocable`] instance hasn't been revoked and won't be revoked
>> > +    /// for the duration of `'a`.
>> 
>> Ah I missed this in my other email, in case you want to directly refer
>> to the lifetime, you should keep it defined. I would still remove the
>> `'s` lifetime though.
>> > +    pub unsafe fn access<'a, 's: 'a>(&'s self) -> &'a T {
>> > +        // SAFETY: By the safety requirement of this function it is guaranteed that
>> > +        // `self.data.get()` is a valid pointer to an instance of `T`.
>> 
>> I don't see how the "not-being revoked" state makes the `data` ptr be
>> valid. Is that an invariant of `Revocable`? (it's not documented to have
>> any invariants)
>
> What else makes it valid?

IMO an `# Invariants` section with the corresponding invariant that
`data` is valid when `is_available` is true.

> AFAICS, try_access() and try_access_with_guard() argue the exact same way,
> except that the reason for not being revoked is the atomic check and the RCU
> read lock.

Just because other code is doing the same mistake doesn't make it
correct. If I had reviewed the patch at that time I'm sure I would have
pointed this out.

I opened an issue about this:

    https://github.com/Rust-for-Linux/linux/issues/1160

Feel free to comment any additional information.

---
Cheers,
Benno

Re: [PATCH 3/3] samples: rust: pci: take advantage of Devres::access_with()

[Benno Lossin]

On Sat Apr 26, 2025 at 11:26 PM CEST, Danilo Krummrich wrote:
> On Sat, Apr 26, 2025 at 08:30:39PM +0000, Benno Lossin wrote:
>> On Sat Apr 26, 2025 at 3:30 PM CEST, Danilo Krummrich wrote:
>> > For the I/O operations executed from the probe() method, take advantage
>> > of Devres::access_with(), avoiding the atomic check and RCU read lock
>> > required otherwise entirely.
>> >
>> > Signed-off-by: Danilo Krummrich <dakr@kernel.org>
>> > ---
>> >  samples/rust/rust_driver_pci.rs | 12 ++++++------
>> >  1 file changed, 6 insertions(+), 6 deletions(-)
>> >
>> > diff --git a/samples/rust/rust_driver_pci.rs b/samples/rust/rust_driver_pci.rs
>> > index 9ce3a7323a16..3e1569e5096e 100644
>> > --- a/samples/rust/rust_driver_pci.rs
>> > +++ b/samples/rust/rust_driver_pci.rs
>> > @@ -83,12 +83,12 @@ fn probe(pdev: &pci::Device<Core>, info: &Self::IdInfo) -> Result<Pin<KBox<Self>
>> >              GFP_KERNEL,
>> >          )?;
>> >  
>> > -        let res = drvdata
>> > -            .bar
>> > -            .try_access_with(|b| Self::testdev(info, b))
>> > -            .ok_or(ENXIO)??;
>> > -
>> > -        dev_info!(pdev.as_ref(), "pci-testdev data-match count: {}\n", res);
>> > +        let bar = drvdata.bar.access_with(pdev.as_ref())?;
>> 
>> Since this code might inspire other code, I don't think that we should
>> return `EINVAL` here (bubbled up from `access_with`). Not sure what the
>> correct thing here would be though...
>
> I can't think of any other error code that would match better, EINVAL seems to
> be the correct thing. Maybe one could argue for ENODEV, but I still think EINVAL
> fits better.

The previous iteration of the sample used the ENXIO error code.

In this sample it should be impossible to trigger the error path. But
others might copy the code into a context where that is not the case and
then might have a horrible time debugging where the `EINVAL` came from.
I don't know if our answer to that should be "improve debugging errors
in general" or "improve the error handling in this case". I have no
idea how the former could look like, maybe something around
`#[track_caller]` and noting the lines where an error was created? For
the latter case, we could do:

    let bar = match drvdata.bar.access_with(pdev.as_ref()) {
        Ok(bar) => bar,
        Err(_) => {
            // `bar` was just created using the `pdev` above, so this should never happen.
            return Err(ENXIO);
        }
    };

---
Cheers,
Benno

Re: [PATCH 1/3] rust: revocable: implement Revocable::access()

[Danilo Krummrich]

On Sun, Apr 27, 2025 at 08:37:00AM +0000, Benno Lossin wrote:
> On Sat Apr 26, 2025 at 11:18 PM CEST, Danilo Krummrich wrote:
> > On Sat, Apr 26, 2025 at 08:24:14PM +0000, Benno Lossin wrote:
> >> On Sat Apr 26, 2025 at 3:30 PM CEST, Danilo Krummrich wrote:
> >> > Implement an unsafe direct accessor for the data stored within the
> >> > Revocable.
> >> >
> >> > This is useful for cases where we can proof that the data stored within
> >> > the Revocable is not and cannot be revoked for the duration of the
> >> > lifetime of the returned reference.
> >> >
> >> > Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> >> > ---
> >> > The explicit lifetimes in access() probably don't serve a practical
> >> > purpose, but I found them to be useful for documentation purposes.
> >> > ---
> >> >  rust/kernel/revocable.rs | 12 ++++++++++++
> >> >  1 file changed, 12 insertions(+)
> >> >
> >> > diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
> >> > index 971d0dc38d83..33535de141ce 100644
> >> > --- a/rust/kernel/revocable.rs
> >> > +++ b/rust/kernel/revocable.rs
> >> > @@ -139,6 +139,18 @@ pub fn try_access_with<R, F: FnOnce(&T) -> R>(&self, f: F) -> Option<R> {
> >> >          self.try_access().map(|t| f(&*t))
> >> >      }
> >> >  
> >> > +    /// Directly access the revocable wrapped object.
> >> > +    ///
> >> > +    /// # Safety
> >> > +    ///
> >> > +    /// The caller must ensure this [`Revocable`] instance hasn't been revoked and won't be revoked
> >> > +    /// for the duration of `'a`.
> >> 
> >> Ah I missed this in my other email, in case you want to directly refer
> >> to the lifetime, you should keep it defined. I would still remove the
> >> `'s` lifetime though.
> >> > +    pub unsafe fn access<'a, 's: 'a>(&'s self) -> &'a T {
> >> > +        // SAFETY: By the safety requirement of this function it is guaranteed that
> >> > +        // `self.data.get()` is a valid pointer to an instance of `T`.
> >> 
> >> I don't see how the "not-being revoked" state makes the `data` ptr be
> >> valid. Is that an invariant of `Revocable`? (it's not documented to have
> >> any invariants)
> >
> > What else makes it valid?
> 
> IMO an `# Invariants` section with the corresponding invariant that
> `data` is valid when `is_available` is true.

Yeah, I agree that the # Invariants section is indeed missing and should be
fixed.

> > AFAICS, try_access() and try_access_with_guard() argue the exact same way,
> > except that the reason for not being revoked is the atomic check and the RCU
> > read lock.
> 
> Just because other code is doing the same mistake doesn't make it
> correct. If I had reviewed the patch at that time I'm sure I would have
> pointed this out.

I would say that try_access() and try_access_with_guard() are wrong, they rely
on the correct thing, we just missed documenting the corresponding invariant.

> I opened an issue about this:
> 
>     https://github.com/Rust-for-Linux/linux/issues/1160

Thanks for creating the issue!

What do you suggest for this patch?

Re: [PATCH 3/3] samples: rust: pci: take advantage of Devres::access_with()

[Danilo Krummrich]

On Sun, Apr 27, 2025 at 08:56:29AM +0000, Benno Lossin wrote:
> On Sat Apr 26, 2025 at 11:26 PM CEST, Danilo Krummrich wrote:
> > On Sat, Apr 26, 2025 at 08:30:39PM +0000, Benno Lossin wrote:
> >> On Sat Apr 26, 2025 at 3:30 PM CEST, Danilo Krummrich wrote:
> >> > For the I/O operations executed from the probe() method, take advantage
> >> > of Devres::access_with(), avoiding the atomic check and RCU read lock
> >> > required otherwise entirely.
> >> >
> >> > Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> >> > ---
> >> >  samples/rust/rust_driver_pci.rs | 12 ++++++------
> >> >  1 file changed, 6 insertions(+), 6 deletions(-)
> >> >
> >> > diff --git a/samples/rust/rust_driver_pci.rs b/samples/rust/rust_driver_pci.rs
> >> > index 9ce3a7323a16..3e1569e5096e 100644
> >> > --- a/samples/rust/rust_driver_pci.rs
> >> > +++ b/samples/rust/rust_driver_pci.rs
> >> > @@ -83,12 +83,12 @@ fn probe(pdev: &pci::Device<Core>, info: &Self::IdInfo) -> Result<Pin<KBox<Self>
> >> >              GFP_KERNEL,
> >> >          )?;
> >> >  
> >> > -        let res = drvdata
> >> > -            .bar
> >> > -            .try_access_with(|b| Self::testdev(info, b))
> >> > -            .ok_or(ENXIO)??;
> >> > -
> >> > -        dev_info!(pdev.as_ref(), "pci-testdev data-match count: {}\n", res);
> >> > +        let bar = drvdata.bar.access_with(pdev.as_ref())?;
> >> 
> >> Since this code might inspire other code, I don't think that we should
> >> return `EINVAL` here (bubbled up from `access_with`). Not sure what the
> >> correct thing here would be though...
> >
> > I can't think of any other error code that would match better, EINVAL seems to
> > be the correct thing. Maybe one could argue for ENODEV, but I still think EINVAL
> > fits better.
> 
> The previous iteration of the sample used the ENXIO error code.

Yes, because the cause of error for try_access_with() failing would have been
that the device was unbound already, hence a different error code.

> In this sample it should be impossible to trigger the error path. But
> others might copy the code into a context where that is not the case and
> then might have a horrible time debugging where the `EINVAL` came from.

I think it should always pretty unlikely design wise to supply a non-matching
device.

> I don't know if our answer to that should be "improve debugging errors
> in general" or "improve the error handling in this case". I have no
> idea how the former could look like, maybe something around
> `#[track_caller]` and noting the lines where an error was created? For
> the latter case, we could do:
> 
>     let bar = match drvdata.bar.access_with(pdev.as_ref()) {
>         Ok(bar) => bar,
>         Err(_) => {
>             // `bar` was just created using the `pdev` above, so this should never happen.
>             return Err(ENXIO);

If the caller really put in a non-matching device we can't say for sure that the
cause is ENXIO, the only thing we know is that the code author confused device
instances, so I think EINVAL is still the correct thing to return.

The problem that it might be difficult to figure out the source of the error
code has always been present in the kernel, and while I'm not saying we
shouldn't aim for improving this, I don't think this patch is quite the place
for this.

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.

Re: [PATCH 3/3] samples: rust: pci: take advantage of Devres::access_with()

[Benno Lossin]

On Sun Apr 27, 2025 at 12:20 PM CEST, Danilo Krummrich wrote:
> On Sun, Apr 27, 2025 at 08:56:29AM +0000, Benno Lossin wrote:
>> On Sat Apr 26, 2025 at 11:26 PM CEST, Danilo Krummrich wrote:
>> > On Sat, Apr 26, 2025 at 08:30:39PM +0000, Benno Lossin wrote:
>> >> On Sat Apr 26, 2025 at 3:30 PM CEST, Danilo Krummrich wrote:
>> >> > For the I/O operations executed from the probe() method, take advantage
>> >> > of Devres::access_with(), avoiding the atomic check and RCU read lock
>> >> > required otherwise entirely.
>> >> >
>> >> > Signed-off-by: Danilo Krummrich <dakr@kernel.org>
>> >> > ---
>> >> >  samples/rust/rust_driver_pci.rs | 12 ++++++------
>> >> >  1 file changed, 6 insertions(+), 6 deletions(-)
>> >> >
>> >> > diff --git a/samples/rust/rust_driver_pci.rs b/samples/rust/rust_driver_pci.rs
>> >> > index 9ce3a7323a16..3e1569e5096e 100644
>> >> > --- a/samples/rust/rust_driver_pci.rs
>> >> > +++ b/samples/rust/rust_driver_pci.rs
>> >> > @@ -83,12 +83,12 @@ fn probe(pdev: &pci::Device<Core>, info: &Self::IdInfo) -> Result<Pin<KBox<Self>
>> >> >              GFP_KERNEL,
>> >> >          )?;
>> >> >  
>> >> > -        let res = drvdata
>> >> > -            .bar
>> >> > -            .try_access_with(|b| Self::testdev(info, b))
>> >> > -            .ok_or(ENXIO)??;
>> >> > -
>> >> > -        dev_info!(pdev.as_ref(), "pci-testdev data-match count: {}\n", res);
>> >> > +        let bar = drvdata.bar.access_with(pdev.as_ref())?;
>> >> 
>> >> Since this code might inspire other code, I don't think that we should
>> >> return `EINVAL` here (bubbled up from `access_with`). Not sure what the
>> >> correct thing here would be though...
>> >
>> > I can't think of any other error code that would match better, EINVAL seems to
>> > be the correct thing. Maybe one could argue for ENODEV, but I still think EINVAL
>> > fits better.
>> 
>> The previous iteration of the sample used the ENXIO error code.
>
> Yes, because the cause of error for try_access_with() failing would have been
> that the device was unbound already, hence a different error code.

Ah I see.

>> In this sample it should be impossible to trigger the error path. But
>> others might copy the code into a context where that is not the case and
>> then might have a horrible time debugging where the `EINVAL` came from.
>
> I think it should always pretty unlikely design wise to supply a non-matching
> device.
>
>> I don't know if our answer to that should be "improve debugging errors
>> in general" or "improve the error handling in this case". I have no
>> idea how the former could look like, maybe something around
>> `#[track_caller]` and noting the lines where an error was created? For
>> the latter case, we could do:
>> 
>>     let bar = match drvdata.bar.access_with(pdev.as_ref()) {
>>         Ok(bar) => bar,
>>         Err(_) => {
>>             // `bar` was just created using the `pdev` above, so this should never happen.
>>             return Err(ENXIO);
>
> If the caller really put in a non-matching device we can't say for sure that the
> cause is ENXIO, the only thing we know is that the code author confused device
> instances, so I think EINVAL is still the correct thing to return.
>
> The problem that it might be difficult to figure out the source of the error
> code has always been present in the kernel, and while I'm not saying we
> shouldn't aim for improving this, I don't think this patch is quite the place
> for this.

Agreed.

---
Cheers,
Benno

Re: [PATCH 1/3] rust: revocable: implement Revocable::access()

[Benno Lossin]

On Sun Apr 27, 2025 at 12:13 PM CEST, Danilo Krummrich wrote:
> On Sun, Apr 27, 2025 at 08:37:00AM +0000, Benno Lossin wrote:
>> On Sat Apr 26, 2025 at 11:18 PM CEST, Danilo Krummrich wrote:
>> > On Sat, Apr 26, 2025 at 08:24:14PM +0000, Benno Lossin wrote:
>> >> On Sat Apr 26, 2025 at 3:30 PM CEST, Danilo Krummrich wrote:
>> >> > Implement an unsafe direct accessor for the data stored within the
>> >> > Revocable.
>> >> >
>> >> > This is useful for cases where we can proof that the data stored within
>> >> > the Revocable is not and cannot be revoked for the duration of the
>> >> > lifetime of the returned reference.
>> >> >
>> >> > Signed-off-by: Danilo Krummrich <dakr@kernel.org>
>> >> > ---
>> >> > The explicit lifetimes in access() probably don't serve a practical
>> >> > purpose, but I found them to be useful for documentation purposes.
>> >> > ---
>> >> >  rust/kernel/revocable.rs | 12 ++++++++++++
>> >> >  1 file changed, 12 insertions(+)
>> >> >
>> >> > diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
>> >> > index 971d0dc38d83..33535de141ce 100644
>> >> > --- a/rust/kernel/revocable.rs
>> >> > +++ b/rust/kernel/revocable.rs
>> >> > @@ -139,6 +139,18 @@ pub fn try_access_with<R, F: FnOnce(&T) -> R>(&self, f: F) -> Option<R> {
>> >> >          self.try_access().map(|t| f(&*t))
>> >> >      }
>> >> >  
>> >> > +    /// Directly access the revocable wrapped object.
>> >> > +    ///
>> >> > +    /// # Safety
>> >> > +    ///
>> >> > +    /// The caller must ensure this [`Revocable`] instance hasn't been revoked and won't be revoked
>> >> > +    /// for the duration of `'a`.
>> >> 
>> >> Ah I missed this in my other email, in case you want to directly refer
>> >> to the lifetime, you should keep it defined. I would still remove the
>> >> `'s` lifetime though.
>> >> > +    pub unsafe fn access<'a, 's: 'a>(&'s self) -> &'a T {
>> >> > +        // SAFETY: By the safety requirement of this function it is guaranteed that
>> >> > +        // `self.data.get()` is a valid pointer to an instance of `T`.
>> >> 
>> >> I don't see how the "not-being revoked" state makes the `data` ptr be
>> >> valid. Is that an invariant of `Revocable`? (it's not documented to have
>> >> any invariants)
>> >
>> > What else makes it valid?
>> 
>> IMO an `# Invariants` section with the corresponding invariant that
>> `data` is valid when `is_available` is true.
>
> Yeah, I agree that the # Invariants section is indeed missing and should be
> fixed.
>
>> > AFAICS, try_access() and try_access_with_guard() argue the exact same way,
>> > except that the reason for not being revoked is the atomic check and the RCU
>> > read lock.
>> 
>> Just because other code is doing the same mistake doesn't make it
>> correct. If I had reviewed the patch at that time I'm sure I would have
>> pointed this out.
>
> I would say that try_access() and try_access_with_guard() are wrong, they rely

Did you mean to write `wouldn't`? Otherwise the second part doesn't
match IMO.

> on the correct thing, we just missed documenting the corresponding invariant.

Yeah it's not a behavior error, but since you agree that something
should be fixed, there also is something that is 'wrong' :)

>> I opened an issue about this:
>> 
>>     https://github.com/Rust-for-Linux/linux/issues/1160
>
> Thanks for creating the issue!
>
> What do you suggest for this patch?

I don't mind if you take it with the lifetime changes, so

Reviewed-by: Benno Lossin <benno.lossin@proton.me>

But I'd like the invariant to be documented (maybe we should tag the
issue with good-first-issue -- I don't actually think it is one, but
maybe you disagree).

---
Cheers,
Benno

Re: [PATCH 1/3] rust: revocable: implement Revocable::access()

[Danilo Krummrich]

On Sun, Apr 27, 2025 at 05:15:48PM +0000, Benno Lossin wrote:
> On Sun Apr 27, 2025 at 12:13 PM CEST, Danilo Krummrich wrote:
> > On Sun, Apr 27, 2025 at 08:37:00AM +0000, Benno Lossin wrote:
> >> On Sat Apr 26, 2025 at 11:18 PM CEST, Danilo Krummrich wrote:
> >> > On Sat, Apr 26, 2025 at 08:24:14PM +0000, Benno Lossin wrote:
> >> >> On Sat Apr 26, 2025 at 3:30 PM CEST, Danilo Krummrich wrote:
> >> >> > Implement an unsafe direct accessor for the data stored within the
> >> >> > Revocable.
> >> >> >
> >> >> > This is useful for cases where we can proof that the data stored within
> >> >> > the Revocable is not and cannot be revoked for the duration of the
> >> >> > lifetime of the returned reference.
> >> >> >
> >> >> > Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> >> >> > ---
> >> >> > The explicit lifetimes in access() probably don't serve a practical
> >> >> > purpose, but I found them to be useful for documentation purposes.
> >> >> > ---
> >> >> >  rust/kernel/revocable.rs | 12 ++++++++++++
> >> >> >  1 file changed, 12 insertions(+)
> >> >> >
> >> >> > diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
> >> >> > index 971d0dc38d83..33535de141ce 100644
> >> >> > --- a/rust/kernel/revocable.rs
> >> >> > +++ b/rust/kernel/revocable.rs
> >> >> > @@ -139,6 +139,18 @@ pub fn try_access_with<R, F: FnOnce(&T) -> R>(&self, f: F) -> Option<R> {
> >> >> >          self.try_access().map(|t| f(&*t))
> >> >> >      }
> >> >> >  
> >> >> > +    /// Directly access the revocable wrapped object.
> >> >> > +    ///
> >> >> > +    /// # Safety
> >> >> > +    ///
> >> >> > +    /// The caller must ensure this [`Revocable`] instance hasn't been revoked and won't be revoked
> >> >> > +    /// for the duration of `'a`.
> >> >> 
> >> >> Ah I missed this in my other email, in case you want to directly refer
> >> >> to the lifetime, you should keep it defined. I would still remove the
> >> >> `'s` lifetime though.
> >> >> > +    pub unsafe fn access<'a, 's: 'a>(&'s self) -> &'a T {
> >> >> > +        // SAFETY: By the safety requirement of this function it is guaranteed that
> >> >> > +        // `self.data.get()` is a valid pointer to an instance of `T`.
> >> >> 
> >> >> I don't see how the "not-being revoked" state makes the `data` ptr be
> >> >> valid. Is that an invariant of `Revocable`? (it's not documented to have
> >> >> any invariants)
> >> >
> >> > What else makes it valid?
> >> 
> >> IMO an `# Invariants` section with the corresponding invariant that
> >> `data` is valid when `is_available` is true.
> >
> > Yeah, I agree that the # Invariants section is indeed missing and should be
> > fixed.
> >
> >> > AFAICS, try_access() and try_access_with_guard() argue the exact same way,
> >> > except that the reason for not being revoked is the atomic check and the RCU
> >> > read lock.
> >> 
> >> Just because other code is doing the same mistake doesn't make it
> >> correct. If I had reviewed the patch at that time I'm sure I would have
> >> pointed this out.
> >
> > I would say that try_access() and try_access_with_guard() are wrong, they rely
> 
> Did you mean to write `wouldn't`? Otherwise the second part doesn't
> match IMO.

Yes, I meant "wouldn't". :)

> 
> > on the correct thing, we just missed documenting the corresponding invariant.
> 
> Yeah it's not a behavior error, but since you agree that something
> should be fixed, there also is something that is 'wrong' :)
> 
> >> I opened an issue about this:
> >> 
> >>     https://github.com/Rust-for-Linux/linux/issues/1160
> >
> > Thanks for creating the issue!
> >
> > What do you suggest for this patch?
> 
> I don't mind if you take it with the lifetime changes, so
> 
> Reviewed-by: Benno Lossin <benno.lossin@proton.me>
> 
> But I'd like the invariant to be documented (maybe we should tag the
> issue with good-first-issue -- I don't actually think it is one, but
> maybe you disagree).

Yes, it should be documented; regarding the issue you created, I'd be fine
marking it as good-first-issue.

But I'd also be fine sending a fix for this myself outside the scope of this
series.