Re: [PATCH 05/11] drm/panthor: Fix theoretical IOMEM access in suspended state

[Liviu Dudau <liviu.dudau@arm.com>]

On Fri, Jun 26, 2026 at 01:40:28PM +0200, Boris Brezillon wrote:
> On Fri, 26 Jun 2026 10:29:54 +0100
> Liviu Dudau <liviu.dudau@arm.com> wrote:
> 
> > On Thu, Jun 25, 2026 at 02:40:31PM +0200, Boris Brezillon wrote:
> > > In theory, our hardirq handler can be called while the device (and
> > > thus the panthor_irq) is suspended, because the IRQ line is shared.
> > > In practice though, in all the designs we've seen, the line is only
> > > shared within the GPU, and because sub-component suspend state is
> > > consistent (all-suspended or all-resumed), we shouldn't end up with
> > > an interrupt triggered while we're suspended.
> > > 
> > > Fix the problem anyway, if nothing else, for our sanity.
> > > 
> > > Fixes: 0b2d86670a84 ("drm/panthor: Rework panthor_irq::suspended into panthor_irq::state")
> > > Reported-by: sashiko-bot@kernel.org
> > > Closes: https://sashiko.dev/#/patchset/20260625-panthor-signal-from-irq-v4-0-3d2908912afa@collabora.com?part=1
> > > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> > > ---
> > >  drivers/gpu/drm/panthor/panthor_device.h | 10 +++++++---
> > >  1 file changed, 7 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/panthor/panthor_device.h b/drivers/gpu/drm/panthor/panthor_device.h
> > > index 35679bfa1f3a..a39386bd6382 100644
> > > --- a/drivers/gpu/drm/panthor/panthor_device.h
> > > +++ b/drivers/gpu/drm/panthor/panthor_device.h
> > > @@ -512,9 +512,6 @@ static irqreturn_t panthor_ ## __name ## _irq_raw_handler(int irq, void *data)
> > >  	struct panthor_irq *pirq = data;							\
> > >  	enum panthor_irq_state old_state;							\
> > >  												\
> > > -	if (!gpu_read(pirq->iomem, INT_STAT))							\
> > > -		return IRQ_NONE;								\
> > > -												\
> > >  	guard(spinlock_irqsave)(&pirq->mask_lock);						\
> > >  	old_state = atomic_cmpxchg(&pirq->state,						\
> > >  				   PANTHOR_IRQ_STATE_ACTIVE,					\
> > > @@ -522,6 +519,13 @@ static irqreturn_t panthor_ ## __name ## _irq_raw_handler(int irq, void *data)
> > >  	if (old_state != PANTHOR_IRQ_STATE_ACTIVE)						\
> > >  		return IRQ_NONE;								\
> > >  												\
> > > +	if (!gpu_read(pirq->iomem, INT_STAT)) {							\
> > > +		atomic_cmpxchg(&pirq->state,							\
> > > +			       PANTHOR_IRQ_STATE_PROCESSING,					\
> > > +			       PANTHOR_IRQ_STATE_ACTIVE);					\
> > > +		return IRQ_NONE;								\
> > > +	}											\  
> > 
> > Hmm,
> > 
> > I get it that you're trying to revert the effect of the previous atomic_cmpxchg() here but it feels
> > like a better option would be to not do the swap at all if the state is not ACTIVE.
> 
> That's what [1] does, but it's not possible until we've made this
> atomic -> lock-based transition, and I want a fix that's not dependent
> on non-Fixes patches so we can backport it.
> 
> [1]https://lore.kernel.org/dri-devel/20260625-panthor-signal-from-irq-v5-8-8836a74e0ef9@collabora.com/

I see.

Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>

Best regards,
Liviu

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