Re: [PATCH v5 06/11] drm/panfrost: Explicitly enable MMU interrupts at device init

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