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: Make CONFIG_RUST_DRM_GEM_SHMEM_HELPER selectable from menuconfig Date: Tue, 28 Apr 2026 15:21:52 +1000 Message-ID: In-Reply-To: <20260426082234.2026908-3-mkchauras@gmail.com> References: <20260426082234.2026908-1-mkchauras@gmail.com> <20260426082234.2026908-3-mkchauras@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 **Verdict: Adding `RUST` dependency is correct. Making it user-selectable i= s questionable.** The change: ```diff config RUST_DRM_GEM_SHMEM_HELPER - bool - depends on DRM && MMU + bool "Rust GEM shmem helper functions" + depends on DRM && MMU && RUST ``` **Adding `depends on RUST`** =E2=80=94 This is a correct fix. Rust code obv= iously cannot build without `CONFIG_RUST`. Good catch. **Adding the description string to make it user-selectable** =E2=80=94 This= is problematic. The C counterpart `DRM_GEM_SHMEM_HELPER` is a hidden `tris= tate` with no description (line 262 of Kconfig: `tristate` with no prompt s= tring). It follows the standard kernel pattern where helper/library Kconfig= options are hidden and get enabled via `select` from the drivers that need= them. Users don't manually enable helper code =E2=80=94 drivers pull in wh= at they need. `RUST_DRM_GEM_SHMEM_HELPER` was designed to follow this same pattern: a hid= den `bool` that a Rust DRM driver would `select`. Currently nothing selects= it (no Rust DRM drivers in-tree yet), so making it user-selectable is pres= umably to allow testing. But this should be documented in the commit messag= e =E2=80=94 the current message doesn't explain *why* a user would want to = manually enable a helper library that no driver uses. If the intent is just testability, a better approach might be to keep it hi= dden and add a `RUST_DRM_KUNIT_TEST` or similar config that selects it, fol= lowing the pattern used elsewhere in the kernel for testing infrastructure = code. **Additional concern:** The commit message says "Along side it's not depend= ent on rust" =E2=80=94 this is grammatically unclear and should be "Additio= nally, it is not dependent on RUST" or similar. Minor, but commit messages = are permanent documentation. **Commit message nit:** The cover letter says "making it selectable from me= nuconfig" but doesn't explain the use case for a user manually enabling a h= elper library with no consumers. --- Generated by Claude Code Patch Reviewer