Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues: - [High] The driver is vulnerable to an asynchronous bus fault (system crash) during driver removal when IRQs are shared. The GPU is powered off before the devm-managed IRQs are freed, but the `is_suspended` flags are not set, allowing the IRQ handlers to read from unpowered registers. - [High] Hardware register MMU_INT_MASK suffers from a TOCTOU race with the IRQ thread, which can leave interrupts enabled when the device is suspended. - [High] Incorrect ordering of hardware state update leaves Address Space (AS) interrupts permanently masked after recovering from a page fault. -- commit edb0f05f6a41299dfda9d57b9f1f096e6d1d4aca Author: Adrián Larumbe <[email protected]> Subject: drm/panfrost: Explicitly enable MMU interrupts at device init This patch addresses an issue where jobs targeting the tiler buffer object could time out between device registration and the first autosuspend. It fixes this by explicitly enabling MMU interrupts at device initialization. Fixes: 73e467f60acd ("drm/panfrost: Consolidate reset handling") > diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c > b/drivers/gpu/drm/panfrost/panfrost_mmu.c > --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c > +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c [ ... ] > @@ -973,6 +978,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 while reviewing the interrupt handling code, does the driver have a vulnerability to an asynchronous bus fault during driver removal when IRQs are shared? In drivers/gpu/drm/panfrost/panfrost_device.c, panfrost_remove() calls pm_runtime_set_suspended(): static void panfrost_remove(struct platform_device *pdev) { // ... pm_runtime_disable(pfdev->base.dev); pm_runtime_set_suspended(pfdev->base.dev); panfrost_device_fini(pfdev); } This bypasses the panfrost_device_runtime_suspend() callback that normally sets PANFROST_COMP_BIT_MMU. Then panfrost_device_fini() powers off the GPU. Because PANFROST_COMP_BIT_MMU is not set, if an interrupt arrives on the shared line before devm cleanup frees the IRQ: drivers/gpu/drm/panfrost/panfrost_mmu.c:panfrost_mmu_irq_handler() if (test_bit(PANFROST_COMP_BIT_MMU, pfdev->is_suspended)) return IRQ_NONE; if (!mmu_read(pfdev, MMU_INT_STAT)) return IRQ_NONE; The handler will bypass the is_suspended check and read MMU_INT_STAT from the powered-off GPU, potentially causing an SError or system crash. [Severity: High] This is also a pre-existing issue, but does MMU_INT_MASK suffer from a TOCTOU race with the IRQ thread that can leave interrupts enabled when the device is suspended? In panfrost_mmu_irq_handler_thread(), the thread checks the suspended state outside the lock: /* Enable interrupts only if we're not about to get suspended */ 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); } If preempted immediately after this evaluates to true, another CPU running panfrost_mmu_suspend_irq() can set the is_suspended bit, write 0 to MMU_INT_MASK, and block on synchronize_irq(): set_bit(PANFROST_COMP_BIT_MMU, pfdev->is_suspended); mmu_write(pfdev, MMU_INT_MASK, 0); synchronize_irq(pfdev->mmu_irq); When the thread resumes, it locks, writes ~pfdev->as_faulty_mask to unmask the interrupts, and exits. This allows synchronize_irq() to return, but leaves the interrupts erroneously unmasked in hardware during suspend. [Severity: High] This is another pre-existing issue, but does the hardware state update ordering in panfrost_mmu_as_get() leave Address Space (AS) interrupts permanently masked after recovering from a page fault? When clearing a faulty AS, the code writes to MMU_INT_MASK before clearing the faulty AS's bit in as_faulty_mask: mmu_write(pfdev, MMU_INT_CLEAR, mask); mmu_write(pfdev, MMU_INT_MASK, ~pfdev->as_faulty_mask); pfdev->as_faulty_mask &= ~mask; panfrost_mmu_enable(pfdev, mmu); The inverted mask writes a 0 for this AS, keeping it disabled in the hardware register. Software state is updated immediately afterward, but the hardware register is never re-written to reflect the unmasked state. This could cause future GPU jobs on this AS to hang and timeout without raising an interrupt. > return 0; > } -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7
