Re: [PATCH v2 1/1] rust: workqueue: add cancel_sync support

["Onur Özkan" <work@onurozkan.dev>]

On Mon, 15 Jun 2026 07:08:37 +0000
Alice Ryhl <aliceryhl@google.com> wrote:

> On Fri, Jun 12, 2026 at 10:45:42PM +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.
> > 
> > Signed-off-by: Onur Özkan <work@onurozkan.dev>
> 
> Overall looks reasonable to me, but some comments below.
> 
> >  rust/kernel/workqueue.rs | 116 +++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 116 insertions(+)
> > 
> > diff --git a/rust/kernel/workqueue.rs b/rust/kernel/workqueue.rs
> > index 7e253b6f299c..4d61d7a10fae 100644
> > --- a/rust/kernel/workqueue.rs
> > +++ b/rust/kernel/workqueue.rs
> > @@ -471,6 +471,23 @@ pub trait WorkItem<const ID: u64 = 0> {
> >      fn run(this: Self::Pointer);
> >  }
> >  
> > +/// Work item pointers that support cancellation.
> > +///
> > +/// # Safety
> > +///
> > +/// Implementers must ensure that `from_raw_work` rebuilds the exact ownership transferred
> > +/// by a successful [`RawWorkItem::__enqueue`] call.
> > +pub unsafe trait SupportsCancelling<const ID: u64>: WorkItemPointer<ID> + Sized {
> 
> Nit; I think it reads nicer as SupportsCancel.
> 
> > +    /// Rebuild this work item's pointer from its embedded `work_struct`.
> > +    ///
> > +    /// # Safety
> > +    ///
> > +    /// 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;
> 
> I think you're missing some condition here about ownership having been
> transferred *out* from the workqueue by cancel_sync() or similar
> methods. Otherwise this currently says I can call the method even though
> the workqueue still owns the work item.
> 

Very true, good point! I will improve this doc in the next version with the nits
applied.

Onur

> > +    /// # Note
> > +    ///
> > +    /// Should be called from a sleepable context if the work was last queued on a non-BH
> > +    /// workqueue.
> 
> Nit: I'd either reword "Note" to something more specific to what the
> note is about, or remove the heading.
> 
> > +    #[inline]
> > +    pub fn cancel_sync(&self) -> Option<T::Pointer>
> > +    where
> > +        T: WorkItem<ID>,
> > +        T::Pointer: SupportsCancelling<ID>,
> > +    {
> > +        let ptr = self.dwork.get();
> > +
> > +        // SAFETY: `ptr` is a valid embedded `delayed_work`.
> > +        if unsafe { bindings::cancel_delayed_work_sync(ptr) } {
> > +            // SAFETY: A `true` return means the work was pending and got canceled, so the queued
> > +            // ownership transfer performed by `__enqueue` is reclaimed here.
> > +            Some(unsafe { T::Pointer::from_raw_work(core::ptr::addr_of_mut!((*ptr).work)) })
> 
> Nit: The addr_of_mut! macro is no longer required. You can do: &raw mut (*ptr).work
> 
> Alice