Hi Danilo,
On 5/14/2025 12:23 PM, Danilo Krummrich wrote:
> I feel like this patch utilizes the Option type way too much and
> often without actual need. Can you please also double check?
>
I found one other instance (vbios.fwsec_image). Other than that, all others are
required AFAICS.
>> +
>> + if cfg!(debug_assertions) {
>> + // Debugging (dumps the table data to dmesg):
>> + if let Some(ref mut last_entry_bytes) = last_entry_bytes {
>> + last_entry_bytes.push(byte, GFP_KERNEL)?;
>> +
>> + let last_entry_bytes_len = last_entry_bytes.len();
>> + if last_entry_bytes_len == entry_len {
>> + pr_info!("Last entry bytes: {:02x?}\n",
>> &last_entry_bytes[..]);
>
> Please use dev_dbg!().
>
This required passing down the pdev here, but did that, thanks.
>> + *last_entry_bytes = KVec::new();
>> + }
>> + }
>> + }
>> + }
>> +
>> + Ok(PmuLookupTable {
>> + version: data[0],
>> + header_len: header_len as u8,
>> + entry_len: entry_len as u8,
>> + entry_count: entry_count as u8,
>> + table_data,
>> + })
>> + }
>> +
>> + fn lookup_index(&self, idx: u8) -> Result<PmuLookupTableEntry> {
>> + if idx >= self.entry_count {
>> + return Err(EINVAL);
>> + }
>> +
>> + let index = (idx as usize) * self.entry_len as usize;
>> + PmuLookupTableEntry::new(&self.table_data[index..])
>> + }
>> +
>> + // find entry by type value
>> + fn find_entry_by_type(&self, entry_type: u8) ->
>> Result<PmuLookupTableEntry> {
>> + for i in 0..self.entry_count {
>> + let entry = self.lookup_index(i)?;
>> + if entry.application_id == entry_type {
>> + return Ok(entry);
>> + }
>> + }
>> +
>> + Err(EINVAL)
>> + }
>> +}
>> +
>> +/// The FwSecBiosImage structure contains the PMU table and the Falcon
>> Ucode.
>> +/// The PMU table contains voltage/frequency tables as well as a pointer to
>> the
>> +/// Falcon Ucode.
>> +impl FwSecBiosImage {
>> + fn setup_falcon_data(
>> + &mut self,
>> + pdev: &pci::Device,
>> + pci_at_image: &PciAtBiosImage,
>> + first_fwsec_image: &FwSecBiosImage,
>> + ) -> Result<()> {
>
> Just Result will do.
>
Fixed.
>> + let mut offset = pci_at_image.falcon_data_ptr(pdev)? as usize;
>> +
>> + // The falcon data pointer assumes that the PciAt and FWSEC images
>> + // are contiguous in memory. However, testing shows the EFI image
>> sits in
>> + // between them. So calculate the offset from the end of the PciAt
>> image
>> + // rather than the start of it. Compensate.
>> + offset -= pci_at_image.base.data.len();
>> +
>> + // The offset is now from the start of the first Fwsec image,
>> however
>> + // the offset points to a location in the second Fwsec image. Since
>> + // the fwsec images are contiguous, subtract the length of the
>> first Fwsec
>> + // image from the offset to get the offset to the start of the
>> second
>> + // Fwsec image.
>> + offset -= first_fwsec_image.base.data.len();
>> +
>> + self.falcon_data_offset = Some(offset);
>> +
>> + // The PmuLookupTable starts at the offset of the falcon data
>> pointer
>> + self.pmu_lookup_table =
>> Some(PmuLookupTable::new(&self.base.data[offset..])?);
>> +
>> + match self
>> + .pmu_lookup_table
>> + .as_ref()
>> + .ok_or(EINVAL)?
>> + .find_entry_by_type(FALCON_UCODE_ENTRY_APPID_FWSEC_PROD)
>> + {
>> + Ok(entry) => {
>> + let mut ucode_offset = entry.data as usize;
>> + ucode_offset -= pci_at_image.base.data.len();
>> + ucode_offset -= first_fwsec_image.base.data.len();
>> + self.falcon_ucode_offset = Some(ucode_offset);
>> + if cfg!(debug_assertions) {
>> + // Print the v3_desc header for debugging
>> + let v3_desc = self.fwsec_header(pdev.as_ref())?;
>> + pr_info!("PmuLookupTableEntry v3_desc: {:#?}\n",
>> v3_desc);
>> + }
>> + }
>> + Err(e) => {
>> + dev_err!(
>> + pdev.as_ref(),
>> + "PmuLookupTableEntry not found, error: {:?}\n",
>> + e
>> + );
>> + }
>> + }
>> + Ok(())
>> + }
>> +
>> + /// TODO: These were borrowed from the old code for integrating this
>> module
>> + /// with the outside world. They should be cleaned up and integrated
>> properly.
>
> Okay, won't review for now then. 🙂
In the next revision, we are removing this TODO and can continue review. :)
thanks,
- Joel
