Claude review: drm/vkms: Fix ABBA deadlock in vblank disable and timer callback

[Claude Code Review Bot <claude-review@example.com>]

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()` → `hrtimer_try_to_cancel()` change is the right direction, matching the approach that mainline adopted in `drm_crtc_vblank_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 executing, meaning the timer was **not** cancelled. The patch relies entirely on the timer self-stopping via `HRTIMER_NORESTART`, but there is a race window: the callback could be between `drm_crtc_handle_vblank()` returning true and evaluating `ret` for the return value. In that window, `ret` is true, so the timer re-arms itself even though vblank was just disabled. Compare with mainline's `drm_crtc_vblank_cancel_timer()` which clears the timer interval to zero under a spinlock as a definitive cancellation signal, then calls `hrtimer_try_to_cancel()`:

```c
spin_lock_irqsave(&vtimer->interval_lock, flags);
vtimer->interval = 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 disabled, but `drm_crtc_handle_vblank()` returns `false` simply when the vblank counter is not enabled — not necessarily an error. There are legitimate 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 this targets "6.18.y" stable, but 6.18.y doesn't exist yet. The patch is written against old VKMS code that had a per-output `vblank_hrtimer`. In mainline (drm-next), this code no longer exists — the `vkms_output` struct no longer contains `vblank_hrtimer`, and `vkms_vblank_simulate()` / `vkms_disable_vblank()` have been removed entirely. If this is truly for a stable backport, it needs a `Fixes:` tag referencing the original commit that introduced the deadlock, and the submitter should specify exactly which stable kernel versions are affected.

4. **Missing `Fixes:` tag:** A stable-targeted patch must have a `Fixes:` tag 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` trailer is missing from the signed-off area.

6. **Incomplete deadlock resolution:** Even if `hrtimer_try_to_cancel()` returns `-1` and the timer callback is running, the callback could still be spinning on `vblank_time_lock` at that very moment. The patch assumes that after `try_to_cancel` returns, Thread A proceeds and releases `vblank_time_lock`, allowing the callback to finish. This is correct for the specific ABBA case described, but it means `vkms_disable_vblank()` returns **without the timer actually being cancelled**. The timer fires one more time, gets `HRTIMER_NORESTART` from the `ret` check, and stops. During that window, vblank events could still be processed after the caller believes vblank is disabled. 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 than relying solely on `drm_crtc_handle_vblank()`'s return value
- Consider whether a cleaner backport of the mainline `DRM_CRTC_VBLANK_TIMER_FUNCS` fix is feasible instead of a partial workaround

---
Generated by Claude Code Patch Reviewer