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.
