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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Workqueue deadlock in `panfrost_job_timedout()` due to waiting on an ordered workqueue from a task executing on the same workqueue.
- [High] The timed-out job is permanently leaked and bypasses karma accounting because the asynchronous reset work executes with a NULL 'bad' job reference.
- [Medium] Unrealistically short wait timeout (60 milliseconds) for the GPU reset sequence.
- [Medium] Premature clearing of the `reset.pending` flag creates a data race with the TDR worker.
--

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

This commit introduces a dedicated reset worker to serialize GPU resets.
It uses an ordered workqueue and wait_event_timeout to block concurrent
reset attempts while the ongoing reset thread completes.

Link: None provided

> diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
> --- a/drivers/gpu/drm/panfrost/panfrost_job.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_job.c
> @@ -797,10 +798,12 @@ static enum drm_gpu_sched_stat panfrost_job_timedout(struct drm_sched_job
>  
>  	panfrost_core_dump(job);
>  
> -	atomic_set(&pfdev->reset.pending, 1);
> -	panfrost_reset(pfdev, sched_job);
> +	panfrost_device_schedule_reset(pfdev);

[Severity: High]
By removing the synchronous panfrost_reset() call and relying on
panfrost_reset_work(), we seem to lose the reference to the timed-out job.

Looking at the new worker function:

static void panfrost_reset_work(struct work_struct *work)
{
	struct panfrost_device *pfdev;

	pfdev = container_of(work, struct panfrost_device, reset.work);
	panfrost_reset(pfdev, NULL);
}

Since the guilty job is passed as NULL, it bypasses karma accounting and
drm_sched_stop() will fail to reinsert it into the pending list. Will this
cause the bad job to permanently leak and hang userspace?

> +	ret = wait_event_timeout(pfdev->reset.wait,
> +				 !atomic_read(&pfdev->reset.pending),
> +				 msecs_to_jiffies(60));

[Severity: High]
Could this wait trigger a workqueue deadlock?

In panfrost_jm_init(), args.timeout_wq is assigned to pfdev->reset.wq.
This means the timeout handler runs on the ordered reset workqueue.

By queuing pfdev->reset.work onto this exact same workqueue and synchronously
waiting for it to complete here, it seems the reset work can never execute
because the single-threaded queue is blocked waiting for it.

[Severity: Medium]
Is this timeout duration sufficient?

A 60 millisecond wait seems extremely short for a full GPU reset, which
involves soft-stopping jobs and polling the hardware. If it expires
prematurely, it will return ENODEV and abandon recovery. Could this have
been intended to be 60 seconds (60000 msecs)?

[Severity: Medium]
Is it safe to rely on the pending flag to synchronize scheduler restart?

In panfrost_reset(), atomic_set(&pfdev->reset.pending, 0) is called
immediately after the hardware reset, but before drm_sched_resubmit_jobs()
and drm_sched_start() have finished.

If this wait_event_timeout() wakes up early or spuriously, it will unblock
the DRM scheduler while the reset worker is still actively modifying
scheduler lists. Should the flag be cleared only after the full reset
sequence completes?

> -	return DRM_GPU_SCHED_STAT_RESET;
> +	return (ret) ? DRM_GPU_SCHED_STAT_RESET : DRM_GPU_SCHED_STAT_ENODEV;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260724-claude-fixes-v3-0-542af61abd0f@collabora.com?part=8