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

[Guenter Roeck <linux@roeck-us.net>]

On 8/14/21 8:05 AM, Pali Rohár wrote:
> 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.
> 

I prefer reverse christmas tree because I think it looks nicer, but
I don't usually enforce it because I think it is at least somewhat POV.
One initialization per line makes sense, though.

As for __maybe_unused and IS_ENABLED(), it is better because it ensures
that the code compiles. Otherwise, especially with debug code like this,
there is always the danger that other changes cause compile errors
if the flag is enabled ... but that isn't found because the flag isn't
enabled.

There is a significant difference here, though: The "#ifdef DEBUG" is replaced
with "IS_ENABLED(CONFIG_DEBUG)". So a local DEBUG define is replaced with
a global one (CONFIG_DEBUG). But there is no "config DEBUG" in any Kconfig file.
So, no, that doesn't work. We can't have local CONFIG_xxx defines because that
might end up conflicting with Kconfig defines.

I would suggest to just drop the #ifdef. The added cost is marginal compared
to the cost of the bios calls, and it may be useful to be able to use debug
output without having to recompile the code.

Guenter

>>
>> -	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
>>