Re: [PATCH v4 07/13] drm/panfrost: Sync with IRQ before MMU disable and reset

[sashiko-bot@kernel.org]

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 <adrian.larumbe@collabora.com>
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/20260729-claude-fixes-v4-0-01968f2ec77a@collabora.com?part=7