On 6/5/2025 12:21 PM, Danilo Krummrich wrote:
> On Thu, Jun 05, 2025 at 12:09:46PM -0400, Joel Fernandes wrote:
>>>> +impl PmuLookupTable {
>>>> + fn new(pdev: &pci::Device, data: &[u8]) -> Result<Self> {
>>>> + if data.len() < 4 {
>>>> + return Err(EINVAL);
>>>> + }
>>>> +
>>>> + let header_len = data[1] as usize;
>>>> + let entry_len = data[2] as usize;
>>>> + let entry_count = data[3] as usize;
>>>> +
>>>> + 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"
>>>> + );
>>>> + return Err(EINVAL);
>>>> + }
>>>> +
>>>> + // Create a copy of only the table data
>>>> + let table_data = {
>>>> + let mut ret = KVec::new();
>>>> + ret.extend_from_slice(&data[header_len..required_bytes],
>>>> GFP_KERNEL)?;
>>>> + ret
>>>> + };
>>>> +
>>>> + // Debug logging of entries (dumps the table data to dmesg)
>>>> + if cfg!(debug_assertions) {
>>>> + for i in (header_len..required_bytes).step_by(entry_len) {
>>>> + dev_dbg!(
>>>> + pdev.as_ref(),
>>>> + "PMU entry: {:02x?}\n",
>>>> + &data[i..][..entry_len]
>>>> + );
>>>> + }
>>>> + }
>>>
>>> Not sure this makes sense - debug_assertions is supposed to be about
>>> assertions, we probably shouldn't try to use it for other things (especially
>>> since we've already got dev_dbg! here)
>>
>> This was suggested by Danilo. I don't really feel strongly either way, IMO I
>> am
>> also Ok with running it in production.
>
> When I suggested this, the code looked like this:
>
> // "last_entry_bytes" is a debugging aid.
> // let mut last_entry_bytes: Option<KVec<u8>> = Some(KVec::new());
>
> for &byte in &data[header_len..required_bytes] {
> table_data.push(byte, GFP_KERNEL)?;
> /*
> * Uncomment for debugging (dumps the table data to dmesg):
> * last_entry_bytes.as_mut().ok_or(EINVAL)?.push(byte, GFP_KERNEL)?;
> *
> * let last_entry_bytes_len =
> last_entry_bytes.as_ref().ok_or(EINVAL)?.len();
> * if last_entry_bytes_len == entry_len {
> * pr_info!("Last entry bytes: {:02x?}\n",
> &last_entry_bytes.as_ref().ok_or(EINVAL)?[..]);
> * last_entry_bytes = Some(KVec::new());
> * }
> */
> }
>
> Now the compiler probably optimizes the loop away, since dev_dbg!() turns
> into a
> noop. So, now we can indeed probably remove it.
Makes sense, Ok then I'll drop the "if cfg!(debug_assertions)".
- Joel
