Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues: - [High] Deadlock between DRM scheduler fence signalling and the MMU threaded IRQ handler during device reset. Pre-existing issues: - [High] Incomplete IRQ synchronization during initialization and probe failure leads to use-after-free and NULL pointer dereferences on `pfdev->reset.wq`. - [High] TOCTOU race in MMU and JM threaded IRQ handlers can lead to IRQ storms after device suspend. -- --- Patch [7]: [PATCH v4 07/13] drm/panfrost: Sync with IRQ before MMU disable and reset --- commit 46502e10e9742128c205fa36652f0fba77751f65 Author: Adrián Larumbe <[email protected]> Subject: drm/panfrost: Sync with IRQ before MMU disable and reset The patch ensures that the MMU threaded IRQ handler is synchronized before disabling the MMU during a reset or device teardown. It adds an IRQ synchronization call during device reset and uses the suspend IRQ routine in the MMU teardown sequence. > --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c > +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c > @@ -347,6 +347,7 @@ void panfrost_mmu_reset(struct panfrost_device *pfdev) > struct panfrost_mmu *mmu, *mmu_tmp; > > clear_bit(PANFROST_COMP_BIT_MMU, pfdev->is_suspended); > + synchronize_irq(pfdev->mmu_irq); [Severity: High] Does this code introduce a deadlock between DRM scheduler fence signalling and the MMU threaded IRQ handler during device reset? During a job timeout recovery, panfrost_reset() establishes a DMA fence signalling critical section via dma_fence_begin_signalling() before calling panfrost_device_reset(). By adding synchronize_irq(pfdev->mmu_irq) here, the fence signalling thread waits for the MMU threaded IRQ handler to complete. However, the page fault handler panfrost_mmu_irq_handler_thread() allocates memory with GFP_KERNEL: bo->sgts = kvmalloc_objs(struct sg_table, bo->base.base.size / SZ_2M, GFP_KERNEL | __GFP_ZERO); If low memory conditions trigger direct reclaim, and direct reclaim waits for GPU fences to signal, will this create a circular lock dependency since the fence signalling thread is blocked on synchronize_irq()? > > spin_lock(&pfdev->as_lock); > [ ... ] > @@ -983,11 +984,6 @@ int panfrost_mmu_init(struct panfrost_device *pfdev) > return 0; > } > > -void panfrost_mmu_fini(struct panfrost_device *pfdev) > -{ > - mmu_write(pfdev, MMU_INT_MASK, 0); > -} > - > void panfrost_mmu_suspend_irq(struct panfrost_device *pfdev) > { > set_bit(PANFROST_COMP_BIT_MMU, pfdev->is_suspended); > @@ -995,3 +991,8 @@ void panfrost_mmu_suspend_irq(struct panfrost_device > *pfdev) > mmu_write(pfdev, MMU_INT_MASK, 0); > synchronize_irq(pfdev->mmu_irq); > } [Severity: High] This isn't a bug introduced by this patch, but does panfrost_mmu_suspend_irq() race with panfrost_mmu_irq_handler_thread() and cause an IRQ storm? In the MMU threaded IRQ handler, the suspended state is checked before unmasking: if (!test_bit(PANFROST_COMP_BIT_MMU, pfdev->is_suspended)) { spin_lock(&pfdev->as_lock); mmu_write(pfdev, MMU_INT_MASK, ~pfdev->as_faulty_mask); ... } If the threaded handler is preempted immediately after passing the test_bit() check, panfrost_mmu_suspend_irq() can execute, set the suspended bit, mask the interrupts, and block on synchronize_irq(). When the threaded handler resumes, does it blindly write the unmask value back to MMU_INT_MASK? Since the hardirq handler panfrost_mmu_irq_handler() returns IRQ_NONE when the suspended bit is set, will any subsequent unmasked interrupt cause an endless IRQ loop? > + > +void panfrost_mmu_fini(struct panfrost_device *pfdev) > +{ > + panfrost_mmu_suspend_irq(pfdev); > +} [Severity: High] This is a pre-existing issue, but can incomplete IRQ synchronization during initialization or probe failure lead to use-after-free or NULL pointer dereferences on pfdev->reset.wq? During initialization, panfrost_device_init() calls panfrost_mmu_init() which enables MMU interrupts, and then it calls panfrost_jm_init() which allocates pfdev->reset.wq. If an MMU interrupt triggers between these two calls, can the handler call panfrost_device_schedule_reset() and invoke queue_work() with a NULL pfdev->reset.wq? During probe failure unwinding, panfrost_device_init() handles errors by executing panfrost_jm_fini() (destroying the workqueue) before calling panfrost_mmu_fini(). If an MMU interrupt fires in this teardown window, will it execute queue_work() on the already freed pfdev->reset.wq? -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7
