[PATCH v4 0/2] rust/revocable: add try_access_with() convenience method

[PATCH v4 0/2] rust/revocable: add try_access_with() convenience method

[Alexandre Courbot]

This is a feature I found useful to have while writing Nova driver code
that accessed registers alongside other operations. I would find myself
quite confused about whether the guard was held or dropped at a given
point of the code, and it felt like walking through a minefield; this
pattern makes things safer and easier to read according to my experience
writing nova-core code.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
Changes in v4:
- Collected Reviewed-by tags.
- Link to v3: https://lore.kernel.org/r/20250406-try_with-v3-0-c0947842e768@nvidia.com

Changes in v3:
- Add Acked-by from v2.

Changes in v2:
- Use FnOnce for the callback type.
- Rename to try_access_with.
- Don't assume that users will want to map failure to ENXIO and return
  an option.
- Use a single method and let users adapt the behavior using their own
  wrappers/macros.

---
Alexandre Courbot (2):
      rust/revocable: add try_access_with() convenience method
      samples: rust: convert PCI rust sample driver to use try_access_with()

 rust/kernel/revocable.rs        | 16 ++++++++++++++++
 samples/rust/rust_driver_pci.rs | 11 +++++------
 2 files changed, 21 insertions(+), 6 deletions(-)
---
base-commit: 0af2f6be1b4281385b618cb86ad946eded089ac8
change-id: 20250313-try_with-cc9f91dd3b60

Best regards,
-- 
Alexandre Courbot <acourbot@nvidia.com>

[PATCH v4 1/2] rust/revocable: add try_access_with() convenience method

[Alexandre Courbot]

Revocable::try_access() returns a guard through which the wrapped object
can be accessed. Code that can sleep is not allowed while the guard is
held; thus, it is common for the caller to explicitly drop it before
running sleepable code, e.g:

    let b = bar.try_access()?;
    let reg = b.readl(...);

    // Don't forget this or things could go wrong!
    drop(b);

    something_that_might_sleep();

    let b = bar.try_access()?;
    let reg2 = b.readl(...);

This is arguably error-prone. try_access_with() provides an arguably
safer alternative, by taking a closure that is run while the guard is
held, and by dropping the guard automatically after the closure
completes. This way, code can be organized more clearly around the
critical sections and the risk of forgetting to release the guard when
needed is considerably reduced:

    let reg = bar.try_access_with(|b| b.readl(...))?;

    something_that_might_sleep();

    let reg2 = bar.try_access_with(|b| b.readl(...))?;

The closure can return nothing, or any value including a Result which is
then wrapped inside the Option returned by try_access_with. Error
management is driver-specific, so users are encouraged to create their
own macros that map and flatten the returned values to something
appropriate for the code they are working on.

Acked-by: Danilo Krummrich <dakr@kernel.org>
Suggested-by: Danilo Krummrich <dakr@kernel.org>
Reviewed-by: Benno Lossin <benno.lossin@proton.me>
Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 rust/kernel/revocable.rs | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
index 1e5a9d25c21b279b01f90b02997492aa4880d84f..b91e40e8160be0cc0ff8e0699e48e063c9dbce22 100644
--- a/rust/kernel/revocable.rs
+++ b/rust/kernel/revocable.rs
@@ -123,6 +123,22 @@ pub fn try_access_with_guard<'a>(&'a self, _guard: &'a rcu::Guard) -> Option<&'a
         }
     }
 
+    /// Tries to access the wrapped object and run a closure on it while the guard is held.
+    ///
+    /// This is a convenience method to run short non-sleepable code blocks while ensuring the
+    /// guard is dropped afterwards. [`Self::try_access`] carries the risk that the caller will
+    /// forget to explicitly drop that returned guard before calling sleepable code; this method
+    /// adds an extra safety to make sure it doesn't happen.
+    ///
+    /// Returns `None` if the object has been revoked and is therefore no longer accessible, or the
+    /// result of the closure wrapped in `Some`. If the closure returns a [`Result`] then the
+    /// return type becomes `Option<Result<>>`, which can be inconvenient. Users are encouraged to
+    /// define their own macro that turns the `Option` into a proper error code and flattens the
+    /// inner result into it if it makes sense within their subsystem.
+    pub fn try_access_with<R, F: FnOnce(&T) -> R>(&self, f: F) -> Option<R> {
+        self.try_access().map(|t| f(&*t))
+    }
+
     /// # Safety
     ///
     /// Callers must ensure that there are no more concurrent users of the revocable object.

-- 
2.49.0

[PATCH v4 2/2] samples: rust: convert PCI rust sample driver to use try_access_with()

[Alexandre Courbot]

This method limits the scope of the revocable guard and is considered
safer to use for most cases, so let's showcase it here.

Acked-by: Danilo Krummrich <dakr@kernel.org>
Reviewed-by: Benno Lossin <benno.lossin@proton.me>
Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 samples/rust/rust_driver_pci.rs | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

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

-- 
2.49.0

Re: [PATCH v4 0/2] rust/revocable: add try_access_with() convenience method

[Danilo Krummrich]

On Fri, Apr 11, 2025 at 09:09:37PM +0900, Alexandre Courbot wrote:
> This is a feature I found useful to have while writing Nova driver code
> that accessed registers alongside other operations. I would find myself
> quite confused about whether the guard was held or dropped at a given
> point of the code, and it felt like walking through a minefield; this
> pattern makes things safer and easier to read according to my experience
> writing nova-core code.

Any concerns taking this through the nova tree?

Re: [v4,1/2] rust/revocable: add try_access_with() convenience method

[Joel Fernandes]

Hello, Alexandre,

