Re: [PATCH 2/4] hwmon: (dell-smm) Rework SMM function debugging

["Pali Rohár" <pali@kernel.org>]

On Saturday 14 August 2021 16:36:35 W_Armin@gmx.de wrote:
> From: Armin Wolf <W_Armin@gmx.de>
> 
> Use IS_ENABLED() instead of #ifdef and use ktime_us_delta()
> for improved precision.
> 
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> ---
>  drivers/hwmon/dell-smm-hwmon.c | 26 ++++++++++----------------
>  1 file changed, 10 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
> index 68af95c1d90c..3aa09c1e4b1d 100644
> --- a/drivers/hwmon/dell-smm-hwmon.c
> +++ b/drivers/hwmon/dell-smm-hwmon.c
> @@ -158,17 +158,13 @@ static inline const char __init *i8k_get_dmi_data(int field)
>   */
>  static int i8k_smm_func(void *par)
>  {
> -	int rc;
>  	struct smm_regs *regs = par;
> -	int eax = regs->eax;
> -
> -#ifdef DEBUG
> -	int ebx = regs->ebx;
> -	unsigned long duration;
> -	ktime_t calltime, delta, rettime;
> +	int rc, eax = regs->eax, __maybe_unused ebx = regs->ebx;
> +	long long __maybe_unused duration;
> +	ktime_t __maybe_unused calltime;

I think that new coding style for declaring variables is
"reverse christmas tree", longer line before shorted line.

But here, I'm not sure if initializing more variables with its default
values should be at one line...

Also I'm not sure if usage of __maybe_unused is better than hiding
variable behind #ifdef. #ifdef guards variable to not be used when it
should not be.

> 
> -	calltime = ktime_get();
> -#endif
> +	if (IS_ENABLED(CONFIG_DEBUG))
> +		calltime = ktime_get();
> 
>  	/* SMM requires CPU 0 */
>  	if (smp_processor_id() != 0)
> @@ -230,13 +226,11 @@ static int i8k_smm_func(void *par)
>  	if (rc != 0 || (regs->eax & 0xffff) == 0xffff || regs->eax == eax)
>  		rc = -EINVAL;
> 
> -#ifdef DEBUG
> -	rettime = ktime_get();
> -	delta = ktime_sub(rettime, calltime);
> -	duration = ktime_to_ns(delta) >> 10;
> -	pr_debug("smm(0x%.4x 0x%.4x) = 0x%.4x  (took %7lu usecs)\n", eax, ebx,
> -		(rc ? 0xffff : regs->eax & 0xffff), duration);
> -#endif
> +	if (IS_ENABLED(CONFIG_DEBUG)) {
> +		duration = ktime_us_delta(ktime_get(), calltime);

But I like this ktime_us_delta() as it fixed conversion from ns to us!
Current conversion is incorrect (>>10 is /1024; but it should be /1000).

> +		pr_debug("smm(0x%.4x 0x%.4x) = 0x%.4x  (took %7lld usecs)\n", eax, ebx,
> +			 (rc ? 0xffff : regs->eax & 0xffff), duration);
> +	}
> 
>  	return rc;
>  }
> --
> 2.20.1
>