From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm/vkms: Fix ABBA deadlock in vblank disable and timer callback Date: Sat, 16 May 2026 09:05:04 +1000 Message-ID: In-Reply-To: <20260515131826.388154-1-w15303746062@163.com> References: <20260515131826.388154-1-w15303746062@163.com> <20260515131826.388154-1-w15303746062@163.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **Positive aspects:** - The deadlock analysis in the commit message is well-explained, with clear= thread interleaving showing the ABBA pattern. - The `hrtimer_cancel()` =E2=86=92 `hrtimer_try_to_cancel()` change is the = right direction, matching the approach that mainline adopted in `drm_crtc_v= blank_cancel_timer()`. **Issues:** 1. **`hrtimer_try_to_cancel()` return value is silently ignored:** ```c - hrtimer_cancel(&out->vblank_hrtimer); + hrtimer_try_to_cancel(&out->vblank_hrtimer); ``` `hrtimer_try_to_cancel()` returns `-1` when the callback is currently execu= ting, meaning the timer was **not** cancelled. The patch relies entirely on= the timer self-stopping via `HRTIMER_NORESTART`, but there is a race windo= w: the callback could be between `drm_crtc_handle_vblank()` returning true = and evaluating `ret` for the return value. In that window, `ret` is true, s= o the timer re-arms itself even though vblank was just disabled. Compare wi= th mainline's `drm_crtc_vblank_cancel_timer()` which clears the timer inter= val to zero under a spinlock as a definitive cancellation signal, then call= s `hrtimer_try_to_cancel()`: ```c spin_lock_irqsave(&vtimer->interval_lock, flags); vtimer->interval =3D 0; spin_unlock_irqrestore(&vtimer->interval_lock, flags); hrtimer_try_to_cancel(&vtimer->timer); ``` The patch lacks an equivalent mechanism to guarantee the timer won't re-arm. 2. **The `ret` variable semantics are unclear:** ```c - return HRTIMER_RESTART; + return ret ? HRTIMER_RESTART : HRTIMER_NORESTART; ``` The commit message says `drm_crtc_handle_vblank()` "fails" when vblank is d= isabled, but `drm_crtc_handle_vblank()` returns `false` simply when the vbl= ank counter is not enabled =E2=80=94 not necessarily an error. There are le= gitimate scenarios where it might return false transiently without implying= the timer should permanently stop. This change could cause premature timer= termination in edge cases during enable/disable transitions. 3. **Targeting wrong tree / applicability concern:** The cover note says th= is targets "6.18.y" stable, but 6.18.y doesn't exist yet. The patch is writ= ten against old VKMS code that had a per-output `vblank_hrtimer`. In mainli= ne (drm-next), this code no longer exists =E2=80=94 the `vkms_output` struc= t no longer contains `vblank_hrtimer`, and `vkms_vblank_simulate()` / `vkms= _disable_vblank()` have been removed entirely. If this is truly for a stabl= e backport, it needs a `Fixes:` tag referencing the original commit that in= troduced the deadlock, and the submitter should specify exactly which stabl= e kernel versions are affected. 4. **Missing `Fixes:` tag:** A stable-targeted patch must have a `Fixes:` t= ag to identify which commit introduced the bug. Without it, the stable team= cannot determine which kernels need the fix. 5. **No `Cc: stable` in the commit itself:** While `stable@vger.kernel.org`= appears in the email Cc, the conventional `Cc: stable@vger.kernel.org` tra= iler is missing from the signed-off area. 6. **Incomplete deadlock resolution:** Even if `hrtimer_try_to_cancel()` re= turns `-1` and the timer callback is running, the callback could still be s= pinning on `vblank_time_lock` at that very moment. The patch assumes that a= fter `try_to_cancel` returns, Thread A proceeds and releases `vblank_time_l= ock`, allowing the callback to finish. This is correct for the specific ABB= A case described, but it means `vkms_disable_vblank()` returns **without th= e timer actually being cancelled**. The timer fires one more time, gets `HR= TIMER_NORESTART` from the `ret` check, and stops. During that window, vblan= k events could still be processed after the caller believes vblank is disab= led. The mainline fix handles this more robustly by zeroing the interval. **Recommendation:** If the submitter intends this for an older stable tree,= they should: - Add a `Fixes:` tag - Specify exactly which stable versions are affected - Add a flag/state check (similar to mainline's interval-clearing) rather t= han relying solely on `drm_crtc_handle_vblank()`'s return value - Consider whether a cleaner backport of the mainline `DRM_CRTC_VBLANK_TIME= R_FUNCS` fix is feasible instead of a partial workaround --- Generated by Claude Code Patch Reviewer