From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: Re: [PATCH] drm/vmwgfx: Fix hrtimer interrupt storm due to 0-period vblank Date: Mon, 25 May 2026 18:01:52 +1000 Message-ID: In-Reply-To: References: <20260518071741.441794-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 **This is the active patch for review.** **Correctness:** The fix is in the right place. After `drm_calc_timestampin= g_constants()` stores `framedur_ns` at drm_vblank.c:673, the check at the p= roposed insertion point catches the zero value before it reaches: ```c vtimer->interval =3D ns_to_ktime(vblank->framedur_ns); ``` and the subsequent `hrtimer_start()`. Returning `-EINVAL` prevents the time= r from being armed with a zero period. **Issues:** 1. **Missing `Fixes:` tag and `Cc: stable`.** The v1 had `Fixes: cd2eb57df1= b8 ("drm/vmwgfx: Implement virtual kms")` and `Cc: stable@vger.kernel.org` = in the commit message, but the v2 dropped both. The `Fixes:` tag should be = updated to reference the commit that introduced `drm_crtc_vblank_start_time= r()` (since that's where the bug now lives), and `Cc: stable` should be ret= ained for backport eligibility. The `stable@` address is in the email Cc bu= t not in the commit message trailers =E2=80=94 maintainers and tooling look= at the trailers. 2. **No diagnostic logging.** When the check triggers and returns `-EINVAL`= , there's no log message. A `drm_dbg()` or `drm_err()` would help developer= s diagnose why vblank enablement silently failed. For example: ```c if (unlikely(vblank->framedur_ns =3D=3D 0)) { drm_err(crtc->dev, "crtc %u: refusing 0-ns frame duration\n", crtc->base.id); return -EINVAL; } ``` 3. **Comment is overly verbose and uses non-standard labeling.** The `/* DE= FENSIVE CHECK: ... */` block is 6 lines of comment for 2 lines of code. Ker= nel style prefers terse comments. Something like: ```c /* Reject modes where integer truncation yields a 0-ns frame duration. */ ``` would suffice. 4. **Consider whether `drm_calc_timestamping_constants()` itself should be = hardened.** That function is also called from other paths (e.g., `drm_updat= e_vblank_count()` via `drm_crtc_vblank_helper_get_vblank_timestamp_internal= `). A `framedur_ns` of 0 stored in `vblank->framedur_ns` could potentially = cause division-by-zero or similar issues in other consumers. Clamping to a = minimum of 1 ns in `drm_calc_timestamping_constants()` itself (or better, e= nforcing a sane minimum frame duration) would be more robust. However, that= 's arguably a separate follow-up concern and this patch is valid as a targe= ted fix. 5. **Timer state on error path.** If this is the first call (the `!vtimer->= crtc` branch was taken), the timer data structures have been initialized bu= t the function returns an error. On a subsequent call, the `else` branch wi= ll execute `hrtimer_try_to_cancel()` on a timer that was never started. Thi= s should be harmless since `hrtimer_setup()` initializes the timer to an in= active state, but it's worth verifying there are no side effects from the p= artially-initialized `vtimer` (e.g., `vtimer->crtc` is now set, so the init= branch won't run again). **Minor:** The `unlikely()` annotation is fine =E2=80=94 this is genuinely = an exceptional path. **Verdict:** The approach is sound and fixes a real vulnerability. Needs th= e `Fixes:` tag updated for the correct commit, `Cc: stable` re-added to tra= ilers, a log message on the error path, and the comment trimmed. With those= addressed, this should be ready. --- Generated by Claude Code Patch Reviewer