On Tue, 15 Apr 2025 04:37:01 GMT, Alexandre Courbot wrote:
> Revocable::try_access() returns a guard through which the wrapped object
> can be accessed. Code that can sleep is not allowed while the guard is
> held; thus, it is common for the caller to explicitly drop it before
> running sleepable code, e.g:
> 
>     let b = bar.try_access()?;
>     let reg = b.readl(...);
> 
>     // Don't forget this or things could go wrong!
>     drop(b);
> 
>     something_that_might_sleep();
> 
>     let b = bar.try_access()?;
>     let reg2 = b.readl(...);
> 
> This is arguably error-prone. try_access_with() provides an arguably
> safer alternative, by taking a closure that is run while the guard is
> held, and by dropping the guard automatically after the closure
> completes. This way, code can be organized more clearly around the
> critical sections and the risk of forgetting to release the guard when
> needed is considerably reduced:
> 
>     let reg = bar.try_access_with(|b| b.readl(...))?;
> 
>     something_that_might_sleep();
> 
>     let reg2 = bar.try_access_with(|b| b.readl(...))?;
> 
> The closure can return nothing, or any value including a Result which is
> then wrapped inside the Option returned by try_access_with. Error
> management is driver-specific, so users are encouraged to create their
> own macros that map and flatten the returned values to something
> appropriate for the code they are working on.
> 
> Acked-by: Danilo Krummrich <dakr@kernel.org>
> Suggested-by: Danilo Krummrich <dakr@kernel.org>
> Reviewed-by: Benno Lossin <benno.lossin@proton.me>
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>

Reviewed-by: Joel Fernandes <joelagnelf@nvidia.com>

Thanks.

Re: [v4,1/2] rust/revocable: add try_access_with() convenience method

[Joel Fernandes]

Hello, Alexandre,

On Tue, 15 Apr 2025 04:37:01 GMT, Alexandre Courbot wrote:
> Revocable::try_access() returns a guard through which the wrapped object
> can be accessed. Code that can sleep is not allowed while the guard is
> held; thus, it is common for the caller to explicitly drop it before
> running sleepable code, e.g:
> 
>     let b = bar.try_access()?;
>     let reg = b.readl(...);
> 
>     // Don't forget this or things could go wrong!
>     drop(b);
> 
>     something_that_might_sleep();
> 
>     let b = bar.try_access()?;
>     let reg2 = b.readl(...);
> 
> This is arguably error-prone. try_access_with() provides an arguably
> safer alternative, by taking a closure that is run while the guard is
> held, and by dropping the guard automatically after the closure
> completes. This way, code can be organized more clearly around the
> critical sections and the risk of forgetting to release the guard when
> needed is considerably reduced:
> 
>     let reg = bar.try_access_with(|b| b.readl(...))?;
> 
>     something_that_might_sleep();
> 
>     let reg2 = bar.try_access_with(|b| b.readl(...))?;
> 
> The closure can return nothing, or any value including a Result which is
> then wrapped inside the Option returned by try_access_with. Error
> management is driver-specific, so users are encouraged to create their
> own macros that map and flatten the returned values to something
> appropriate for the code they are working on.
> 
> Acked-by: Danilo Krummrich <dakr@kernel.org>
> Suggested-by: Danilo Krummrich <dakr@kernel.org>
> Reviewed-by: Benno Lossin <benno.lossin@proton.me>
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>

Reviewed-by: Joel Fernandes <joelagnelf@nvidia.com>

Thanks.

Re: [PATCH v4 0/2] rust/revocable: add try_access_with() convenience method

[Danilo Krummrich]

On 4/11/25 3:15 PM, Danilo Krummrich wrote:
> On Fri, Apr 11, 2025 at 09:09:37PM +0900, Alexandre Courbot wrote:
>> This is a feature I found useful to have while writing Nova driver code
>> that accessed registers alongside other operations. I would find myself
>> quite confused about whether the guard was held or dropped at a given
>> point of the code, and it felt like walking through a minefield; this
>> pattern makes things safer and easier to read according to my experience
>> writing nova-core code.
> 
> Any concerns taking this through the nova tree?

@Miguel: Can I get an ACK for taking it through the nova tree?

Re: [PATCH v4 0/2] rust/revocable: add try_access_with() convenience method

[Miguel Ojeda]

On Tue, Apr 22, 2025 at 10:01 AM Danilo Krummrich <dakr@kernel.org> wrote:
>
> @Miguel: Can I get an ACK for taking it through the nova tree?

Sure, please go ahead:

Acked-by: Miguel Ojeda <ojeda@kernel.org>

(Sorry, I missed replying to the other one)

Cheers,
Miguel

Re: [PATCH v4 1/2] rust/revocable: add try_access_with() convenience method

[Miguel Ojeda]

On Fri, Apr 11, 2025 at 2:09 PM Alexandre Courbot <acourbot@nvidia.com> wrote:
>
> +    /// Returns `None` if the object has been revoked and is therefore no longer accessible, or the

[`None`]

> +    /// result of the closure wrapped in `Some`. If the closure returns a [`Result`] then the

[`Some`]

> +    /// define their own macro that turns the `Option` into a proper error code and flattens the

[`Option`]

(Assuming each one works)

Cheers,
Miguel

Re: [PATCH v4 0/2] rust/revocable: add try_access_with() convenience method

[Danilo Krummrich]

On Fri, Apr 11, 2025 at 09:09:37PM +0900, Alexandre Courbot wrote:
> This is a feature I found useful to have while writing Nova driver code
> that accessed registers alongside other operations. I would find myself
> quite confused about whether the guard was held or dropped at a given
> point of the code, and it felt like walking through a minefield; this
> pattern makes things safer and easier to read according to my experience
> writing nova-core code.
> 
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>

With the following changes, applied to nova-next, thanks!

  * link `None`, `Some`, `Option` in doc-comment

- Danilo