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@xxxxxx wrote:
> > > From: Armin Wolf <W_Armin@xxxxxx>
> > >
> > > Use IS_ENABLED() instead of #ifdef and use ktime_us_delta()
> > > for improved precision.
> > >
> > > Signed-off-by: Armin Wolf <W_Armin@xxxxxx>
> > > ---
> > > 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
> > >
>
