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: vbios: simplify setup_falcon_data Date: Thu, 23 Apr 2026 08:42:25 +1000 Message-ID: In-Reply-To: <20260421-fix-vbios-v3-9-8f648aef7a85@nvidia.com> References: <20260421-fix-vbios-v3-0-8f648aef7a85@nvidia.com> <20260421-fix-vbios-v3-9-8f648aef7a85@nvidia.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit MIME-Version: 1.0 Patch Review Good simplification. Eliminates the mutable `offset` and `pmu_in_first_fwsec` flag by computing `pmu_lookup_data` directly in a single branch. The success path is un-nested. **Concern:** In the `else` branch: ```rust self.base.data.get(offset - first_fwsec.base.data.len()..) ``` This subtraction `offset - first_fwsec.base.data.len()` is unchecked. The `else` arm is entered when `offset >= first_fwsec.base.data.len()`, so this cannot underflow. However, the earlier patches in this series were specifically about hardening against firmware-derived values, and `offset` is firmware-derived. The condition `offset >= first_fwsec.base.data.len()` does guarantee no underflow, so this is technically safe, but using `checked_sub` would be more consistent with the spirit of the series. This is a minor nit since the `.get()` after the subtraction still prevents OOB reads. Also note this `else` branch uses `self.base.data` which is the second FwSec builder's data -- this gets updated to `second_fwsec.data` in patch 10 which is fine. --- Generated by Claude Code Patch Reviewer