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 <[email protected]> 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/[email protected]?part=8
