* [PATCH 0/2] gpu: nova-core: vbios: simplify device use
@ 2025-08-08 2:46 Alexandre Courbot
2025-08-08 2:46 ` [PATCH 1/2] gpu: nova-core: vbios: replace pci::Device with device::Device Alexandre Courbot
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Alexandre Courbot @ 2025-08-08 2:46 UTC (permalink / raw)
To: Danilo Krummrich, David Airlie, Simona Vetter, Joel Fernandes
Cc: nouveau, dri-devel, rust-for-linux, linux-kernel,
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>
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH 1/2] gpu: nova-core: vbios: replace pci::Device with device::Device 2025-08-08 2:46 [PATCH 0/2] gpu: nova-core: vbios: simplify device use Alexandre Courbot @ 2025-08-08 2:46 ` Alexandre Courbot 2025-08-29 13:43 ` Joel Fernandes 2025-08-08 2:46 ` [PATCH 2/2] gpu: nova-core: vbios: store reference to Device where relevant Alexandre Courbot 2025-09-01 10:37 ` [PATCH 0/2] gpu: nova-core: vbios: simplify device use Danilo Krummrich 2 siblings, 1 reply; 7+ messages in thread From: Alexandre Courbot @ 2025-08-08 2:46 UTC (permalink / raw) To: Danilo Krummrich, David Airlie, Simona Vetter, Joel Fernandes Cc: nouveau, dri-devel, rust-for-linux, linux-kernel, 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] gpu: nova-core: vbios: replace pci::Device with device::Device 2025-08-08 2:46 ` [PATCH 1/2] gpu: nova-core: vbios: replace pci::Device with device::Device Alexandre Courbot @ 2025-08-29 13:43 ` Joel Fernandes 0 siblings, 0 replies; 7+ messages in thread From: Joel Fernandes @ 2025-08-29 13:43 UTC (permalink / raw) To: Alexandre Courbot Cc: Danilo Krummrich, David Airlie, Simona Vetter, nouveau, dri-devel, rust-for-linux, linux-kernel 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 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 2/2] gpu: nova-core: vbios: store reference to Device where relevant 2025-08-08 2:46 [PATCH 0/2] gpu: nova-core: vbios: simplify device use Alexandre Courbot 2025-08-08 2:46 ` [PATCH 1/2] gpu: nova-core: vbios: replace pci::Device with device::Device Alexandre Courbot @ 2025-08-08 2:46 ` Alexandre Courbot 2025-08-29 13:43 ` Joel Fernandes 2025-09-01 10:37 ` [PATCH 0/2] gpu: nova-core: vbios: simplify device use Danilo Krummrich 2 siblings, 1 reply; 7+ messages in thread From: Alexandre Courbot @ 2025-08-08 2:46 UTC (permalink / raw) To: Danilo Krummrich, David Airlie, Simona Vetter, Joel Fernandes Cc: nouveau, dri-devel, rust-for-linux, linux-kernel, 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] gpu: nova-core: vbios: store reference to Device where relevant 2025-08-08 2:46 ` [PATCH 2/2] gpu: nova-core: vbios: store reference to Device where relevant Alexandre Courbot @ 2025-08-29 13:43 ` Joel Fernandes 0 siblings, 0 replies; 7+ messages in thread From: Joel Fernandes @ 2025-08-29 13:43 UTC (permalink / raw) To: Alexandre Courbot Cc: Danilo Krummrich, David Airlie, Simona Vetter, nouveau, dri-devel, rust-for-linux, linux-kernel 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 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 0/2] gpu: nova-core: vbios: simplify device use 2025-08-08 2:46 [PATCH 0/2] gpu: nova-core: vbios: simplify device use Alexandre Courbot 2025-08-08 2:46 ` [PATCH 1/2] gpu: nova-core: vbios: replace pci::Device with device::Device Alexandre Courbot 2025-08-08 2:46 ` [PATCH 2/2] gpu: nova-core: vbios: store reference to Device where relevant Alexandre Courbot @ 2025-09-01 10:37 ` Danilo Krummrich 2025-09-01 13:30 ` Alexandre Courbot 2 siblings, 1 reply; 7+ messages in thread From: Danilo Krummrich @ 2025-09-01 10:37 UTC (permalink / raw) To: Alexandre Courbot Cc: David Airlie, Simona Vetter, Joel Fernandes, nouveau, dri-devel, rust-for-linux, linux-kernel 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> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 0/2] gpu: nova-core: vbios: simplify device use 2025-09-01 10:37 ` [PATCH 0/2] gpu: nova-core: vbios: simplify device use Danilo Krummrich @ 2025-09-01 13:30 ` Alexandre Courbot 0 siblings, 0 replies; 7+ messages in thread From: Alexandre Courbot @ 2025-09-01 13:30 UTC (permalink / raw) To: Danilo Krummrich Cc: David Airlie, Simona Vetter, Joel Fernandes, nouveau, dri-devel, rust-for-linux, linux-kernel 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2025-09-01 13:30 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-08-08 2:46 [PATCH 0/2] gpu: nova-core: vbios: simplify device use Alexandre Courbot 2025-08-08 2:46 ` [PATCH 1/2] gpu: nova-core: vbios: replace pci::Device with device::Device Alexandre Courbot 2025-08-29 13:43 ` Joel Fernandes 2025-08-08 2:46 ` [PATCH 2/2] gpu: nova-core: vbios: store reference to Device where relevant Alexandre Courbot 2025-08-29 13:43 ` Joel Fernandes 2025-09-01 10:37 ` [PATCH 0/2] gpu: nova-core: vbios: simplify device use Danilo Krummrich 2025-09-01 13:30 ` Alexandre Courbot
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).