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-core: add Architecture::uses_sec2() helper Date: Sun, 12 Apr 2026 11:19:37 +1000 Message-ID: In-Reply-To: <20260409-b4-blackwell-unload-v1-2-0f5a2ff838dd@nvidia.com> References: <20260409-b4-blackwell-unload-v1-0-0f5a2ff838dd@nvidia.com> <20260409-b4-blackwell-unload-v1-2-0f5a2ff838dd@nvidia.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 **Purpose:** Extract the `matches!(self, Turing | Ampere | Ada)` check into= a named method on `Architecture`, so it can be reused across boot and unlo= ad paths. ```rust + /// Returns `true` if this architecture uses SEC2 to boot GSP. + /// + /// Turing/Ampere/Ada use FWSEC + SEC2 booter firmware. Hopper and lat= er use FSP instead. + pub(crate) const fn uses_sec2(&self) -> bool { + matches!(self, Self::Turing | Self::Ampere | Self::Ada) + } ``` And in `boot.rs`: ```rust - let uses_sec2 =3D matches!( - chipset.arch(), - Architecture::Turing | Architecture::Ampere | Architecture::Ada - ); + let uses_sec2 =3D chipset.arch().uses_sec2(); ``` **Observations:** - **Good refactoring**: The doc comment clearly explains the hardware seman= tics. This is the right abstraction =E2=80=94 it names the concept ("uses S= EC2") rather than just listing architectures. - **`const fn` with `&self`**: This works in modern Rust and is appropriate= for a pure pattern match on an enum. - **Negative-logic consideration**: The method defines the "old" path posit= ively. An alternative would be something like `uses_fsp()` for the Hopper+ = path, but `uses_sec2()` is arguably better since the SEC2 path is the one w= ith more complex steps (FWSEC-SB, booter unloader), so guarding that behind= the check is natural. - **Future architectures**: When new architectures are added (e.g., Rubin),= they will naturally not match `Turing | Ampere | Ada`, so `uses_sec2()` wi= ll return `false`. This is the correct default for Hopper-and-later GPUs. T= he exhaustive-match pattern in `Chipset::arch()` will force explicit handli= ng when new chipsets are added. **Verdict:** Clean, correct refactoring. No issues. --- --- Generated by Claude Code Patch Reviewer