| Message ID | 20210814143637.11922-3-W_Armin@gmx.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | hwmon: (dell-smm) Misc cleanups | expand |
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 >
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 >>
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 > > > >
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; - 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); + pr_debug("smm(0x%.4x 0x%.4x) = 0x%.4x (took %7lld usecs)\n", eax, ebx, + (rc ? 0xffff : regs->eax & 0xffff), duration); + } return rc; }