On 6/2/2025 9:33 AM, Danilo Krummrich wrote:
>> + /// Try to find NPDE in the data, the NPDE is right after the PCIR.
>> + fn find_in_data(
>> + pdev: &pci::Device,
>> + data: &[u8],
>> + rom_header: &PciRomHeader,
>> + pcir: &PcirStruct,
>> + ) -> Option<Self> {
>> + // Calculate the offset where NPDE might be located
>> + // NPDE should be right after the PCIR structure, aligned to 16
>> bytes
>> + let pcir_offset = rom_header.pci_data_struct_offset as usize;
>> + let npde_start = (pcir_offset + pcir.pci_data_struct_len as usize +
>> 0x0F) & !0x0F;
>
> What's this magic offset and mask?
>
Oh, hmm. I had a comment on that above though ("NPDE should be right after the
PCIR structure, aligned to 16 bytes"), does that suffice? I could move the
comment further down.
>> +
>> + // Check if we have enough data
>> + if npde_start + 11 > data.len() {
>
> '+ 11'?
Good point, I replaced this and the above with core::mem::size_of::<Self>().
>
>> + dev_err!(pdev.as_ref(), "Not enough data for NPDE\n");
> BiosImageBase declares this as "NVIDIA PCI Data Extension (optional)". If it's
> really optional, why is this an error?
>
>> + return None;
>> + }
>> +
>> + // Try to create NPDE from the data
>> + NpdeStruct::new(pdev, &data[npde_start..])
>> + .inspect_err(|e| {
>> + dev_err!(pdev.as_ref(), "Error creating NpdeStruct:
>> {:?}\n", e);
>> + })
>> + .ok()
>
> So, this returns None if it's a real error. This indicates that the return
> type
> should just be Result<Option<Self>>.
I made NpdeStruct::new() return Option only for next revision since NPDE is
optional.
thanks,
- Joel
