Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues: - [High] Masking GPU interrupts during device teardown creates an interrupt storm deadlock if jobs are actively running. Pre-existing issues: - [High] Race condition during device teardown where a pending reset work item executes concurrently with or after scheduler destruction, causing a Use-After-Free and erroneously re-enabling interrupts. -- commit a45fd41d29f7c5d2faeeffea961ca1caad97ff77 Author: Adrián Larumbe <[email protected]> drm/panfrost: Split subsystem init/reset from interrupt enablement Because MMU interrupts are only enabled when the device is reset, it happened that after DRM device registration, the very first job targeting the tiler heap BO would always time out. The reason is the reset sequence is only part of PM runtime resume, which is not called explicitly at driver probe time, and an actual reset work item manually triggered after a HW error. > diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c > b/drivers/gpu/drm/panfrost/panfrost_device.c > index 9549ddb7854a3..0cc277efb5850 100644 > --- a/drivers/gpu/drm/panfrost/panfrost_device.c > +++ b/drivers/gpu/drm/panfrost/panfrost_device.c [ ... ] > @@ -341,6 +365,8 @@ void panfrost_device_fini(struct panfrost_device *pfdev) > pm_runtime_get_sync(pfdev->base.dev); > pm_runtime_disable(pfdev->base.dev); > > + panfrost_device_disable_hw(pfdev); > + [Severity: High] Does calling panfrost_device_disable_hw() unconditionally on driver unload create an interrupt storm risk? Since jobs are not guaranteed to be stopped yet, the GPU may generate a fault or completion interrupt in panfrost_gpu_suspend_irq() right after setting the software flag but before masking the hardware interrupt: drivers/gpu/drm/panfrost/panfrost_gpu.c:panfrost_gpu_suspend_irq() { set_bit(PANFROST_COMP_BIT_GPU, pfdev->is_suspended); <-- interrupt fires here gpu_write(pfdev, GPU_INT_MASK, 0); } The panfrost_gpu_irq_handler() sees is_suspended set and returns IRQ_NONE without clearing the hardware interrupt. Because the IRQ is level-triggered, it would re-fire immediately upon exit, leading to a soft lockup and forcing the kernel to permanently disable the shared IRQ line. > panfrost_gem_fini(pfdev); > panfrost_perfcnt_fini(pfdev); > panfrost_jm_fini(pfdev); [ ... ] > diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c > b/drivers/gpu/drm/panfrost/panfrost_job.c > index 5016d2d53da26..087905cd347a5 100644 > --- a/drivers/gpu/drm/panfrost/panfrost_job.c > +++ b/drivers/gpu/drm/panfrost/panfrost_job.c [ ... ] > @@ -753,8 +753,8 @@ panfrost_reset(struct panfrost_device *pfdev, > for (i = 0; i < NUM_JOB_SLOTS; i++) > drm_sched_start(&pfdev->js->queue[i].sched, 0); > > - /* Re-enable job interrupts now that everything has been restarted. */ > - panfrost_jm_enable_interrupts(pfdev); > + /* Re-enable interrupts now that everything has been restarted. */ > + panfrost_device_enable_int(pfdev); [Severity: High] This is a pre-existing issue, but does adding panfrost_device_enable_int() here exacerbate a teardown race with the reset workqueue? During teardown, panfrost_device_fini() disables hardware and then calls panfrost_jm_fini(), which destroys the schedulers before synchronizing the reset workqueue: drivers/gpu/drm/panfrost/panfrost_job.c:panfrost_jm_fini() { for (j = 0; j < NUM_JOB_SLOTS; j++) { drm_sched_fini(&js->queue[j].sched); } cancel_work_sync(&pfdev->reset.work); } If a job timeout occurs before or during panfrost_device_fini(), the concurrent reset work item can execute after scheduler destruction, causing a use-after-free. Furthermore, because this patch adds panfrost_device_enable_int() to the end of the reset sequence, the racing reset work could erroneously clear the is_suspended bits and re-enable hardware interrupts right before the clocks are turned off in teardown. > > dma_fence_end_signalling(cookie); > } -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7
