Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] Missing `synchronize_irq()` in `panfrost_mmu_fini()` allows the MMU IRQ handler thread to execute concurrently with downstream teardown steps, potentially causing a kernel panic. - [High] Writing to `MMU_INT_MASK` before clearing `as_faulty_mask` leaves the Address Space (AS) interrupt masked. -- --- Patch [6]: [PATCH v5 06/11] drm/panfrost: Explicitly enable MMU interrupts at device init --- commit d0741154918a6f24b94515670ed05d32bd8b695a Author: Adrián Larumbe <[email protected]> Subject: drm/panfrost: Explicitly enable MMU interrupts at device init This commit explicitly enables MMU interrupts at device initialization. Because the device must be in a position to accept jobs between the time drm_dev_register() is called and autosuspend first kicks in, there's a very narrow window where jobs could time out since the device's PM status is active but no MMU interrupts were enabled. > diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c > b/drivers/gpu/drm/panfrost/panfrost_mmu.c > index 3fe37dd123609..a8ce5d65825c9 100644 > --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c > +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c [ ... ] > @@ -336,6 +336,12 @@ void panfrost_mmu_as_put(struct panfrost_device *pfdev, > struct panfrost_mmu *mmu > WARN_ON(atomic_read(&mmu->as_count) < 0); > } > > +static void panfrost_mmu_enable_interrupts(struct panfrost_device *pfdev) > +{ > + mmu_write(pfdev, MMU_INT_CLEAR, ~0); > + mmu_write(pfdev, MMU_INT_MASK, ~0); [Severity: High] This isn't a bug introduced by this patch, but is there an issue with how MMU_INT_MASK is programmed when re-acquiring a faulty address space? Looking at panfrost_mmu_as_get() in the same file: panfrost_mmu_as_get() { ... mmu_write(pfdev, MMU_INT_MASK, ~pfdev->as_faulty_mask); pfdev->as_faulty_mask &= ~mask; ... } Because the inverted faulty mask is written to hardware before the bit is cleared from pfdev->as_faulty_mask in software, does this program the hardware with a 0 (masked) for that address space? If a workload causes a page fault (making the address space faulty) and then attempts to reuse it, will the required interrupt remain masked, causing jobs to time out and hang indefinitely? > +} > + [ ... ] > @@ -970,6 +975,8 @@ int panfrost_mmu_init(struct panfrost_device *pfdev) > return err; > } > > + panfrost_mmu_enable_interrupts(pfdev); [Severity: High] This is a pre-existing issue, but does enabling the interrupts earlier in probe expand a race condition during teardown? If panfrost_device_init() fails after panfrost_mmu_init() (or during driver unbind), panfrost_mmu_fini() is called: panfrost_mmu_fini() { mmu_write(pfdev, MMU_INT_MASK, 0); } This masks the interrupt but doesn't call synchronize_irq(). Subsequently, the downstream teardown disables the clocks: panfrost_device_fini() { panfrost_mmu_fini(pfdev); panfrost_gpu_fini(pfdev); panfrost_devfreq_fini(pfdev); panfrost_regulator_fini(pfdev); panfrost_clk_fini(pfdev); } If panfrost_mmu_irq_handler_thread() is still running concurrently after the clocks are disabled, could this lead to a synchronous external abort or kernel panic when accessing memory-mapped GPU registers? > + > return 0; > } -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6
