Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] Scheduler teardown races with pending reset work in panfrost_jm_fini(). - [High] panfrost_reset() drops PM references prematurely, allowing concurrent autosuspend to power off the GPU before reset. -- commit 41ac0c5362c3dc2a1e03fb9d2659c68f74cd414d Author: Adrián Larumbe <[email protected]> drm/panfrost: Introduce a reset lock This commit adds a read/write semaphore to the panfrost device reset structure. It protects hardware register access during an ongoing reset sequence, specifically preventing the timestamp query ioctl from racing with the reset process. > diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c > b/drivers/gpu/drm/panfrost/panfrost_device.c > index f171abe26f80d..e90efcff5ce7e 100644 > --- a/drivers/gpu/drm/panfrost/panfrost_device.c > +++ b/drivers/gpu/drm/panfrost/panfrost_device.c > @@ -466,6 +466,8 @@ bool panfrost_exception_needs_reset(const struct > panfrost_device *pfdev, > > void panfrost_device_reset(struct panfrost_device *pfdev, bool > enable_job_int) > { > + guard(rwsem_write)(&pfdev->reset.lock); > + > panfrost_gpu_soft_reset(pfdev); > > panfrost_gpu_power_on(pfdev); [Severity: High] This isn't a bug introduced by this patch, but looking at how panfrost_device_reset() is called from panfrost_reset() in drivers/gpu/drm/panfrost/panfrost_job.c, is there a race with GPU autosuspend? In panfrost_reset(), the PM references for stuck jobs are dropped right before calling panfrost_device_reset(): pm_runtime_put_noidle(pfdev->base.dev); panfrost_devfreq_record_idle(&pfdev->pfdevfreq); ... /* Proceed with reset now. */ panfrost_device_reset(pfdev, false); If an autosuspend timer expires in this small window, could panfrost_device_runtime_suspend() execute and power off the GPU? If so, the subsequent register accesses inside panfrost_device_reset() (like writing to GPU_INT_MASK in panfrost_gpu_soft_reset()) might hit powered-off hardware, potentially causing a bus fault. [ ... ] > diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c > b/drivers/gpu/drm/panfrost/panfrost_job.c > index 00fb36c577df7..c2b1f1087e21e 100644 > --- a/drivers/gpu/drm/panfrost/panfrost_job.c > +++ b/drivers/gpu/drm/panfrost/panfrost_job.c > @@ -877,6 +877,7 @@ int panfrost_jm_init(struct panfrost_device *pfdev) > INIT_WORK(&pfdev->reset.work, panfrost_reset_work); > spin_lock_init(&js->job_lock); > mutex_init(&pfdev->sched_lock); > + init_rwsem(&pfdev->reset.lock); > > js->irq = platform_get_irq_byname(to_platform_device(pfdev->base.dev), > "job"); > if (js->irq < 0) [Severity: High] This is a pre-existing issue, but while we are setting up the reset worker and its locks here, does the teardown order in panfrost_jm_fini() race with the reset work? Looking at panfrost_jm_fini() in drivers/gpu/drm/panfrost/panfrost_job.c, it destroys the schedulers before cancelling the pending reset work: for (j = 0; j < NUM_JOB_SLOTS; j++) { drm_sched_fini(&js->queue[j].sched); } cancel_work_sync(&pfdev->reset.work); If a user triggers a GPU reset via debugfs or a delayed reset work is queued just as the module is unloaded, could panfrost_reset_work() execute concurrently? Since panfrost_reset() calls drm_sched_stop() and drm_sched_start() on the torn-down schedulers, this sequence could lead to accessing freed scheduler memory. -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=14
