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
