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/i915: use 'depends on' with visible DEBUG_OBJECTS for DRM_I915_DEBUG and DRM_I915_SW_FENCE_DEBUG_OBJECTS Date: Mon, 25 May 2026 20:01:46 +1000 Message-ID: In-Reply-To: <20260521144518.410717-1-julianbraha@gmail.com> References: <20260521144518.410717-1-julianbraha@gmail.com> <20260521144518.410717-1-julianbraha@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 **Issue 1 (Major): Adding `depends on DEBUG_OBJECTS` to `DRM_I915_DEBUG` is= the wrong fix and degrades usability** ```diff config DRM_I915_DEBUG bool "Enable additional driver debugging" + depends on DEBUG_OBJECTS depends on DRM_I915 ``` `DRM_I915_DEBUG` is the umbrella debugging option that developers enable as= a single switch to turn on all i915 debugging. By adding `depends on DEBUG= _OBJECTS`, the option will not even be *visible* in menuconfig unless `DEBU= G_OBJECTS` (and its dependency `DEBUG_KERNEL`) are already enabled. This ma= kes it significantly less discoverable and more cumbersome to use. The current tree has `DRM_I915_DEBUG` selecting `DRM_I915_SW_FENCE_DEBUG_OB= JECTS`, which in turn selects `DEBUG_OBJECTS`. The "unmet dependency" the c= ommit message references (from a previous patch version) was a problem with= the *previous* patch's approach, not with the current tree. In the current= tree, the `select` chain (`DRM_I915_DEBUG` =E2=86=92 `select DRM_I915_SW_F= ENCE_DEBUG_OBJECTS` =E2=86=92 `select DEBUG_OBJECTS`) works correctly =E2= =80=94 `DEBUG_OBJECTS` gets force-enabled transitively. If the goal is to stop `DRM_I915_SW_FENCE_DEBUG_OBJECTS` from selecting `DE= BUG_OBJECTS`, then `DRM_I915_DEBUG` should *also* `select DEBUG_OBJECTS` (n= ot `depends on` it), since `DRM_I915_DEBUG` already selects many other symb= ols and is designed as a "turn everything on" switch. Alternatively, `DRM_I= 915_DEBUG` could just add `select DEBUG_OBJECTS` directly to replace the tr= ansitive select that would be lost. **Issue 2 (Medium): Changing `select` to `depends on` for `DRM_I915_SW_FENC= E_DEBUG_OBJECTS` changes semantics** ```diff config DRM_I915_SW_FENCE_DEBUG_OBJECTS bool "Enable additional driver debugging for fence objects" depends on DRM_I915 - select DEBUG_OBJECTS + depends on DEBUG_OBJECTS ``` This is the core of the Kconfig anti-pattern fix and is directionally corre= ct =E2=80=94 `select`-ing a visible, user-configurable symbol that itself h= as dependencies (`DEBUG_KERNEL`, `PREEMPT_COUNT || !DEFERRED_STRUCT_PAGE_IN= IT`) can cause unmet dependency warnings. However, this change means that `= DRM_I915_SW_FENCE_DEBUG_OBJECTS` will now be hidden unless the user has sep= arately enabled `DEBUG_OBJECTS`. Previously, enabling `DRM_I915_SW_FENCE_DE= BUG_OBJECTS` would automatically pull in `DEBUG_OBJECTS`. This is a trade-off. The Kconfig community generally considers `depends on`= correct for visible symbols, so this half of the patch is defensible. But = the `DRM_I915_DEBUG` change above needs to compensate for this by using `se= lect DEBUG_OBJECTS` rather than `depends on DEBUG_OBJECTS`. **Issue 3 (Minor): Commit message formatting** The commit message uses "Link 1:" as a label, but the kernel convention is = either a `Link:` tag or `[1]` footnote reference. The commit message says "= A previous version of this patch[1]" with a `[1]` reference but then uses "= Link 1:" instead of a proper `[1]` footnote or `Link:` tag: ``` Link 1: https://lore.kernel.org/all/20260502191932.4491-1-julianbraha@gmail.com/ ``` This should be: ``` Link: https://lore.kernel.org/all/20260502191932.4491-1-julianbraha@gmail.c= om/ [1] ``` **Suggested fix**: Change `DRM_I915_DEBUG` to use `select DEBUG_OBJECTS` in= stead of `depends on DEBUG_OBJECTS`. This preserves the "one-switch" nature= of the umbrella debug option while still fixing the anti-pattern in `DRM_I= 915_SW_FENCE_DEBUG_OBJECTS`. The diff should look like: ``` config DRM_I915_DEBUG bool "Enable additional driver debugging" depends on DRM_I915 depends on EXPERT depends on !COMPILE_TEST select DEBUG_FS select PREEMPT_COUNT + select DEBUG_OBJECTS ... select DRM_I915_SW_FENCE_DEBUG_OBJECTS ``` This way, `DRM_I915_DEBUG` continues to be a self-contained developer switc= h, `DEBUG_OBJECTS` gets enabled when needed, and `DRM_I915_SW_FENCE_DEBUG_O= BJECTS` no longer misuses `select` on a visible symbol. --- Generated by Claude Code Patch Reviewer