| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20250829134307.GA1832415@joelbox2>
Date: Fri, 29 Aug 2025 09:43:07 -0400
From: Joel Fernandes <joelagnelf@...dia.com>
To: Alexandre Courbot <acourbot@...dia.com>
Cc: Danilo Krummrich <dakr@...nel.org>, David Airlie <airlied@...il.com>,
Simona Vetter <simona@...ll.ch>, nouveau@...ts.freedesktop.org,
dri-devel@...ts.freedesktop.org, rust-for-linux@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] gpu: nova-core: vbios: replace pci::Device with
device::Device
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@...dia.com>
Reviewed-by: Joel Fernandes <joelagnelf@...dia.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
>
Powered by blists - more mailing lists