Re: [PATCH v1] rust: workqueue: add cancel_sync support

[Alice Ryhl <aliceryhl@google.com>]

On Sun, May 10, 2026 at 06:53:10PM +0300, Onur Özkan wrote:
> On Sun, 10 May 2026 13:43:59 +0000
> Alice Ryhl <aliceryhl@google.com> wrote:
> 
> > On Sun, May 10, 2026 at 11:21:57AM +0300, Onur Özkan wrote:
> > > Drivers can use this during teardown to cancel pending work and wait for
> > > running work to finish before dropping related resources.
> > > 
> > > This is not implemented for Pin<KBox<T>> because queuing a boxed work
> > > item transfers ownership of the box to the workqueue. There is therefore
> > > no separate safe owner that can cancel the boxed work while it is pending.
> > > 
> > > The immediate motivation is the Tyr reset infrastructure [1], which needs
> > > to cancel pending reset work and wait for any running reset work during
> > > teardown before dropping the resources used by that work.
> > > 
> > > [1]: https://lore.kernel.org/all/20260416171728.205141-1-work@onurozkan.dev
> > > 
> > > Signed-off-by: Onur Özkan <work@onurozkan.dev>
> > > ---
> > >  rust/kernel/workqueue.rs | 134 ++++++++++++++++++++++++++++++++-------
> > >  1 file changed, 112 insertions(+), 22 deletions(-)
> > > 
> > > diff --git a/rust/kernel/workqueue.rs b/rust/kernel/workqueue.rs
> > > index 7e253b6f299c..a10daa2763ac 100644
> > > --- a/rust/kernel/workqueue.rs
> > > +++ b/rust/kernel/workqueue.rs
> > > @@ -442,23 +442,44 @@ 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.
> > > +/// Implementers must ensure that [`__enqueue`] uses a `work_struct` initialized with [`run`] as
> > > +/// its function pointer, and that [`from_raw_work`] rebuilds the exact ownership transferred by
> > > +/// a successful [`__enqueue`] call.
> > >  ///
> > >  /// [`__enqueue`]: RawWorkItem::__enqueue
> > > +/// [`from_raw_work`]: WorkItemPointer::from_raw_work
> > >  /// [`run`]: WorkItemPointer::run
> > > -pub unsafe trait WorkItemPointer<const ID: u64>: RawWorkItem<ID> {
> > > -    /// Run this work item.
> > > +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.
> > >      ///
> > > -    /// [`__enqueue`]: RawWorkItem::__enqueue
> > > -    unsafe extern "C" fn run(ptr: *mut bindings::work_struct);
> > > +    /// # Safety
> > > +    ///
> > > +    /// 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) },
> > > +        );
> > > +    }
> > >  }
> > >  
> > > +/// Marker for work item types that support cancellation.
> > > +pub trait SupportsCancelling<const ID: u64>: WorkItemPointer<ID> {}
> > 
> > Shouldn't 'from_raw_work()' be a method on SupportsCancelling instead?
> 
> Not necessarily. I wanted to keep it like that because it removes a bit of
> duplication and makes run() simpler. It's also more extensible e.g. can be
> used for disable_sync too if needed in the future.

It still seems wrong to me because you are forcing all pointer types to
provide from_raw_work() even though not all pointer types needs it.

As for disable_sync(), I think for that case we'd want three traits:

1. WorkItemPointer, defines run()
2. CancelableWorkItemPtr: WorkItemPointer, defines from_raw_work()
3. DisablableWorkItemPtr: CancelableWorkItemPtr, no methods

Alice