Re: [PATCH v3 09/11] gpu: nova-core: vbios: simplify setup_falcon_data

Alexandre Courbot
2026-04-29T14:28:54Z

On Tue Apr 21, 2026 at 5:20 PM JST, Eliot Courtney wrote:
> The code first computes `pmu_in_first_fwsec` or adjusts the offset and
> then uses it in a branch just once to get the correct source for the PMU
> table. This can be simplified to a single branch while also avoiding the
> mutation of `offset`. Also, adjust the code after this to keep the
> success case non-nested.
>
> Reviewed-by: Joel Fernandes <joelagnelf@nvidia.com>
> Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
> ---
> drivers/gpu/nova-core/vbios.rs | 59 +++++++++++++++++++-----------------------
> 1 file changed, 26 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
> index 0c0e0402e715..d71ff5de794f 100644
> --- a/drivers/gpu/nova-core/vbios.rs
> +++ b/drivers/gpu/nova-core/vbios.rs
> @@ -904,48 +904,41 @@ fn setup_falcon_data(
> pci_at_image: &PciAtBiosImage,
> first_fwsec: &FwSecBiosBuilder,
> ) -> Result {
> - let mut offset = pci_at_image.falcon_data_offset()?;
> - let mut pmu_in_first_fwsec = false;
> + let offset = pci_at_image.falcon_data_offset()?;
>
> - // The offset is now from the start of the first Fwsec image, however
> - // the offset points to a location in the second Fwsec image. Since
> - // the fwsec images are contiguous, subtract the length of the first Fwsec
> - // image from the offset to get the offset to the start of the second
> - // Fwsec image.
> - if offset < first_fwsec.base.data.len() {
> - pmu_in_first_fwsec = true;
> + // The offset is from the start of the first FwSec image, but it
> + // may point into the second FwSec image. Treat the two FwSec images
> + // as contiguous here and subtract the first image length when the
> + // target lies in the second one.
> + let pmu_lookup_data = if offset < first_fwsec.base.data.len() {
> + first_fwsec.base.data.get(offset..)
> } else {
> - offset -= first_fwsec.base.data.len();
> - }
> -
> - let pmu_lookup_data = if pmu_in_first_fwsec {
> - &first_fwsec.base.data[offset..]
> - } else {
> - self.base.data.get(offset..).ok_or(EINVAL)?
> + self.base.data.get(offset - first_fwsec.base.data.len()..)
> };
> - let pmu_lookup_table = PmuLookupTable::new(&self.base.dev, pmu_lookup_data)?;
>
> - match pmu_lookup_table.find_entry_by_type(FALCON_UCODE_ENTRY_APPID_FWSEC_PROD) {
> - Ok(entry) => {
> - self.falcon_ucode_offset = Some(
> - usize::from_safe_cast(entry.data)
> - .checked_sub(pci_at_image.base.data.len())
> - .and_then(|o| o.checked_sub(first_fwsec.base.data.len()))
> - .ok_or(EINVAL)
> - .inspect_err(|_| {
> - dev_err!(self.base.dev, "Falcon Ucode offset not in second Fwsec.\n");
> - })?,
> - );
> - }
> - Err(e) => {
> + let pmu_lookup_table = pmu_lookup_data
> + .ok_or(EINVAL)
> + .and_then(|data| PmuLookupTable::new(&self.base.dev, data))?;

It looks a bit weird to check the result of the previous statement in
this one. How about:

let pmu_lookup_data = if offset < first_fwsec.base.data.len() {
...
}
.ok_or(EINVAL);

let pmu_lookup_table = PmuLookupTable::new(&self.base.dev, pmu_lookup_data)?;

> +
> + let entry = pmu_lookup_table
> + .find_entry_by_type(FALCON_UCODE_ENTRY_APPID_FWSEC_PROD)

It doesn't necessarily need to be addressed in this series, but if you
feel like it I believe there is also potential to use `FromBytes` to
create `PmuLookupTableEntry` instead of building it byte-by-byte.