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 <[email protected]> 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/[email protected]?part=1
