Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues: - [High] panfrost_device_runtime_suspend unconditionally dereferences pfdev->js, causing a kernel panic on early probe failure. - [High] Missing IRQ synchronization in panfrost_jm_fini and panfrost_gpu_fini allows concurrent IRQ handlers to access freed memory or disabled hardware. - [High] A Time-of-Check to Time-of-Use (TOCTOU) race condition between the threaded IRQ handler and the IRQ suspend function can leave MMU hardware interrupts enabled, leading to an unhandled interrupt storm. -- --- Patch [7]: [PATCH v3 07/13] drm/panfrost: Sync with IRQ before MMU disable and reset --- commit 50e811614859715a2c6f2e0b562e19db3f2c571c Author: Adrián Larumbe <[email protected]> drm/panfrost: Sync with IRQ before MMU disable and reset This commit synchronizes the MMU IRQ handler before disabling and resetting the MMU, preventing HW register accesses from spurious interrupts during device teardown or reset. It introduces panfrost_mmu_suspend_irq() to handle the IRQ suspension sequence. > diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c > b/drivers/gpu/drm/panfrost/panfrost_mmu.c > index bb6c1614964b1..f067bcd360a89 100644 > --- 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); > > spin_lock(&pfdev->as_lock); [ ... ] > @@ -980,11 +981,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); > @@ -992,3 +988,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 is a pre-existing issue, but is there a Time-of-Check to Time-of-Use (TOCTOU) race condition between this suspend function and the threaded IRQ handler? If the threaded handler panfrost_mmu_irq_handler_thread() checks PANFROST_COMP_BIT_MMU without holding locks: 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); spin_unlock(&pfdev->as_lock); } Concurrently, panfrost_mmu_suspend_irq() sets the bit, writes 0 to MMU_INT_MASK, and calls synchronize_irq(). If the threaded handler performs the test_bit() check just before the bit is set, it evaluates to true. Then suspend_irq() masks interrupts and waits, and finally the threaded handler overwrites the 0 with ~pfdev->as_faulty_mask, re-enabling interrupts. Could this lead to an unhandled interrupt storm if the device asserts the interrupt and panfrost_mmu_irq_handler() returns IRQ_NONE? > + > +void panfrost_mmu_fini(struct panfrost_device *pfdev) > +{ > + panfrost_mmu_suspend_irq(pfdev); > +} [Severity: High] This isn't a bug introduced by this patch, but does early probe failure cause a kernel panic due to a NULL pointer dereference? If the device probe fails early (e.g., in panfrost_gpu_init() or panfrost_mmu_init()), panfrost_jm_init() is never reached, leaving pfdev->js as NULL. The error unwinding path calls panfrost_try_suspend_device(), which synchronously triggers panfrost_device_runtime_suspend(). This suspend handler unconditionally calls panfrost_jm_is_idle(pfdev): drivers/gpu/drm/panfrost/panfrost_device.c:panfrost_device_runtime_suspend() { ... if (!panfrost_jm_is_idle(pfdev)) return -EBUSY; ... } Which then dereferences pfdev->js->queue: drivers/gpu/drm/panfrost/panfrost_job.c:panfrost_jm_is_idle() { struct panfrost_job_slot *js = pfdev->js; ... if (atomic_read(&js->queue[i].sched.credit_count)) ... } Can we safely call panfrost_jm_is_idle() before verifying if the job manager is initialized? [Severity: High] This is a pre-existing issue, but are there missing IRQ synchronizations in panfrost_jm_fini() and panfrost_gpu_fini() that could allow concurrent IRQ handlers to access freed memory or disabled hardware? In panfrost_jm_fini(), the driver masks job interrupts but fails to set the suspend bit (PANFROST_COMP_BIT_JOB) or wait for executing threaded handlers: drivers/gpu/drm/panfrost/panfrost_job.c:panfrost_jm_fini() { ... job_write(pfdev, JOB_INT_MASK, 0); for (j = 0; j < NUM_JOB_SLOTS; j++) { drm_sched_fini(&js->queue[j].sched); } ... } Could a concurrently running IRQ thread access the destroyed scheduler queues or re-enable the interrupts? Similarly, in panfrost_gpu_fini(), the GPU is powered off without synchronizing or suspending the shared GPU IRQ: drivers/gpu/drm/panfrost/panfrost_gpu.c:panfrost_gpu_fini() { panfrost_gpu_power_off(pfdev); } If a shared interrupt fires, panfrost_gpu_irq_handler() will read GPU_FAULT_STATUS from powered-off hardware. Could this cause a bus fault? -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7
