Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues: - [High] Race condition between the threaded IRQ handler and the wait timeout logic causes spurious GPU resets. -- --- Patch [2]: [PATCH v2 2/3] drm/panthor: Revisit reqs_lock handling in flush/reset paths --- commit ec14154f1ff35bca85396f0f6f71d7d7b1d6eb6d Author: Nicolas Frattaroli <[email protected]> drm/panthor: Revisit reqs_lock handling in flush/reset paths The patch updates the handling of reqs_lock in panthor_gpu_flush_caches() and panthor_gpu_soft_reset(). It replaces wait_event_timeout() with wait_event_lock_timeout(), and uses spin_lock() instead of spin_lock_irqsave() since the lock is not acquired in an atomic context. > diff --git a/drivers/gpu/drm/panthor/panthor_gpu.c > b/drivers/gpu/drm/panthor/panthor_gpu.c > index e52c5675981f5..cd591c17198be 100644 > --- a/drivers/gpu/drm/panthor/panthor_gpu.c > +++ b/drivers/gpu/drm/panthor/panthor_gpu.c > @@ -329,35 +329,34 @@ int panthor_gpu_flush_caches(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_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; [Severity: High] This isn't a bug introduced by this patch, but is there a race condition here with the threaded IRQ handler? If panthor_gpu_irq_handler() clears the hardware interrupt status via INT_CLEAR before acquiring reqs_lock, it can be preempted: gpu_write(gpu->irq.iomem, INT_CLEAR, status); // ... preemptible window ... spin_lock(&ptdev->gpu->reqs_lock); if (status & ptdev->gpu->pending_reqs) { If the 100ms timeout expires during this precise window, the fallback condition above evaluates under reqs_lock. It finds that pending_reqs still indicates the request is pending, but checking INT_RAWSTAT shows the interrupt is not pending (because it was just cleared by the preempted thread). Could this cause the wait logic to falsely conclude the hardware timed out, return -ETIMEDOUT, and schedule a spurious full GPU reset? [ ... ] > @@ -377,27 +376,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; [Severity: High] This is a pre-existing issue, but does this suffer from the same race condition with the threaded IRQ handler as the flush caches timeout logic above? > else > ptdev->gpu->pending_reqs &= ~GPU_IRQ_RESET_COMPLETED; > - spin_unlock_irqrestore(&ptdev->gpu->reqs_lock, flags); > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
