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

[Alice Ryhl]

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@xxxxxxxxxxxxx>

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.

> +    /// # 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