[PATCH v2] rust: lock: Export Guard::do_unlocked()

[PATCH v2] rust: lock: Export Guard::do_unlocked()

[Lyude Paul]

In RVKMS, I discovered a silly issue where as a result of our HrTimer for
vblank emulation and our vblank enable/disable callbacks sharing a
spinlock, it was possible to deadlock while trying to disable the vblank
timer.

The solution for this ended up being simple: keep track of when the HrTimer
could potentially acquire the shared spinlock, and simply drop the spinlock
temporarily from our vblank enable/disable callbacks when stopping the
timer. And do_unlocked() ended up being perfect for this.

Since this seems like it's useful, let's export this for use by the rest of
the world and write short documentation for it.

Signed-off-by: Lyude Paul <lyude@redhat.com>

---
V2:
* Fix documentation for do_unlocked
* Add an example

You can find an example usage of this here:

https://gitlab.freedesktop.org/lyudess/linux/-/blob/rvkms-slim/drivers/gpu/drm/rvkms/crtc.rs

 rust/kernel/sync/lock.rs | 36 +++++++++++++++++++++++++++++++++++-
 1 file changed, 35 insertions(+), 1 deletion(-)

diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs
index e82fa5be289c1..e43ee5e2e4b9f 100644
--- a/rust/kernel/sync/lock.rs
+++ b/rust/kernel/sync/lock.rs
@@ -228,7 +228,41 @@ pub fn lock_ref(&self) -> &'a Lock<T, B> {
         self.lock
     }
 
