Re: [PATCH 2/3] drm/panthor: Revisit reqs_lock handling in flush/reset paths

[sashiko-bot@kernel.org]

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 <nicolas.frattaroli@collabora.com>

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/20260729-panthor-cache-flush-fix-v1-0-205921ed3c81@collabora.com?part=2