On Fri, Aug 08, 2025 at 11:46:42AM +0900, Alexandre Courbot wrote:
> Now that the vbios code uses a non-bound `Device` instance, store an
> `ARef` to it at construction time so we can use it for logging without
> having to carry an extra argument on every method for that sole purpose.
>
> Signed-off-by: Alexandre Courbot <acour...@nvidia.com>
Reviewed-by: Joel Fernandes <joelagn...@nvidia.com>
thanks,
- Joel
> ---
> drivers/gpu/nova-core/firmware/fwsec.rs | 8 ++--
> drivers/gpu/nova-core/vbios.rs | 69
> ++++++++++++++++++++-------------
> 2 files changed, 46 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/gpu/nova-core/firmware/fwsec.rs
> b/drivers/gpu/nova-core/firmware/fwsec.rs
> index
> 0dff3cfa90afee0cd4c3348023c8bfd7edccdb29..d9b9d1f92880cbcd36dac84b9e86a84e6465cf5d
> 100644
> --- a/drivers/gpu/nova-core/firmware/fwsec.rs
> +++ b/drivers/gpu/nova-core/firmware/fwsec.rs
> @@ -253,8 +253,8 @@ impl FalconFirmware for FwsecFirmware {
>
> impl FirmwareDmaObject<FwsecFirmware, Unsigned> {
> fn new_fwsec(dev: &Device<device::Bound>, bios: &Vbios, cmd:
> FwsecCommand) -> Result<Self> {
> - let desc = bios.fwsec_image().header(dev)?;
> - let ucode = bios.fwsec_image().ucode(dev, desc)?;
> + let desc = bios.fwsec_image().header()?;
> + let ucode = bios.fwsec_image().ucode(desc)?;
> let mut dma_object = DmaObject::from_data(dev, ucode)?;
>
> let hdr_offset = (desc.imem_load_size + desc.interface_offset) as
> usize;
> @@ -343,7 +343,7 @@ pub(crate) fn new(
> let ucode_dma = FirmwareDmaObject::<Self, _>::new_fwsec(dev, bios,
> cmd)?;
>
> // Patch signature if needed.
> - let desc = bios.fwsec_image().header(dev)?;
> + let desc = bios.fwsec_image().header()?;
> let ucode_signed = if desc.signature_count != 0 {
> let sig_base_img = (desc.imem_load_size + desc.pkc_data_offset)
> as usize;
> let desc_sig_versions = u32::from(desc.signature_versions);
> @@ -382,7 +382,7 @@ pub(crate) fn new(
> dev_dbg!(dev, "patching signature with index {}\n",
> signature_idx);
> let signature = bios
> .fwsec_image()
> - .sigs(dev, desc)
> + .sigs(desc)
> .and_then(|sigs| sigs.get(signature_idx).ok_or(EINVAL))?;
>
> ucode_dma.patch_signature(signature, sig_base_img)?
> diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
> index
> b5564b4d3e4758e77178aa403635e4780f0378cc..6fc06b1b83655a7dec00308880dbdfc32d7105ce
> 100644
> --- a/drivers/gpu/nova-core/vbios.rs
> +++ b/drivers/gpu/nova-core/vbios.rs
> @@ -9,6 +9,7 @@
> use kernel::device;
> use kernel::error::Result;
> use kernel::prelude::*;
> +use kernel::types::ARef;
>
> /// The offset of the VBIOS ROM in the BAR0 space.
> const ROM_OFFSET: usize = 0x300000;
> @@ -230,10 +231,10 @@ pub(crate) fn new(dev: &device::Device, bar0: &Bar0) ->
> Result<Vbios> {
> (second_fwsec_image, first_fwsec_image, pci_at_image)
> {
> second
> - .setup_falcon_data(dev, &pci_at, &first)
> + .setup_falcon_data(&pci_at, &first)
> .inspect_err(|e| dev_err!(dev, "Falcon data setup failed:
> {:?}\n", e))?;
> Ok(Vbios {
> - fwsec_image: second.build(dev)?,
> + fwsec_image: second.build()?,
> })
> } else {
> dev_err!(
> @@ -742,9 +743,10 @@ fn try_from(base: BiosImageBase) -> Result<Self> {
> ///
> /// Each BiosImage type has a BiosImageBase type along with other
> image-specific fields. Note that
> /// Rust favors composition of types over inheritance.
> -#[derive(Debug)]
> #[expect(dead_code)]
> struct BiosImageBase {
> + /// Used for logging.
> + dev: ARef<device::Device>,
> /// PCI ROM Expansion Header
> rom_header: PciRomHeader,
> /// PCI Data Structure
> @@ -801,6 +803,7 @@ fn new(dev: &device::Device, data: &[u8]) -> Result<Self>
> {
> data_copy.extend_from_slice(data, GFP_KERNEL)?;
>
> Ok(BiosImageBase {
> + dev: dev.into(),
> rom_header,
> pcir,
> npde,
> @@ -836,7 +839,7 @@ fn get_bit_token(&self, token_id: u8) -> Result<BitToken>
> {
> ///
> /// This is just a 4 byte structure that contains a pointer to the
> Falcon data in the FWSEC
> /// image.
> - fn falcon_data_ptr(&self, dev: &device::Device) -> Result<u32> {
> + fn falcon_data_ptr(&self) -> Result<u32> {
> let token = self.get_bit_token(BIT_TOKEN_ID_FALCON_DATA)?;
>
> // Make sure we don't go out of bounds
> @@ -847,14 +850,14 @@ fn falcon_data_ptr(&self, dev: &device::Device) ->
> Result<u32> {
> // read the 4 bytes at the offset specified in the token
> let offset = token.data_offset as usize;
> let bytes: [u8; 4] = self.base.data[offset..offset +
> 4].try_into().map_err(|_| {
> - dev_err!(dev, "Failed to convert data slice to array");
> + dev_err!(self.base.dev, "Failed to convert data slice to array");
> EINVAL
> })?;
>
> let data_ptr = u32::from_le_bytes(bytes);
>
> if (data_ptr as usize) < self.base.data.len() {
> - dev_err!(dev, "Falcon data pointer out of bounds\n");
> + dev_err!(self.base.dev, "Falcon data pointer out of bounds\n");
> return Err(EINVAL);
> }
>
> @@ -978,11 +981,10 @@ fn find_entry_by_type(&self, entry_type: u8) ->
> Result<PmuLookupTableEntry> {
> impl FwSecBiosBuilder {
> fn setup_falcon_data(
> &mut self,
> - dev: &device::Device,
> pci_at_image: &PciAtBiosImage,
> first_fwsec: &FwSecBiosBuilder,
> ) -> Result {
> - let mut offset = pci_at_image.falcon_data_ptr(dev)? as usize;
> + let mut offset = pci_at_image.falcon_data_ptr()? as usize;
> let mut pmu_in_first_fwsec = false;
>
> // The falcon data pointer assumes that the PciAt and FWSEC images
> @@ -1005,10 +1007,15 @@ fn setup_falcon_data(
> self.falcon_data_offset = Some(offset);
>
> if pmu_in_first_fwsec {
> - self.pmu_lookup_table =
> - Some(PmuLookupTable::new(dev,
> &first_fwsec.base.data[offset..])?);
> + self.pmu_lookup_table = Some(PmuLookupTable::new(
> + &self.base.dev,
> + &first_fwsec.base.data[offset..],
> + )?);
> } else {
> - self.pmu_lookup_table = Some(PmuLookupTable::new(dev,
> &self.base.data[offset..])?);
> + self.pmu_lookup_table = Some(PmuLookupTable::new(
> + &self.base.dev,
> + &self.base.data[offset..],
> + )?);
> }
>
> match self
> @@ -1021,14 +1028,18 @@ fn setup_falcon_data(
> let mut ucode_offset = entry.data as usize;
> ucode_offset -= pci_at_image.base.data.len();
> if ucode_offset < first_fwsec.base.data.len() {
> - dev_err!(dev, "Falcon Ucode offset not in second
> Fwsec.\n");
> + dev_err!(self.base.dev, "Falcon Ucode offset not in
> second Fwsec.\n");
> return Err(EINVAL);
> }
> ucode_offset -= first_fwsec.base.data.len();
> self.falcon_ucode_offset = Some(ucode_offset);
> }
> Err(e) => {
> - dev_err!(dev, "PmuLookupTableEntry not found, error:
> {:?}\n", e);
> + dev_err!(
> + self.base.dev,
> + "PmuLookupTableEntry not found, error: {:?}\n",
> + e
> + );
> return Err(EINVAL);
> }
> }
> @@ -1036,7 +1047,7 @@ fn setup_falcon_data(
> }
>
> /// Build the final FwSecBiosImage from this builder
> - fn build(self, dev: &device::Device) -> Result<FwSecBiosImage> {
> + fn build(self) -> Result<FwSecBiosImage> {
> let ret = FwSecBiosImage {
> base: self.base,
> falcon_ucode_offset: self.falcon_ucode_offset.ok_or(EINVAL)?,
> @@ -1044,8 +1055,8 @@ fn build(self, dev: &device::Device) ->
> Result<FwSecBiosImage> {
>
> if cfg!(debug_assertions) {
> // Print the desc header for debugging
> - let desc = ret.header(dev)?;
> - dev_dbg!(dev, "PmuLookupTableEntry desc: {:#?}\n", desc);
> + let desc = ret.header()?;
> + dev_dbg!(ret.base.dev, "PmuLookupTableEntry desc: {:#?}\n",
> desc);
> }
>
> Ok(ret)
> @@ -1054,13 +1065,16 @@ fn build(self, dev: &device::Device) ->
> Result<FwSecBiosImage> {
>
> impl FwSecBiosImage {
> /// Get the FwSec header ([`FalconUCodeDescV3`]).
> - pub(crate) fn header(&self, dev: &device::Device) ->
> Result<&FalconUCodeDescV3> {
> + pub(crate) fn header(&self) -> Result<&FalconUCodeDescV3> {
> // Get the falcon ucode offset that was found in setup_falcon_data.
> let falcon_ucode_offset = self.falcon_ucode_offset;
>
> // Make sure the offset is within the data bounds.
> if falcon_ucode_offset + core::mem::size_of::<FalconUCodeDescV3>() >
> self.base.data.len() {
> - dev_err!(dev, "fwsec-frts header not contained within BIOS
> bounds\n");
> + dev_err!(
> + self.base.dev,
> + "fwsec-frts header not contained within BIOS bounds\n"
> + );
> return Err(ERANGE);
> }
>
> @@ -1072,7 +1086,7 @@ pub(crate) fn header(&self, dev: &device::Device) ->
> Result<&FalconUCodeDescV3>
> let ver = (hdr & 0xff00) >> 8;
>
> if ver != 3 {
> - dev_err!(dev, "invalid fwsec firmware version: {:?}\n", ver);
> + dev_err!(self.base.dev, "invalid fwsec firmware version:
> {:?}\n", ver);
> return Err(EINVAL);
> }
>
> @@ -1092,7 +1106,7 @@ pub(crate) fn header(&self, dev: &device::Device) ->
> Result<&FalconUCodeDescV3>
> }
>
> /// Get the ucode data as a byte slice
> - pub(crate) fn ucode(&self, dev: &device::Device, desc:
> &FalconUCodeDescV3) -> Result<&[u8]> {
> + pub(crate) fn ucode(&self, desc: &FalconUCodeDescV3) -> Result<&[u8]> {
> let falcon_ucode_offset = self.falcon_ucode_offset;
>
> // The ucode data follows the descriptor.
> @@ -1104,15 +1118,16 @@ pub(crate) fn ucode(&self, dev: &device::Device,
> desc: &FalconUCodeDescV3) -> Re
> .data
> .get(ucode_data_offset..ucode_data_offset + size)
> .ok_or(ERANGE)
> - .inspect_err(|_| dev_err!(dev, "fwsec ucode data not contained
> within BIOS bounds\n"))
> + .inspect_err(|_| {
> + dev_err!(
> + self.base.dev,
> + "fwsec ucode data not contained within BIOS bounds\n"
> + )
> + })
> }
>
> /// Get the signatures as a byte slice
> - pub(crate) fn sigs(
> - &self,
> - dev: &device::Device,
> - desc: &FalconUCodeDescV3,
> - ) -> Result<&[Bcrt30Rsa3kSignature]> {
> + pub(crate) fn sigs(&self, desc: &FalconUCodeDescV3) ->
> Result<&[Bcrt30Rsa3kSignature]> {
> // The signatures data follows the descriptor.
> let sigs_data_offset = self.falcon_ucode_offset +
> core::mem::size_of::<FalconUCodeDescV3>();
> let sigs_size =
> @@ -1121,7 +1136,7 @@ pub(crate) fn sigs(
> // Make sure the data is within bounds.
> if sigs_data_offset + sigs_size > self.base.data.len() {
> dev_err!(
> - dev,
> + self.base.dev,
> "fwsec signatures data not contained within BIOS bounds\n"
> );
> return Err(ERANGE);
>
> --
> 2.50.1
>
