Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] rust: dev_dbg!: unconditional evaluation of formatting arguments - [Low] rust: Device::printk missing #[inline] attribute --
commit b813060ef1de2d7eca47689d919683f6792237c4 Author: Thomas Weißschuh <[email protected]> Subject: rust: device: remove logging methods This commit removes the logging wrapper methods from the Device struct, moving the logic into macros that directly call Device::printk. This change allows the dynamic debug core to statically allocate descriptors at the macro callsite. > diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs > index 2291d85b6849e..2d49063f4f7c8 100644 > --- a/rust/kernel/device.rs > +++ b/rust/kernel/device.rs [ ... ] > @@ -387,7 +305,7 @@ pub fn pr_dbg(&self, args: fmt::Arguments<'_>) { > /// Callers must ensure that `klevel` is null-terminated; in particular, > one of the > /// `KERN_*`constants, for example, `KERN_CRIT`, `KERN_ALERT`, etc. > #[cfg_attr(not(CONFIG_PRINTK), allow(unused_variables))] > - unsafe fn printk(&self, klevel: &[u8], msg: fmt::Arguments<'_>) { > + pub unsafe fn printk(&self, klevel: &[u8], msg: fmt::Arguments<'_>) { [Severity: Low] Is it intentional to omit the #[inline] attribute on this function? According to the Rust subsystem guidelines, small abstraction functions that forward to C bindings should be annotated with #[inline] to ensure optimal inlining. > // SAFETY: `klevel` is null-terminated and one of the kernel > constants. `self.as_raw` > // is valid because `self` is valid. The "%pA" format string expects > a pointer to [ ... ] > @@ -948,5 +915,14 @@ macro_rules! dev_info { > /// ``` > #[macro_export] > macro_rules! dev_dbg { > - ($($f:tt)*) => { $crate::dev_printk!(pr_dbg, $($f)*) } > + ($dev:expr, $($f:tt)*) => { > + match (&$dev, $crate::prelude::fmt!($($f)*)) { > + (dev, args) => { > + if cfg!(debug_assertions) { [Severity: Medium] Does this match block unconditionally evaluate the formatting arguments before the debug_assertions check is performed? Because the match scrutinee evaluates fmt!($($f)*) eagerly, any computationally expensive operations or side effects in the arguments might be executed even in release builds. Could the if cfg!(debug_assertions) check wrap the entire match block instead to ensure zero overhead when debugging is disabled? > + // SAFETY: `klevel` is null-terminated, uses one of the > kernel constants. > + unsafe { > $crate::dev_printk!($crate::bindings::KERN_DEBUG, dev, args) } > + } > + } > + } > + } > } -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=10
