On Wednesday, 29 July 2026 08:34:30 Central European Summer Time
[email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s)
> to consider:
>
> Pre-existing issues:
> - [High] The timeout fallback logic creates a TOCTOU race with the threaded
> IRQ handler, potentially causing subsequent requests to falsely succeed
> immediately.
> - [High] If a soft reset actually times out in hardware, the pending flag is
> never cleared, permanently breaking all future soft reset attempts.
> --
>
> --- Patch [2]: [PATCH 2/3] drm/panthor: Revisit reqs_lock handling in
> flush/reset paths ---
> commit 179344ff59dcf475c58ff1e22983a6fa7e1203f4
> Author: Nicolas Frattaroli <[email protected]>
>
> drm/panthor: Revisit reqs_lock handling in flush/reset paths
>
> This patch revisits the handling of the reqs_lock in the panthor driver's
> flush and reset paths. It switches from using spin_lock_irqsave() around a
> wait_event_timeout() block to using wait_event_lock_timeout() with standard
> spinlocks. This ensures the pending_reqs register is read and updated under
> the lock without unnecessarily disabling interrupts.
>
> > diff --git a/drivers/gpu/drm/panthor/panthor_gpu.c
> > b/drivers/gpu/drm/panthor/panthor_gpu.c
> > index c013d6bf9a599..f015bde80abf0 100644
> > --- a/drivers/gpu/drm/panthor/panthor_gpu.c
> > +++ b/drivers/gpu/drm/panthor/panthor_gpu.c
> [ ... ]
> > @@ -330,35 +330,34 @@ int panthor_gpu_flush_caches(struct panthor_device
> > *ptdev,
> > u32 l2, u32 lsc, u32 other)
> > {
> [ ... ]
> > - if (!wait_event_timeout(ptdev->gpu->reqs_acked,
> > + if (!wait_event_lock_timeout(ptdev->gpu->reqs_acked,
> > !(ptdev->gpu->pending_reqs &
> > GPU_IRQ_CLEAN_CACHES_COMPLETED),
> > - msecs_to_jiffies(100))) {
> > - spin_lock_irqsave(&ptdev->gpu->reqs_lock, flags);
> > + ptdev->gpu->reqs_lock, msecs_to_jiffies(100))) {
> > if ((ptdev->gpu->pending_reqs & GPU_IRQ_CLEAN_CACHES_COMPLETED)
> > != 0 &&
> > !(gpu_read(gpu->irq.iomem, INT_RAWSTAT) &
> > GPU_IRQ_CLEAN_CACHES_COMPLETED))
> > ret = -ETIMEDOUT;
> > else
> > ptdev->gpu->pending_reqs &=
> > ~GPU_IRQ_CLEAN_CACHES_COMPLETED;
>
> [Severity: High]
> This isn't a bug introduced by this patch, but does this timeout fallback
> logic create a race condition with the threaded IRQ handler?
>
> If the timeout occurs because the threaded IRQ handler was delayed (e.g. by
> high CPU load), the fallback path clears the pending_reqs flag based on
> INT_RAWSTAT, but it doesn't clear the physical interrupt via INT_CLEAR.
>
> When the threaded IRQ handler eventually runs, won't it read the pending
> interrupt and clear pending_reqs again? If a subsequent request was submitted
> in the meantime, could its pending flag be spuriously cleared, causing its
> wait to falsely succeed immediately?
Not that the special case logic isn't in need of some explanation which
I do not have handy, but: no, there's no pre-existing TOCTOU here as
far as I can tell. If the threaded handler didn't run, it wouldn't have
woken up the waitqueue here.
The fallback likely shouldn't exist in the first place, since it'd only
modify pending_reqs if the timeout and event wakeup coincided, but we
acquired the spinlock and pending_reqs still hasn't updated, which is
impossible, because the wakeup is fired by the threaded handler with
the spinlock held after pending_reqs is updated.
If `ptdev->gpu->pending_reqs & GPU_IRQ_CLEAN_CACHES_COMPLETED) != 0` is
false, the else branch is essentially a no-op.
> > - spin_unlock_irqrestore(&ptdev->gpu->reqs_lock, flags);
> > }
> > + spin_unlock(&ptdev->gpu->reqs_lock);
> >
> > if (ret) {
> > panthor_device_schedule_reset(ptdev);
> [ ... ]
> > @@ -378,27 +377,25 @@ int panthor_gpu_soft_reset(struct panthor_device
> > *ptdev)
> > {
> [ ... ]
> > - if (!wait_event_timeout(ptdev->gpu->reqs_acked,
> > + if (!wait_event_lock_timeout(ptdev->gpu->reqs_acked,
> > !(ptdev->gpu->pending_reqs &
> > GPU_IRQ_RESET_COMPLETED),
> > + ptdev->gpu->reqs_lock,
> > msecs_to_jiffies(100))) {
> > - spin_lock_irqsave(&ptdev->gpu->reqs_lock, flags);
> > if ((ptdev->gpu->pending_reqs & GPU_IRQ_RESET_COMPLETED) != 0 &&
> > !(gpu_read(gpu->irq.iomem, INT_RAWSTAT) &
> > GPU_IRQ_RESET_COMPLETED))
> > timedout = true;
> > else
> > ptdev->gpu->pending_reqs &= ~GPU_IRQ_RESET_COMPLETED;
>
> [Severity: High]
> This is the same pre-existing race condition as in panthor_gpu_flush_caches(),
> where clearing the pending flag without clearing the hardware interrupt could
> allow a subsequent soft reset to succeed spuriously.
>
> > - spin_unlock_irqrestore(&ptdev->gpu->reqs_lock, flags);
> > }
> >
> > if (timedout) {
>
> [Severity: High]
> This is a pre-existing issue, but if a soft reset actually times out in
> hardware and timedout is set to true, does this early return bypass
> clearing GPU_IRQ_RESET_COMPLETED from pending_reqs?
>
> Looking at the end of panthor_gpu_soft_reset(), there is a
> ptdev->gpu->pending_reqs = 0; after the timeout check. If we return early
> here via return -ETIMEDOUT, the pending flag is left set.
>
> Would any subsequent calls to panthor_gpu_soft_reset() hit the drm_WARN_ON
> check at the start of the function, skip issuing the hardware command, and
> permanently fail with a timeout?
>
The bigger problem is that none of the callers even care about whether
it timed out.
