Re: [PATCH v4 08/13] drm/panfrost: Rewire reset sequence to avoid concurrent attempts

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Removing the synchronous `atomic_set(&pfdev->reset.pending, 1)` from `panfrost_job_handle_err` opens a vulnerable window where new jobs are erroneously submitted to faulted hardware.
- [Medium] The introduced `pfdev->reset.wait` wait queue is never waited on, rendering it dead code and breaking the intended synchronization.
--

--- Patch [8]: [PATCH v4 08/13] drm/panfrost: Rewire reset sequence to avoid concurrent attempts ---
commit c66abdaa510df5e2a3ec11be0069267b5ffde774
Author: Adrián Larumbe <adrian.larumbe@collabora.com>

drm/panfrost: Rewire reset sequence to avoid concurrent attempts

This rewires the reset sequence to use a single reset thread and a wait queue
for other threads wanting to trigger a reset.

> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h
> index eeca57efcbec0..f0acfc634cd47 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_device.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_device.h
[ ... ]
> @@ -341,8 +342,8 @@ bool panfrost_exception_needs_reset(const struct panfrost_device *pfdev,
>  static inline void
>  panfrost_device_schedule_reset(struct panfrost_device *pfdev)
>  {
> -	atomic_set(&pfdev->reset.pending, 1);
> -	queue_work(pfdev->reset.wq, &pfdev->reset.work);
> +	if (!atomic_cmpxchg(&pfdev->reset.pending, 0, 1))
> +		queue_work(pfdev->reset.wq, &pfdev->reset.work);
>  }

[Severity: Medium]
The commit message states that other threads will wait on the ongoing reset
to finish, and a wait queue is added and woken up in panfrost_reset().
However, there do not appear to be any wait_event() calls added for
pfdev->reset.wait in the codebase.

When the atomic_cmpxchg() fails here, the caller returns immediately instead
of blocking on the ongoing reset. Are we missing the wait logic for
concurrent reset requests?

> diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
> index bda1494eb430c..ed8a1fabd47b4 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_job.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_job.c
[ ... ]
> @@ -499,10 +499,8 @@ static void panfrost_job_handle_err(struct panfrost_device *pfdev,
>  
>  	pm_runtime_put_autosuspend(pfdev->base.dev);
>  
> -	if (panfrost_exception_needs_reset(pfdev, js_status)) {
> -		atomic_set(&pfdev->reset.pending, 1);
> +	if (panfrost_exception_needs_reset(pfdev, js_status))
>  		drm_sched_fault(&pfdev->js->queue[js].sched);
> -	}
>  }

[Severity: High]
Does removing the synchronous atomic_set() here create a vulnerability window?

By delegating this to the async timeout worker via drm_sched_fault(),
reset.pending remains 0 when panfrost_job_handle_err() returns.

If reset.pending is still 0, will panfrost_jm_handle_irq() incorrectly
requeue jobs onto the faulted hardware?

panfrost_jm_handle_irq() {
    ...
    } else if (!atomic_read(&pfdev->reset.pending)) {
        /* Requeue the job we removed if no reset is pending */
        job_write(pfdev, JS_COMMAND_NEXT(j), JS_COMMAND_START);
    }
}

Similarly, concurrent job submissions from the scheduler in
panfrost_job_hw_submit() might also incorrectly proceed because they check
if (!atomic_read(&pfdev->reset.pending)).

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260729-claude-fixes-v4-0-01968f2ec77a@collabora.com?part=8