From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drivers/gpu/drm/drm_print: fix drm_printer dynamic debug bypass Date: Tue, 10 Mar 2026 12:46:56 +1000 Message-ID: In-Reply-To: <20260308223538.96729-3-jim.cromie@gmail.com> References: <20260308223538.96729-1-jim.cromie@gmail.com> <20260308223538.96729-3-jim.cromie@gmail.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 **Problem analysis**: Correct and well-explained. Under `CONFIG_DRM_USE_DYN= AMIC_DEBUG=3DY`, `__drm_debug_enabled(category)` expands to `true` (confirm= ed at `linux/include/drm/drm_print.h:161`). Since `__drm_printfn_dbg()` is = a shared helper not behind a dyndbg static-key guard, using `__drm_debug_en= abled()` means the check is always bypassed, causing all debug output to pr= int. **First hunk** (line 474-475): ```c - if (!__drm_debug_enabled(category)) + if (!drm_debug_enabled_instrumented(category)) ``` **Concern**: `drm_debug_enabled_instrumented()` includes a `pr_debug("todo:= is this frequent enough to optimize ?\n")` call (confirmed at `linux/inclu= de/drm/drm_print.h:149-153`). This is a debugging/instrumentation macro tha= t was clearly intended for development purposes =E2=80=94 it emits a `pr_de= bug` on every invocation. While the commit message acknowledges this ("It a= lso adds a pr_debug() to help track the frequency of this bit-test"), using= an instrumentation/development macro in production code is questionable. T= his `pr_debug` could itself become a performance issue if someone enables i= t, and the "todo:" text in the pr_debug string suggests it's not meant for = permanent use. **Recommendation**: Use `drm_debug_enabled()` here instead. Under `CONFIG_D= RM_USE_DYNAMIC_DEBUG=3DY`, `drm_debug_enabled()` is already defined as `drm= _debug_enabled_instrumented()` (line 162), but under the `!CONFIG_DRM_USE_D= YNAMIC_DEBUG` path, it maps to `drm_debug_enabled_raw()` which is the simpl= e bitmask check without the pr_debug overhead. However, the real fix for `_= _drm_printfn_dbg()` is just to use `drm_debug_enabled()` =E2=80=94 both pat= hs do a real bitmask check, which is exactly what's needed here. Actually, looking more carefully: `drm_debug_enabled()` under `CONFIG_DRM_U= SE_DYNAMIC_DEBUG=3DY` already equals `drm_debug_enabled_instrumented()`, so= using `drm_debug_enabled()` would give the same result but is the canonica= l API. Using `drm_debug_enabled_instrumented()` directly bypasses the abstr= action and couples the code to an internal implementation detail. **Second hunk** (line 483-484): ```c - if (!__drm_debug_enabled(category)) + if (!drm_debug_enabled(category)) ``` This change to `__drm_dev_dbg()` is correct and harmless. As the commit mes= sage notes, `__drm_dev_dbg()` is wrapped by dyndbg so the `__drm_debug_enab= led()` =E2=86=92 `true` shortcut is actually valid there. But switching to = `drm_debug_enabled()` adds a redundant (but cheap) bitmask check and is cle= aner. Fine as-is. **Overall for patch 2**: The fix is correct in principle but the first hunk= should use `drm_debug_enabled()` rather than `drm_debug_enabled_instrument= ed()`. The `_instrumented` variant with its embedded `pr_debug("todo: ...")= ` is clearly a temporary diagnostic tool, not something that should be bake= d into production code paths. --- Generated by Claude Code Patch Reviewer