[PATCH v2 0/5] hwmon: (dell-smm) Miscellaneous Improvments

[PATCH v2 0/5] hwmon: (dell-smm) Miscellaneous Improvments

[W_Armin]

From: Armin Wolf <W_Armin@gmx.de>

This patch series contains various improvments to the
dell-smm-hwmon driver. The first patch is merely a cosmetic
change. The next two patches improve the ioctl handling
and have been tested with i8kutils (the only program using
this interface). The fourth patch adds some clarification
about i8k_config_data[], and the last patch
accelerates the setting of fan speed and has been tested as well
over the legacy interface and the hwmon sysfs interface.

Tested on a Dell Inspiron 3505, the Dell Latitude C600 is
currently out-of-service.
---
Changes in v2:
- drop patch 4 and instead add a comment on when to use
i8k_config_data[]
- reword patch 5

Armin Wolf (5):
  hwmon: (dell-smm) Sort includes in alphabetical order
  hwmon: (dell-smm) Use strscpy_pad()
  hwmon: (dell-smm) Return -ENOIOCTLCMD instead of -EINVAL
  hwmon: (dell-smm) Add comment explaining usage of i8k_config_data[]
  hwmon: (dell-smm) Speed up setting of fan speed

 drivers/hwmon/dell-smm-hwmon.c | 40 +++++++++++++++++++++-------------
 1 file changed, 25 insertions(+), 15 deletions(-)

--
2.20.1

[PATCH v2 1/5] hwmon: (dell-smm) Sort includes in alphabetical order

[W_Armin]

From: Armin Wolf <W_Armin@gmx.de>

Sort includes for better overview.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
Acked-by: Pali Rohár <pali@kernel.org>
---
 drivers/hwmon/dell-smm-hwmon.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
index af0d0d2b6e99..9773d6c0477a 100644
--- a/drivers/hwmon/dell-smm-hwmon.c
+++ b/drivers/hwmon/dell-smm-hwmon.c
@@ -12,22 +12,22 @@

 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

+#include <linux/capability.h>
 #include <linux/cpu.h>
+#include <linux/ctype.h>
 #include <linux/delay.h>
+#include <linux/dmi.h>
 #include <linux/err.h>
+#include <linux/hwmon.h>
+#include <linux/init.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/platform_device.h>
-#include <linux/types.h>
-#include <linux/init.h>
 #include <linux/proc_fs.h>
 #include <linux/seq_file.h>
-#include <linux/dmi.h>
-#include <linux/capability.h>
-#include <linux/mutex.h>
-#include <linux/hwmon.h>
-#include <linux/uaccess.h>
-#include <linux/ctype.h>
 #include <linux/smp.h>
+#include <linux/types.h>
+#include <linux/uaccess.h>

 #include <linux/i8k.h>

--
2.20.1

[PATCH v2 2/5] hwmon: (dell-smm) Use strscpy_pad()

[W_Armin]

From: Armin Wolf <W_Armin@gmx.de>

Using strscpy_pad() allows for fewer memory accesses
since memset() will not unconditionally zero-out
the whole buffer.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
Acked-by: Pali Rohár <pali@kernel.org>
---
 drivers/hwmon/dell-smm-hwmon.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
index 9773d6c0477a..b0c591bb761a 100644
--- a/drivers/hwmon/dell-smm-hwmon.c
+++ b/drivers/hwmon/dell-smm-hwmon.c
@@ -25,6 +25,7 @@
 #include <linux/platform_device.h>
 #include <linux/proc_fs.h>
 #include <linux/seq_file.h>
+#include <linux/string.h>
 #include <linux/smp.h>
 #include <linux/types.h>
 #include <linux/uaccess.h>