-    pub(crate) fn do_unlocked<U>(&mut self, cb: impl FnOnce() -> U) -> U {
+    /// Releases this [`Guard`]'s lock temporary, executes `cb` and then re-acquires it.
+    ///
+    /// This can be useful for situations where you may need to do a temporary unlock dance to avoid
+    /// issues like circular locking dependencies.
+    ///
+    /// If the closure returns a value, it will be returned by this function.
+    ///
+    /// # Examples
+    ///
+    /// The following example shows how to use [`Guard::do_unlocked`] to temporarily release a lock,
+    /// do some work, then re-lock it.
+    ///
+    /// ```
+    /// # use kernel::{new_spinlock, sync::lock::{Backend, Guard, Lock}};
+    /// # use pin_init::stack_pin_init;
+    ///
+    /// fn assert_held<T, B: Backend>(guard: &Guard<'_, T, B>, lock: &Lock<T, B>) {
+    ///     // Address-equal means the same lock.
+    ///     assert!(core::ptr::eq(guard.lock_ref(), lock));
+    /// }
+    ///
+    /// stack_pin_init! {
+    ///     let l = new_spinlock!(42)
+    /// }
+    ///
+    /// let mut g = l.lock();
+    /// let val = *g;
+    ///
+    /// // The lock will be released, but only temporarily
+    /// g.do_unlocked(|| assert_eq!(val, 42));
+    ///
+    /// // `g` originates from `l` and should be relocked now.
+    /// assert_held(&g, &l);
+    /// ```
+    pub fn do_unlocked<U>(&mut self, cb: impl FnOnce() -> U) -> U {
         // SAFETY: The caller owns the lock, so it is safe to unlock it.
         unsafe { B::unlock(self.lock.state.get(), &self.state) };
 

base-commit: dff64b072708ffef23c117fa1ee1ea59eb417807
-- 
2.50.0

Re: [PATCH v2] rust: lock: Export Guard::do_unlocked()

[Boqun Feng]

On Thu, Jul 24, 2025 at 02:27:16PM -0400, Lyude Paul wrote:
> In RVKMS, I discovered a silly issue where as a result of our HrTimer for
> vblank emulation and our vblank enable/disable callbacks sharing a
> spinlock, it was possible to deadlock while trying to disable the vblank
> timer.
> 
> The solution for this ended up being simple: keep track of when the HrTimer
> could potentially acquire the shared spinlock, and simply drop the spinlock
> temporarily from our vblank enable/disable callbacks when stopping the
> timer. And do_unlocked() ended up being perfect for this.
> 
> Since this seems like it's useful, let's export this for use by the rest of
> the world and write short documentation for it.
> 
> Signed-off-by: Lyude Paul <lyude@redhat.com>
> 

Looks good to me. It'll probably go into v6.18 since we are at -rc7 now.
I will wait for a few more reviews. Thanks!

Regards,
Boqun

> ---
> V2:
> * Fix documentation for do_unlocked
> * Add an example
> 
> You can find an example usage of this here:
> 
> https://gitlab.freedesktop.org/lyudess/linux/-/blob/rvkms-slim/drivers/gpu/drm/rvkms/crtc.rs
> 
>  rust/kernel/sync/lock.rs | 36 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 35 insertions(+), 1 deletion(-)
> 
> diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs
> index e82fa5be289c1..e43ee5e2e4b9f 100644
> --- a/rust/kernel/sync/lock.rs
> +++ b/rust/kernel/sync/lock.rs
> @@ -228,7 +228,41 @@ pub fn lock_ref(&self) -> &'a Lock<T, B> {
>          self.lock
>      }
>  
> -    pub(crate) fn do_unlocked<U>(&mut self, cb: impl FnOnce() -> U) -> U {
> +    /// Releases this [`Guard`]'s lock temporary, executes `cb` and then re-acquires it.
> +    ///
> +    /// This can be useful for situations where you may need to do a temporary unlock dance to avoid
> +    /// issues like circular locking dependencies.
> +    ///
> +    /// If the closure returns a value, it will be returned by this function.
> +    ///
> +    /// # Examples
> +    ///
> +    /// The following example shows how to use [`Guard::do_unlocked`] to temporarily release a lock,
> +    /// do some work, then re-lock it.
> +    ///
> +    /// ```
> +    /// # use kernel::{new_spinlock, sync::lock::{Backend, Guard, Lock}};
> +    /// # use pin_init::stack_pin_init;
> +    ///
> +    /// fn assert_held<T, B: Backend>(guard: &Guard<'_, T, B>, lock: &Lock<T, B>) {
> +    ///     // Address-equal means the same lock.
> +    ///     assert!(core::ptr::eq(guard.lock_ref(), lock));
> +    /// }
> +    ///
> +    /// stack_pin_init! {
> +    ///     let l = new_spinlock!(42)
> +    /// }
> +    ///
> +    /// let mut g = l.lock();
> +    /// let val = *g;
> +    ///
> +    /// // The lock will be released, but only temporarily
> +    /// g.do_unlocked(|| assert_eq!(val, 42));
> +    ///
> +    /// // `g` originates from `l` and should be relocked now.
> +    /// assert_held(&g, &l);
> +    /// ```
> +    pub fn do_unlocked<U>(&mut self, cb: impl FnOnce() -> U) -> U {
>          // SAFETY: The caller owns the lock, so it is safe to unlock it.
>          unsafe { B::unlock(self.lock.state.get(), &self.state) };
>  
> 
> base-commit: dff64b072708ffef23c117fa1ee1ea59eb417807
> -- 
> 2.50.0
> 

Re: [PATCH v2] rust: lock: Export Guard::do_unlocked()

[Miguel Ojeda]

On Thu, Jul 24, 2025 at 8:28 PM Lyude Paul <lyude@redhat.com> wrote:
>
> +    /// Releases this [`Guard`]'s lock temporary, executes `cb` and then re-acquires it.

temporarily?

> +    /// If the closure returns a value, it will be returned by this function.

Perhaps we can simplify by avoiding the conditional:

    It returns the value returned by the closure.

> +    /// # use pin_init::stack_pin_init;
> +    ///
> +    /// fn assert_held<T, B: Backend>(guard: &Guard<'_, T, B>, lock: &Lock<T, B>) {

We need to remove the newline to avoid an empty line in the rendered output.

Cheers,
Miguel