Message-ID: <20251029183538.226257-1-lyude@redhat.com>
Date: Wed, 29 Oct 2025 14:35:38 -0400
From: Lyude Paul <lyude@...hat.com>
To: linux-kernel@...r.kernel.org,
	rust-for-linux@...r.kernel.org,
	Benno Lossin <lossin@...nel.org>
Cc: Peter Zijlstra <peterz@...radead.org>,
	Ingo Molnar <mingo@...hat.com>,
	Will Deacon <will@...nel.org>,
	Boqun Feng <boqun.feng@...il.com>,
	Waiman Long <longman@...hat.com>,
	Miguel Ojeda <ojeda@...nel.org>,
	Alex Gaynor <alex.gaynor@...il.com>,
	Gary Guo <gary@...yguo.net>,
	Björn Roy Baron <bjorn3_gh@...tonmail.com>,
	Andreas Hindborg <a.hindborg@...nel.org>,
	Alice Ryhl <aliceryhl@...gle.com>,
	Trevor Gross <tmgross@...ch.edu>,
	Danilo Krummrich <dakr@...nel.org>
Subject: [PATCH v4] rust: lock: Export Guard::do_unlocked()

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@...hat.com>

---
V2:
* Fix documentation for do_unlocked
* Add an example
V3:
* Documentation changes from Miguel
V4:
* Improve the example to actually demonstrate a situation where
  do_unlocked() would be useful.
* Remove unneeded sentence above example in do_unlocked()

Signed-off-by: Lyude Paul <lyude@...hat.com>
---
 rust/kernel/sync/lock.rs | 71 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 70 insertions(+), 1 deletion(-)

diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs
index 5d7991e6d3736..c5f049a115d09 100644
--- a/rust/kernel/sync/lock.rs
+++ b/rust/kernel/sync/lock.rs
@@ -230,7 +230,76 @@ 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 temporarily, 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.
+    ///
+    /// It returns the value returned by the closure.
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// # use kernel::{
+    /// #     new_mutex,
+    /// #     sync::{lock::{Backend, Guard, Lock}, Arc, Mutex, Completion},
+    /// #     workqueue::{self, impl_has_work, new_work, Work, WorkItem},
+    /// # };
+    /// #[pin_data]
+    /// struct ExampleWork {
+    ///     #[pin]
+    ///     work: Work<Self>,
+    ///
+    ///     #[pin]
+    ///     lock: Mutex<i32>,
+    ///
+    ///     #[pin]
+    ///     done: Completion,
+    /// }
+    ///
+    /// impl_has_work! {
+    ///     impl HasWork<Self> for ExampleWork { self.work }
+    /// }
+    ///
+    /// impl WorkItem for ExampleWork {
+    ///     type Pointer = Arc<ExampleWork>;
+    ///
+    ///     fn run(this: Arc<ExampleWork>) {
+    ///         let mut g = this.lock.lock();
+    ///
+    ///         assert_eq!(*g, 41);
+    ///         *g += 1;
+    ///
+    ///         this.done.complete_all();
+    ///     }
+    /// }
+    ///
+    /// impl ExampleWork {
+    ///     pub(crate) fn new() -> Result<Arc<Self>> {
+    ///         Arc::pin_init(pin_init!(Self {
+    ///             work <- new_work!(),
+    ///             lock <- new_mutex!(41),
+    ///             done <- Completion::new(),
+    ///         }), GFP_KERNEL)
+    ///     }
+    /// }
+    ///
+    /// let work = ExampleWork::new().unwrap();
+    /// let mut g = work.lock.lock();
+    ///
+    /// let _ = workqueue::system().enqueue(work.clone());
+    ///
+    /// // This would deadlock:
+    /// //
+    /// //     work.done.wait_for_completion()
+    /// //
+    /// // Since we hold work.lock, which work will also try to acquire in WorkItem::run. Dropping
+    /// // the lock temporarily while we wait for completion works around this.
+    /// g.do_unlocked(|| work.done.wait_for_completion());
+    ///
+    /// assert_eq!(*g, 42);
+    /// ```
+    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: 3b83f5d5e78ac5cddd811a5e431af73959864390
-- 
2.51.0

Powered by blists - more mailing lists