@@ -472,8 +473,7 @@ i8k_ioctl_unlocked(struct file *fp, struct dell_smm_data *data, unsigned int cmd
 		if (restricted && !capable(CAP_SYS_ADMIN))
 			return -EPERM;

-		memset(buff, 0, sizeof(buff));
-		strscpy(buff, data->bios_machineid, sizeof(buff));
+		strscpy_pad(buff, data->bios_machineid, sizeof(buff));
 		break;

 	case I8K_FN_STATUS:
--
2.20.1

[PATCH v2 3/5] hwmon: (dell-smm) Return -ENOIOCTLCMD instead of -EINVAL

[W_Armin]

From: Armin Wolf <W_Armin@gmx.de>

Returning -ENOIOCTLCMD gives the callers a better
hint of what went wrong and is the recommended
behavior.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
Acked-by: Pali Rohár <pali@kernel.org>
---
 drivers/hwmon/dell-smm-hwmon.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
index b0c591bb761a..5f0338b4a717 100644
--- a/drivers/hwmon/dell-smm-hwmon.c
+++ b/drivers/hwmon/dell-smm-hwmon.c
@@ -18,6 +18,7 @@
 #include <linux/delay.h>
 #include <linux/dmi.h>
 #include <linux/err.h>
+#include <linux/errno.h>
 #include <linux/hwmon.h>
 #include <linux/init.h>
 #include <linux/module.h>
@@ -516,7 +517,7 @@ i8k_ioctl_unlocked(struct file *fp, struct dell_smm_data *data, unsigned int cmd
 		break;

 	default:
-		return -EINVAL;
+		return -ENOIOCTLCMD;
 	}

 	if (val < 0)
--
2.20.1

[PATCH v2 4/5] hwmon: (dell-smm) Add comment explaining usage of i8k_config_data[]

[W_Armin]

From: Armin Wolf <W_Armin@gmx.de>

i8k_config_data[] should only be used for applying device specific
quirks in case autoconfig does not work properly on certain
devices.

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

diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
index 5f0338b4a717..0e1bc3a2dd12 100644
--- a/drivers/hwmon/dell-smm-hwmon.c
+++ b/drivers/hwmon/dell-smm-hwmon.c
@@ -996,6 +996,11 @@ enum i8k_configs {
 	DELL_XPS,
 };

+/*
+ * Only use for machines which need some special configuration
+ * in order to work correctly (e.g. if autoconfig fails on this machines).
+ */
+
 static const struct i8k_config_data i8k_config_data[] __initconst = {
 	[DELL_LATITUDE_D520] = {
 		.fan_mult = 1,
--
2.20.1

Re: [PATCH v2 4/5] hwmon: (dell-smm) Add comment explaining usage of i8k_config_data[]

[Pali Rohár]

On Thursday 21 October 2021 21:05:30 W_Armin@gmx.de wrote:
> From: Armin Wolf <W_Armin@gmx.de>
> 
> i8k_config_data[] should only be used for applying device specific
> quirks in case autoconfig does not work properly on certain
> devices.
> 
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>

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

> ---
>  drivers/hwmon/dell-smm-hwmon.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
> index 5f0338b4a717..0e1bc3a2dd12 100644
> --- a/drivers/hwmon/dell-smm-hwmon.c
> +++ b/drivers/hwmon/dell-smm-hwmon.c
> @@ -996,6 +996,11 @@ enum i8k_configs {
>  	DELL_XPS,
>  };
> 
> +/*
> + * Only use for machines which need some special configuration
> + * in order to work correctly (e.g. if autoconfig fails on this machines).
> + */
> +
>  static const struct i8k_config_data i8k_config_data[] __initconst = {
>  	[DELL_LATITUDE_D520] = {
>  		.fan_mult = 1,
> --
> 2.20.1
> 

[PATCH v2 5/5] hwmon: (dell-smm) Speed up setting of fan speed

[W_Armin]

From: Armin Wolf <W_Armin@gmx.de>

When setting the fan speed, i8k_set_fan() calls i8k_get_fan_status(),
causing an unnecessary SMM call since from the two users of this
function, only i8k_ioctl_unlocked() needs to know the new fan status
while dell_smm_write() ignores the new fan status.
Since SMM calls can be very slow while also making error reporting
difficult for dell_smm_write(), remove the function call from
i8k_set_fan() and call it separately in i8k_ioctl_unlocked().

Tested on a Dell Inspiron 3505.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
Reviewed-by: Pali Rohár <pali@kernel.org>
---
 drivers/hwmon/dell-smm-hwmon.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
index 0e1bc3a2dd12..eaace478f508 100644
--- a/drivers/hwmon/dell-smm-hwmon.c
+++ b/drivers/hwmon/dell-smm-hwmon.c
@@ -327,7 +327,7 @@ static int i8k_enable_fan_auto_mode(const struct dell_smm_data *data, bool enabl
 }

 /*
- * Set the fan speed (off, low, high). Returns the new fan status.
+ * Set the fan speed (off, low, high, ...).
  */
 static int i8k_set_fan(const struct dell_smm_data *data, int fan, int speed)
 {
@@ -339,7 +339,7 @@ static int i8k_set_fan(const struct dell_smm_data *data, int fan, int speed)
 	speed = (speed < 0) ? 0 : ((speed > data->i8k_fan_max) ? data->i8k_fan_max : speed);
 	regs.ebx = (fan & 0xff) | (speed << 8);

-	return i8k_smm(&regs) ? : i8k_get_fan_status(data, fan);
+	return i8k_smm(&regs);
 }

 static int __init i8k_get_temp_type(int sensor)
@@ -453,7 +453,7 @@ static int
 i8k_ioctl_unlocked(struct file *fp, struct dell_smm_data *data, unsigned int cmd, unsigned long arg)
 {
 	int val = 0;
-	int speed;
+	int speed, err;
 	unsigned char buff[16];
 	int __user *argp = (int __user *)arg;

@@ -513,7 +513,11 @@ i8k_ioctl_unlocked(struct file *fp, struct dell_smm_data *data, unsigned int cmd
 		if (copy_from_user(&speed, argp + 1, sizeof(int)))
 			return -EFAULT;

-		val = i8k_set_fan(data, val, speed);
+		err = i8k_set_fan(data, val, speed);
+		if (err < 0)
+			return err;
+
+		val = i8k_get_fan_status(data, val);
 		break;

 	default:
--
2.20.1

Re: [PATCH v2 0/5] hwmon: (dell-smm) Miscellaneous Improvments

[Guenter Roeck]

On Thu, Oct 21, 2021 at 09:05:26PM +0200, W_Armin@gmx.de wrote:
> From: Armin Wolf <W_Armin@gmx.de>
> 
> This patch series contains various improvments to the
> dell-smm-hwmon driver. The first patch is merely a cosmetic
> change. The next two patches improve the ioctl handling
> and have been tested with i8kutils (the only program using
> this interface). The fourth patch adds some clarification
> about i8k_config_data[], and the last patch
> accelerates the setting of fan speed and has been tested as well
> over the legacy interface and the hwmon sysfs interface.
> 
> Tested on a Dell Inspiron 3505, the Dell Latitude C600 is
> currently out-of-service.

Series applied.

Thanks,
Guenter

> ---
> Changes in v2:
> - drop patch 4 and instead add a comment on when to use
> i8k_config_data[]
> - reword patch 5
> 
> Armin Wolf (5):
>   hwmon: (dell-smm) Sort includes in alphabetical order
>   hwmon: (dell-smm) Use strscpy_pad()
>   hwmon: (dell-smm) Return -ENOIOCTLCMD instead of -EINVAL
>   hwmon: (dell-smm) Add comment explaining usage of i8k_config_data[]
>   hwmon: (dell-smm) Speed up setting of fan speed
> 
>  drivers/hwmon/dell-smm-hwmon.c | 40 +++++++++++++++++++++-------------
>  1 file changed, 25 insertions(+), 15 deletions(-)
> 
> --
> 2.20.1
>