Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] This problem wasn't introduced by this patch, but the GPU reset sequence (`panfrost_reset`) synchronizes the Job Management (JM) IRQ but fails to mask or synchronize the MMU IRQ, leading to a TOCTOU data race that can corrupt the driver state for newly submitted jobs. - [High] This problem wasn't introduced by this patch, but during driver unbind (`panfrost_remove`) or probe failure, clocks and power domains are disabled while shared IRQ handlers remain active, which can lead to kernel crashes if the shared IRQ line is asserted. -- commit 8161749d65dccd8c767c11da6233beaf16346d71 Author: Adrián Larumbe <[email protected]> drm/panfrost: Explicitly enable MMU interrupts at device init Because the device must be in a position to accept jobs between the time drm_dev_register() is called and autosuspend first kicks in, this commit enables MMU interrupts at device initialisation time. This avoids a narrow window where jobs targeting the tiler buffer object could time out. > diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c > b/drivers/gpu/drm/panfrost/panfrost_mmu.c > index 5c393ed6e3101..faa62643caf1d 100644 > --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c > +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c [ ... ] > @@ -355,8 +361,7 @@ void panfrost_mmu_reset(struct panfrost_device *pfdev) > > spin_unlock(&pfdev->as_lock); > > - mmu_write(pfdev, MMU_INT_CLEAR, ~0); > - mmu_write(pfdev, MMU_INT_MASK, ~0); > + panfrost_mmu_enable_interrupts(pfdev); [Severity: High] This isn't a bug introduced by this patch, but does the GPU reset sequence have a data race here? When panfrost_reset() in drivers/gpu/drm/panfrost/panfrost_job.c triggers a reset, it synchronizes the JM IRQ but fails to mask or synchronize the MMU IRQ: drivers/gpu/drm/panfrost/panfrost_job.c:panfrost_reset() { ... job_write(pfdev, JOB_INT_MASK, 0); synchronize_irq(pfdev->js->irq); ... } Because the MMU IRQ isn't synchronized, panfrost_mmu_irq_handler_thread() can execute concurrently with panfrost_device_reset() and panfrost_mmu_reset(). Since panfrost_mmu_enable_interrupts() modifies the MMU_INT_MASK hardware register outside of pfdev->as_lock, could this race cause pfdev->as_faulty_mask and the hardware register to become inconsistent if a fault occurs exactly during reset, leading to timeouts for newly submitted jobs? > } > [ ... ] > @@ -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 could enabling interrupts here cause a bus error if the probe fails or during driver unbind? If probe fails later in panfrost_device_init() in drivers/gpu/drm/panfrost/panfrost_device.c, or during module unload in panfrost_remove() in drivers/gpu/drm/panfrost/panfrost_drv.c, panfrost_mmu_fini() is called to clear the mask: drivers/gpu/drm/panfrost/panfrost_mmu.c:panfrost_mmu_fini() { mmu_write(pfdev, MMU_INT_MASK, 0); } However, this doesn't set PANFROST_COMP_BIT_MMU in pfdev->is_suspended or call synchronize_irq(). The clocks and power domains are then disabled, but the devm-managed shared IRQ remains registered until the probe or remove function fully returns. If the shared IRQ line is asserted during this teardown window, won't panfrost_mmu_irq_handler() bypass the is_suspended check and read MMU_INT_STAT without active clocks, resulting in a synchronous external abort? > return 0; > } > -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7
