[PATCH 0/4] hwmon: (dell-smm) Misc cleanups

[PATCH 0/4] hwmon: (dell-smm) Misc cleanups

[W_Armin]

From: Armin Wolf <W_Armin@gmx.de>

This patch series contains several small cleanups
for the dell-smm-hwmon driver.

While also improving the smm function debugging,
the only change noticable to userspace is
the 3rd patch.

All patches have been tested on a Dell Latitude C600.

Armin Wolf (4):
  hwmon: (dell-smm) Mark tables as __initconst
  hwmon: (dell-smm) Rework SMM function debugging
  hwmon: (dell-smm) Enable automatic fan speed control for all channels
  hwmon: (dell-smm) Mark i8k_get_fan_nominal_speed as __init

 Documentation/hwmon/dell-smm-hwmon.rst | 14 ++++-----
 drivers/hwmon/dell-smm-hwmon.c         | 40 +++++++++++---------------
 2 files changed, 24 insertions(+), 30 deletions(-)

--
2.20.1

[PATCH 1/4] hwmon: (dell-smm) Mark tables as __initconst

[W_Armin]

From: Armin Wolf <W_Armin@gmx.de>

Both the config and the DMI tables never change and
are only used during module init for setting up
the device data struct.
Mark all of them as const and __initconst for a
smaller runtime memory footprint.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/hwmon/dell-smm-hwmon.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
index 43da32ad2dce..68af95c1d90c 100644
--- a/drivers/hwmon/dell-smm-hwmon.c
+++ b/drivers/hwmon/dell-smm-hwmon.c
@@ -957,7 +957,7 @@ enum i8k_configs {
 	DELL_XPS,
 };

