[PATCH 0/2] gpu: nova-core: vbios: simplify device use

[PATCH 0/2] gpu: nova-core: vbios: simplify device use

[Alexandre Courbot]

This small cleanup series simplifies the use of `Device` in vbios
methods.

The device is used for logging purposes only; thus we don't need a
pci::Device, neither do we need it to be bound. This latter property
allows us to store an `ARef` to it into structures that require logging
instead of having all their methods take an extra `dev` argument.
Removing this argument streamlines the code a bit.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
Alexandre Courbot (2):
      gpu: nova-core: vbios: replace pci::Device with device::Device
      gpu: nova-core: vbios: store reference to Device where relevant

 drivers/gpu/nova-core/firmware/fwsec.rs |   8 +-
 drivers/gpu/nova-core/gpu.rs            |   2 +-
 drivers/gpu/nova-core/vbios.rs          | 168 +++++++++++++++-----------------
 3 files changed, 85 insertions(+), 93 deletions(-)
---
base-commit: 14ae91a81ec8fa0bc23170d4aa16dd2a20d54105
change-id: 20250808-vbios_device-b0a912aff149

Best regards,
-- 
Alexandre Courbot <acourbot@nvidia.com>

[PATCH 1/2] gpu: nova-core: vbios: replace pci::Device with device::Device

[Alexandre Courbot]

The passed pci::Device is exclusively used for logging purposes, so it
can be replaced by a regular device::Device, which allows us to remove
the `as_ref()` indirections at each logging site.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 drivers/gpu/nova-core/gpu.rs   |   2 +-
 drivers/gpu/nova-core/vbios.rs | 135 +++++++++++++++++------------------------
 2 files changed, 57 insertions(+), 80 deletions(-)

diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs
index 72d40b0124f0c1a2a381484172c289af523511df..33082ac45873ee4cf91d7d8af499efa984af4ba9 100644
--- a/drivers/gpu/nova-core/gpu.rs
+++ b/drivers/gpu/nova-core/gpu.rs
@@ -298,7 +298,7 @@ pub(crate) fn new(
         let fb_layout = FbLayout::new(spec.chipset, bar)?;
         dev_dbg!(pdev.as_ref(), "{:#x?}\n", fb_layout);
 
-        let bios = Vbios::new(pdev, bar)?;
+        let bios = Vbios::new(pdev.as_ref(), bar)?;
 
         Self::run_fwsec_frts(pdev.as_ref(), &gsp_falcon, bar, &bios, &fb_layout)?;
 
diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
index 5b5d9f38cbb3a6b1c374c1e0eee2509eb8d5660c..b5564b4d3e4758e77178aa403635e4780f0378cc 100644
--- a/drivers/gpu/nova-core/vbios.rs
+++ b/drivers/gpu/nova-core/vbios.rs
@@ -8,7 +8,6 @@
 use core::convert::TryFrom;
 use kernel::device;
 use kernel::error::Result;
-use kernel::pci;
 use kernel::prelude::*;
 
 /// The offset of the VBIOS ROM in the BAR0 space.
@@ -31,7 +30,7 @@
 
 /// Vbios Reader for constructing the VBIOS data.
 struct VbiosIterator<'a> {
-    pdev: &'a pci::Device,
+    dev: &'a device::Device,
     bar0: &'a Bar0,
     /// VBIOS data vector: As BIOS images are scanned, they are added to this vector for reference
     /// or copying into other data structures. It is the entire scanned contents of the VBIOS which
@@ -46,9 +45,9 @@ struct VbiosIterator<'a> {
 }
 
 impl<'a> VbiosIterator<'a> {
-    fn new(pdev: &'a pci::Device, bar0: &'a Bar0) -> Result<Self> {
+    fn new(dev: &'a device::Device, bar0: &'a Bar0) -> Result<Self> {
         Ok(Self {
-            pdev,
+            dev,
             bar0,
             data: KVec::new(),
             current_offset: 0,
@@ -64,7 +63,7 @@ fn read_more(&mut self, len: usize) -> Result {
         // Ensure length is a multiple of 4 for 32-bit reads
         if len % core::mem::size_of::<u32>() != 0 {
             dev_err!(
-                self.pdev.as_ref(),
+                self.dev,
                 "VBIOS read length {} is not a multiple of 4\n",
                 len
             );
@@ -89,7 +88,7 @@ fn read_more(&mut self, len: usize) -> Result {
     /// Read bytes at a specific offset, filling any gap.
     fn read_more_at_offset(&mut self, offset: usize, len: usize) -> Result {
         if offset > BIOS_MAX_SCAN_LEN {
-            dev_err!(self.pdev.as_ref(), "Error: exceeded BIOS scan limit.\n");
+            dev_err!(self.dev, "Error: exceeded BIOS scan limit.\n");
             return Err(EINVAL);
         }
 
@@ -115,7 +114,7 @@ fn read_bios_image_at_offset(
         if offset + len > data_len {
             self.read_more_at_offset(offset, len).inspect_err(|e| {
                 dev_err!(
-                    self.pdev.as_ref(),
+                    self.dev,
                     "Failed to read more at offset {:#x}: {:?}\n",
                     offset,
                     e
@@ -123,9 +122,9 @@ fn read_bios_image_at_offset(
             })?;
         }
 
-        BiosImage::new(self.pdev, &self.data[offset..offset + len]).inspect_err(|err| {
+        BiosImage::new(self.dev, &self.data[offset..offset + len]).inspect_err(|err| {
             dev_err!(
-                self.pdev.as_ref(),
+                self.dev,
                 "Failed to {} at offset {:#x}: {:?}\n",
                 context,
                 offset,
@@ -146,10 +145,7 @@ fn next(&mut self) -> Option<Self::Item> {
         }
 
         if self.current_offset > BIOS_MAX_SCAN_LEN {
-            dev_err!(
-                self.pdev.as_ref(),
-                "Error: exceeded BIOS scan limit, stopping scan\n"
-            );
+            dev_err!(self.dev, "Error: exceeded BIOS scan limit, stopping scan\n");
             return None;
         }
 
@@ -192,18 +188,18 @@ impl Vbios {
     /// Probe for VBIOS extraction.
     ///
     /// Once the VBIOS object is built, `bar0` is not read for [`Vbios`] purposes anymore.
-    pub(crate) fn new(pdev: &pci::Device, bar0: &Bar0) -> Result<Vbios> {
+    pub(crate) fn new(dev: &device::Device, bar0: &Bar0) -> Result<Vbios> {
         // Images to extract from iteration
         let mut pci_at_image: Option<PciAtBiosImage> = None;
         let mut first_fwsec_image: Option<FwSecBiosBuilder> = None;
         let mut second_fwsec_image: Option<FwSecBiosBuilder> = None;
 
         // Parse all VBIOS images in the ROM
-        for image_result in VbiosIterator::new(pdev, bar0)? {
+        for image_result in VbiosIterator::new(dev, bar0)? {
             let full_image = image_result?;
 
             dev_dbg!(
-                pdev.as_ref(),
+                dev,
                 "Found BIOS image: size: {:#x}, type: {}, last: {}\n",
                 full_image.image_size_bytes(),
                 full_image.image_type_str(),
@@ -234,14 +230,14 @@ pub(crate) fn new(pdev: &pci::Device, bar0: &Bar0) -> Result<Vbios> {
             (second_fwsec_image, first_fwsec_image, pci_at_image)
         {
             second
-                .setup_falcon_data(pdev, &pci_at, &first)
-                .inspect_err(|e| dev_err!(pdev.as_ref(), "Falcon data setup failed: {:?}\n", e))?;
+                .setup_falcon_data(dev, &pci_at, &first)
+                .inspect_err(|e| dev_err!(dev, "Falcon data setup failed: {:?}\n", e))?;
             Ok(Vbios {
-                fwsec_image: second.build(pdev)?,
+                fwsec_image: second.build(dev)?,
             })
         } else {
             dev_err!(
-                pdev.as_ref(),
+                dev,
                 "Missing required images for falcon data setup, skipping\n"
             );
             Err(EINVAL)
@@ -284,9 +280,9 @@ struct PcirStruct {
 }
 
 impl PcirStruct {
-    fn new(pdev: &pci::Device, data: &[u8]) -> Result<Self> {
+    fn new(dev: &device::Device, data: &[u8]) -> Result<Self> {
         if data.len() < core::mem::size_of::<PcirStruct>() {
-            dev_err!(pdev.as_ref(), "Not enough data for PcirStruct\n");
+            dev_err!(dev, "Not enough data for PcirStruct\n");
             return Err(EINVAL);
         }
 
@@ -295,11 +291,7 @@ fn new(pdev: &pci::Device, data: &[u8]) -> Result<Self> {
 
         // Signature should be "PCIR" (0x52494350) or "NPDS" (0x5344504e).
         if &signature != b"PCIR" && &signature != b"NPDS" {
-            dev_err!(
-                pdev.as_ref(),
-                "Invalid signature for PcirStruct: {:?}\n",
-                signature
-            );
+            dev_err!(dev, "Invalid signature for PcirStruct: {:?}\n", signature);
             return Err(EINVAL);
         }
 
@@ -308,7 +300,7 @@ fn new(pdev: &pci::Device, data: &[u8]) -> Result<Self> {
 
         let image_len = u16::from_le_bytes([data[16], data[17]]);
         if image_len == 0 {
-            dev_err!(pdev.as_ref(), "Invalid image length: 0\n");
+            dev_err!(dev, "Invalid image length: 0\n");
             return Err(EINVAL);
         }
 
@@ -467,7 +459,7 @@ struct PciRomHeader {
 }
 
 impl PciRomHeader {
-    fn new(pdev: &pci::Device, data: &[u8]) -> Result<Self> {
+    fn new(dev: &device::Device, data: &[u8]) -> Result<Self> {
         if data.len() < 26 {
             // Need at least 26 bytes to read pciDataStrucPtr and sizeOfBlock.
             return Err(EINVAL);
@@ -479,7 +471,7 @@ fn new(pdev: &pci::Device, data: &[u8]) -> Result<Self> {
         match signature {
             0xAA55 | 0xBB77 | 0x4E56 => {}
             _ => {
-                dev_err!(pdev.as_ref(), "ROM signature unknown {:#x}\n", signature);
+                dev_err!(dev, "ROM signature unknown {:#x}\n", signature);
                 return Err(EINVAL);
             }
         }
@@ -538,9 +530,9 @@ struct NpdeStruct {
 }
 
 impl NpdeStruct {
-    fn new(pdev: &pci::Device, data: &[u8]) -> Option<Self> {
+    fn new(dev: &device::Device, data: &[u8]) -> Option<Self> {
         if data.len() < core::mem::size_of::<Self>() {
-            dev_dbg!(pdev.as_ref(), "Not enough data for NpdeStruct\n");
+            dev_dbg!(dev, "Not enough data for NpdeStruct\n");
             return None;
         }
 
@@ -549,17 +541,13 @@ fn new(pdev: &pci::Device, data: &[u8]) -> Option<Self> {
 
         // Signature should be "NPDE" (0x4544504E).
         if &signature != b"NPDE" {
-            dev_dbg!(
-                pdev.as_ref(),
-                "Invalid signature for NpdeStruct: {:?}\n",
-                signature
-            );
+            dev_dbg!(dev, "Invalid signature for NpdeStruct: {:?}\n", signature);
             return None;
         }
 
         let subimage_len = u16::from_le_bytes([data[8], data[9]]);
         if subimage_len == 0 {
-            dev_dbg!(pdev.as_ref(), "Invalid subimage length: 0\n");
+            dev_dbg!(dev, "Invalid subimage length: 0\n");
             return None;
         }
 
@@ -584,7 +572,7 @@ fn image_size_bytes(&self) -> usize {
 
     /// Try to find NPDE in the data, the NPDE is right after the PCIR.
     fn find_in_data(
-        pdev: &pci::Device,
+        dev: &device::Device,
         data: &[u8],
         rom_header: &PciRomHeader,
         pcir: &PcirStruct,
@@ -596,12 +584,12 @@ fn find_in_data(
 
         // Check if we have enough data
         if npde_start + core::mem::size_of::<Self>() > data.len() {
-            dev_dbg!(pdev.as_ref(), "Not enough data for NPDE\n");
+            dev_dbg!(dev, "Not enough data for NPDE\n");
             return None;
         }
 
         // Try to create NPDE from the data
-        NpdeStruct::new(pdev, &data[npde_start..])
+        NpdeStruct::new(dev, &data[npde_start..])
     }
 }
 
@@ -669,10 +657,10 @@ fn image_size_bytes(&self) -> usize {
 
     /// Create a [`BiosImageBase`] from a byte slice and convert it to a [`BiosImage`] which
     /// triggers the constructor of the specific BiosImage enum variant.
-    fn new(pdev: &pci::Device, data: &[u8]) -> Result<Self> {
-        let base = BiosImageBase::new(pdev, data)?;
+    fn new(dev: &device::Device, data: &[u8]) -> Result<Self> {
+        let base = BiosImageBase::new(dev, data)?;
         let image = base.into_image().inspect_err(|e| {
-            dev_err!(pdev.as_ref(), "Failed to create BiosImage: {:?}\n", e);
+            dev_err!(dev, "Failed to create BiosImage: {:?}\n", e);
         })?;
 
         Ok(image)
@@ -773,16 +761,16 @@ fn into_image(self) -> Result<BiosImage> {
     }
 
     /// Creates a new BiosImageBase from raw byte data.
-    fn new(pdev: &pci::Device, data: &[u8]) -> Result<Self> {
+    fn new(dev: &device::Device, data: &[u8]) -> Result<Self> {
         // Ensure we have enough data for the ROM header.
         if data.len() < 26 {
-            dev_err!(pdev.as_ref(), "Not enough data for ROM header\n");
+            dev_err!(dev, "Not enough data for ROM header\n");
             return Err(EINVAL);
         }
 
         // Parse the ROM header.
-        let rom_header = PciRomHeader::new(pdev, &data[0..26])
-            .inspect_err(|e| dev_err!(pdev.as_ref(), "Failed to create PciRomHeader: {:?}\n", e))?;
+        let rom_header = PciRomHeader::new(dev, &data[0..26])
+            .inspect_err(|e| dev_err!(dev, "Failed to create PciRomHeader: {:?}\n", e))?;
 
         // Get the PCI Data Structure using the pointer from the ROM header.
         let pcir_offset = rom_header.pci_data_struct_offset as usize;
@@ -791,22 +779,22 @@ fn new(pdev: &pci::Device, data: &[u8]) -> Result<Self> {
             .ok_or(EINVAL)
             .inspect_err(|_| {
                 dev_err!(
-                    pdev.as_ref(),
+                    dev,
                     "PCIR offset {:#x} out of bounds (data length: {})\n",
                     pcir_offset,
                     data.len()
                 );
                 dev_err!(
-                    pdev.as_ref(),
+                    dev,
                     "Consider reading more data for construction of BiosImage\n"
                 );
             })?;
 
-        let pcir = PcirStruct::new(pdev, pcir_data)
-            .inspect_err(|e| dev_err!(pdev.as_ref(), "Failed to create PcirStruct: {:?}\n", e))?;
+        let pcir = PcirStruct::new(dev, pcir_data)
+            .inspect_err(|e| dev_err!(dev, "Failed to create PcirStruct: {:?}\n", e))?;
 
         // Look for NPDE structure if this is not an NBSI image (type != 0x70).
-        let npde = NpdeStruct::find_in_data(pdev, data, &rom_header, &pcir);
+        let npde = NpdeStruct::find_in_data(dev, data, &rom_header, &pcir);
 
         // Create a copy of the data.
         let mut data_copy = KVec::new();
@@ -848,7 +836,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, pdev: &pci::Device) -> Result<u32> {
+    fn falcon_data_ptr(&self, dev: &device::Device) -> Result<u32> {
         let token = self.get_bit_token(BIT_TOKEN_ID_FALCON_DATA)?;
 
         // Make sure we don't go out of bounds
@@ -859,14 +847,14 @@ fn falcon_data_ptr(&self, pdev: &pci::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!(pdev.as_ref(), "Failed to convert data slice to array");
+            dev_err!(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!(pdev.as_ref(), "Falcon data pointer out of bounds\n");
+            dev_err!(dev, "Falcon data pointer out of bounds\n");
             return Err(EINVAL);
         }
 
@@ -928,7 +916,7 @@ struct PmuLookupTable {
 }
 
 impl PmuLookupTable {
-    fn new(pdev: &pci::Device, data: &[u8]) -> Result<Self> {
+    fn new(dev: &device::Device, data: &[u8]) -> Result<Self> {
         if data.len() < 4 {
             return Err(EINVAL);
         }
@@ -940,10 +928,7 @@ fn new(pdev: &pci::Device, data: &[u8]) -> Result<Self> {
         let required_bytes = header_len + (entry_count * entry_len);
 
         if data.len() < required_bytes {
-            dev_err!(
-                pdev.as_ref(),
-                "PmuLookupTable data length less than required\n"
-            );
+            dev_err!(dev, "PmuLookupTable data length less than required\n");
             return Err(EINVAL);
         }
 
@@ -956,11 +941,7 @@ fn new(pdev: &pci::Device, data: &[u8]) -> Result<Self> {
 
         // Debug logging of entries (dumps the table data to dmesg)
         for i in (header_len..required_bytes).step_by(entry_len) {
-            dev_dbg!(
-                pdev.as_ref(),
-                "PMU entry: {:02x?}\n",
-                &data[i..][..entry_len]
-            );
+            dev_dbg!(dev, "PMU entry: {:02x?}\n", &data[i..][..entry_len]);
         }
 
         Ok(PmuLookupTable {
@@ -997,11 +978,11 @@ fn find_entry_by_type(&self, entry_type: u8) -> Result<PmuLookupTableEntry> {
 impl FwSecBiosBuilder {
     fn setup_falcon_data(
         &mut self,
-        pdev: &pci::Device,
+        dev: &device::Device,
         pci_at_image: &PciAtBiosImage,
         first_fwsec: &FwSecBiosBuilder,
     ) -> Result {
-        let mut offset = pci_at_image.falcon_data_ptr(pdev)? as usize;
+        let mut offset = pci_at_image.falcon_data_ptr(dev)? as usize;
         let mut pmu_in_first_fwsec = false;
 
         // The falcon data pointer assumes that the PciAt and FWSEC images
@@ -1025,9 +1006,9 @@ fn setup_falcon_data(
 
         if pmu_in_first_fwsec {
             self.pmu_lookup_table =
-                Some(PmuLookupTable::new(pdev, &first_fwsec.base.data[offset..])?);
+                Some(PmuLookupTable::new(dev, &first_fwsec.base.data[offset..])?);
         } else {
-            self.pmu_lookup_table = Some(PmuLookupTable::new(pdev, &self.base.data[offset..])?);
+            self.pmu_lookup_table = Some(PmuLookupTable::new(dev, &self.base.data[offset..])?);
         }
 
         match self
@@ -1040,18 +1021,14 @@ 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!(pdev.as_ref(), "Falcon Ucode offset not in second Fwsec.\n");
+                    dev_err!(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!(
-                    pdev.as_ref(),
-                    "PmuLookupTableEntry not found, error: {:?}\n",
-                    e
-                );
+                dev_err!(dev, "PmuLookupTableEntry not found, error: {:?}\n", e);
                 return Err(EINVAL);
             }
         }
@@ -1059,7 +1036,7 @@ fn setup_falcon_data(
     }
 
     /// Build the final FwSecBiosImage from this builder
-    fn build(self, pdev: &pci::Device) -> Result<FwSecBiosImage> {
+    fn build(self, dev: &device::Device) -> Result<FwSecBiosImage> {
         let ret = FwSecBiosImage {
             base: self.base,
             falcon_ucode_offset: self.falcon_ucode_offset.ok_or(EINVAL)?,
@@ -1067,8 +1044,8 @@ fn build(self, pdev: &pci::Device) -> Result<FwSecBiosImage> {
 
         if cfg!(debug_assertions) {
             // Print the desc header for debugging
-            let desc = ret.header(pdev.as_ref())?;
-            dev_dbg!(pdev.as_ref(), "PmuLookupTableEntry desc: {:#?}\n", desc);
+            let desc = ret.header(dev)?;
+            dev_dbg!(dev, "PmuLookupTableEntry desc: {:#?}\n", desc);
         }
 
         Ok(ret)

-- 
2.50.1

[PATCH 2/2] gpu: nova-core: vbios: store reference to Device where relevant

[Alexandre Courbot]

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 <acourbot@nvidia.com>
---
 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

Re: [PATCH 1/2] gpu: nova-core: vbios: replace pci::Device with device::Device

[Joel Fernandes]

On Fri, Aug 08, 2025 at 11:46:41AM +0900, Alexandre Courbot wrote:
> The passed pci::Device is exclusively used for logging purposes, so it
> can be replaced by a regular device::Device, which allows us to remove
> the `as_ref()` indirections at each logging site.
> 
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>

Reviewed-by: Joel Fernandes <joelagnelf@nvidia.com>

thanks,

 - Joel

> ---
>  drivers/gpu/nova-core/gpu.rs   |   2 +-
>  drivers/gpu/nova-core/vbios.rs | 135 +++++++++++++++++------------------------
>  2 files changed, 57 insertions(+), 80 deletions(-)
> 
> diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs
> index 72d40b0124f0c1a2a381484172c289af523511df..33082ac45873ee4cf91d7d8af499efa984af4ba9 100644
> --- a/drivers/gpu/nova-core/gpu.rs
> +++ b/drivers/gpu/nova-core/gpu.rs
> @@ -298,7 +298,7 @@ pub(crate) fn new(
>          let fb_layout = FbLayout::new(spec.chipset, bar)?;
>          dev_dbg!(pdev.as_ref(), "{:#x?}\n", fb_layout);
>  
> -        let bios = Vbios::new(pdev, bar)?;
> +        let bios = Vbios::new(pdev.as_ref(), bar)?;
>  
>          Self::run_fwsec_frts(pdev.as_ref(), &gsp_falcon, bar, &bios, &fb_layout)?;
>  
> diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
> index 5b5d9f38cbb3a6b1c374c1e0eee2509eb8d5660c..b5564b4d3e4758e77178aa403635e4780f0378cc 100644
> --- a/drivers/gpu/nova-core/vbios.rs
> +++ b/drivers/gpu/nova-core/vbios.rs
> @@ -8,7 +8,6 @@
>  use core::convert::TryFrom;
>  use kernel::device;
>  use kernel::error::Result;
> -use kernel::pci;
>  use kernel::prelude::*;
>  
>  /// The offset of the VBIOS ROM in the BAR0 space.
> @@ -31,7 +30,7 @@
>  
>  /// Vbios Reader for constructing the VBIOS data.
>  struct VbiosIterator<'a> {
> -    pdev: &'a pci::Device,
> +    dev: &'a device::Device,
>      bar0: &'a Bar0,
>      /// VBIOS data vector: As BIOS images are scanned, they are added to this vector for reference
>      /// or copying into other data structures. It is the entire scanned contents of the VBIOS which
> @@ -46,9 +45,9 @@ struct VbiosIterator<'a> {
>  }
>  
>  impl<'a> VbiosIterator<'a> {
> -    fn new(pdev: &'a pci::Device, bar0: &'a Bar0) -> Result<Self> {
> +    fn new(dev: &'a device::Device, bar0: &'a Bar0) -> Result<Self> {
>          Ok(Self {
> -            pdev,
> +            dev,
>              bar0,
>              data: KVec::new(),
>              current_offset: 0,
> @@ -64,7 +63,7 @@ fn read_more(&mut self, len: usize) -> Result {
>          // Ensure length is a multiple of 4 for 32-bit reads
>          if len % core::mem::size_of::<u32>() != 0 {
>              dev_err!(
> -                self.pdev.as_ref(),
> +                self.dev,
>                  "VBIOS read length {} is not a multiple of 4\n",
>                  len
>              );
> @@ -89,7 +88,7 @@ fn read_more(&mut self, len: usize) -> Result {
>      /// Read bytes at a specific offset, filling any gap.
>      fn read_more_at_offset(&mut self, offset: usize, len: usize) -> Result {
>          if offset > BIOS_MAX_SCAN_LEN {
> -            dev_err!(self.pdev.as_ref(), "Error: exceeded BIOS scan limit.\n");
> +            dev_err!(self.dev, "Error: exceeded BIOS scan limit.\n");
>              return Err(EINVAL);
>          }
>  
> @@ -115,7 +114,7 @@ fn read_bios_image_at_offset(
>          if offset + len > data_len {
>              self.read_more_at_offset(offset, len).inspect_err(|e| {
>                  dev_err!(
> -                    self.pdev.as_ref(),
> +                    self.dev,
>                      "Failed to read more at offset {:#x}: {:?}\n",
>                      offset,
>                      e
> @@ -123,9 +122,9 @@ fn read_bios_image_at_offset(
>              })?;
>          }
>  
> -        BiosImage::new(self.pdev, &self.data[offset..offset + len]).inspect_err(|err| {
> +        BiosImage::new(self.dev, &self.data[offset..offset + len]).inspect_err(|err| {
>              dev_err!(
> -                self.pdev.as_ref(),
> +                self.dev,
>                  "Failed to {} at offset {:#x}: {:?}\n",
>                  context,
>                  offset,
> @@ -146,10 +145,7 @@ fn next(&mut self) -> Option<Self::Item> {
>          }
>  
>          if self.current_offset > BIOS_MAX_SCAN_LEN {
> -            dev_err!(
> -                self.pdev.as_ref(),
> -                "Error: exceeded BIOS scan limit, stopping scan\n"
> -            );
> +            dev_err!(self.dev, "Error: exceeded BIOS scan limit, stopping scan\n");
>              return None;
>          }
>  
> @@ -192,18 +188,18 @@ impl Vbios {
>      /// Probe for VBIOS extraction.
>      ///
>      /// Once the VBIOS object is built, `bar0` is not read for [`Vbios`] purposes anymore.
> -    pub(crate) fn new(pdev: &pci::Device, bar0: &Bar0) -> Result<Vbios> {
> +    pub(crate) fn new(dev: &device::Device, bar0: &Bar0) -> Result<Vbios> {
>          // Images to extract from iteration
>          let mut pci_at_image: Option<PciAtBiosImage> = None;
>          let mut first_fwsec_image: Option<FwSecBiosBuilder> = None;
>          let mut second_fwsec_image: Option<FwSecBiosBuilder> = None;
>  
>          // Parse all VBIOS images in the ROM
> -        for image_result in VbiosIterator::new(pdev, bar0)? {
> +        for image_result in VbiosIterator::new(dev, bar0)? {
>              let full_image = image_result?;
>  
>              dev_dbg!(
> -                pdev.as_ref(),
> +                dev,
>                  "Found BIOS image: size: {:#x}, type: {}, last: {}\n",
>                  full_image.image_size_bytes(),
>                  full_image.image_type_str(),
> @@ -234,14 +230,14 @@ pub(crate) fn new(pdev: &pci::Device, bar0: &Bar0) -> Result<Vbios> {
>              (second_fwsec_image, first_fwsec_image, pci_at_image)
>          {
>              second
> -                .setup_falcon_data(pdev, &pci_at, &first)
> -                .inspect_err(|e| dev_err!(pdev.as_ref(), "Falcon data setup failed: {:?}\n", e))?;
> +                .setup_falcon_data(dev, &pci_at, &first)
> +                .inspect_err(|e| dev_err!(dev, "Falcon data setup failed: {:?}\n", e))?;
>              Ok(Vbios {
> -                fwsec_image: second.build(pdev)?,
> +                fwsec_image: second.build(dev)?,
>              })
>          } else {
>              dev_err!(
> -                pdev.as_ref(),
> +                dev,
>                  "Missing required images for falcon data setup, skipping\n"
>              );
>              Err(EINVAL)
> @@ -284,9 +280,9 @@ struct PcirStruct {
>  }
>  
>  impl PcirStruct {
> -    fn new(pdev: &pci::Device, data: &[u8]) -> Result<Self> {
> +    fn new(dev: &device::Device, data: &[u8]) -> Result<Self> {
>          if data.len() < core::mem::size_of::<PcirStruct>() {
> -            dev_err!(pdev.as_ref(), "Not enough data for PcirStruct\n");
> +            dev_err!(dev, "Not enough data for PcirStruct\n");
>              return Err(EINVAL);
>          }
>  
> @@ -295,11 +291,7 @@ fn new(pdev: &pci::Device, data: &[u8]) -> Result<Self> {
>  
>          // Signature should be "PCIR" (0x52494350) or "NPDS" (0x5344504e).
>          if &signature != b"PCIR" && &signature != b"NPDS" {
> -            dev_err!(
> -                pdev.as_ref(),
> -                "Invalid signature for PcirStruct: {:?}\n",
> -                signature
> -            );
> +            dev_err!(dev, "Invalid signature for PcirStruct: {:?}\n", signature);
>              return Err(EINVAL);
>          }
>  
> @@ -308,7 +300,7 @@ fn new(pdev: &pci::Device, data: &[u8]) -> Result<Self> {
>  
>          let image_len = u16::from_le_bytes([data[16], data[17]]);
>          if image_len == 0 {
> -            dev_err!(pdev.as_ref(), "Invalid image length: 0\n");
> +            dev_err!(dev, "Invalid image length: 0\n");
>              return Err(EINVAL);
>          }
>  
> @@ -467,7 +459,7 @@ struct PciRomHeader {
>  }
>  
>  impl PciRomHeader {
> -    fn new(pdev: &pci::Device, data: &[u8]) -> Result<Self> {
> +    fn new(dev: &device::Device, data: &[u8]) -> Result<Self> {
>          if data.len() < 26 {
>              // Need at least 26 bytes to read pciDataStrucPtr and sizeOfBlock.
>              return Err(EINVAL);
> @@ -479,7 +471,7 @@ fn new(pdev: &pci::Device, data: &[u8]) -> Result<Self> {
>          match signature {
>              0xAA55 | 0xBB77 | 0x4E56 => {}
>              _ => {
> -                dev_err!(pdev.as_ref(), "ROM signature unknown {:#x}\n", signature);
> +                dev_err!(dev, "ROM signature unknown {:#x}\n", signature);
>                  return Err(EINVAL);
>              }
>          }
> @@ -538,9 +530,9 @@ struct NpdeStruct {
>  }
>  
>  impl NpdeStruct {
> -    fn new(pdev: &pci::Device, data: &[u8]) -> Option<Self> {
> +    fn new(dev: &device::Device, data: &[u8]) -> Option<Self> {
>          if data.len() < core::mem::size_of::<Self>() {
> -            dev_dbg!(pdev.as_ref(), "Not enough data for NpdeStruct\n");
> +            dev_dbg!(dev, "Not enough data for NpdeStruct\n");
>              return None;
>          }
>  
> @@ -549,17 +541,13 @@ fn new(pdev: &pci::Device, data: &[u8]) -> Option<Self> {
>  
>          // Signature should be "NPDE" (0x4544504E).
>          if &signature != b"NPDE" {
> -            dev_dbg!(
> -                pdev.as_ref(),
> -                "Invalid signature for NpdeStruct: {:?}\n",
> -                signature
> -            );
> +            dev_dbg!(dev, "Invalid signature for NpdeStruct: {:?}\n", signature);
>              return None;
>          }
>  
>          let subimage_len = u16::from_le_bytes([data[8], data[9]]);
>          if subimage_len == 0 {
> -            dev_dbg!(pdev.as_ref(), "Invalid subimage length: 0\n");
> +            dev_dbg!(dev, "Invalid subimage length: 0\n");
>              return None;
>          }
>  
> @@ -584,7 +572,7 @@ fn image_size_bytes(&self) -> usize {
>  
>      /// Try to find NPDE in the data, the NPDE is right after the PCIR.
>      fn find_in_data(
> -        pdev: &pci::Device,
> +        dev: &device::Device,
>          data: &[u8],
>          rom_header: &PciRomHeader,
>          pcir: &PcirStruct,
> @@ -596,12 +584,12 @@ fn find_in_data(
>  
>          // Check if we have enough data
>          if npde_start + core::mem::size_of::<Self>() > data.len() {
> -            dev_dbg!(pdev.as_ref(), "Not enough data for NPDE\n");
> +            dev_dbg!(dev, "Not enough data for NPDE\n");
>              return None;
>          }
>  
>          // Try to create NPDE from the data
> -        NpdeStruct::new(pdev, &data[npde_start..])
> +        NpdeStruct::new(dev, &data[npde_start..])
>      }
>  }
>  
> @@ -669,10 +657,10 @@ fn image_size_bytes(&self) -> usize {
>  
>      /// Create a [`BiosImageBase`] from a byte slice and convert it to a [`BiosImage`] which
>      /// triggers the constructor of the specific BiosImage enum variant.
> -    fn new(pdev: &pci::Device, data: &[u8]) -> Result<Self> {
> -        let base = BiosImageBase::new(pdev, data)?;
> +    fn new(dev: &device::Device, data: &[u8]) -> Result<Self> {
> +        let base = BiosImageBase::new(dev, data)?;
>          let image = base.into_image().inspect_err(|e| {
> -            dev_err!(pdev.as_ref(), "Failed to create BiosImage: {:?}\n", e);
> +            dev_err!(dev, "Failed to create BiosImage: {:?}\n", e);
>          })?;
>  
>          Ok(image)
> @@ -773,16 +761,16 @@ fn into_image(self) -> Result<BiosImage> {
>      }
>  
>      /// Creates a new BiosImageBase from raw byte data.
> -    fn new(pdev: &pci::Device, data: &[u8]) -> Result<Self> {
> +    fn new(dev: &device::Device, data: &[u8]) -> Result<Self> {
>          // Ensure we have enough data for the ROM header.
>          if data.len() < 26 {
> -            dev_err!(pdev.as_ref(), "Not enough data for ROM header\n");
> +            dev_err!(dev, "Not enough data for ROM header\n");
>              return Err(EINVAL);
>          }
>  
>          // Parse the ROM header.
> -        let rom_header = PciRomHeader::new(pdev, &data[0..26])
> -            .inspect_err(|e| dev_err!(pdev.as_ref(), "Failed to create PciRomHeader: {:?}\n", e))?;
> +        let rom_header = PciRomHeader::new(dev, &data[0..26])
> +            .inspect_err(|e| dev_err!(dev, "Failed to create PciRomHeader: {:?}\n", e))?;
>  
>          // Get the PCI Data Structure using the pointer from the ROM header.
>          let pcir_offset = rom_header.pci_data_struct_offset as usize;
> @@ -791,22 +779,22 @@ fn new(pdev: &pci::Device, data: &[u8]) -> Result<Self> {
>              .ok_or(EINVAL)
>              .inspect_err(|_| {
>                  dev_err!(
> -                    pdev.as_ref(),
> +                    dev,
>                      "PCIR offset {:#x} out of bounds (data length: {})\n",
>                      pcir_offset,
>                      data.len()
>                  );
>                  dev_err!(
> -                    pdev.as_ref(),
> +                    dev,
>                      "Consider reading more data for construction of BiosImage\n"
>                  );
>              })?;
>  
> -        let pcir = PcirStruct::new(pdev, pcir_data)
> -            .inspect_err(|e| dev_err!(pdev.as_ref(), "Failed to create PcirStruct: {:?}\n", e))?;
> +        let pcir = PcirStruct::new(dev, pcir_data)
> +            .inspect_err(|e| dev_err!(dev, "Failed to create PcirStruct: {:?}\n", e))?;
>  
>          // Look for NPDE structure if this is not an NBSI image (type != 0x70).
> -        let npde = NpdeStruct::find_in_data(pdev, data, &rom_header, &pcir);
> +        let npde = NpdeStruct::find_in_data(dev, data, &rom_header, &pcir);
>  
>          // Create a copy of the data.
>          let mut data_copy = KVec::new();
> @@ -848,7 +836,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, pdev: &pci::Device) -> Result<u32> {
> +    fn falcon_data_ptr(&self, dev: &device::Device) -> Result<u32> {
>          let token = self.get_bit_token(BIT_TOKEN_ID_FALCON_DATA)?;
>  
>          // Make sure we don't go out of bounds
> @@ -859,14 +847,14 @@ fn falcon_data_ptr(&self, pdev: &pci::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!(pdev.as_ref(), "Failed to convert data slice to array");
> +            dev_err!(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!(pdev.as_ref(), "Falcon data pointer out of bounds\n");
> +            dev_err!(dev, "Falcon data pointer out of bounds\n");
>              return Err(EINVAL);
>          }
>  
> @@ -928,7 +916,7 @@ struct PmuLookupTable {
>  }
>  
>  impl PmuLookupTable {
> -    fn new(pdev: &pci::Device, data: &[u8]) -> Result<Self> {
> +    fn new(dev: &device::Device, data: &[u8]) -> Result<Self> {
>          if data.len() < 4 {
>              return Err(EINVAL);
>          }
> @@ -940,10 +928,7 @@ fn new(pdev: &pci::Device, data: &[u8]) -> Result<Self> {
>          let required_bytes = header_len + (entry_count * entry_len);
>  
>          if data.len() < required_bytes {
> -            dev_err!(
> -                pdev.as_ref(),
> -                "PmuLookupTable data length less than required\n"
> -            );
> +            dev_err!(dev, "PmuLookupTable data length less than required\n");
>              return Err(EINVAL);
>          }
>  
> @@ -956,11 +941,7 @@ fn new(pdev: &pci::Device, data: &[u8]) -> Result<Self> {
>  
>          // Debug logging of entries (dumps the table data to dmesg)
>          for i in (header_len..required_bytes).step_by(entry_len) {
> -            dev_dbg!(
> -                pdev.as_ref(),
> -                "PMU entry: {:02x?}\n",
> -                &data[i..][..entry_len]
> -            );
> +            dev_dbg!(dev, "PMU entry: {:02x?}\n", &data[i..][..entry_len]);
>          }
>  
>          Ok(PmuLookupTable {
> @@ -997,11 +978,11 @@ fn find_entry_by_type(&self, entry_type: u8) -> Result<PmuLookupTableEntry> {
>  impl FwSecBiosBuilder {
>      fn setup_falcon_data(
>          &mut self,
> -        pdev: &pci::Device,
> +        dev: &device::Device,
>          pci_at_image: &PciAtBiosImage,
>          first_fwsec: &FwSecBiosBuilder,
>      ) -> Result {
> -        let mut offset = pci_at_image.falcon_data_ptr(pdev)? as usize;
> +        let mut offset = pci_at_image.falcon_data_ptr(dev)? as usize;
>          let mut pmu_in_first_fwsec = false;
>  
>          // The falcon data pointer assumes that the PciAt and FWSEC images
> @@ -1025,9 +1006,9 @@ fn setup_falcon_data(
>  
>          if pmu_in_first_fwsec {
>              self.pmu_lookup_table =
> -                Some(PmuLookupTable::new(pdev, &first_fwsec.base.data[offset..])?);
> +                Some(PmuLookupTable::new(dev, &first_fwsec.base.data[offset..])?);
>          } else {
> -            self.pmu_lookup_table = Some(PmuLookupTable::new(pdev, &self.base.data[offset..])?);
> +            self.pmu_lookup_table = Some(PmuLookupTable::new(dev, &self.base.data[offset..])?);
>          }
>  
>          match self
> @@ -1040,18 +1021,14 @@ 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!(pdev.as_ref(), "Falcon Ucode offset not in second Fwsec.\n");
> +                    dev_err!(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!(
> -                    pdev.as_ref(),
> -                    "PmuLookupTableEntry not found, error: {:?}\n",
> -                    e
> -                );
> +                dev_err!(dev, "PmuLookupTableEntry not found, error: {:?}\n", e);
>                  return Err(EINVAL);
>              }
>          }
> @@ -1059,7 +1036,7 @@ fn setup_falcon_data(
>      }
>  
>      /// Build the final FwSecBiosImage from this builder
> -    fn build(self, pdev: &pci::Device) -> Result<FwSecBiosImage> {
> +    fn build(self, dev: &device::Device) -> Result<FwSecBiosImage> {
>          let ret = FwSecBiosImage {
>              base: self.base,
>              falcon_ucode_offset: self.falcon_ucode_offset.ok_or(EINVAL)?,
> @@ -1067,8 +1044,8 @@ fn build(self, pdev: &pci::Device) -> Result<FwSecBiosImage> {
>  
>          if cfg!(debug_assertions) {
>              // Print the desc header for debugging
> -            let desc = ret.header(pdev.as_ref())?;
> -            dev_dbg!(pdev.as_ref(), "PmuLookupTableEntry desc: {:#?}\n", desc);
> +            let desc = ret.header(dev)?;
> +            dev_dbg!(dev, "PmuLookupTableEntry desc: {:#?}\n", desc);
>          }
>  
>          Ok(ret)
> 
> -- 
> 2.50.1
> 

Re: [PATCH 2/2] gpu: nova-core: vbios: store reference to Device where relevant

[Joel Fernandes]

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 <acourbot@nvidia.com>

Reviewed-by: Joel Fernandes <joelagnelf@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
> 

Re: [PATCH 0/2] gpu: nova-core: vbios: simplify device use

[Danilo Krummrich]

On 8/8/25 4:46 AM, Alexandre Courbot wrote:
> This small cleanup series simplifies the use of `Device` in vbios
> methods.
> 
> The device is used for logging purposes only; thus we don't need a
> pci::Device, neither do we need it to be bound. This latter property
> allows us to store an `ARef` to it into structures that require logging
> instead of having all their methods take an extra `dev` argument.
> Removing this argument streamlines the code a bit.
> 
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>

Acked-by: Danilo Krummrich <dakr@kernel.org>

Re: [PATCH 0/2] gpu: nova-core: vbios: simplify device use

[Alexandre Courbot]

On Mon Sep 1, 2025 at 7:37 PM JST, Danilo Krummrich wrote:
> On 8/8/25 4:46 AM, Alexandre Courbot wrote:
>> This small cleanup series simplifies the use of `Device` in vbios
>> methods.
>> 
>> The device is used for logging purposes only; thus we don't need a
>> pci::Device, neither do we need it to be bound. This latter property
>> allows us to store an `ARef` to it into structures that require logging
>> instead of having all their methods take an extra `dev` argument.
>> Removing this argument streamlines the code a bit.
>> 
>> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
>
> Acked-by: Danilo Krummrich <dakr@kernel.org>

Thanks! Pushed into nova-next.