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

[Alexandre Courbot]

On Mon Apr 7, 2025 at 6:20 AM JST, Benno Lossin wrote:
> On Sun Apr 6, 2025 at 3:58 PM CEST, 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@xxxxxxxxxx>
>> Suggested-by: Danilo Krummrich <dakr@xxxxxxxxxx>
>> Signed-off-by: Alexandre Courbot <acourbot@xxxxxxxxxx>
>
> Reviewed-by: Benno Lossin <benno.lossin@xxxxxxxxx>
>
>> ---
>>  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.
>
> I personally wouldn't have mentioned this in the docs, since to me such
> a helper would be obvious, but I don't mind it either.

Using a helper did not immediately occur to me, which is why I came with
two different methods initially. I'm fine with removing this part of the
doc if it sounds like it is overstepping the purpose of documentation
tough.