Re: [PATCH RFC 10/11] rust: device: remove logging methods

[sashiko-bot@kernel.org]

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 <linux@weissschuh.net>
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/20261011-rust-dyndbg-v1-0-832e4039b71f@weissschuh.net?part=10