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

[Laura Nao]

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

Best,

Laura