From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: gpu: nova: rename drivers to use kebab-case convention Date: Wed, 11 Mar 2026 13:37:59 +1000 Message-ID: In-Reply-To: <20260310062443.2763230-1-yphbchou0911@gmail.com> References: <20260310062443.2763230-1-yphbchou0911@gmail.com> <20260310062443.2763230-1-yphbchou0911@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 **Positive aspects:** - The change is well-motivated =E2=80=94 fixing naming before UAPI exposure= is the right time. - The three modified files are internally consistent: `NOVA_CORE_MODULE_NAM= E` in `driver.rs:39` is changed to `c"nova-core"`, which matches the `name:= "nova-core"` in `nova_core.rs:25`. This is critical because `NOVA_CORE_MOD= ULE_NAME` is used to construct the auxiliary device table entry at `driver.= rs:47`: ```rust auxiliary::DeviceId::new(NOVA_CORE_MODULE_NAME, AUXILIARY_NAME), ``` and must match the actual module name used when registering auxiliary dev= ices at `driver.rs:92`: ```rust auxiliary::Registration::new(pdev.as_ref(), c"nova-drm", 0, crate::MODULE= _NAME) ``` where `MODULE_NAME` derives from the `name` field in `module_pci_driver!`. **Issue 1: DRM driver `name` vs module `name` confusion** The patch changes the DRM `DriverInfo::name` field from `"nova"` to `"nova-= drm"`: ```rust - name: c"nova", + name: c"nova-drm", ``` This is the DRM driver name exposed to userspace via `/sys/class/drm/cardN/= device/driver/name` and through the `DRM_IOCTL_VERSION` ioctl. This is a di= fferent concept from the kernel module name. The DRM driver name `"nova"` s= eems intentionally distinct from the module name =E2=80=94 other DRM driver= s don't append `-drm` to their DRM name (e.g., `amdgpu` the module name and= `amdgpu` the DRM name, not `amdgpu-drm`). Changing this could break usersp= ace tools that use the DRM driver name. The commit message doesn't discuss = this change specifically, suggesting it may be unintentional conflation of = the two different "name" concepts. This change should likely be dropped or = explicitly justified. **Issue 2: Module `name` in `module_auxiliary_driver!`** The change from `"Nova"` to `"nova-drm"` in `nova.rs`: ```rust - name: "Nova", + name: "nova-drm", ``` This is the kernel module name for the nova DRM module. The actual `.ko` fi= le is `nova.ko` (based on the Kconfig/Makefile naming `CONFIG_DRM_NOVA`). T= he `name` field in `module_*_driver!` macros in Rust maps to the `MODULE_NA= ME` / module metadata. Having the module name be `"nova-drm"` when the `.ko= ` is `nova.ko` could create confusion. It would be worth verifying whether = this should be `"nova"` (matching the `.ko` filename) or `"nova-drm"` (matc= hing the auxiliary device name), and confirming that the Kconfig/Makefile e= ntries align. The convention for kernel modules is typically to match the `= .ko` name. **Summary:** The nova-core rename from `"NovaCore"` to `"nova-core"` is cle= arly correct. The nova-drm module name change is reasonable but the DRM `Dr= iverInfo::name` change from `"nova"` to `"nova-drm"` looks wrong =E2=80=94 = this is a userspace-visible DRM driver name that should remain `"nova"`. Th= e patch should be split or the DRM driver name change should be reverted. --- Generated by Claude Code Patch Reviewer