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

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