Re: [PATCH] drm/msm: split DPU core IRQ handler under CONFIG_PREEMPT_RT

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] Setting `IRQF_NO_THREAD` for backends without `irq_thread` forces them into hardirq context on PREEMPT_RT, causing kernel panics.
- [High] Race condition in threaded IRQ handler leading to NULL pointer dereference or Use-After-Free on PREEMPT_RT.
- [High] Acquiring sleepable `spinlock_t` inside `raw_spinlock_t` on non-RT kernels violates wait-context rules, triggering lockdep splats.
- [High] `dpu_core_irq_read()` reads hardware registers directly without checking `irq_pending_mask`, causing fault recovery loops to falsely timeout on PREEMPT_RT.
- [Medium] Primary IRQ handler incorrectly returns `IRQ_NONE` after clearing masked interrupts, risking permanent IRQ disablement.
--

commit de5a98b2b15837b087edf7ff28d14a0650953129
Author: Naman S Thaker <namathak@xxxxxxxxxxxxxxxx>
Subject: drm/msm: split DPU core IRQ handler under CONFIG_PREEMPT_RT

This commit splits dpu_core_irq() into a minimal hardirq handler and a new
dpu_core_irq_thread() to dispatch per-encoder callbacks on PREEMPT_RT
kernels. It also changes irq_lock to raw_spinlock_t and drops IRQF_NO_THREAD
where threaded handlers are not implemented.

> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
> index 5b7cd5241f450..95b016e8d7df1 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
> @@ -322,6 +322,52 @@ static void dpu_core_irq_callback_handler(struct dpu_kms *dpu_kms, unsigned int
>  	irq_entry->cb(irq_entry->arg);
>  }
>  
> +/*
> + * dpu_core_irq_dispatch() runs the fired bits for @reg_idx through their
> + * registered callbacks directly. Only used on non-PREEMPT_RT kernels, where
> + * dpu_core_irq() itself is allowed to take the sleepable locks reached via
> + * those callbacks.
> + */
> +static void dpu_core_irq_dispatch(struct dpu_kms *dpu_kms, int reg_idx, u32 irq_status)
> +{
> +	unsigned int irq_idx;
> +	int bit;
> +
> +	/*
> +	 * Search through matching intr status.
> +	 */
> +	while ((bit = ffs(irq_status)) != 0) {
> +		irq_idx = DPU_IRQ_IDX(reg_idx, bit - 1);
> +
> +		dpu_core_irq_callback_handler(dpu_kms, irq_idx);

[Severity: High]
On non-RT kernels, this dispatch happens while holding the raw_spin_lock 
acquired in dpu_core_irq(). Does this violate wait-context rules?

The dispatched callbacks (e.g., DRM vblank event handlers) acquire standard
spinlock_t locks. Acquiring a spinlock_t (wait context LD_WAIT_CONFIG) while
holding a raw_spinlock_t (wait context LD_WAIT_SPIN) constitutes an invalid
wait context and triggers a lockdep splat on non-RT kernels.

[ ... ]

> @@ -332,16 +378,14 @@ irqreturn_t dpu_core_irq(struct msm_kms *kms)
>  	struct dpu_kms *dpu_kms = to_dpu_kms(kms);
>  	struct dpu_hw_intr *intr = dpu_kms->hw_intr;
>  	int reg_idx;
> -	unsigned int irq_idx;
>  	u32 irq_status;
>  	u32 enable_mask;
> -	int bit;
> -	unsigned long irq_flags;
> +	bool wake_thread = false;
>  
>  	if (!intr)
>  		return IRQ_NONE;
>  
> -	spin_lock_irqsave(&intr->irq_lock, irq_flags);
> +	raw_spin_lock(&intr->irq_lock);
>  	for (reg_idx = 0; reg_idx < MDP_INTR_MAX; reg_idx++) {
>  		if (!test_bit(reg_idx, &intr->irq_mask))
>  			continue;
> @@ -363,6 +407,52 @@ irqreturn_t dpu_core_irq(struct msm_kms *kms)
>  		if (!irq_status)
>  			continue;

[Severity: Medium]
Will this lead to the generic IRQ subsystem permanently disabling the display
IRQ line?

If an interrupt fires but is filtered out by enable_mask, the hardware
interrupt is actively cleared a few lines above this, but irq_status
becomes 0, meaning the loop continues and wake_thread remains false.

>  
> +		if (IS_ENABLED(CONFIG_PREEMPT_RT)) {
> +			dpu_core_irq_defer_to_thread(intr, reg_idx, irq_status);
> +			wake_thread = true;
> +		} else {
> +			dpu_core_irq_dispatch(dpu_kms, reg_idx, irq_status);
> +		}
> +	}
> +
> +	/* ensure register writes go through */
> +	wmb();
> +
> +	raw_spin_unlock(&intr->irq_lock);
> +
> +	if (IS_ENABLED(CONFIG_PREEMPT_RT))
> +		return wake_thread ? IRQ_WAKE_THREAD : IRQ_NONE;

[Severity: Medium]
(Continued from above)
On PREEMPT_RT, returning IRQ_NONE after actively clearing a masked interrupt
falsely tells the generic IRQ subsystem that the interrupt was unhandled.
This could cause the spurious interrupt detector to disable the IRQ line.

> +
> +	return IRQ_HANDLED;
> +}
> +
> +#ifdef CONFIG_PREEMPT_RT
> +/*
> + * dpu_core_irq_thread() runs in a genuine preemptible IRQ thread (woken via
> + * IRQ_WAKE_THREAD from dpu_core_irq() above), so it's safe for it -- and the
> + * per-encoder callbacks it dispatches to -- to take spinlock_t/rt_mutex
> + * locks such as enc_spinlock, dpu_crtc::spin_lock, and the various DRM-core
> + * locks reached via vblank/CRC/writeback handling.
> + */
> +irqreturn_t dpu_core_irq_thread(struct msm_kms *kms)
> +{
> +	struct dpu_kms *dpu_kms = to_dpu_kms(kms);
> +	struct dpu_hw_intr *intr = dpu_kms->hw_intr;
> +	int reg_idx;
> +	unsigned int irq_idx;
> +	u32 irq_status;
> +	unsigned long irq_flags;
> +	int bit;
> +
> +	if (!intr)
> +		return IRQ_NONE;
> +
> +	for (reg_idx = 0; reg_idx < MDP_INTR_MAX; reg_idx++) {
> +		raw_spin_lock_irqsave(&intr->irq_lock, irq_flags);
> +		irq_status = intr->irq_pending_mask[reg_idx];
> +		intr->irq_pending_mask[reg_idx] = 0;
> +		raw_spin_unlock_irqrestore(&intr->irq_lock, irq_flags);
> +
>  		/*
>  		 * Search through matching intr status.
>  		 */

[Severity: High]
Does this leave the callback dispatch completely unprotected from concurrent
modifications?

Since raw_spin_unlock_irqrestore() is called before iterating over the bits,
the callback pointers can be concurrently set to NULL by
dpu_core_irq_unregister_callback() (which acquires irq_lock). 

This allows the callback pointer to be zeroed between the check and execution
in the thread, which could lead to a deterministic NULL pointer dereference.
Furthermore, if the caller frees the callback context immediately after
unregistering, it causes a Use-After-Free since no synchronization prevents
the thread from executing the concurrently running callback.

[ ... ]

> @@ -554,7 +640,7 @@ u32 dpu_core_irq_read(struct dpu_kms *dpu_kms,
>  		return 0;
>  	}
>  
> -	spin_lock_irqsave(&intr->irq_lock, irq_flags);
> +	raw_spin_lock_irqsave(&intr->irq_lock, irq_flags);
>  
>  	reg_idx = DPU_IRQ_REG(irq_idx);
>  	intr_status = DPU_REG_READ(&intr->hw,
> @@ -567,7 +653,7 @@ u32 dpu_core_irq_read(struct dpu_kms *dpu_kms,
>  	/* ensure register writes go through */
>  	wmb();
>  
> -	spin_unlock_irqrestore(&intr->irq_lock, irq_flags);
> +	raw_spin_unlock_irqrestore(&intr->irq_lock, irq_flags);
>  
>  	return intr_status;
>  }

[Severity: High]
Could this result in false timeout errors on PREEMPT_RT?

dpu_core_irq_read() is used by driver wait paths to verify if a hardware event
fired but the software callback was delayed. Because it reads the hardware
register directly (which reads as 0 because the hardirq already cleared it)
and ignores the new irq_pending_mask, it could falsely conclude that the
interrupt never fired.

[ ... ]

> diff --git a/drivers/gpu/drm/msm/msm_kms.c b/drivers/gpu/drm/msm/msm_kms.c
> index e5d0ea6294484..f7d045f283ad9 100644
> --- a/drivers/gpu/drm/msm/msm_kms.c
> +++ b/drivers/gpu/drm/msm/msm_kms.c
> @@ -76,7 +90,25 @@ static int msm_irq_install(struct drm_device *dev, unsigned int irq)
>  
>  	msm_irq_preinstall(dev);
>  
> +#ifdef CONFIG_PREEMPT_RT
> +	/*
> +	 * Some KMS backends (e.g. dpu1) split their handler into a minimal
> +	 * hardirq primary handler that only acks hardware and a threaded
> +	 * handler that does the actual (sleep-capable) callback dispatch.
> +	 * IRQF_ONESHOT keeps the primary handler running as a true hardirq
> +	 * even under PREEMPT_RT's forced-threading (see
> +	 * irq_setup_forced_threading() in kernel/irq/manage.c), the same
> +	 * property IRQF_NO_THREAD gives the backends that don't split their
> +	 * handler and must run their whole ->irq() in hardirq context.
> +	 */
> +	if (kms->funcs->irq_thread)
> +		ret = request_threaded_irq(irq, msm_irq, msm_irq_thread,
> +					   IRQF_ONESHOT, dev->driver->name, dev);
> +	else
> +		ret = request_irq(irq, msm_irq, IRQF_NO_THREAD, dev->driver->name, dev);

[Severity: Critical]
Will setting IRQF_NO_THREAD force legacy backends into hardirq context on
PREEMPT_RT, causing kernel panics?

The patch sets IRQF_NO_THREAD for legacy KMS backends (e.g., MDP5, MDP4)
because they don't implement the new irq_thread callback. However, this
explicitly disables forced-threading on PREEMPT_RT. Because these legacy
handlers acquire standard spinlock_t locks (which are sleepable rt_mutexes
on RT), running them strictly in hardirq context is invalid and will result
in a "scheduling while atomic" crash.

> +#else
>  	ret = request_irq(irq, msm_irq, 0, dev->driver->name, dev);
> +#endif
>  	if (ret)
>  		return ret;
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909-drm-mis-next-split-irq-v1-1-89bc9c512c53@xxxxxxxxxxxxxxxx?part=1