[PATCH v2 0/1] rust: add Work::disable_sync

[PATCH v2 0/1] rust: add Work::disable_sync

[Onur Özkan]

The immediate motivation is the Tyr reset infrastructure [1] which needs
to stop queued or running reset work during teardown before dropping the
resources used by that work. The reset series started to require too many
independent dependencies, so this is split out as a standalone change to
keep the reset series focused on the reset logic and easier to review,
rebase and land.

[1]: https://lore.kernel.org/all/20260416171728.205141-1-work@onurozkan.dev

Changes since v1:
- Fixed a soundness issue where Work::disable_sync() could make a later
  Pin<KBox<T>> enqueue fail and hit unreachable_unchecked().
- Removed the WorkItemPointer::cancel() helper and explicitly call drop()
  in disable_sync() after C has already canceled the work.

v1: https://lore.kernel.org/all/20260428104459.174602-1-work@onurozkan.dev

Onur Özkan (1):
  rust: add Work::disable_sync

 rust/kernel/workqueue.rs | 121 ++++++++++++++++++++++++++-------------
 1 file changed, 81 insertions(+), 40 deletions(-)

-- 
2.51.2

[PATCH v2 1/1] rust: add Work::disable_sync

[Onur Özkan]

Adds Work::disable_sync() as a safe wrapper for disable_work_sync().

Drivers can use this during teardown to stop new queueing and wait for
queued or running work to finish before dropping related resources.

Signed-off-by: Onur Özkan <work@onurozkan.dev>
---
 rust/kernel/workqueue.rs | 121 ++++++++++++++++++++++++++-------------
 1 file changed, 81 insertions(+), 40 deletions(-)

diff --git a/rust/kernel/workqueue.rs b/rust/kernel/workqueue.rs
index 7e253b6f299c..d0f9b4ba7f27 100644
--- a/rust/kernel/workqueue.rs
+++ b/rust/kernel/workqueue.rs
@@ -267,7 +267,7 @@ pub unsafe fn from_raw<'a>(ptr: *const bindings::workqueue_struct) -> &'a Queue
 
     /// Enqueues a work item.
     ///
-    /// This may fail if the work item is already enqueued in a workqueue.
+    /// This may fail if the work item is already enqueued in a workqueue or disabled.
     ///
     /// The work item will be submitted using `WORK_CPU_UNBOUND`.
     pub fn enqueue<W, const ID: u64>(&self, w: W) -> W::EnqueueOutput
@@ -276,8 +276,9 @@ pub fn enqueue<W, const ID: u64>(&self, w: W) -> W::EnqueueOutput
     {
         let queue_ptr = self.0.get();
 
-        // SAFETY: We only return `false` if the `work_struct` is already in a workqueue. The other
-        // `__enqueue` requirements are not relevant since `W` is `Send` and static.
+        // SAFETY: We only return `false` if the `work_struct` is already in a workqueue or
+        // disabled. The other `__enqueue` requirements are not relevant since `W` is `Send` and
+        // static.
         //
         // The call to `bindings::queue_work_on` will dereference the provided raw pointer, which
         // is ok because `__enqueue` guarantees that the pointer is valid for the duration of this
@@ -300,7 +301,7 @@ pub fn enqueue<W, const ID: u64>(&self, w: W) -> W::EnqueueOutput
 
     /// Enqueues a delayed work item.
     ///
-    /// This may fail if the work item is already enqueued in a workqueue.
+    /// This may fail if the work item is already enqueued in a workqueue or disabled.
     ///
     /// The work item will be submitted using `WORK_CPU_UNBOUND`.
     pub fn enqueue_delayed<W, const ID: u64>(&self, w: W, delay: Jiffies) -> W::EnqueueOutput
@@ -309,8 +310,9 @@ pub fn enqueue_delayed<W, const ID: u64>(&self, w: W, delay: Jiffies) -> W::Enqu
     {
         let queue_ptr = self.0.get();
 
-        // SAFETY: We only return `false` if the `work_struct` is already in a workqueue. The other
-        // `__enqueue` requirements are not relevant since `W` is `Send` and static.
+        // SAFETY: We only return `false` if the `work_struct` is already in a workqueue or
+        // disabled. The other `__enqueue` requirements are not relevant since `W` is `Send` and
+        // static.
         //
         // The call to `bindings::queue_delayed_work_on` will dereference the provided raw pointer,
         // which is ok because `__enqueue` guarantees that the pointer is valid for the duration of
@@ -347,7 +349,7 @@ pub fn try_spawn<T: 'static + Send + FnOnce()>(
             func: Some(func),
         });
 
-        self.enqueue(KBox::pin_init(init, flags).map_err(|_| AllocError)?);
+        let _ = self.enqueue(KBox::pin_init(init, flags).map_err(|_| AllocError)?);
         Ok(())
     }
 }
