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 17:35:10 +1000 Message-ID: In-Reply-To: <20260523154121.147103-1-julianbraha@gmail.com> References: <20260523154121.147103-1-julianbraha@gmail.com> <20260523154121.147103-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 **i915 Kconfig changes =E2=80=94 Correct** The two changes in `drivers/gpu/drm/i915/Kconfig.debug` are logically sound: 1. Changing `DRM_I915_SW_FENCE_DEBUG_OBJECTS` from `select DEBUG_OBJECTS` t= o `depends on DEBUG_OBJECTS` (line 134=E2=86=92157) is the right fix for th= e select-visible anti-pattern. 2. Adding `depends on DEBUG_OBJECTS` to `DRM_I915_DEBUG` (line 125=E2=86=92= 38) is necessary because `DRM_I915_DEBUG` selects `DRM_I915_SW_FENCE_DEBUG_= OBJECTS` (line 63), which now depends on `DEBUG_OBJECTS`. Without this, Kco= nfig would select a symbol whose dependency isn't met. These two changes are consistent: when `DRM_I915_DEBUG=3Dy`, `DEBUG_OBJECTS= ` must be on (new depends), so the selected `DRM_I915_SW_FENCE_DEBUG_OBJECT= S` has its `DEBUG_OBJECTS` dependency satisfied. **lib/Kconfig.debug change =E2=80=94 Bug: inverted condition** The change to `DEBUG_OBJECTS` in `lib/Kconfig.debug` has the condition back= wards: ``` - depends on PREEMPT_COUNT || !DEFERRED_STRUCT_PAGE_INIT + select PREEMPT_COUNT if !DEFERRED_STRUCT_PAGE_INIT ``` The original `depends on` expression means: `DEBUG_OBJECTS` requires `PREEM= PT_COUNT` **when `DEFERRED_STRUCT_PAGE_INIT=3Dy`**. When `DEFERRED_STRUCT_P= AGE_INIT=3Dn`, `PREEMPT_COUNT` is not needed (the `!DEFERRED_STRUCT_PAGE_IN= IT` clause satisfies the dependency). The replacement `select PREEMPT_COUNT if !DEFERRED_STRUCT_PAGE_INIT` does t= he opposite: - When `DEFERRED_STRUCT_PAGE_INIT=3Dn`: forces `PREEMPT_COUNT=3Dy` =E2=80= =94 **unnecessary**, the original constraint was already satisfied without = it. - When `DEFERRED_STRUCT_PAGE_INIT=3Dy`: does nothing =E2=80=94 **the danger= ous case is left unguarded**. `DEBUG_OBJECTS` can now be enabled with `PREE= MPT_COUNT=3Dn` and `DEFERRED_STRUCT_PAGE_INIT=3Dy`, a configuration the ori= ginal dependency explicitly prohibited. The correct replacement would be: ``` select PREEMPT_COUNT if DEFERRED_STRUCT_PAGE_INIT ``` This ensures that when `DEFERRED_STRUCT_PAGE_INIT=3Dy`, `PREEMPT_COUNT` is = force-enabled, which is the constraint the original `depends on` was enforc= ing. **Scope concern: modifying core kernel Kconfig from a driver patch** The `lib/Kconfig.debug` change modifies `DEBUG_OBJECTS`, a core kernel debu= g option that affects far more than i915. The patch is CC'd only to i915/DR= M maintainers and mailing lists. This change should be reviewed by the main= tainers of `lib/Kconfig.debug` (likely Andrew Morton / the debug infrastruc= ture maintainers). At minimum it needs a CC to `linux-kernel@vger.kernel.or= g` with relevant subsystem maintainers, and ideally should be a separate pa= tch with its own justification rather than bundled into an i915 fix. **Why the `select` approach is needed at all** The commit message says "select PREEMPT_COUNT, avoid recursive dependency."= The underlying issue is real: `PREEMPT_COUNT` is a prompt-less bool (in `k= ernel/Kconfig.preempt`), so it can only be enabled via `select`. With the o= riginal `depends on PREEMPT_COUNT` in `DEBUG_OBJECTS`, if nothing else in t= he config happens to select `PREEMPT_COUNT`, then `DEBUG_OBJECTS` becomes u= navailable. Converting the dependency to `select` is a reasonable approach = =E2=80=94 but the condition must be correct. **Minor: `default n` is unnecessary** ``` depends on DEBUG_OBJECTS default n ``` In Kconfig, `bool` options default to `n` implicitly. The `default n` on `D= RM_I915_SW_FENCE_DEBUG_OBJECTS` (line 158) is redundant. This is pre-existi= ng and not introduced by this patch, but could be cleaned up opportunistica= lly. **Summary of required changes:** 1. Fix the inverted condition: `select PREEMPT_COUNT if DEFERRED_STRUCT_PAG= E_INIT` (not `if !DEFERRED_STRUCT_PAGE_INIT`) 2. Consider splitting the `lib/Kconfig.debug` change into a separate patch = with appropriate CC to debug infrastructure maintainers 3. Test the resulting config to verify `DEBUG_OBJECTS` remains selectable i= n all relevant configurations --- Generated by Claude Code Patch Reviewer