Re: [PATCH v6 2/2] drm/tyr: add Job IRQ handling

[Alice Ryhl <aliceryhl@google.com>]

On Mon, Oct 05, 2026 at 03:40:10PM +0200, Laura Nao wrote:
> Hi Alice,
> 
> On 9/23/26 10:37, Alice Ryhl wrote:
> > I agree with sashiko's review here.
> >
> > This needs to happen after the free_irq() call in the destructor of
> > TyrIrq.
> 
> Thanks for the feedback.
> 
> In v5, I was masking the interrupts in TyrIrq's PinnedDrop impl which
> resulted in clear_mask() being called after free_irq(). However, sashiko
> warned the device could then assert irqs after free_irq() completes
> but before clear_mask() runs, potentially leading to an IRQ storm that
> could result in the shared line being permanently disabled. 
> 
> So calling clear_mask() before free_irq() should avoid this, but we
> still have to deal with threaded handlers potentially re-enabling the
> mask after clear_mask() has run (as per sashiko's review on this current
> revision). Adding the atomic state should help with that though, as you
> suggested:
> 
> > And most likely, we need an atomic along the lines of ACTIVE /
> > PROCESSING / SUSPENDING in panthor_irq.
> >
> 
> So I'm thinking something like this for the state:
> 
> #[derive(Clone, Copy, PartialEq, Eq)]
> #[repr(i32)]
> enum IrqState {
>     Active = 0,
>     Processing,
>     Unregistering,
> }
> 
> unsafe impl AtomicType for IrqState {
>     type Repr = i32;
> }
> 
> This could then be stored in TyrIrq:
> 
> #[pin_data]
> pub(crate) struct TyrIrq<T: TyrIrqTrait> {
>     irq: T,
>     state: Arc<Atomic<IrqState>>,
>     #[pin]
>     _pin: PhantomPinned,
> }
> 
> Then TyrIrq::handle() proceeds only when the state is active and
> TyrIrq::handle_threaded() re-enables the mask only if the state is not
> `Unregistering`.
> 
> Does this make sense to you?
> 
> As for masking before free_irq() runs, I'm thinking of possible
> alternatives to JobIrqMaskGuard and its ordering convention (i.e. must
> be stored in a field declared before the corresponding
> `ThreadedRegistration` in the struct that owns both). Would it make
> sense to define a TyrIrqRegistration struct that wraps
> ThreadedRegistration instead? and then clear the mask in
> TyrIrqRegistration's drop impl.
> 
> Something like:
> 
> #[pin_data(PinnedDrop)]
> pub(crate) struct TyrIrqRegistration<'a, T: TyrIrqTrait> {
>     #[pin]
>     registration: ThreadedRegistration<'a, TyrIrq<T>>,
> }
> 
> #[pinned_drop]
> impl<T: TyrIrqTrait> PinnedDrop for TyrIrqRegistration<'_, T> {
>     fn drop(self: Pin<&mut Self>) {
>         let handler = self.registration.handler();
>         handler.state.store(IrqState::Unregistering, Release);
>         handler.irq.clear_mask();
>     }
> }
> 
> This should make sure teardown order is still respected without relying
> on the user correctly putting the guard before `ThreadedRegistration`.
> 
> Any thoughts on this approach? In case it helps as a reference, I've
> drafted both IrqState and TyrIrqRegistration in [1].
> 
> [1] https://gitlab.freedesktop.org/laura.nao/linux/-/commit/c78e296690e643b790fb3195660f1229e3cdb5ca

I believe there is still a race. In this code:

        if self
            .state
            .cmpxchg(IrqState::Processing, IrqState::Active, Full)
            .is_ok()
        {
	    // <--- thread is preempted here
            self.irq.reenable_mask();
        }

If the above code performs the cmpxchg, but then gets preempted before
it runs the reenable_mask() call, then another thread can come along and
execute:

        handler.state.store(IrqState::Unregistering, Release);
        handler.irq.clear_mask();

clearing the mask. Once the original code starts running again, it calls
reenable_mask() even though the state is now Unregistering.

This incorrectly leaves the irq enabled even though the
TyrIrqRegistration has been dropped.

Adding a lock around these critical regions would solve this, because
it would make preemption between `state = Active` and `reenable_mask()`
impossible.

Alice