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

[Alice Ryhl <aliceryhl@google.com>]

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