-static const struct i8k_config_data i8k_config_data[] = {
+static const struct i8k_config_data i8k_config_data[] __initconst = {
 	[DELL_LATITUDE_D520] = {
 		.fan_mult = 1,
 		.fan_max = I8K_FAN_TURBO,
@@ -1115,7 +1115,7 @@ static const struct dmi_system_id i8k_blacklist_fan_type_dmi_table[] __initconst
  * support for affected blacklisted Dell machines stay disabled.
  * See bug: https://bugzilla.kernel.org/show_bug.cgi?id=195751
  */
-static struct dmi_system_id i8k_blacklist_fan_support_dmi_table[] __initdata = {
+static const struct dmi_system_id i8k_blacklist_fan_support_dmi_table[] __initconst = {
 	{
 		.ident = "Dell Inspiron 7720",
 		.matches = {
@@ -1156,14 +1156,14 @@ enum i8k_fan_controls {
 	I8K_FAN_34A3_35A3,
 };

-static const struct i8k_fan_control_data i8k_fan_control_data[] = {
+static const struct i8k_fan_control_data i8k_fan_control_data[] __initconst = {
 	[I8K_FAN_34A3_35A3] = {
 		.manual_fan = 0x34a3,
 		.auto_fan = 0x35a3,
 	},
 };

-static struct dmi_system_id i8k_whitelist_fan_control[] __initdata = {
+static const struct dmi_system_id i8k_whitelist_fan_control[] __initconst = {
 	{
 		.ident = "Dell Latitude 5480",
 		.matches = {
--
2.20.1

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

[W_Armin]

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;

-	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;
 }
--
2.20.1

[PATCH 3/4] hwmon: (dell-smm) Enable automatic fan speed control for all channels

[W_Armin]

From: Armin Wolf <W_Armin@gmx.de>

Add automatic fan speed control for the remaining two pwm channels
since the pwmX_enable setting affects all pwm channels.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 Documentation/hwmon/dell-smm-hwmon.rst | 14 +++++++-------
 drivers/hwmon/dell-smm-hwmon.c         |  4 ++--
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/Documentation/hwmon/dell-smm-hwmon.rst b/Documentation/hwmon/dell-smm-hwmon.rst
index 3bf77a5df995..57b30fc9d03a 100644
--- a/Documentation/hwmon/dell-smm-hwmon.rst
+++ b/Documentation/hwmon/dell-smm-hwmon.rst
@@ -35,7 +35,7 @@ Name				Perm	Description
 fan[1-3]_input                  RO      Fan speed in RPM.
 fan[1-3]_label                  RO      Fan label.
 pwm[1-3]                        RW      Control the fan PWM duty-cycle.
-pwm1_enable                     WO      Enable or disable automatic BIOS fan
+pwm[1-3]_enable                 WO      Enable or disable automatic BIOS fan
                                         control (not supported on all laptops,
                                         see below for details).
 temp[1-10]_input                RO      Temperature reading in milli-degrees
@@ -52,13 +52,13 @@ overwritten.

 There is experimental support for disabling automatic BIOS fan
 control, at least on laptops where the corresponding SMM command is
-known, by writing the value ``1`` in the attribute ``pwm1_enable``
-(writing ``2`` enables automatic BIOS control again). Even if you have
+known, by writing the value ``1`` in the attribute ``pwm[1-3]_enable``
+(writing ``2`` enables automatic BIOS control again). If you have
 more than one fan, all of them are set to either enabled or disabled
-automatic fan control at the same time and, notwithstanding the name,
-``pwm1_enable`` sets automatic control for all fans.
+automatic fan control at the same time so ``pwm[1-3]_enable``
+sets automatic fan control for **all** fans.

-If ``pwm1_enable`` is not available, then it means that SMM codes for
+If ``pwm[1-3]_enable`` is not available, then it means that SMM codes for
 enabling and disabling automatic BIOS fan control are not whitelisted
 for your hardware. It is possible that codes that work for other
 laptops actually work for yours as well, or that you have to discover
@@ -67,7 +67,7 @@ new codes.
 Check the list ``i8k_whitelist_fan_control`` in file
 ``drivers/hwmon/dell-smm-hwmon.c`` in the kernel tree: as a first
 attempt you can try to add your machine and use an already-known code
-pair. If, after recompiling the kernel, you see that ``pwm1_enable``
+pair. If, after recompiling the kernel, you see that ``pwm[1-3]_enable``
 is present and works (i.e., you can manually control the fan speed),
 then please submit your finding as a kernel patch, so that other users
 can benefit from it. Please see
diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
index 3aa09c1e4b1d..0e229e3dae33 100644
--- a/drivers/hwmon/dell-smm-hwmon.c
+++ b/drivers/hwmon/dell-smm-hwmon.c
@@ -898,8 +898,8 @@ static const struct hwmon_channel_info *dell_smm_info[] = {
 			   ),
 	HWMON_CHANNEL_INFO(pwm,
 			   HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
-			   HWMON_PWM_INPUT,
-			   HWMON_PWM_INPUT
+			   HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
+			   HWMON_PWM_INPUT | HWMON_PWM_ENABLE
 			   ),
 	NULL
 };
--
2.20.1

[PATCH 4/4] hwmon: (dell-smm) Mark i8k_get_fan_nominal_speed as __init

[W_Armin]

From: Armin Wolf <W_Armin@gmx.de>

Mark function i8k_get_fan_nominal_speed() as __init since
it is only used in code also marked as __init.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/hwmon/dell-smm-hwmon.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
index 0e229e3dae33..8bbeeda13faf 100644
--- a/drivers/hwmon/dell-smm-hwmon.c
+++ b/drivers/hwmon/dell-smm-hwmon.c
@@ -303,7 +303,7 @@ static int i8k_get_fan_type(struct dell_smm_data *data, int fan)
 /*
  * Read the fan nominal rpm for specific fan speed.
  */
-static int i8k_get_fan_nominal_speed(const struct dell_smm_data *data, int fan, int speed)
+static int __init i8k_get_fan_nominal_speed(const struct dell_smm_data *data, int fan, int speed)
 {
 	struct smm_regs regs = { .eax = I8K_SMM_GET_NOM_SPEED, };

--
2.20.1

Re: [PATCH 1/4] hwmon: (dell-smm) Mark tables as __initconst

[Pali Rohár]

On Saturday 14 August 2021 16:36:34 W_Armin@gmx.de wrote:
> From: Armin Wolf <W_Armin@gmx.de>
> 
> Both the config and the DMI tables never change and
> are only used during module init for setting up
> the device data struct.
> Mark all of them as const and __initconst for a
> smaller runtime memory footprint.
> 
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>

Looks good,

Reviewed-by: Pali Rohár <pali@kernel.org>

> ---
>  drivers/hwmon/dell-smm-hwmon.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
> index 43da32ad2dce..68af95c1d90c 100644
> --- a/drivers/hwmon/dell-smm-hwmon.c
> +++ b/drivers/hwmon/dell-smm-hwmon.c
> @@ -957,7 +957,7 @@ enum i8k_configs {
>  	DELL_XPS,
>  };
> 
> -static const struct i8k_config_data i8k_config_data[] = {
> +static const struct i8k_config_data i8k_config_data[] __initconst = {
>  	[DELL_LATITUDE_D520] = {
>  		.fan_mult = 1,
>  		.fan_max = I8K_FAN_TURBO,
> @@ -1115,7 +1115,7 @@ static const struct dmi_system_id i8k_blacklist_fan_type_dmi_table[] __initconst
>   * support for affected blacklisted Dell machines stay disabled.
>   * See bug: https://bugzilla.kernel.org/show_bug.cgi?id=195751
>   */
> -static struct dmi_system_id i8k_blacklist_fan_support_dmi_table[] __initdata = {
> +static const struct dmi_system_id i8k_blacklist_fan_support_dmi_table[] __initconst = {
>  	{
>  		.ident = "Dell Inspiron 7720",
>  		.matches = {
> @@ -1156,14 +1156,14 @@ enum i8k_fan_controls {
>  	I8K_FAN_34A3_35A3,
>  };
> 
> -static const struct i8k_fan_control_data i8k_fan_control_data[] = {
> +static const struct i8k_fan_control_data i8k_fan_control_data[] __initconst = {
>  	[I8K_FAN_34A3_35A3] = {
>  		.manual_fan = 0x34a3,
>  		.auto_fan = 0x35a3,
>  	},
>  };
> 
> -static struct dmi_system_id i8k_whitelist_fan_control[] __initdata = {
> +static const struct dmi_system_id i8k_whitelist_fan_control[] __initconst = {
>  	{
>  		.ident = "Dell Latitude 5480",
>  		.matches = {
> --
> 2.20.1
> 

Re: [PATCH 4/4] hwmon: (dell-smm) Mark i8k_get_fan_nominal_speed as __init

[Pali Rohár]

On Saturday 14 August 2021 16:36:37 W_Armin@gmx.de wrote:
> From: Armin Wolf <W_Armin@gmx.de>
> 
> Mark function i8k_get_fan_nominal_speed() as __init since
> it is only used in code also marked as __init.
> 
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>

Looks good,

Reviewed-by: Pali Rohár <pali@kernel.org>

> ---
>  drivers/hwmon/dell-smm-hwmon.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
> index 0e229e3dae33..8bbeeda13faf 100644
> --- a/drivers/hwmon/dell-smm-hwmon.c
> +++ b/drivers/hwmon/dell-smm-hwmon.c
> @@ -303,7 +303,7 @@ static int i8k_get_fan_type(struct dell_smm_data *data, int fan)
>  /*
>   * Read the fan nominal rpm for specific fan speed.
>   */
> -static int i8k_get_fan_nominal_speed(const struct dell_smm_data *data, int fan, int speed)
> +static int __init i8k_get_fan_nominal_speed(const struct dell_smm_data *data, int fan, int speed)
>  {
>  	struct smm_regs regs = { .eax = I8K_SMM_GET_NOM_SPEED, };
> 
> --
> 2.20.1
> 

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

[Pali Rohár]

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
> 

Re: [PATCH 3/4] hwmon: (dell-smm) Enable automatic fan speed control for all channels

[Pali Rohár]

On Saturday 14 August 2021 16:36:36 W_Armin@gmx.de wrote:
> From: Armin Wolf <W_Armin@gmx.de>
> 
> Add automatic fan speed control for the remaining two pwm channels
> since the pwmX_enable setting affects all pwm channels.
> 
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>

This behavior was already rejected by Guenter:
https://lore.kernel.org/linux-hwmon/3a10f96a-06e1-39f4-74a6-908d25b1f496@roeck-us.net/

"Having three attributes do all the same is not very valuable.
I would suggest to stick with pwm1_enable and document that it applies
to all pwm channels."

> ---
>  Documentation/hwmon/dell-smm-hwmon.rst | 14 +++++++-------
>  drivers/hwmon/dell-smm-hwmon.c         |  4 ++--
>  2 files changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/Documentation/hwmon/dell-smm-hwmon.rst b/Documentation/hwmon/dell-smm-hwmon.rst
> index 3bf77a5df995..57b30fc9d03a 100644
> --- a/Documentation/hwmon/dell-smm-hwmon.rst
> +++ b/Documentation/hwmon/dell-smm-hwmon.rst
> @@ -35,7 +35,7 @@ Name				Perm	Description
>  fan[1-3]_input                  RO      Fan speed in RPM.
>  fan[1-3]_label                  RO      Fan label.
>  pwm[1-3]                        RW      Control the fan PWM duty-cycle.
> -pwm1_enable                     WO      Enable or disable automatic BIOS fan
> +pwm[1-3]_enable                 WO      Enable or disable automatic BIOS fan
>                                          control (not supported on all laptops,
>                                          see below for details).
>  temp[1-10]_input                RO      Temperature reading in milli-degrees
> @@ -52,13 +52,13 @@ overwritten.
> 
>  There is experimental support for disabling automatic BIOS fan
>  control, at least on laptops where the corresponding SMM command is
> -known, by writing the value ``1`` in the attribute ``pwm1_enable``
> -(writing ``2`` enables automatic BIOS control again). Even if you have
> +known, by writing the value ``1`` in the attribute ``pwm[1-3]_enable``
> +(writing ``2`` enables automatic BIOS control again). If you have
>  more than one fan, all of them are set to either enabled or disabled
> -automatic fan control at the same time and, notwithstanding the name,
> -``pwm1_enable`` sets automatic control for all fans.
> +automatic fan control at the same time so ``pwm[1-3]_enable``
> +sets automatic fan control for **all** fans.
> 
> -If ``pwm1_enable`` is not available, then it means that SMM codes for
> +If ``pwm[1-3]_enable`` is not available, then it means that SMM codes for
>  enabling and disabling automatic BIOS fan control are not whitelisted
>  for your hardware. It is possible that codes that work for other
>  laptops actually work for yours as well, or that you have to discover
> @@ -67,7 +67,7 @@ new codes.
>  Check the list ``i8k_whitelist_fan_control`` in file
>  ``drivers/hwmon/dell-smm-hwmon.c`` in the kernel tree: as a first
>  attempt you can try to add your machine and use an already-known code
> -pair. If, after recompiling the kernel, you see that ``pwm1_enable``
> +pair. If, after recompiling the kernel, you see that ``pwm[1-3]_enable``
>  is present and works (i.e., you can manually control the fan speed),
>  then please submit your finding as a kernel patch, so that other users
>  can benefit from it. Please see
> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
> index 3aa09c1e4b1d..0e229e3dae33 100644
> --- a/drivers/hwmon/dell-smm-hwmon.c
> +++ b/drivers/hwmon/dell-smm-hwmon.c
> @@ -898,8 +898,8 @@ static const struct hwmon_channel_info *dell_smm_info[] = {
>  			   ),
>  	HWMON_CHANNEL_INFO(pwm,
>  			   HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
> -			   HWMON_PWM_INPUT,
> -			   HWMON_PWM_INPUT
> +			   HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
> +			   HWMON_PWM_INPUT | HWMON_PWM_ENABLE
>  			   ),
>  	NULL
>  };
> --
> 2.20.1
> 

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

[Guenter Roeck]

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

Re: [PATCH 3/4] hwmon: (dell-smm) Enable automatic fan speed control for all channels

[Guenter Roeck]

On 8/14/21 8:13 AM, Pali Rohár wrote:
> On Saturday 14 August 2021 16:36:36 W_Armin@gmx.de wrote:
>> From: Armin Wolf <W_Armin@gmx.de>
>>
>> Add automatic fan speed control for the remaining two pwm channels
>> since the pwmX_enable setting affects all pwm channels.
>>
>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> 
> This behavior was already rejected by Guenter:
> https://lore.kernel.org/linux-hwmon/3a10f96a-06e1-39f4-74a6-908d25b1f496@roeck-us.net/
> 
> "Having three attributes do all the same is not very valuable.
> I would suggest to stick with pwm1_enable and document that it applies
> to all pwm channels."
> 

Yes. In situations like this I normally I suggest to have one read-write
attribute and have the others as read-only. However, that doesn't work here
because the attribute is write-only. Having more than one really doesn't
add value.

Guenter

>> ---
>>   Documentation/hwmon/dell-smm-hwmon.rst | 14 +++++++-------
>>   drivers/hwmon/dell-smm-hwmon.c         |  4 ++--
>>   2 files changed, 9 insertions(+), 9 deletions(-)
>>
>> diff --git a/Documentation/hwmon/dell-smm-hwmon.rst b/Documentation/hwmon/dell-smm-hwmon.rst
>> index 3bf77a5df995..57b30fc9d03a 100644
>> --- a/Documentation/hwmon/dell-smm-hwmon.rst
>> +++ b/Documentation/hwmon/dell-smm-hwmon.rst
>> @@ -35,7 +35,7 @@ Name				Perm	Description
>>   fan[1-3]_input                  RO      Fan speed in RPM.
>>   fan[1-3]_label                  RO      Fan label.
>>   pwm[1-3]                        RW      Control the fan PWM duty-cycle.
>> -pwm1_enable                     WO      Enable or disable automatic BIOS fan
>> +pwm[1-3]_enable                 WO      Enable or disable automatic BIOS fan
>>                                           control (not supported on all laptops,
>>                                           see below for details).
>>   temp[1-10]_input                RO      Temperature reading in milli-degrees
>> @@ -52,13 +52,13 @@ overwritten.
>>
>>   There is experimental support for disabling automatic BIOS fan
>>   control, at least on laptops where the corresponding SMM command is
>> -known, by writing the value ``1`` in the attribute ``pwm1_enable``
>> -(writing ``2`` enables automatic BIOS control again). Even if you have
>> +known, by writing the value ``1`` in the attribute ``pwm[1-3]_enable``
>> +(writing ``2`` enables automatic BIOS control again). If you have
>>   more than one fan, all of them are set to either enabled or disabled
>> -automatic fan control at the same time and, notwithstanding the name,
>> -``pwm1_enable`` sets automatic control for all fans.
>> +automatic fan control at the same time so ``pwm[1-3]_enable``
>> +sets automatic fan control for **all** fans.
>>
>> -If ``pwm1_enable`` is not available, then it means that SMM codes for
>> +If ``pwm[1-3]_enable`` is not available, then it means that SMM codes for
>>   enabling and disabling automatic BIOS fan control are not whitelisted
>>   for your hardware. It is possible that codes that work for other
>>   laptops actually work for yours as well, or that you have to discover
>> @@ -67,7 +67,7 @@ new codes.
>>   Check the list ``i8k_whitelist_fan_control`` in file
>>   ``drivers/hwmon/dell-smm-hwmon.c`` in the kernel tree: as a first
>>   attempt you can try to add your machine and use an already-known code
>> -pair. If, after recompiling the kernel, you see that ``pwm1_enable``
>> +pair. If, after recompiling the kernel, you see that ``pwm[1-3]_enable``
>>   is present and works (i.e., you can manually control the fan speed),
>>   then please submit your finding as a kernel patch, so that other users
>>   can benefit from it. Please see
>> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
>> index 3aa09c1e4b1d..0e229e3dae33 100644
>> --- a/drivers/hwmon/dell-smm-hwmon.c
>> +++ b/drivers/hwmon/dell-smm-hwmon.c
>> @@ -898,8 +898,8 @@ static const struct hwmon_channel_info *dell_smm_info[] = {
>>   			   ),
>>   	HWMON_CHANNEL_INFO(pwm,
>>   			   HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
>> -			   HWMON_PWM_INPUT,
>> -			   HWMON_PWM_INPUT
>> +			   HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
>> +			   HWMON_PWM_INPUT | HWMON_PWM_ENABLE
>>   			   ),
>>   	NULL
>>   };
>> --
>> 2.20.1
>>

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

[Pali Rohár]

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

Re: [PATCH 1/4] hwmon: (dell-smm) Mark tables as __initconst

[Guenter Roeck]

On Sat, Aug 14, 2021 at 04:36:34PM +0200, W_Armin@gmx.de wrote:
> From: Armin Wolf <W_Armin@gmx.de>
> 
> Both the config and the DMI tables never change and
> are only used during module init for setting up
> the device data struct.
> Mark all of them as const and __initconst for a
> smaller runtime memory footprint.
> 
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> Reviewed-by: Pali Rohár <pali@kernel.org>

Applied.

Thanks,
Guenter

> ---
>  drivers/hwmon/dell-smm-hwmon.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> --
> 2.20.1
> 
> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
> index 43da32ad2dce..68af95c1d90c 100644
> --- a/drivers/hwmon/dell-smm-hwmon.c
> +++ b/drivers/hwmon/dell-smm-hwmon.c
> @@ -957,7 +957,7 @@ enum i8k_configs {
>  	DELL_XPS,
>  };
> 
> -static const struct i8k_config_data i8k_config_data[] = {
> +static const struct i8k_config_data i8k_config_data[] __initconst = {
>  	[DELL_LATITUDE_D520] = {
>  		.fan_mult = 1,
>  		.fan_max = I8K_FAN_TURBO,
> @@ -1115,7 +1115,7 @@ static const struct dmi_system_id i8k_blacklist_fan_type_dmi_table[] __initconst
>   * support for affected blacklisted Dell machines stay disabled.
>   * See bug: https://bugzilla.kernel.org/show_bug.cgi?id=195751
>   */
> -static struct dmi_system_id i8k_blacklist_fan_support_dmi_table[] __initdata = {
> +static const struct dmi_system_id i8k_blacklist_fan_support_dmi_table[] __initconst = {
>  	{
>  		.ident = "Dell Inspiron 7720",
>  		.matches = {
> @@ -1156,14 +1156,14 @@ enum i8k_fan_controls {
>  	I8K_FAN_34A3_35A3,
>  };
> 
> -static const struct i8k_fan_control_data i8k_fan_control_data[] = {
> +static const struct i8k_fan_control_data i8k_fan_control_data[] __initconst = {
>  	[I8K_FAN_34A3_35A3] = {
>  		.manual_fan = 0x34a3,
>  		.auto_fan = 0x35a3,
>  	},
>  };
> 
> -static struct dmi_system_id i8k_whitelist_fan_control[] __initdata = {
> +static const struct dmi_system_id i8k_whitelist_fan_control[] __initconst = {
>  	{
>  		.ident = "Dell Latitude 5480",
>  		.matches = {

Re: [PATCH 4/4] hwmon: (dell-smm) Mark i8k_get_fan_nominal_speed as __init

[Guenter Roeck]

On Sat, Aug 14, 2021 at 04:36:37PM +0200, W_Armin@gmx.de wrote:
> From: Armin Wolf <W_Armin@gmx.de>
> 
> Mark function i8k_get_fan_nominal_speed() as __init since
> it is only used in code also marked as __init.
> 
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> Reviewed-by: Pali Rohár <pali@kernel.org>

Applied.

Thanks,
Guenter

> ---
>  drivers/hwmon/dell-smm-hwmon.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> --
> 2.20.1
> 
> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
> index 0e229e3dae33..8bbeeda13faf 100644
> --- a/drivers/hwmon/dell-smm-hwmon.c
> +++ b/drivers/hwmon/dell-smm-hwmon.c
> @@ -303,7 +303,7 @@ static int i8k_get_fan_type(struct dell_smm_data *data, int fan)
>  /*
>   * Read the fan nominal rpm for specific fan speed.
>   */
> -static int i8k_get_fan_nominal_speed(const struct dell_smm_data *data, int fan, int speed)
> +static int __init i8k_get_fan_nominal_speed(const struct dell_smm_data *data, int fan, int speed)
>  {
>  	struct smm_regs regs = { .eax = I8K_SMM_GET_NOM_SPEED, };