Re: [PATCH v3 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:

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 <adrian.larumbe@collabora.com>

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/20260724-claude-fixes-v3-0-542af61abd0f@collabora.com?part=7