On Friday, 9 January 2026 16:59:46 Central European Standard Time Steven Price wrote: > On 08/01/2026 14:19, Nicolas Frattaroli wrote: > > The current IRQ helpers do not guarantee mutual exclusion that covers > > the entire transaction from accessing the mask member and modifying the > > mask register. > > > > This makes it hard, if not impossible, to implement mask modification > > helpers that may change one of these outside the normal > > suspend/resume/isr code paths. > > > > Add a spinlock to struct panthor_irq that protects both the mask member > > and register. Acquire it in all code paths that access these, but drop > > it before processing the threaded handler function. Then, add the > > aforementioned new helpers: enable_events, and disable_events. They work > > by ORing and NANDing the mask bits. > > > > resume is changed to no longer have a mask passed, as pirq->mask is > > supposed to be the user-requested mask now, rather than a mirror of the > > INT_MASK register contents. Users of the resume helper are adjusted > > accordingly, including a rather painful refactor in panthor_mmu.c. > > > > panthor_irq::suspended remains an atomic, as it's necessarily written to > > outside the mask_lock in the suspend path. > > > > Signed-off-by: Nicolas Frattaroli <[email protected]> > > --- > > drivers/gpu/drm/panthor/panthor_device.h | 60 ++++++-- > > drivers/gpu/drm/panthor/panthor_fw.c | 3 +- > > drivers/gpu/drm/panthor/panthor_gpu.c | 2 +- > > drivers/gpu/drm/panthor/panthor_mmu.c | 247 > > ++++++++++++++++--------------- > > drivers/gpu/drm/panthor/panthor_pwr.c | 2 +- > > 5 files changed, 179 insertions(+), 135 deletions(-) > > > > diff --git a/drivers/gpu/drm/panthor/panthor_device.h > > b/drivers/gpu/drm/panthor/panthor_device.h > > index f35e52b9546a..cf76a8abca76 100644 > > --- a/drivers/gpu/drm/panthor/panthor_device.h > > +++ b/drivers/gpu/drm/panthor/panthor_device.h > > @@ -73,11 +73,14 @@ struct panthor_irq { > > /** @irq: IRQ number. */ > > int irq; > > > > - /** @mask: Current mask being applied to xxx_INT_MASK. */ > > + /** @mask: Values to write to xxx_INT_MASK if active. */ > > u32 mask; > > > > /** @suspended: Set to true when the IRQ is suspended. */ > > atomic_t suspended; > > + > > + /** @mask_lock: protects modifications to _INT_MASK and @mask */ > > + spinlock_t mask_lock; > > }; > > > > /** > > @@ -410,6 +413,8 @@ static irqreturn_t panthor_ ## __name ## > > _irq_raw_handler(int irq, void *data) > > struct panthor_irq *pirq = data; > > \ > > struct panthor_device *ptdev = pirq->ptdev; > > \ > > > > \ > > + guard(spinlock_irqsave)(&pirq->mask_lock); > > \ > > + > > \ > > if (atomic_read(&pirq->suspended)) > > \ > > return IRQ_NONE; > > \ > > if (!gpu_read(ptdev, __reg_prefix ## _INT_STAT)) > > \ > > @@ -424,9 +429,14 @@ static irqreturn_t panthor_ ## __name ## > > _irq_threaded_handler(int irq, void *da > > struct panthor_irq *pirq = data; > > \ > > struct panthor_device *ptdev = pirq->ptdev; > > \ > > irqreturn_t ret = IRQ_NONE; > > \ > > + u32 mask; > > \ > > + > > \ > > + scoped_guard(spinlock_irqsave, &pirq->mask_lock) { > > \ > > + mask = pirq->mask; > > \ > > + } > > \ > > > > \ > > while (true) { > > \ > > - u32 status = gpu_read(ptdev, __reg_prefix ## _INT_RAWSTAT) & > > pirq->mask; \ > > + u32 status = (gpu_read(ptdev, __reg_prefix ## _INT_RAWSTAT) & > > mask); \ > > > > \ > > if (!status) > > \ > > break; > > \ > > @@ -435,26 +445,34 @@ static irqreturn_t panthor_ ## __name ## > > _irq_threaded_handler(int irq, void *da > > ret = IRQ_HANDLED; > > \ > > } > > \ > > > > \ > > - if (!atomic_read(&pirq->suspended)) > > \ > > - gpu_write(ptdev, __reg_prefix ## _INT_MASK, pirq->mask); > > \ > > + scoped_guard(spinlock_irqsave, &pirq->mask_lock) { > > \ > > + if (!atomic_read(&pirq->suspended)) { > > \ > > + /* Only restore the bits that were used and are still > > enabled */ \ > > + gpu_write(ptdev, __reg_prefix ## _INT_MASK, > > \ > > + gpu_read(ptdev, __reg_prefix ## _INT_MASK) | > > \ > > + (mask & pirq->mask)); > > \ > > + } > > \ > > + } > > \ > > > > \ > > return ret; > > \ > > } > > \ > > > > \ > > static inline void panthor_ ## __name ## _irq_suspend(struct panthor_irq > > *pirq) \ > > { > > \ > > - pirq->mask = 0; > > \ > > - gpu_write(pirq->ptdev, __reg_prefix ## _INT_MASK, 0); > > \ > > + scoped_guard(spinlock_irqsave, &pirq->mask_lock) { > > \ > > + gpu_write(pirq->ptdev, __reg_prefix ## _INT_MASK, 0); > > \ > > + } > > \ > > synchronize_irq(pirq->irq); > > \ > > This isn't quite safe with the threaded handler. The following can occur: > > CPU 0 | CPU 1 > --------------------------------+---------------------------- > Running _irq_threaded_handler() | > Enters __handler() callback | > | Enters _irq_suspend > | Writes 0 to _INT_MASK > | Drops scoped_guard() > | Waits for the threaded handler > Enters the final scoped_guard | > pirq->suspended is non-zero | > Reads pirq->mask/mask | > Writes non-zero to _INT_MASK | > | Sets suspended, but it's too late > > Leading to the suspend occurring with interrupts not masked. > > In the next patches you introduce the SUSPENDING flag which I think > might fix this, but with just this patch it's broken so we could have > bisection issues.
Yeah, when I sent it out I was aware it could have a problem because I did the conversion in the follow-up patch. I figured at the time that this was worth not having a giant "do everything" patch, but now you've pointed out to me that I could just reorder the follow-up to be before this one and things will work out. > > Admittedly the old code was a little dodgy with the usage of irq->mask > (I think really we should have atomic accesses to ensure that the write > of pirq->mask is observed before the gpu_write). > > Can you reorder the patches - introduce the panthor_irq_state enum first > (with just SUSPENDED and ACTIVE states) and then do the conversion in > one step? Will do, thanks for pointing this out as a possibility. Shouldn't be too painful either, hopefully. > > Thanks, > Steve > > [ ... snip ... ]
