On Fri, 26 Jun 2026 10:29:54 +0100
Liviu Dudau <[email protected]> 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: [email protected]
> > Closes:
> > https://sashiko.dev/#/patchset/[email protected]?part=1
> > Signed-off-by: Boris Brezillon <[email protected]>
> > ---
> > 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/[email protected]/
