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

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

On Saturday 14 August 2021 08:29:56 Guenter Roeck wrote:
> 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.

Make sense. Drop #if DEBUG. pr_debug can be already enabled / disabled
and runtime measuring time is not problematic. Also these smm calls are
not too frequent and I guess that smm call itself (when CPU is in SMM
mode) is much more longer than time measurement around.

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