Re: [PATCH v8 07/16] drm/panfrost: Explicitly enable MMU interrupts at device init

[sashiko-bot@kernel.org]

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 <adrian.larumbe@collabora.com>
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/20260907-claude-fixes-v8-0-c2bcb5e82184@collabora.com?part=7