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? > - 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? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
