Re: [PATCH v5 1/3] drm/panthor: Extend IRQ helpers for mask modification/restoration

[Nicolas Frattaroli <nicolas.frattaroli@collabora.com>]

On Monday, 22 December 2025 16:23:35 Central European Standard Time Steven Price wrote:
> On 21/12/2025 17:10, 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. Then, add
> > the aforementioned new helpers: mask_enable, mask_disable, and
> > resume_restore. The first two work by ORing and NANDing the mask bits,
> > and the latter relies on the new behaviour that panthor_irq::mask is not
> > set to 0 on suspend.
> > 
> > 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 <nicolas.frattaroli@collabora.com>
> > ---
> >  drivers/gpu/drm/panthor/panthor_device.h | 55 +++++++++++++++++++++++++++-----
> >  1 file changed, 47 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/panthor/panthor_device.h b/drivers/gpu/drm/panthor/panthor_device.h
> > index f35e52b9546a..eb75c83e2db3 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))					\
> > @@ -425,8 +430,10 @@ static irqreturn_t panthor_ ## __name ## _irq_threaded_handler(int irq, void *da
> >  	struct panthor_device *ptdev = pirq->ptdev;						\
> >  	irqreturn_t ret = IRQ_NONE;								\
> >  												\
> > +	guard(spinlock_irqsave)(&pirq->mask_lock);						\
> 
> Woah! You can't do that. That means there's a spinlock held while
> calling the __handler() function. So you'll get "sleeping while atomic"
> bug reports. Specifically I can see that panthor_mmu_irq_handler() takes
> a mutex.
> 
> But the whole point of a threaded handler is so that it can sleep, so we
> definitely don't want a spinlock held.

Hmm, yeah, I was worried about that. The core issue is that I'm
trying to make sure we don't restore the mask if we suspended
in the meantime, and don't mess anything up if the mask got
modified in the meantime as well.

I guess I can get us there by reading the mask member into a
local with the lock held for the &'ing, dropping it before
we enter the while, and at the mask restoration point we
take the lock and OR the mask register contents with the
mask we squirrelled away into the local, unless we're
suspended at this point.

I'm not sure if the behaviour when the mask is modified while
we're in the while and we're still using the old one is the
one we want, but going off vibes it seems that in this case,
it'd just conclude that all it has to process has been
processed, and the handler will presumably run again with
the new remaining interrupt status bits.

Kind regards,
Nicolas Frattaroli

> 
> Thanks,
> Steve
> 
> > +												\
> >  	while (true) {										\
> > -		u32 status = gpu_read(ptdev, __reg_prefix ## _INT_RAWSTAT) & pirq->mask;	\
> > +		u32 status = (gpu_read(ptdev, __reg_prefix ## _INT_RAWSTAT) & pirq->mask);	\
> >  												\
> >  		if (!status)									\
> >  			break;									\
> > @@ -443,18 +450,30 @@ static irqreturn_t panthor_ ## __name ## _irq_threaded_handler(int irq, void *da
> >  												\
> >  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);								\
> >  	atomic_set(&pirq->suspended, true);							\
> >  }												\
> >  												\
> > [...]