Re: [PATCH v4 14/20] gpu: nova-core: vbios: construct `FwSecBiosImage` directly from BIOS images

[John Hubbard]

On 5/18/26 7:55 PM, Eliot Courtney wrote:
> `FwSecBiosBuilder` now only contains `falcon_ucode_offset` which just
> gets passed directly into `FwSecBiosImage`. Remove `FwSecBiosBuilder`
> and construct `FwSecBiosImage` directly, as a simplification.
> 
> Reviewed-by: Joel Fernandes <joelagnelf@xxxxxxxxxx>
> Signed-off-by: Eliot Courtney <ecourtney@xxxxxxxxxx>
> ---
>  drivers/gpu/nova-core/vbios.rs | 98 +++++++++++++++++-------------------------
>  1 file changed, 39 insertions(+), 59 deletions(-)
...
> -impl FwSecBiosBuilder {
> -    fn setup_falcon_data(
> -        &mut self,
> -        pci_at_image: &PciAtBiosImage,
> -        first_fwsec: &FwSecBiosBuilder,
> -    ) -> Result {
> +impl FwSecBiosImage {
> +    /// Build the final `FwSecBiosImage` from the PCI-AT and FWSEC BIOS images

This needs a period at the end of the sentence.

The patch looks good, I had to look pretty hard in order find that
tiny nit. :)

Reviewed-by: John Hubbard <jhubbard@xxxxxxxxxx>

thanks,
-- 
John Hubbard

> +    fn new(
> +        pci_at_image: PciAtBiosImage,
> +        first_fwsec: BiosImage,
> +        second_fwsec: BiosImage,
> +    ) -> Result<FwSecBiosImage> {
>          let offset = pci_at_image.falcon_data_offset()?;
>  
>          // 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..)
> +        let pmu_lookup_data = if offset < first_fwsec.data.len() {
> +            first_fwsec.data.get(offset..)
>          } else {
> -            self.base.data.get(offset - first_fwsec.base.data.len()..)
> +            second_fwsec.data.get(offset - first_fwsec.data.len()..)
>          }
>          .ok_or(EINVAL)?;
>  
> -        let pmu_lookup_table = PmuLookupTable::new(&self.base.dev, pmu_lookup_data)?;
> +        let pmu_lookup_table = PmuLookupTable::new(&second_fwsec.dev, pmu_lookup_data)?;
>  
>          let entry = pmu_lookup_table
>              .find_entry_by_type(FALCON_UCODE_ENTRY_APPID_FWSEC_PROD)
>              .inspect_err(|e| {
>                  dev_err!(
> -                    self.base.dev,
> +                    second_fwsec.dev,
>                      "PmuLookupTableEntry not found, error: {:?}\n",
>                      e
>                  );
> @@ -987,34 +980,21 @@ fn setup_falcon_data(
>  
>          let falcon_ucode_offset = 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()))
> +            .and_then(|o| o.checked_sub(first_fwsec.data.len()))
>              .ok_or(EINVAL)
>              .inspect_err(|_| {
> -                dev_err!(self.base.dev, "Falcon Ucode offset not in second Fwsec.\n");
> +                dev_err!(
> +                    second_fwsec.dev,
> +                    "Falcon Ucode offset not in second Fwsec.\n"
> +                );
>              })?;
>  
> -        self.falcon_ucode_offset = Some(falcon_ucode_offset);
> -        Ok(())
> +        Ok(FwSecBiosImage {
> +            base: second_fwsec,
> +            falcon_ucode_offset,
> +        })
>      }
>  
> -    /// Build the final FwSecBiosImage from this builder
> -    fn build(self) -> Result<FwSecBiosImage> {
> -        let ret = FwSecBiosImage {
> -            base: self.base,
> -            falcon_ucode_offset: self.falcon_ucode_offset.ok_or(EINVAL)?,
> -        };
> -
> -        if cfg!(debug_assertions) {
> -            // Print the desc header for debugging
> -            let desc = ret.header()?;
> -            dev_dbg!(ret.base.dev, "PmuLookupTableEntry desc: {:#?}\n", desc);
> -        }
> -
> -        Ok(ret)
> -    }
> -}
> -
> -impl FwSecBiosImage {
>      /// Get the FwSec header ([`FalconUCodeDesc`]).
>      pub(crate) fn header(&self) -> Result<FalconUCodeDesc> {
>          let data = self
>