From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: i915: Fix NULL pointer dereference in intel_dmc_update_dc6_allowed_count() Date: Tue, 03 Mar 2026 14:19:14 +1000 Message-ID: In-Reply-To: <20260228130946.50919-2-ltao@redhat.com> References: <20260228130946.50919-2-ltao@redhat.com> <20260228130946.50919-2-ltao@redhat.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 **Correctness: The fix is correct.** The NULL guard before the dereference = is the right approach, and early-returning when `dmc` is NULL is the correc= t behavior =E2=80=94 if DMC firmware isn't loaded, there's no DC6 tracking = state to update. ```c - if (DISPLAY_VER(dmc->display) < 14) + if (!dmc || DISPLAY_VER(dmc->display) < 14) ``` **Minor suggestions:** 1. **Missing `Fixes:` tag.** A crash fix like this should include a `Fixes:= ` tag pointing to the commit that introduced `intel_dmc_update_dc6_allowed_= count()`. This helps backport tracking. 2. **Missing `Cc: stable@vger.kernel.org`.** Since this is a NULL dereferen= ce panic that prevents kdump from generating vmcores, it's a candidate for = stable backports. 3. **Style consideration:** The version check uses `dmc->display` when the = function already receives `display` as a parameter (and `dmc->display` is t= he same pointer =E2=80=94 set during `intel_dmc_init()`). The function coul= d use `display` directly for the version check and keep the `dmc` NULL chec= k separate for clarity: ```c if (!dmc || DISPLAY_VER(display) < 14) return; ``` This makes the two conditions logically distinct: "is DMC available?" vs= . "is the display version new enough?" =E2=80=94 and avoids dereferencing `= dmc` unnecessarily for the version check. However, this is a minor style po= int and the patch as-is is functionally correct. 4. **Broader observation:** The subagent research found several other funct= ions in `intel_dmc.c` that also dereference `dmc` without a NULL check (e.g= ., `dmc_load_mmio()`, `dmc_load_program()`, `dmc_configure_event()`). Those= are likely safe because they're only called from firmware-loading paths wh= ere `dmc` is known non-NULL, but the inconsistency in NULL-checking pattern= s across the file is worth noting for future hardening. **Overall: The patch is a valid and minimal crash fix. Recommend adding `Fi= xes:` and `Cc: stable` tags.** --- Generated by Claude Code Patch Reviewer