@@ -407,7 +409,8 @@ pub unsafe trait RawWorkItem<const ID: u64> {
     ///
     /// # Safety
     ///
-    /// The provided closure may only return `false` if the `work_struct` is already in a workqueue.
+    /// The provided closure may only return `false` if the `work_struct` is already in a workqueue
+    /// or disabled.
     ///
     /// If the work item type is annotated with any lifetimes, then you must not call the function
     /// pointer after any such lifetime expires. (Never calling the function pointer is okay.)
@@ -442,21 +445,34 @@ pub unsafe trait RawDelayedWorkItem<const ID: u64>: RawWorkItem<ID> {}
 ///
 /// # Safety
 ///
-/// Implementers must ensure that [`__enqueue`] uses a `work_struct` initialized with the [`run`]
-/// method of this trait as the function pointer.
-///
-/// [`__enqueue`]: RawWorkItem::__enqueue
-/// [`run`]: WorkItemPointer::run
-pub unsafe trait WorkItemPointer<const ID: u64>: RawWorkItem<ID> {
-    /// Run this work item.
+/// Implementers must ensure that [`WorkItemPointer::from_raw_work`] rebuilds the exact ownership
+/// transferred by a successful [`RawWorkItem::__enqueue`] call.
+pub unsafe trait WorkItemPointer<const ID: u64>: RawWorkItem<ID> + Sized {
+    /// The work item type containing the embedded `work_struct`.
+    type Item: WorkItem<ID, Pointer = Self> + ?Sized;
+
+    /// Rebuild this work item's pointer from its embedded `work_struct`.
     ///
     /// # Safety
     ///
-    /// The provided `work_struct` pointer must originate from a previous call to [`__enqueue`]
-    /// where the `queue_work_on` closure returned true, and the pointer must still be valid.
+    /// The provided `work_struct` pointer must originate from a previous call to
+    /// [`RawWorkItem::__enqueue`] where the `queue_work_on` closure returned true
+    /// and the pointer must still be valid.
+    unsafe fn from_raw_work(ptr: *mut bindings::work_struct) -> Self;
+
+    /// Run this work item.
+    ///
+    /// # Safety
     ///
-    /// [`__enqueue`]: RawWorkItem::__enqueue
-    unsafe extern "C" fn run(ptr: *mut bindings::work_struct);
+    /// The provided `work_struct` pointer must satisfy the same requirements as
+    /// [`WorkItemPointer::from_raw_work`].
+    #[inline]
+    unsafe extern "C" fn run(ptr: *mut bindings::work_struct) {
+        <Self::Item as WorkItem<ID>>::run(
+            // SAFETY: The requirements for `run` are exactly those of `from_raw_work`.
+            unsafe { Self::from_raw_work(ptr) },
+        );
+    }
 }
 
 /// Defines the method that should be called when this work item is executed.
@@ -537,6 +553,28 @@ pub unsafe fn raw_get(ptr: *const Self) -> *mut bindings::work_struct {
         // the compiler does not complain that the `work` field is unused.
         unsafe { Opaque::cast_into(core::ptr::addr_of!((*ptr).work)) }
     }
+
+    /// Disables this work item and waits for queued/running executions to finish.
+    ///
+    /// # Note
+    ///
+    /// Should be called from a sleepable context if the work was last queued on a non-BH
+    /// workqueue.
+    #[inline]
+    pub fn disable_sync(&self)
+    where
+        T: WorkItem<ID>,
+    {
+        let ptr: *const Self = self;
+        // SAFETY: `self` points to a valid initialized work.
+        let raw_work = unsafe { Self::raw_get(ptr) };
+        // SAFETY: `raw_work` is a valid embedded `work_struct`.
+        if unsafe { bindings::disable_work_sync(raw_work) } {
+            // SAFETY: A `true` return means the work was pending and got canceled, so the queued
+            // ownership transfer performed by `__enqueue` is reclaimed here.
+            drop(unsafe { T::Pointer::from_raw_work(raw_work) });
+        }
+    }
 }
 
 /// Declares that a type contains a [`Work<T, ID>`].
@@ -817,22 +855,22 @@ unsafe fn work_container_of(
 //   - `Work::new` makes sure that `T::Pointer::run` is passed to `init_work_with_key`.
 //   - Finally `Work` and `RawWorkItem` guarantee that the correct `Work` field
 //     will be used because of the ID const generic bound. This makes sure that `T::raw_get_work`
-//     uses the correct offset for the `Work` field, and `Work::new` picks the correct
-//     implementation of `WorkItemPointer` for `Arc<T>`.
+//     uses the correct offset for the `Work` field, and `T::Pointer::from_raw_work` rebuilds the
+//     correct pointer type for `Arc<T>`.
 unsafe impl<T, const ID: u64> WorkItemPointer<ID> for Arc<T>
 where
     T: WorkItem<ID, Pointer = Self>,
     T: HasWork<T, ID>,
 {
-    unsafe extern "C" fn run(ptr: *mut bindings::work_struct) {
+    type Item = T;
+
+    unsafe fn from_raw_work(ptr: *mut bindings::work_struct) -> Self {
         // The `__enqueue` method always uses a `work_struct` stored in a `Work<T, ID>`.
         let ptr = ptr.cast::<Work<T, ID>>();
         // SAFETY: This computes the pointer that `__enqueue` got from `Arc::into_raw`.
         let ptr = unsafe { T::work_container_of(ptr) };
         // SAFETY: This pointer comes from `Arc::into_raw` and we've been given back ownership.
-        let arc = unsafe { Arc::from_raw(ptr) };
-
-        T::run(arc)
+        unsafe { Arc::from_raw(ptr) }
     }
 }
 
@@ -887,7 +925,9 @@ unsafe impl<T, const ID: u64> WorkItemPointer<ID> for Pin<KBox<T>>
     T: WorkItem<ID, Pointer = Self>,
     T: HasWork<T, ID>,
 {
-    unsafe extern "C" fn run(ptr: *mut bindings::work_struct) {
+    type Item = T;
+
+    unsafe fn from_raw_work(ptr: *mut bindings::work_struct) -> Self {
         // The `__enqueue` method always uses a `work_struct` stored in a `Work<T, ID>`.
         let ptr = ptr.cast::<Work<T, ID>>();
         // SAFETY: This computes the pointer that `__enqueue` got from `Arc::into_raw`.
@@ -895,9 +935,7 @@ unsafe impl<T, const ID: u64> WorkItemPointer<ID> for Pin<KBox<T>>
         // SAFETY: This pointer comes from `Arc::into_raw` and we've been given back ownership.
         let boxed = unsafe { KBox::from_raw(ptr) };
         // SAFETY: The box was already pinned when it was enqueued.
-        let pinned = unsafe { Pin::new_unchecked(boxed) };
-
-        T::run(pinned)
+        unsafe { Pin::new_unchecked(boxed) }
     }
 }
 
@@ -907,7 +945,7 @@ unsafe impl<T, const ID: u64> RawWorkItem<ID> for Pin<KBox<T>>
     T: WorkItem<ID, Pointer = Self>,
     T: HasWork<T, ID>,
 {
-    type EnqueueOutput = ();
+    type EnqueueOutput = Result<(), Self>;
 
     unsafe fn __enqueue<F>(self, queue_work_on: F) -> Self::EnqueueOutput
     where
@@ -923,10 +961,13 @@ unsafe fn __enqueue<F>(self, queue_work_on: F) -> Self::EnqueueOutput
         // SAFETY: `raw_get_work` returns a pointer to a valid value.
         let work_ptr = unsafe { Work::raw_get(work_ptr) };
 
-        if !queue_work_on(work_ptr) {
-            // SAFETY: This method requires exclusive ownership of the box, so it cannot be in a
-            // workqueue.
-            unsafe { ::core::hint::unreachable_unchecked() }
+        if queue_work_on(work_ptr) {
+            Ok(())
+        } else {
+            // SAFETY: The work queue has not taken ownership of the pointer.
+            let boxed = unsafe { KBox::from_raw(ptr) };
+            // SAFETY: The box was pinned before this enqueue attempt.
+            Err(unsafe { Pin::new_unchecked(boxed) })
         }
     }
 }
@@ -950,15 +991,17 @@ unsafe impl<T, const ID: u64> RawDelayedWorkItem<ID> for Pin<KBox<T>>
 //   - `Work::new` makes sure that `T::Pointer::run` is passed to `init_work_with_key`.
 //   - Finally `Work` and `RawWorkItem` guarantee that the correct `Work` field
 //     will be used because of the ID const generic bound. This makes sure that `T::raw_get_work`
-//     uses the correct offset for the `Work` field, and `Work::new` picks the correct
-//     implementation of `WorkItemPointer` for `ARef<T>`.
+//     uses the correct offset for the `Work` field, and `T::Pointer::from_raw_work` rebuilds the
+//     correct pointer type for `ARef<T>`.
 unsafe impl<T, const ID: u64> WorkItemPointer<ID> for ARef<T>
 where
     T: AlwaysRefCounted,
     T: WorkItem<ID, Pointer = Self>,
     T: HasWork<T, ID>,
 {
-    unsafe extern "C" fn run(ptr: *mut bindings::work_struct) {
+    type Item = T;
+
+    unsafe fn from_raw_work(ptr: *mut bindings::work_struct) -> Self {
         // The `__enqueue` method always uses a `work_struct` stored in a `Work<T, ID>`.
         let ptr = ptr.cast::<Work<T, ID>>();
 
@@ -972,9 +1015,7 @@ unsafe impl<T, const ID: u64> WorkItemPointer<ID> for ARef<T>
 
         // SAFETY: This pointer comes from `ARef::into_raw` and we've been given
         // back ownership.
-        let aref = unsafe { ARef::from_raw(ptr) };
-
-        T::run(aref)
+        unsafe { ARef::from_raw(ptr) }
     }
 }
 
-- 
2.51.2

Re: [PATCH v2 1/1] rust: add Work::disable_sync

[Alice Ryhl]

On Fri, May 01, 2026 at 10:11:22PM +0300, Onur Özkan wrote:
> Adds Work::disable_sync() as a safe wrapper for disable_work_sync().
> 
> Drivers can use this during teardown to stop new queueing and wait for
> queued or running work to finish before dropping related resources.
> 
> Signed-off-by: Onur Özkan <work@onurozkan.dev>
> ---
>  rust/kernel/workqueue.rs | 121 ++++++++++++++++++++++++++-------------
>  1 file changed, 81 insertions(+), 40 deletions(-)
> 
> diff --git a/rust/kernel/workqueue.rs b/rust/kernel/workqueue.rs
> index 7e253b6f299c..d0f9b4ba7f27 100644
> --- a/rust/kernel/workqueue.rs
> +++ b/rust/kernel/workqueue.rs
> @@ -267,7 +267,7 @@ pub unsafe fn from_raw<'a>(ptr: *const bindings::workqueue_struct) -> &'a Queue
>  
>      /// Enqueues a work item.
>      ///
> -    /// This may fail if the work item is already enqueued in a workqueue.
> +    /// This may fail if the work item is already enqueued in a workqueue or disabled.
>      ///
>      /// The work item will be submitted using `WORK_CPU_UNBOUND`.
>      pub fn enqueue<W, const ID: u64>(&self, w: W) -> W::EnqueueOutput

Can you elaborate on the case where disable leads to failure here? Can
you not enqueue a work item again after disabling it? Is there a doc
test illustrating this case that I can run for myself to see the
behavior in action?

Alice

Re: [PATCH v2 0/1] rust: add Work::disable_sync

[Alice Ryhl]

On Fri, May 01, 2026 at 10:11:21PM +0300, Onur Özkan wrote:
> The immediate motivation is the Tyr reset infrastructure [1] which needs
> to stop queued or running reset work during teardown before dropping the
> resources used by that work. The reset series started to require too many
> independent dependencies, so this is split out as a standalone change to
> keep the reset series focused on the reset logic and easier to review,
> rebase and land.
> 
> [1]: https://lore.kernel.org/all/20260416171728.205141-1-work@onurozkan.dev

The fact that it's motivated by Tyr could be in the commit message
itself.

> Changes since v1:
> - Fixed a soundness issue where Work::disable_sync() could make a later
>   Pin<KBox<T>> enqueue fail and hit unreachable_unchecked().
> - Removed the WorkItemPointer::cancel() helper and explicitly call drop()
>   in disable_sync() after C has already canceled the work.
> 
> v1: https://lore.kernel.org/all/20260428104459.174602-1-work@onurozkan.dev
> 
> Onur Özkan (1):
>   rust: add Work::disable_sync
> 
>  rust/kernel/workqueue.rs | 121 ++++++++++++++++++++++++++-------------
>  1 file changed, 81 insertions(+), 40 deletions(-)
> 
> -- 
> 2.51.2
> 

Re: [PATCH v2 1/1] rust: add Work::disable_sync

[Onur Özkan]

On Mon, 04 May 2026 07:54:54 +0000
Alice Ryhl <aliceryhl@google.com> wrote:

> On Fri, May 01, 2026 at 10:11:22PM +0300, Onur Özkan wrote:
> > Adds Work::disable_sync() as a safe wrapper for disable_work_sync().
> > 
> > Drivers can use this during teardown to stop new queueing and wait for
> > queued or running work to finish before dropping related resources.
> > 
> > Signed-off-by: Onur Özkan <work@onurozkan.dev>
> > ---
> >  rust/kernel/workqueue.rs | 121 ++++++++++++++++++++++++++-------------
> >  1 file changed, 81 insertions(+), 40 deletions(-)
> > 
> > diff --git a/rust/kernel/workqueue.rs b/rust/kernel/workqueue.rs
> > index 7e253b6f299c..d0f9b4ba7f27 100644
> > --- a/rust/kernel/workqueue.rs
> > +++ b/rust/kernel/workqueue.rs
> > @@ -267,7 +267,7 @@ pub unsafe fn from_raw<'a>(ptr: *const bindings::workqueue_struct) -> &'a Queue
> >  
> >      /// Enqueues a work item.
> >      ///
> > -    /// This may fail if the work item is already enqueued in a workqueue.
> > +    /// This may fail if the work item is already enqueued in a workqueue or disabled.
> >      ///
> >      /// The work item will be submitted using `WORK_CPU_UNBOUND`.
> >      pub fn enqueue<W, const ID: u64>(&self, w: W) -> W::EnqueueOutput
> 
> Can you elaborate on the case where disable leads to failure here? Can
> you not enqueue a work item again after disabling it? Is there a doc
> test illustrating this case that I can run for myself to see the
> behavior in action?

As we discussed on yesterday's call, I looked into cancel_work_sync and
it seems we can make this work in the tyr reset implementation. We already
store an atomic reset state, it can be used to prevent future enqueues in
reset scheduling.

I will send a patch for the cancel_sync function soon.

Thanks,
Onur

> 
> Alice

Re: [PATCH v2 1/1] rust: add Work::disable_sync

[Alice Ryhl]

On Tue, May 05, 2026 at 09:07:19AM +0300, Onur Özkan wrote:
> On Mon, 04 May 2026 07:54:54 +0000
> Alice Ryhl <aliceryhl@google.com> wrote:
> 
> > On Fri, May 01, 2026 at 10:11:22PM +0300, Onur Özkan wrote:
> > > Adds Work::disable_sync() as a safe wrapper for disable_work_sync().
> > > 
> > > Drivers can use this during teardown to stop new queueing and wait for
> > > queued or running work to finish before dropping related resources.
> > > 
> > > Signed-off-by: Onur Özkan <work@onurozkan.dev>
> > > ---
> > >  rust/kernel/workqueue.rs | 121 ++++++++++++++++++++++++++-------------
> > >  1 file changed, 81 insertions(+), 40 deletions(-)
> > > 
> > > diff --git a/rust/kernel/workqueue.rs b/rust/kernel/workqueue.rs
> > > index 7e253b6f299c..d0f9b4ba7f27 100644
> > > --- a/rust/kernel/workqueue.rs
> > > +++ b/rust/kernel/workqueue.rs
> > > @@ -267,7 +267,7 @@ pub unsafe fn from_raw<'a>(ptr: *const bindings::workqueue_struct) -> &'a Queue
> > >  
> > >      /// Enqueues a work item.
> > >      ///
> > > -    /// This may fail if the work item is already enqueued in a workqueue.
> > > +    /// This may fail if the work item is already enqueued in a workqueue or disabled.
> > >      ///
> > >      /// The work item will be submitted using `WORK_CPU_UNBOUND`.
> > >      pub fn enqueue<W, const ID: u64>(&self, w: W) -> W::EnqueueOutput
> > 
> > Can you elaborate on the case where disable leads to failure here? Can
> > you not enqueue a work item again after disabling it? Is there a doc
> > test illustrating this case that I can run for myself to see the
> > behavior in action?
> 
> As we discussed on yesterday's call, I looked into cancel_work_sync and
> it seems we can make this work in the tyr reset implementation. We already
> store an atomic reset state, it can be used to prevent future enqueues in
> reset scheduling.
> 
> I will send a patch for the cancel_sync function soon.

I did hear from Boris that Panthor actually does make use of the disable
feature. 

What I would probably suggest here is to choose whether you can disable
a work item based on the pointer type. If using a pointer type where
enqueue is *already* fallible, we might as well also support disabling
it.

pub trait SupportsDisable<const ID: u64>: WorkItemPointer { }

unsafe impl<T, const ID: u64> SupportsDisable<ID> for ARef<T>
where
    Self: WorkItemPointer<ID>,
    T: AlwaysRefCounted,
{
}

So you can implement the trait as above for ARef and Arc, but not for
Box.

Then, when you add the method for actually performing a disable, you add
a requirement that the pointer type implements this trait:

/// Disables this work item and waits for queued/running executions to finish.
///
/// # Note
///
/// Should be called from a sleepable context if the work was last queued on a non-BH
/// workqueue.
#[inline]
pub fn disable_sync(&self)
where
    T: WorkItem<ID>,
    <T as WorkItem<ID>>::Pointer: SupportsDisable<ID>,
{ ... }

This way the pointer type in use determines whether to opt-in to
exposing the disable bit.

I do not think there is any harm to not exposing `disable_sync()` for
box, because you can't even call it on an enqueued work item to begin
with. If it's enqueued, the workqueue has ownership over the box, so you
have no way of obtaining a reference to it, which you would need to call
disable_sync(). So we would only be taking away the ability to call that
method in cases where you *know* the work item is not enqueued anyway.

Alice

Re: [PATCH v2 1/1] rust: add Work::disable_sync

[Onur Özkan]

On Tue, 05 May 2026 08:47:45 +0000
Alice Ryhl <aliceryhl@google.com> wrote:

> On Tue, May 05, 2026 at 09:07:19AM +0300, Onur Özkan wrote:
> > On Mon, 04 May 2026 07:54:54 +0000
> > Alice Ryhl <aliceryhl@google.com> wrote:
> > 
> > > On Fri, May 01, 2026 at 10:11:22PM +0300, Onur Özkan wrote:
> > > > Adds Work::disable_sync() as a safe wrapper for disable_work_sync().
> > > > 
> > > > Drivers can use this during teardown to stop new queueing and wait for
> > > > queued or running work to finish before dropping related resources.
> > > > 
> > > > Signed-off-by: Onur Özkan <work@onurozkan.dev>
> > > > ---
> > > >  rust/kernel/workqueue.rs | 121 ++++++++++++++++++++++++++-------------
> > > >  1 file changed, 81 insertions(+), 40 deletions(-)
> > > > 
> > > > diff --git a/rust/kernel/workqueue.rs b/rust/kernel/workqueue.rs
> > > > index 7e253b6f299c..d0f9b4ba7f27 100644
> > > > --- a/rust/kernel/workqueue.rs
> > > > +++ b/rust/kernel/workqueue.rs
> > > > @@ -267,7 +267,7 @@ pub unsafe fn from_raw<'a>(ptr: *const bindings::workqueue_struct) -> &'a Queue
> > > >  
> > > >      /// Enqueues a work item.
> > > >      ///
> > > > -    /// This may fail if the work item is already enqueued in a workqueue.
> > > > +    /// This may fail if the work item is already enqueued in a workqueue or disabled.
> > > >      ///
> > > >      /// The work item will be submitted using `WORK_CPU_UNBOUND`.
> > > >      pub fn enqueue<W, const ID: u64>(&self, w: W) -> W::EnqueueOutput
> > > 
> > > Can you elaborate on the case where disable leads to failure here? Can
> > > you not enqueue a work item again after disabling it? Is there a doc
> > > test illustrating this case that I can run for myself to see the
> > > behavior in action?
> > 
> > As we discussed on yesterday's call, I looked into cancel_work_sync and
> > it seems we can make this work in the tyr reset implementation. We already
> > store an atomic reset state, it can be used to prevent future enqueues in
> > reset scheduling.
> > 
> > I will send a patch for the cancel_sync function soon.
> 
> I did hear from Boris that Panthor actually does make use of the disable
> feature. 

I don't have any idea what Boris said, perhaps he should write it here as well.
Maybe Boris said something that isn't covered on the tyr reset yet in my series,
I don't know.

What I do know is that I can achieve essentially the same behavior using
cancel_work_sync instead of disable_work_sync on tyr. Like i said there's an
atomic reset state we can use to prevent future work from being enqueued. The
only real difference is that supporting disable_work_sync in the Rust workqueue
would introduce more complexity than supporting cancel_work_sync. There is also
the corresponding enable part we have to support if we want to go with
disable_work_sync path.

	Onur

> 
> What I would probably suggest here is to choose whether you can disable
> a work item based on the pointer type. If using a pointer type where
> enqueue is *already* fallible, we might as well also support disabling
> it.
> 
> pub trait SupportsDisable<const ID: u64>: WorkItemPointer { }
> 
> unsafe impl<T, const ID: u64> SupportsDisable<ID> for ARef<T>
> where
>     Self: WorkItemPointer<ID>,
>     T: AlwaysRefCounted,
> {
> }
> 
> So you can implement the trait as above for ARef and Arc, but not for
> Box.
> 
> Then, when you add the method for actually performing a disable, you add
> a requirement that the pointer type implements this trait:
> 
> /// Disables this work item and waits for queued/running executions to finish.
> ///
> /// # Note
> ///
> /// Should be called from a sleepable context if the work was last queued on a non-BH
> /// workqueue.
> #[inline]
> pub fn disable_sync(&self)
> where
>     T: WorkItem<ID>,
>     <T as WorkItem<ID>>::Pointer: SupportsDisable<ID>,
> { ... }
> 
> This way the pointer type in use determines whether to opt-in to
> exposing the disable bit.
> 
> I do not think there is any harm to not exposing `disable_sync()` for
> box, because you can't even call it on an enqueued work item to begin
> with. If it's enqueued, the workqueue has ownership over the box, so you
> have no way of obtaining a reference to it, which you would need to call
> disable_sync(). So we would only be taking away the ability to call that
> method in cases where you *know* the work item is not enqueued anyway.
> 
> Alice

Re: [PATCH v2 1/1] rust: add Work::disable_sync

[Boris Brezillon]

On Tue,  5 May 2026 12:16:12 +0300
Onur Özkan <work@onurozkan.dev> wrote:

> On Tue, 05 May 2026 08:47:45 +0000
> Alice Ryhl <aliceryhl@google.com> wrote:
> 
> > On Tue, May 05, 2026 at 09:07:19AM +0300, Onur Özkan wrote:  
> > > On Mon, 04 May 2026 07:54:54 +0000
> > > Alice Ryhl <aliceryhl@google.com> wrote:
> > >   
> > > > On Fri, May 01, 2026 at 10:11:22PM +0300, Onur Özkan wrote:  
> > > > > Adds Work::disable_sync() as a safe wrapper for disable_work_sync().
> > > > > 
> > > > > Drivers can use this during teardown to stop new queueing and wait for
> > > > > queued or running work to finish before dropping related resources.
> > > > > 
> > > > > Signed-off-by: Onur Özkan <work@onurozkan.dev>
> > > > > ---
> > > > >  rust/kernel/workqueue.rs | 121 ++++++++++++++++++++++++++-------------
> > > > >  1 file changed, 81 insertions(+), 40 deletions(-)
> > > > > 
> > > > > diff --git a/rust/kernel/workqueue.rs b/rust/kernel/workqueue.rs
> > > > > index 7e253b6f299c..d0f9b4ba7f27 100644
> > > > > --- a/rust/kernel/workqueue.rs
> > > > > +++ b/rust/kernel/workqueue.rs
> > > > > @@ -267,7 +267,7 @@ pub unsafe fn from_raw<'a>(ptr: *const bindings::workqueue_struct) -> &'a Queue
> > > > >  
> > > > >      /// Enqueues a work item.
> > > > >      ///
> > > > > -    /// This may fail if the work item is already enqueued in a workqueue.
> > > > > +    /// This may fail if the work item is already enqueued in a workqueue or disabled.
> > > > >      ///
> > > > >      /// The work item will be submitted using `WORK_CPU_UNBOUND`.
> > > > >      pub fn enqueue<W, const ID: u64>(&self, w: W) -> W::EnqueueOutput  
> > > > 
> > > > Can you elaborate on the case where disable leads to failure here? Can
> > > > you not enqueue a work item again after disabling it? Is there a doc
> > > > test illustrating this case that I can run for myself to see the
> > > > behavior in action?  
> > > 
> > > As we discussed on yesterday's call, I looked into cancel_work_sync and
> > > it seems we can make this work in the tyr reset implementation. We already
> > > store an atomic reset state, it can be used to prevent future enqueues in
> > > reset scheduling.
> > > 
> > > I will send a patch for the cancel_sync function soon.  
> > 
> > I did hear from Boris that Panthor actually does make use of the disable
> > feature.   
> 
> I don't have any idea what Boris said, perhaps he should write it here as well.
> Maybe Boris said something that isn't covered on the tyr reset yet in my series,
> I don't know.

So, I checked where those disable_[delayed_]work[_sync]() are, and they
seem to cover mostly the unplug path. There's a couple non-unplug
related use, which both cover the per-queue watchdog[3][4].

As for why we ended up using disable_work instead of cancel_work, it's
all explained in [1] and [2]. Yes, it could have been done differently,
but disable_work was the most convenient way of solving these UAFs in C.

> 
> What I do know is that I can achieve essentially the same behavior using
> cancel_work_sync instead of disable_work_sync on tyr. Like i said there's an
> atomic reset state we can use to prevent future work from being enqueued.

If the atomic is already there to prevent queuing more works, or
checking if a reset is pending, sure. It might just be more custom
checks to add, and if we think we'll need to support work items that can
be disabled further down the line anyway, maybe it makes sense to work
on it now, dunno.

Actually, looking back at panthor to write this reply, I'm now
considering replacing the custom checks we have in sched_queue_work()
by disable_[delayed_]work() calls in the
panthor_device_schedule_reset() path.

Of course, none of this has to drive how it's done in rust/Tyr, and if
you think it's better handled with a separate atomic and custom checks,
feel free to go for this alternative.

> The
> only real difference is that supporting disable_work_sync in the Rust workqueue
> would introduce more complexity than supporting cancel_work_sync. There is also
> the corresponding enable part we have to support if we want to go with
> disable_work_sync path.

Yep, if you use it in the reset path, you'll have to support
enable_work as well. For unplug, it's not needed, because the device is
gone after that.

[1]https://lore.kernel.org/all/20251027140217.121274-1-ketil.johnsen@arm.com/
[2]https://lore.kernel.org/all/20251029111412.924104-1-ketil.johnsen@arm.com/
[3]https://elixir.bootlin.com/linux/v7.0.1/source/drivers/gpu/drm/panthor/panthor_sched.c#L2728
[4]https://elixir.bootlin.com/linux/v7.0.1/source/drivers/gpu/drm/panthor/panthor_sched.c#L919

Re: [PATCH v2 1/1] rust: add Work::disable_sync

[Onur Özkan]

On Tue, 05 May 2026 11:50:14 +0200
Boris Brezillon <boris.brezillon@collabora.com> wrote:

> On Tue,  5 May 2026 12:16:12 +0300
> Onur Özkan <work@onurozkan.dev> wrote:
> 
> > On Tue, 05 May 2026 08:47:45 +0000
> > Alice Ryhl <aliceryhl@google.com> wrote:
> > 
> > > On Tue, May 05, 2026 at 09:07:19AM +0300, Onur Özkan wrote:  
> > > > On Mon, 04 May 2026 07:54:54 +0000
> > > > Alice Ryhl <aliceryhl@google.com> wrote:
> > > >   
> > > > > On Fri, May 01, 2026 at 10:11:22PM +0300, Onur Özkan wrote:  
> > > > > > Adds Work::disable_sync() as a safe wrapper for disable_work_sync().
> > > > > > 
> > > > > > Drivers can use this during teardown to stop new queueing and wait for
> > > > > > queued or running work to finish before dropping related resources.
> > > > > > 
> > > > > > Signed-off-by: Onur Özkan <work@onurozkan.dev>
> > > > > > ---
> > > > > >  rust/kernel/workqueue.rs | 121 ++++++++++++++++++++++++++-------------
> > > > > >  1 file changed, 81 insertions(+), 40 deletions(-)
> > > > > > 
> > > > > > diff --git a/rust/kernel/workqueue.rs b/rust/kernel/workqueue.rs
> > > > > > index 7e253b6f299c..d0f9b4ba7f27 100644
> > > > > > --- a/rust/kernel/workqueue.rs
> > > > > > +++ b/rust/kernel/workqueue.rs
> > > > > > @@ -267,7 +267,7 @@ pub unsafe fn from_raw<'a>(ptr: *const bindings::workqueue_struct) -> &'a Queue
> > > > > >  
> > > > > >      /// Enqueues a work item.
> > > > > >      ///
> > > > > > -    /// This may fail if the work item is already enqueued in a workqueue.
> > > > > > +    /// This may fail if the work item is already enqueued in a workqueue or disabled.
> > > > > >      ///
> > > > > >      /// The work item will be submitted using `WORK_CPU_UNBOUND`.
> > > > > >      pub fn enqueue<W, const ID: u64>(&self, w: W) -> W::EnqueueOutput  
> > > > > 
> > > > > Can you elaborate on the case where disable leads to failure here? Can
> > > > > you not enqueue a work item again after disabling it? Is there a doc
> > > > > test illustrating this case that I can run for myself to see the
> > > > > behavior in action?  
> > > > 
> > > > As we discussed on yesterday's call, I looked into cancel_work_sync and
> > > > it seems we can make this work in the tyr reset implementation. We already
> > > > store an atomic reset state, it can be used to prevent future enqueues in
> > > > reset scheduling.
> > > > 
> > > > I will send a patch for the cancel_sync function soon.  
> > > 
> > > I did hear from Boris that Panthor actually does make use of the disable
> > > feature.   
> > 
> > I don't have any idea what Boris said, perhaps he should write it here as well.
> > Maybe Boris said something that isn't covered on the tyr reset yet in my series,
> > I don't know.
> 
> So, I checked where those disable_[delayed_]work[_sync]() are, and they
> seem to cover mostly the unplug path. There's a couple non-unplug
> related use, which both cover the per-queue watchdog[3][4].
> 
> As for why we ended up using disable_work instead of cancel_work, it's
> all explained in [1] and [2]. Yes, it could have been done differently,
> but disable_work was the most convenient way of solving these UAFs in C.
> 
> > 
> > What I do know is that I can achieve essentially the same behavior using
> > cancel_work_sync instead of disable_work_sync on tyr. Like i said there's an
> > atomic reset state we can use to prevent future work from being enqueued.
> 
> If the atomic is already there to prevent queuing more works, or
> checking if a reset is pending, sure. It might just be more custom
> checks to add, and if we think we'll need to support work items that can
> be disabled further down the line anyway, maybe it makes sense to work
> on it now, dunno.

With the disable_work_sync path, there are the 2 things I don't really like:

	1. Having multiple sources of control for the reset path: one being the
	   atomic state and the other the workqueue. It feels odd to have both at
	   the same time.
	2. The added complexity on workqueue.rs

Regarding 1, IMO only one mechanism should own this logic. If we have to choose,
we can't drop the atomic state since it's also useful in other parts. It's also
more explicit and easier to understand (devs wouldn't need to know the workqueue
internals to understand the scheduling behavior).

Regarding 2, Alice's suggestion at [1] seems quite complicated (even without the
enable part) compare to how simple cancel_work_sync would be done.

I think I will go with the cancel_work_sync + atomic state approach. At least until
we run into a case where it doesn't work (which I cannot guess at the moment) and
disable_work_sync becomes necessary.

[1]: https://lore.kernel.org/rust-for-linux/afmusSlYdOR2Alvp@google.com/

	Onur

> 
> Actually, looking back at panthor to write this reply, I'm now
> considering replacing the custom checks we have in sched_queue_work()
> by disable_[delayed_]work() calls in the
> panthor_device_schedule_reset() path.
> 
> Of course, none of this has to drive how it's done in rust/Tyr, and if
> you think it's better handled with a separate atomic and custom checks,
> feel free to go for this alternative.
> 
> > The
> > only real difference is that supporting disable_work_sync in the Rust workqueue
> > would introduce more complexity than supporting cancel_work_sync. There is also
> > the corresponding enable part we have to support if we want to go with
> > disable_work_sync path.
> 
> Yep, if you use it in the reset path, you'll have to support
> enable_work as well. For unplug, it's not needed, because the device is
> gone after that.
> 
> [1]https://lore.kernel.org/all/20251027140217.121274-1-ketil.johnsen@arm.com/
> [2]https://lore.kernel.org/all/20251029111412.924104-1-ketil.johnsen@arm.com/
> [3]https://elixir.bootlin.com/linux/v7.0.1/source/drivers/gpu/drm/panthor/panthor_sched.c#L2728
> [4]https://elixir.bootlin.com/linux/v7.0.1/source/drivers/gpu/drm/panthor/panthor_sched.c#L919

Re: [PATCH v2 1/1] rust: add Work::disable_sync

[Boris Brezillon]

On Tue,  5 May 2026 13:10:32 +0300
Onur Özkan <work@onurozkan.dev> wrote:

> On Tue, 05 May 2026 11:50:14 +0200
> Boris Brezillon <boris.brezillon@collabora.com> wrote:
> 
> > On Tue,  5 May 2026 12:16:12 +0300
> > Onur Özkan <work@onurozkan.dev> wrote:
> >   
> > > On Tue, 05 May 2026 08:47:45 +0000
> > > Alice Ryhl <aliceryhl@google.com> wrote:
> > >   
> > > > On Tue, May 05, 2026 at 09:07:19AM +0300, Onur Özkan wrote:    
> > > > > On Mon, 04 May 2026 07:54:54 +0000
> > > > > Alice Ryhl <aliceryhl@google.com> wrote:
> > > > >     
> > > > > > On Fri, May 01, 2026 at 10:11:22PM +0300, Onur Özkan wrote:    
> > > > > > > Adds Work::disable_sync() as a safe wrapper for disable_work_sync().
> > > > > > > 
> > > > > > > Drivers can use this during teardown to stop new queueing and wait for
> > > > > > > queued or running work to finish before dropping related resources.
> > > > > > > 
> > > > > > > Signed-off-by: Onur Özkan <work@onurozkan.dev>
> > > > > > > ---
> > > > > > >  rust/kernel/workqueue.rs | 121 ++++++++++++++++++++++++++-------------
> > > > > > >  1 file changed, 81 insertions(+), 40 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/rust/kernel/workqueue.rs b/rust/kernel/workqueue.rs
> > > > > > > index 7e253b6f299c..d0f9b4ba7f27 100644
> > > > > > > --- a/rust/kernel/workqueue.rs
> > > > > > > +++ b/rust/kernel/workqueue.rs
> > > > > > > @@ -267,7 +267,7 @@ pub unsafe fn from_raw<'a>(ptr: *const bindings::workqueue_struct) -> &'a Queue
> > > > > > >  
> > > > > > >      /// Enqueues a work item.
> > > > > > >      ///
> > > > > > > -    /// This may fail if the work item is already enqueued in a workqueue.
> > > > > > > +    /// This may fail if the work item is already enqueued in a workqueue or disabled.
> > > > > > >      ///
> > > > > > >      /// The work item will be submitted using `WORK_CPU_UNBOUND`.
> > > > > > >      pub fn enqueue<W, const ID: u64>(&self, w: W) -> W::EnqueueOutput    
> > > > > > 
> > > > > > Can you elaborate on the case where disable leads to failure here? Can
> > > > > > you not enqueue a work item again after disabling it? Is there a doc
> > > > > > test illustrating this case that I can run for myself to see the
> > > > > > behavior in action?    
> > > > > 
> > > > > As we discussed on yesterday's call, I looked into cancel_work_sync and
> > > > > it seems we can make this work in the tyr reset implementation. We already
> > > > > store an atomic reset state, it can be used to prevent future enqueues in
> > > > > reset scheduling.
> > > > > 
> > > > > I will send a patch for the cancel_sync function soon.    
> > > > 
> > > > I did hear from Boris that Panthor actually does make use of the disable
> > > > feature.     
> > > 
> > > I don't have any idea what Boris said, perhaps he should write it here as well.
> > > Maybe Boris said something that isn't covered on the tyr reset yet in my series,
> > > I don't know.  
> > 
> > So, I checked where those disable_[delayed_]work[_sync]() are, and they
> > seem to cover mostly the unplug path. There's a couple non-unplug
> > related use, which both cover the per-queue watchdog[3][4].
> > 
> > As for why we ended up using disable_work instead of cancel_work, it's
> > all explained in [1] and [2]. Yes, it could have been done differently,
> > but disable_work was the most convenient way of solving these UAFs in C.
> >   
> > > 
> > > What I do know is that I can achieve essentially the same behavior using
> > > cancel_work_sync instead of disable_work_sync on tyr. Like i said there's an
> > > atomic reset state we can use to prevent future work from being enqueued.  
> > 
> > If the atomic is already there to prevent queuing more works, or
> > checking if a reset is pending, sure. It might just be more custom
> > checks to add, and if we think we'll need to support work items that can
> > be disabled further down the line anyway, maybe it makes sense to work
> > on it now, dunno.  
> 
> With the disable_work_sync path, there are the 2 things I don't really like:
> 
> 	1. Having multiple sources of control for the reset path: one being the
> 	   atomic state and the other the workqueue. It feels odd to have both at
> 	   the same time.

The source of truth would still be the reset_pending atomic. The
question is more, how do you ensure the work items that are covered by
this reset_pending state can't be scheduled until this reset_pending is
restored to false? In panthor we do that with the
sched_queue_[delayed_]work() helpers, which works okay as long as you
ensure all sched-related works go through this helper. Using
disable_work() would provide a more robust solution. In rust it's
likely that you have other ways to enforce that.

> 	2. The added complexity on workqueue.rs
> 
> Regarding 1, IMO only one mechanism should own this logic. If we have to choose,
> we can't drop the atomic state since it's also useful in other parts. It's also
> more explicit and easier to understand (devs wouldn't need to know the workqueue
> internals to understand the scheduling behavior).

I agree, but it still relies on the fact you have a
thread-safe/non-blocking way to enforce that no one will ever be able to
reschedule a work item between the moment you call cancel_work_sync()
on the various work items you want to stop before doing the reset, and
the moment you're done with the reset. {disable,enable}_work() provides
a generic+race-free solution for that, which is why we used it in the
unplug path in panthor.

> 
> Regarding 2, Alice's suggestion at [1] seems quite complicated (even without the
> enable part) compare to how simple cancel_work_sync would be done.

Doesn't seem utterly complicated to me, but I'm probably not the best
person to comment on rust code. BTW, how is cancel_work_sync()
different in term of what it means for Box containers? You still can't
cancel a work item if it's owned by the workqueue, can you? So all this
really changes is the fact you have to re-enable a disabled work item
before enqueuing it again if you want it to do anything.

BTW, in panthor, we also heavily rely on queue_work() not doing anything
and returning false if the work item is already scheduled, so accepting
::enqueue() failures in Tyr is going to be standard behavior, I think.

> 
> I think I will go with the cancel_work_sync + atomic state approach. At least until
> we run into a case where it doesn't work (which I cannot guess at the moment) and
> disable_work_sync becomes necessary.

Sounds good.