Re: [PATCH] drm/msm/dpu: delete timeout timer when encoder is not busy

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] A Time-of-Check to Time-of-Use (TOCTOU) race condition in the lockless timer deletion allows an IRQ handler to inadvertently delete the watchdog timer for a newly started frame on another CPU.
- [Medium] The patch fails to address the root cause of the race condition and relies on a subsequent, unrelated IRQ to mask the bug, leaving the original false timeout issue unfixed in many scenarios.

Pre-existing issues:
- [High] Missing synchronization in `dpu_encoder_virt_atomic_disable` allows the driver to disable hardware resources while the timer callback is still executing, leading to crashes.
--

commit 91dd8b2e3a127693bc37ea9250d6d6a4bd503cf0
Author: Jun Nie <jun.nie@linaro.org>

drm/msm/dpu: delete timeout timer when encoder is not busy

This commit attempts to fix a spurious timeout error message during encoder
disable. It adds a check in the frame done IRQ callback to delete the
watchdog timer if the frame busy bits are cleared.

> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 1f20695f81e35..d83d387973311 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -1573,6 +1573,21 @@ void dpu_encoder_frame_done_callback(
>  			| DPU_ENCODER_FRAME_EVENT_PANEL_DEAD)) {
>  
>  		if (!dpu_enc->frame_busy_mask[0]) {
> +			/*
> +			 * dpu_crtc_commit_kickoff calls dpu_encoder_kickoff to
> +			 * mark busy bits, starts the framedone timer next.
> +			 * It is possibile that irq happens between the 2
> +			 * operations. Thus timer is running with busy bits
> +			 * cleared by irq handler and timer will not be deleted
> +			 * anymore. Then false timeout introduces unnecessary
> +			 * confusion and visual defect.
> +			 * delete the timer here to fix it.
> +			 */
> +			if (atomic_read(&dpu_enc->frame_done_timeout_ms)) {
> +				atomic_set(&dpu_enc->frame_done_timeout_ms, 0);
> +				timer_delete(&dpu_enc->frame_done_timer);
> +			}

[Severity: Medium]
Does this patch rely on a subsequent IRQ to fix the false timeout in all
scenarios?

If no subsequent IRQ (like autorefresh or a TE interrupt) fires after the
timer is armed with an empty frame_busy_mask, the timer will expire normally.
When it expires, dpu_encoder_frame_done_timeout() returns early because
frame_busy_mask is empty, but it fails to clear frame_done_timeout_ms:

dpu_encoder_frame_done_timeout() {
    ...
    if (!dpu_enc->frame_busy_mask[0] || !dpu_enc->crtc) {
        return;
    }
    ...
}

Later, dpu_encoder_virt_atomic_disable() checks frame_done_timeout_ms,
finds it non-zero, and still falsely prints the 'timeout pending' error.
Will this leave the bug unfixed for standard operations that do not trigger
extra IRQs?

[Severity: High]
Could this lockless timer deletion introduce a race condition with a new
commit?

CPU1 handling a spurious IRQ in dpu_encoder_frame_done_callback()
evaluates if (!dpu_enc->frame_busy_mask[0]) as true locklessly.

Concurrently, CPU2 starts a new commit:
dpu_encoder_kickoff() sets frame_busy_mask to 1.
dpu_encoder_start_frame_done_timer() arms the timer and sets
frame_done_timeout_ms to 500.

CPU1 continues execution, reads frame_done_timeout_ms (now 500 from CPU2),
sets it to 0, and calls timer_delete().

Does this silently delete the newly armed watchdog timer for CPU2's commit,
leaving it without watchdog protection if the hardware hangs?

[Severity: High]
This is a pre-existing issue, but does dpu_encoder_virt_atomic_disable()
lack synchronization when disabling hardware resources?

dpu_encoder_frame_done_timeout() clears frame_done_timeout_ms via
atomic_xchg() early in its execution:

dpu_encoder_frame_done_timeout() {
    ...
    } else if (!atomic_xchg(&dpu_enc->frame_done_timeout_ms, 0)) {
        return;
    }
    ...
}

If dpu_encoder_virt_atomic_disable() runs concurrently, it checks:

    if (atomic_xchg(&dpu_enc->frame_done_timeout_ms, 0)) {
        timer_delete_sync(&dpu_enc->frame_done_timer);
    }

If the timeout callback has already cleared the variable, the disable path
sees 0, skips timer_delete_sync(), and proceeds to disable clocks and power
domains. Meanwhile, the timeout callback is still executing and accesses
hardware registers, which can cause a synchronous external abort when clocks
are disabled. Should this path always synchronize with the timer callback?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903143004.1026181-1-jun.nie@linaro.org?part=1