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