[PATCH 0/7] hwmon: (dell-smm) Miscellaneous improvements

[PATCH 0/7] hwmon: (dell-smm) Miscellaneous improvements

[Armin Wolf]

This patch set contains miscellaneous improvements for the
dell_smm_hwmon driver.
The first two patches allow for testing known SMM code pairs
for automatic fan mode enable/disable without having to
manually edit and recompile the module, which makes testing
easier and is compatible with Secure Boot/Kernel Lockdown.
Because of the later, users cannot specify arbitrary SMM codes,
but are limited to two promising code pairs (for now).
The fifth patch improves the inline assembly code, so
saving/restoring the registers is done by the compiler,
including the flags register. As a side effect, it now works
on both 32 and 64 bit x86, while being faster too.
The remaining patches are smaller improvments.

All patches have been tested on a Dell Inspiron 3505.

Armin Wolf (7):
  hwmon: (dell-smm) Allow for specifying fan control method as module
    parameter
  hwmon: (dell-smm) Add additional fan mode command combination
  hwmon: (dell-smm) Make fan/temp sensor number a u8
  hwmon: (dell-smm) Improve temperature sensors detection
  hwmon: (dell-smm) Improve assembly code
  hwmon: (dell-smm) Add SMM interface documentation
  hwmon: (dell-smm) Reword and mark parameter "force" as unsafe

 .../admin-guide/kernel-parameters.txt         |   3 +
 Documentation/hwmon/dell-smm-hwmon.rst        | 202 +++++++++++++++++-
 drivers/hwmon/dell-smm-hwmon.c                | 178 +++++++--------
 3 files changed, 290 insertions(+), 93 deletions(-)

--
2.30.2

[PATCH 1/7] hwmon: (dell-smm) Allow for specifying fan control method as module parameter

[Armin Wolf]

Right now, the only way to test if setting manual/auto fan control works
is to edit and recompile the module, which may be too cumbersome for
the average user.
Allow for specifying the desired fan mode control method when loading
the module, but taint the kernel if so since there is the possibility
for strange side effects on non-whitelisted models.
Also update docs and kernel-parameters.txt accordingly.

Tested on a Dell Inspiron 3505.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 .../admin-guide/kernel-parameters.txt         |  3 +++
 Documentation/hwmon/dell-smm-hwmon.rst        | 21 ++++++++++------
 drivers/hwmon/dell-smm-hwmon.c                | 25 +++++++++++++------
 3 files changed, 35 insertions(+), 14 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index d68053db21cc..4f1b6c2b7ed1 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -968,6 +968,9 @@
 	dell_smm_hwmon.fan_max=
 			[HW] Maximum configurable fan speed.

+	dell_smm_hwmon.fan_mode_method=
+			[HW] Method to use for changing fan mode.
+
 	dfltcc=		[HW,S390]
 			Format: { on | off | def_only | inf_only | always }
 			on:       s390 zlib hardware support for compression on
diff --git a/Documentation/hwmon/dell-smm-hwmon.rst b/Documentation/hwmon/dell-smm-hwmon.rst
index beec88491171..564d99cda869 100644
--- a/Documentation/hwmon/dell-smm-hwmon.rst
+++ b/Documentation/hwmon/dell-smm-hwmon.rst
@@ -67,13 +67,16 @@ 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
 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``
-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
+As a first step, you can load the module with the module parameter
+``fan_mode_method`` set to 1 to test if your hardware works with
+an already know method for disabling automatic BIOS fan control.
+If ``pwm1_enable`` is now 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.
+Just add your model to the list ``i8k_whitelist_fan_control`` in
+file ``drivers/hwmon/dell-smm-hwmon.c`` in the kernel tree and use
+the already known code pair.
+Please read
 :ref:`Documentation/process/submitting-patches.rst <submittingpatches>`
 for information on submitting patches.

@@ -120,6 +123,10 @@ Module parameters
                    Maximum configurable fan speed. (default:
                    autodetect)

+* fan_mode_method:uint
+                   Method to use for changing fan mode (default:
+                   from whitelist)
+
 Legacy ``/proc`` interface
 --------------------------

diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
index 9949eeb79378..1c4cc516c8b2 100644
--- a/drivers/hwmon/dell-smm-hwmon.c
+++ b/drivers/hwmon/dell-smm-hwmon.c
@@ -111,6 +111,10 @@ static uint fan_max;
 module_param(fan_max, uint, 0);
 MODULE_PARM_DESC(fan_max, "Maximum configurable fan speed (default: autodetect)");

+static uint fan_mode_method;
+module_param_unsafe(fan_mode_method, uint, 0);
+MODULE_PARM_DESC(fan_mode_method, "Method to use for changing fan mode (default: from whitelist)");
+
 struct smm_regs {
 	unsigned int eax;
 	unsigned int ebx;
@@ -677,7 +681,7 @@ static umode_t dell_smm_is_visible(const void *drvdata, enum hwmon_sensor_types

 			break;
 		case hwmon_pwm_enable:
-			if (data->auto_fan)
+			if (data->auto_fan && data->manual_fan)
 				/*
 				 * There is no command for retrieve the current status
 				 * from BIOS, and userspace/firmware itself can change
@@ -1282,14 +1286,21 @@ static int __init dell_smm_probe(struct platform_device *pdev)
 	data->i8k_fan_max = fan_max ? : I8K_FAN_HIGH;	/* Must not be 0 */
 	data->i8k_pwm_mult = DIV_ROUND_UP(255, data->i8k_fan_max);

-	fan_control = dmi_first_match(i8k_whitelist_fan_control);
-	if (fan_control && fan_control->driver_data) {
-		const struct i8k_fan_control_data *control = fan_control->driver_data;
+	/* value specified via module param overrides whitelist */
+	if (fan_mode_method > 0 && fan_mode_method <= ARRAY_SIZE(i8k_fan_control_data)) {
+		data->manual_fan = i8k_fan_control_data[fan_mode_method - 1].manual_fan;
+		data->auto_fan = i8k_fan_control_data[fan_mode_method - 1].auto_fan;
+	} else {
+		fan_control = dmi_first_match(i8k_whitelist_fan_control);
+		if (fan_control && fan_control->driver_data) {
+			const struct i8k_fan_control_data *control = fan_control->driver_data;

-		data->manual_fan = control->manual_fan;
-		data->auto_fan = control->auto_fan;
-		dev_info(&pdev->dev, "enabling support for setting automatic/manual fan control\n");
+			data->manual_fan = control->manual_fan;
+			data->auto_fan = control->auto_fan;
+		}
 	}
+	if (data->manual_fan && data->auto_fan)
+		dev_info(&pdev->dev, "enabling support for setting automatic/manual fan control\n");

 	if (!fan_mult) {
 		/*
--
2.30.2

[PATCH 2/7] hwmon: (dell-smm) Add additional fan mode command combination

[Armin Wolf]

This data was retrieved from the dellfan utility
(https://github.com/clopez/dellfan).
The combination is currently not used by default
on any models, but users can still select it for
testing purposes.

Tested on a Dell Insprion 3505.

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

diff --git a/Documentation/hwmon/dell-smm-hwmon.rst b/Documentation/hwmon/dell-smm-hwmon.rst
index 564d99cda869..cfaee682a245 100644
--- a/Documentation/hwmon/dell-smm-hwmon.rst
+++ b/Documentation/hwmon/dell-smm-hwmon.rst
@@ -68,14 +68,15 @@ laptops actually work for yours as well, or that you have to discover
 new codes.

 As a first step, you can load the module with the module parameter
-``fan_mode_method`` set to 1 to test if your hardware works with
-an already know method for disabling automatic BIOS fan control.
+``fan_mode_method`` set to 1 or 2 to test if your hardware works with
+already know methods for disabling automatic BIOS fan control.
 If ``pwm1_enable`` is now 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.
 Just add your model to the list ``i8k_whitelist_fan_control`` in
 file ``drivers/hwmon/dell-smm-hwmon.c`` in the kernel tree and use
-the already known code pair.
+the already known code pairs (number of method equals the number of
+entry in ``i8k_fan_controls``).
 Please read
 :ref:`Documentation/process/submitting-patches.rst <submittingpatches>`
 for information on submitting patches.
diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
index 1c4cc516c8b2..3b49e55d060f 100644
--- a/drivers/hwmon/dell-smm-hwmon.c
+++ b/drivers/hwmon/dell-smm-hwmon.c
@@ -1182,10 +1182,15 @@ struct i8k_fan_control_data {
 };

 enum i8k_fan_controls {
+	I8K_FAN_30A3_31A3,
 	I8K_FAN_34A3_35A3,
 };

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

[PATCH 3/7] hwmon: (dell-smm) Make fan/temp sensor number a u8

[Armin Wolf]

Right now, we only use bits 0 to 7 of the fan/temp sensor number
by doing number & 0xff. Passing the value as a u8 makes this
step unnecessary. Also add checks to the ioctl handler since
users might get confused when passing 0x00000101 does the same
as passing 0x00000001.

Tested on a Dell Inspiron 3505.

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

diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
index 3b49e55d060f..a102034a1d38 100644
--- a/drivers/hwmon/dell-smm-hwmon.c
+++ b/drivers/hwmon/dell-smm-hwmon.c
@@ -21,6 +21,7 @@
 #include <linux/errno.h>
 #include <linux/hwmon.h>
 #include <linux/init.h>
+#include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/platform_device.h>
@@ -254,46 +255,52 @@ static int i8k_smm(struct smm_regs *regs)
 /*
  * Read the fan status.
  */
-static int i8k_get_fan_status(const struct dell_smm_data *data, int fan)
+static int i8k_get_fan_status(const struct dell_smm_data *data, u8 fan)
 {
-	struct smm_regs regs = { .eax = I8K_SMM_GET_FAN, };
+	struct smm_regs regs = {
+		.eax = I8K_SMM_GET_FAN,
+		.ebx = fan,
+	};

 	if (data->disallow_fan_support)
 		return -EINVAL;

-	regs.ebx = fan & 0xff;
 	return i8k_smm(&regs) ? : regs.eax & 0xff;
 }

 /*
  * Read the fan speed in RPM.
  */
-static int i8k_get_fan_speed(const struct dell_smm_data *data, int fan)
+static int i8k_get_fan_speed(const struct dell_smm_data *data, u8 fan)
 {
-	struct smm_regs regs = { .eax = I8K_SMM_GET_SPEED, };
+	struct smm_regs regs = {
+		.eax = I8K_SMM_GET_SPEED,
+		.ebx = fan,
+	};

 	if (data->disallow_fan_support)
 		return -EINVAL;

-	regs.ebx = fan & 0xff;
 	return i8k_smm(&regs) ? : (regs.eax & 0xffff) * data->i8k_fan_mult;
 }

 /*
  * Read the fan type.
  */
-static int _i8k_get_fan_type(const struct dell_smm_data *data, int fan)
+static int _i8k_get_fan_type(const struct dell_smm_data *data, u8 fan)
 {
-	struct smm_regs regs = { .eax = I8K_SMM_GET_FAN_TYPE, };
+	struct smm_regs regs = {
+		.eax = I8K_SMM_GET_FAN_TYPE,
+		.ebx = fan,
+	};

 	if (data->disallow_fan_support || data->disallow_fan_type_call)
 		return -EINVAL;

-	regs.ebx = fan & 0xff;
 	return i8k_smm(&regs) ? : regs.eax & 0xff;
 }

-static int i8k_get_fan_type(struct dell_smm_data *data, int fan)
+static int i8k_get_fan_type(struct dell_smm_data *data, u8 fan)
 {
 	/* I8K_SMM_GET_FAN_TYPE SMM call is expensive, so cache values */
 	if (data->fan_type[fan] == INT_MIN)
@@ -305,14 +312,16 @@ static int i8k_get_fan_type(struct dell_smm_data *data, int fan)
 /*
  * Read the fan nominal rpm for specific fan speed.
  */
-static int __init 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, u8 fan, int speed)
 {
-	struct smm_regs regs = { .eax = I8K_SMM_GET_NOM_SPEED, };
+	struct smm_regs regs = {
+		.eax = I8K_SMM_GET_NOM_SPEED,
+		.ebx = fan | (speed << 8),
+	};

 	if (data->disallow_fan_support)
 		return -EINVAL;

-	regs.ebx = (fan & 0xff) | (speed << 8);
 	return i8k_smm(&regs) ? : (regs.eax & 0xffff) * data->i8k_fan_mult;
 }

@@ -333,7 +342,7 @@ static int i8k_enable_fan_auto_mode(const struct dell_smm_data *data, bool enabl
 /*
  * Set the fan speed (off, low, high, ...).
  */
-static int i8k_set_fan(const struct dell_smm_data *data, int fan, int speed)
+static int i8k_set_fan(const struct dell_smm_data *data, u8 fan, int speed)
 {
 	struct smm_regs regs = { .eax = I8K_SMM_SET_FAN, };

@@ -341,33 +350,35 @@ static int i8k_set_fan(const struct dell_smm_data *data, int fan, int speed)
 		return -EINVAL;

 	speed = (speed < 0) ? 0 : ((speed > data->i8k_fan_max) ? data->i8k_fan_max : speed);
-	regs.ebx = (fan & 0xff) | (speed << 8);
+	regs.ebx = fan | (speed << 8);

 	return i8k_smm(&regs);
 }

-static int __init i8k_get_temp_type(int sensor)
+static int __init i8k_get_temp_type(u8 sensor)
 {
-	struct smm_regs regs = { .eax = I8K_SMM_GET_TEMP_TYPE, };
+	struct smm_regs regs = {
+		.eax = I8K_SMM_GET_TEMP_TYPE,
+		.ebx = sensor,
+	};

-	regs.ebx = sensor & 0xff;
 	return i8k_smm(&regs) ? : regs.eax & 0xff;
 }

 /*
  * Read the cpu temperature.
  */
-static int _i8k_get_temp(int sensor)
+static int _i8k_get_temp(u8 sensor)
 {
 	struct smm_regs regs = {
 		.eax = I8K_SMM_GET_TEMP,
-		.ebx = sensor & 0xff,
+		.ebx = sensor,
 	};

 	return i8k_smm(&regs) ? : regs.eax & 0xff;
 }

-static int i8k_get_temp(int sensor)
+static int i8k_get_temp(u8 sensor)
 {
 	int temp = _i8k_get_temp(sensor);

@@ -500,6 +511,9 @@ static long i8k_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
 		if (copy_from_user(&val, argp, sizeof(int)))
 			return -EFAULT;

+		if (val > U8_MAX || val < 0)
+			return -EINVAL;
+
 		val = i8k_get_fan_speed(data, val);
 		break;

@@ -507,6 +521,9 @@ static long i8k_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
 		if (copy_from_user(&val, argp, sizeof(int)))
 			return -EFAULT;

+		if (val > U8_MAX || val < 0)
+			return -EINVAL;
+
 		val = i8k_get_fan_status(data, val);
 		break;

@@ -517,6 +534,9 @@ static long i8k_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
 		if (copy_from_user(&val, argp, sizeof(int)))
 			return -EFAULT;

+		if (val > U8_MAX || val < 0)
+			return -EINVAL;
+
 		if (copy_from_user(&speed, argp + 1, sizeof(int)))
 			return -EFAULT;

@@ -924,7 +944,8 @@ static int __init dell_smm_init_hwmon(struct device *dev)
 {
 	struct dell_smm_data *data = dev_get_drvdata(dev);
 	struct device *dell_smm_hwmon_dev;
-	int i, state, err;
+	int state, err;
+	u8 i;

 	for (i = 0; i < DELL_SMM_NO_TEMP; i++) {
 		data->temp_type[i] = i8k_get_temp_type(i);
@@ -1245,7 +1266,8 @@ static int __init dell_smm_probe(struct platform_device *pdev)
 {
 	struct dell_smm_data *data;
 	const struct dmi_system_id *id, *fan_control;
-	int fan, ret;
+	int ret;
+	u8 fan;

 	data = devm_kzalloc(&pdev->dev, sizeof(struct dell_smm_data), GFP_KERNEL);
 	if (!data)
--
2.30.2

[PATCH 4/7] hwmon: (dell-smm) Improve temperature sensors detection

[Armin Wolf]

On the Dell Inspiron 3505, three temperature sensors are
available through the SMM interface. However since they
do not have an associated type, they are not detected.
Probe for those sensors in case no type was detected.
_i8k_get_temp() is used instead of i8k_get_temp()
since it is sometimes faster and the result is
easier to check (no -ENODATA) since we do not
care about the actual temp value.

Tested on a Dell Inspiron 3505.

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 a102034a1d38..b7016971bb2e 100644
--- a/drivers/hwmon/dell-smm-hwmon.c
+++ b/drivers/hwmon/dell-smm-hwmon.c
@@ -655,6 +655,11 @@ static umode_t dell_smm_is_visible(const void *drvdata, enum hwmon_sensor_types
 	case hwmon_temp:
 		switch (attr) {
 		case hwmon_temp_input:
+			/* _i8k_get_temp() is fine since we do not care about the actual value */
+			if (data->temp_type[channel] >= 0 || _i8k_get_temp(channel) >= 0)
+				return 0444;
+
+			break;
 		case hwmon_temp_label:
 			if (data->temp_type[channel] >= 0)
 				return 0444;
--
2.30.2

[PATCH 5/7] hwmon: (dell-smm) Improve assembly code

[Armin Wolf]

The new assembly code works on both 32bit and 64bit cpus
and allows for more compiler optimisations by not
requiring smm_regs to be packed. Also since the
SMM handler seems to modify the carry flag, the new
code informs the compiler that the flags register
needs to be saved/restored.

Tested with 32 bit and 64 bit kernels on a Dell Inspiron 3505.

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

diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
index b7016971bb2e..04a41d59da60 100644
--- a/drivers/hwmon/dell-smm-hwmon.c
+++ b/drivers/hwmon/dell-smm-hwmon.c
@@ -123,7 +123,7 @@ struct smm_regs {
 	unsigned int edx;
 	unsigned int esi;
 	unsigned int edi;
-} __packed;
+};

 static const char * const temp_labels[] = {
 	"CPU",
@@ -175,59 +175,22 @@ static int i8k_smm_func(void *par)
 	if (smp_processor_id() != 0)
 		return -EBUSY;

-#if defined(CONFIG_X86_64)
-	asm volatile("pushq %%rax\n\t"
-		"movl 0(%%rax),%%edx\n\t"
-		"pushq %%rdx\n\t"
-		"movl 4(%%rax),%%ebx\n\t"
-		"movl 8(%%rax),%%ecx\n\t"
-		"movl 12(%%rax),%%edx\n\t"
-		"movl 16(%%rax),%%esi\n\t"
-		"movl 20(%%rax),%%edi\n\t"
-		"popq %%rax\n\t"
-		"out %%al,$0xb2\n\t"
-		"out %%al,$0x84\n\t"
-		"xchgq %%rax,(%%rsp)\n\t"
-		"movl %%ebx,4(%%rax)\n\t"
-		"movl %%ecx,8(%%rax)\n\t"
-		"movl %%edx,12(%%rax)\n\t"
-		"movl %%esi,16(%%rax)\n\t"
-		"movl %%edi,20(%%rax)\n\t"
-		"popq %%rdx\n\t"
-		"movl %%edx,0(%%rax)\n\t"
-		"pushfq\n\t"
-		"popq %%rax\n\t"
-		"andl $1,%%eax\n"
-		: "=a"(rc)
-		:    "a"(regs)
-		:    "%ebx", "%ecx", "%edx", "%esi", "%edi", "memory");
-#else
-	asm volatile("pushl %%eax\n\t"
-	    "movl 0(%%eax),%%edx\n\t"
-	    "push %%edx\n\t"
-	    "movl 4(%%eax),%%ebx\n\t"
-	    "movl 8(%%eax),%%ecx\n\t"
-	    "movl 12(%%eax),%%edx\n\t"
-	    "movl 16(%%eax),%%esi\n\t"
-	    "movl 20(%%eax),%%edi\n\t"
-	    "popl %%eax\n\t"
-	    "out %%al,$0xb2\n\t"
-	    "out %%al,$0x84\n\t"
-	    "xchgl %%eax,(%%esp)\n\t"
-	    "movl %%ebx,4(%%eax)\n\t"
-	    "movl %%ecx,8(%%eax)\n\t"
-	    "movl %%edx,12(%%eax)\n\t"
-	    "movl %%esi,16(%%eax)\n\t"
-	    "movl %%edi,20(%%eax)\n\t"
-	    "popl %%edx\n\t"
-	    "movl %%edx,0(%%eax)\n\t"
-	    "lahf\n\t"
-	    "shrl $8,%%eax\n\t"
-	    "andl $1,%%eax\n"
-	    : "=a"(rc)
-	    :    "a"(regs)
-	    :    "%ebx", "%ecx", "%edx", "%esi", "%edi", "memory");
-#endif
+	asm volatile("out %%al,$0xb2\n\t"
+		     "out %%al,$0x84\n"
+		     : "=a" (regs->eax),
+		       "=b" (regs->ebx),
+		       "=c" (regs->ecx),
+		       "=d" (regs->edx),
+		       "=S" (regs->esi),
+		       "=D" (regs->edi),
+		       CC_OUT(c) (rc)
+		     : "a" (regs->eax),
+		       "b" (regs->ebx),
+		       "c" (regs->ecx),
+		       "d" (regs->edx),
+		       "S" (regs->esi),
+		       "D" (regs->edi));
+
 	if (rc != 0 || (regs->eax & 0xffff) == 0xffff || regs->eax == eax)
 		rc = -EINVAL;

--
2.30.2

[PATCH 6/7] hwmon: (dell-smm) Add SMM interface documentation

[Armin Wolf]

Document the SMM interface as requested by Pali Rohar.
Since Dell does not offer any offical documentation
regarding the SMM interface, the necessary information
was extracted from the dell_smm_hwmon driver and other
sources.

Suggested-by: Pali Rohár <pali@kernel.org>
Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 Documentation/hwmon/dell-smm-hwmon.rst | 180 +++++++++++++++++++++++++
 1 file changed, 180 insertions(+)

diff --git a/Documentation/hwmon/dell-smm-hwmon.rst b/Documentation/hwmon/dell-smm-hwmon.rst
index cfaee682a245..12bba5fd1447 100644
--- a/Documentation/hwmon/dell-smm-hwmon.rst
+++ b/Documentation/hwmon/dell-smm-hwmon.rst
@@ -173,3 +173,183 @@ obtain the same information and to control the fan status. The ioctl
 interface can be accessed from C programs or from shell using the
 i8kctl utility. See the source file of ``i8kutils`` for more
 information on how to use the ioctl interface.
+
+SMM Interface
+-------------
+
+.. warning:: The SMM interface was reverse-engineered by trial-and-error
+             since Dell did not provide any Documentation,
+             please keep that in mind.
+
+The driver uses the SMM interface to send commands to the system BIOS.
+This interface is normally used by Dell's 32-bit diagnostic program or
+on newer notebook models by the buildin BIOS diagnostics.
+The SMM is triggered by writing to the special ioports ``0xb2`` and ``0x84``,
+and may cause short hangs when the BIOS code is taking too long to
+execute.
+
+The SMM handler inside the system BIOS looks at the contents of the
+``eax``, ``ebx``, ``ecx``, ``edx``, ``esi`` and ``edi`` registers.
+Each register has a special purpose:
+
+=============== ==================================
+Register        Purpose
+=============== ==================================
+eax             Holds the command code before SMM,
+                holds the first result after SMM.
+ebx             Holds the arguments.
+ecx             Unknown, set to 0.
+edx             Holds the second result after SMM.
+esi             Unknown, set to 0.
+edi             Unknown, set to 0.
+=============== ==================================
+
+The SMM handler can signal a failure by either:
+
+- setting the lower sixteen bits of ``eax`` to ``0xffff``
+- not modifying ``eax`` at all
+- setting the carry flag
+
+SMM command codes
+-----------------
+
+=============== ======================= ================================================
+Command Code    Command Name            Description
+=============== ======================= ================================================
+``0x0025``      Get Fn key status       Returns the Fn key pressed after SMM:
+
+                                        - 9th bit in ``eax`` indicates Volume up
+                                        - 10th bit in ``eax`` indicates Volume down
+                                        - both bits indicate Volume mute
+
+``0xa069``      Get power status        Returns current power status after SMM:
+
+                                        - 1st bit in ``eax`` indicates Battery connected
+                                        - 3th bit in ``eax`` indicates AC connected
+
+``0x00a3``      Get fan state           Returns current fan state after SMM:
+
+                                        - 1st byte in ``eax`` holds the current
+                                          fan state (0 - 2 or 3)
+
+``0x01a3``      Set fan state           Sets the fan speed:
+
+                                        - 1st byte in ``ebx`` holds the fan number
+                                        - 2nd byte in ``ebx`` holds the desired
+                                          fan state (0 - 2 or 3)
+
+``0x02a3``      Get fan speed           Returns the current fan speed in RPM:
+
+                                        - 1st byte in ``ebx`` holds the fan number
+                                        - 1st word in ``eax`` holds the current
+                                          fan speed in RPM (after SMM)
+
+``0x03a3``      Get fan type            Returns the fan type:
+
+                                        - 1st byte in ``ebx`` holds the fan number
+                                        - 1st byte in ``eax`` holds the
+                                          fan type (after SMM):
+
+                                          - 5th bit indicates docking fan
+                                          - 1 indicates Processor fan
+                                          - 2 indicates Motherboard fan
+                                          - 3 indicates Video fan
+                                          - 4 indicates Power supply fan
+                                          - 5 indicates Chipset fan
+                                          - 6 indicates other fan type
+
+``0x04a3``      Get nominal fan speed   Returns the nominal RPM in each fan state:
+
+                                        - 1st byte in ``ebx`` holds the fan number
+                                        - 2nd byte in ``ebx`` holds the fan state
+                                          in question (0 - 2 or 3)
+                                        - 1st word in ``eax`` holds the nominal
+                                          fan speed in RPM (after SMM)
+
+``0x05a3``      Get fan speed tolerance Returns the speed tolerance for each fan state:
+
+                                        - 1st byte in ``ebx`` holds the fan number
+                                        - 2nd byte in ``ebx`` holds the fan state
+                                          in question (0 - 2 or 3)
+                                        - 1st byte in ``eax`` returns the speed
+                                          tolerance
+
+``0x10a3``      Get sensor temperature  Returns the measured temperature:
+
+                                        - 1st byte in ``ebx`` holds the sensor number
+                                        - 1st byte in ``eax`` holds the measured
+                                          temperature (after SMM)
+
+``0x11a3``      Get sensor type         Returns the sensor type:
+
+                                        - 1st byte in ``ebx`` holds the sensor number
+                                        - 1st byte in ``eax`` holds the
+                                          temperature type (after SMM):
+
+                                          - 1 indicates CPU sensor
+                                          - 2 indicates GPU sensor
+                                          - 3 indicates SODIMM sensor
+                                          - 4 indicates other sensor type
+                                          - 5 indicates Ambient sensor
+                                          - 6 indicates other sensor type
+
+``0xfea3``      Get SMM signature       Returns Dell signature if interface
+                                        is supported (after SMM):
+
+                                        - ``eax`` holds 1145651527
+                                          (0x44494147 or "DIAG")
+                                        - ``edx`` holds 1145392204
+                                          (0x44454c4c or "DELL")
+
+``0xffa3``      Get SMM signature       Same as ``0xfea3``, check both.
+=============== ======================= ================================================
+
+There are additional commands for enabling (``0x31a3`` or ``0x35a3``) and
+disabling (``0x30a3`` or ``0x34a3``) automatic fan speed control.
+The commands are however causing severe sideeffects on many machines, so
+they are not used by default.
+
+On several machines (Inspiron 3505, Precision 490, Vostro 1720, ...), the
+fans supports a 4th "magic" state, which signals the BIOS that automatic
+fan control should be enabled for a specific fan.
+However there are also some machines who do support a 4th regular fan state too,
+but in case of the "magic" state, the nominal RPM reported for this state is a
+placeholder value, which however is not always detectable.
+
+Firmware Bugs
+-------------
+
+The SMM calls can behave erratic on some machines:
+
+======================================================= =================
+Firmware Bug                                            Affected Machines
+======================================================= =================
+Reading of fan states return spurious errors.           Precision 490
+
+Reading of fan types causes erratic fan behaviour.      Studio XPS 8000
+
+                                                        Studio XPS 8100
+
+                                                        Inspiron 580
+
+Fan-related SMM calls take too long (about 500ms).      Inspiron 7720
+
+                                                        Vostro 3360
+
+                                                        XPS 13 9333
+
+                                                        XPS 15 L502X
+======================================================= =================
+
+In case you experience similar issues on your Dell machine, please
+submit a bugreport on bugzilla to we can apply workarounds.
+
+Limitations
+-----------
+
+The SMM calls can take too long to execute on some machines, causing
+short hangs and/or audio glitches.
+Also the fan state needs to be restored after suspend, as well as
+the automatic mode settings.
+When reading a temperature sensor, values above 127 degrees indicate
+a BIOS read error or a deactivated sensor.
--
2.30.2

[PATCH 7/7] hwmon: (dell-smm) Reword and mark parameter "force" as unsafe

[Armin Wolf]

When enabling said module parameter, the driver ignores
all feature blacklists on relevant models, which has the
potential for strange side effects. Also there seems to
be a slight chance for unsupported devices to behave
badly when probed for features.
In such cases, the kernel should be tainted to inform
people that these issues might have been caused by
the dell_smm_hwmon driver with "force" enabled.
Also reword the parameter description to remind users
that enabling "force" also enables blacklisted features.

Tested on a Dell Inspiron 3505.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 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 04a41d59da60..67d63932b48a 100644
--- a/drivers/hwmon/dell-smm-hwmon.c
+++ b/drivers/hwmon/dell-smm-hwmon.c
@@ -87,8 +87,8 @@ MODULE_LICENSE("GPL");
 MODULE_ALIAS("i8k");

 static bool force;
-module_param(force, bool, 0);
-MODULE_PARM_DESC(force, "Force loading without checking for supported models");
+module_param_unsafe(force, bool, 0);
+MODULE_PARM_DESC(force, "Force loading without checking for supported models and features");

 static bool ignore_dmi;
 module_param(ignore_dmi, bool, 0);
--
2.30.2

Re: [PATCH 1/7] hwmon: (dell-smm) Allow for specifying fan control method as module parameter

[Pali Rohár]

On Tuesday 15 February 2022 20:11:07 Armin Wolf wrote:
> Right now, the only way to test if setting manual/auto fan control works
> is to edit and recompile the module, which may be too cumbersome for
> the average user.

There is also another way suitable for testing purposes which do not
requires any kernel patch. Call iopl(3) syscall which changes I/O
privilege level to 3 and which allows to poke I/O registers.

> Allow for specifying the desired fan mode control method when loading
> the module, but taint the kernel if so since there is the possibility
> for strange side effects on non-whitelisted models.
> Also update docs and kernel-parameters.txt accordingly.
> 
> Tested on a Dell Inspiron 3505.
> 
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> ---
>  .../admin-guide/kernel-parameters.txt         |  3 +++
>  Documentation/hwmon/dell-smm-hwmon.rst        | 21 ++++++++++------
>  drivers/hwmon/dell-smm-hwmon.c                | 25 +++++++++++++------
>  3 files changed, 35 insertions(+), 14 deletions(-)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index d68053db21cc..4f1b6c2b7ed1 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -968,6 +968,9 @@
>  	dell_smm_hwmon.fan_max=
>  			[HW] Maximum configurable fan speed.
> 
> +	dell_smm_hwmon.fan_mode_method=
> +			[HW] Method to use for changing fan mode.
> +
>  	dfltcc=		[HW,S390]
>  			Format: { on | off | def_only | inf_only | always }
>  			on:       s390 zlib hardware support for compression on
> diff --git a/Documentation/hwmon/dell-smm-hwmon.rst b/Documentation/hwmon/dell-smm-hwmon.rst
> index beec88491171..564d99cda869 100644
> --- a/Documentation/hwmon/dell-smm-hwmon.rst
> +++ b/Documentation/hwmon/dell-smm-hwmon.rst
> @@ -67,13 +67,16 @@ 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
>  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``
> -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
> +As a first step, you can load the module with the module parameter
> +``fan_mode_method`` set to 1 to test if your hardware works with
> +an already know method for disabling automatic BIOS fan control.
> +If ``pwm1_enable`` is now 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.
> +Just add your model to the list ``i8k_whitelist_fan_control`` in
> +file ``drivers/hwmon/dell-smm-hwmon.c`` in the kernel tree and use
> +the already known code pair.
> +Please read
>  :ref:`Documentation/process/submitting-patches.rst <submittingpatches>`
>  for information on submitting patches.
> 
> @@ -120,6 +123,10 @@ Module parameters
>                     Maximum configurable fan speed. (default:
>                     autodetect)
> 
> +* fan_mode_method:uint
> +                   Method to use for changing fan mode (default:
> +                   from whitelist)
> +
>  Legacy ``/proc`` interface
>  --------------------------
> 
> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
> index 9949eeb79378..1c4cc516c8b2 100644
> --- a/drivers/hwmon/dell-smm-hwmon.c
> +++ b/drivers/hwmon/dell-smm-hwmon.c
> @@ -111,6 +111,10 @@ static uint fan_max;
>  module_param(fan_max, uint, 0);
>  MODULE_PARM_DESC(fan_max, "Maximum configurable fan speed (default: autodetect)");
> 
> +static uint fan_mode_method;
> +module_param_unsafe(fan_mode_method, uint, 0);
> +MODULE_PARM_DESC(fan_mode_method, "Method to use for changing fan mode (default: from whitelist)");

No, please really do not introduce another kernel parameter for this
driver. There are already many and we do not need to extend this list.

> +
>  struct smm_regs {
>  	unsigned int eax;
>  	unsigned int ebx;
> @@ -677,7 +681,7 @@ static umode_t dell_smm_is_visible(const void *drvdata, enum hwmon_sensor_types
> 
>  			break;
>  		case hwmon_pwm_enable:
> -			if (data->auto_fan)
> +			if (data->auto_fan && data->manual_fan)
>  				/*
>  				 * There is no command for retrieve the current status
>  				 * from BIOS, and userspace/firmware itself can change
> @@ -1282,14 +1286,21 @@ static int __init dell_smm_probe(struct platform_device *pdev)
>  	data->i8k_fan_max = fan_max ? : I8K_FAN_HIGH;	/* Must not be 0 */
>  	data->i8k_pwm_mult = DIV_ROUND_UP(255, data->i8k_fan_max);
> 
> -	fan_control = dmi_first_match(i8k_whitelist_fan_control);
> -	if (fan_control && fan_control->driver_data) {
> -		const struct i8k_fan_control_data *control = fan_control->driver_data;
> +	/* value specified via module param overrides whitelist */
> +	if (fan_mode_method > 0 && fan_mode_method <= ARRAY_SIZE(i8k_fan_control_data)) {
> +		data->manual_fan = i8k_fan_control_data[fan_mode_method - 1].manual_fan;
> +		data->auto_fan = i8k_fan_control_data[fan_mode_method - 1].auto_fan;
> +	} else {
> +		fan_control = dmi_first_match(i8k_whitelist_fan_control);
> +		if (fan_control && fan_control->driver_data) {
> +			const struct i8k_fan_control_data *control = fan_control->driver_data;
> 
> -		data->manual_fan = control->manual_fan;
> -		data->auto_fan = control->auto_fan;
> -		dev_info(&pdev->dev, "enabling support for setting automatic/manual fan control\n");
> +			data->manual_fan = control->manual_fan;
> +			data->auto_fan = control->auto_fan;
> +		}
>  	}
> +	if (data->manual_fan && data->auto_fan)
> +		dev_info(&pdev->dev, "enabling support for setting automatic/manual fan control\n");
> 
>  	if (!fan_mult) {
>  		/*
> --
> 2.30.2
> 

Re: [PATCH 6/7] hwmon: (dell-smm) Add SMM interface documentation

[Pali Rohár]

On Tuesday 15 February 2022 20:11:12 Armin Wolf wrote:
> Document the SMM interface as requested by Pali Rohar.
> Since Dell does not offer any offical documentation
> regarding the SMM interface, the necessary information
> was extracted from the dell_smm_hwmon driver and other
> sources.
> 
> Suggested-by: Pali Rohár <pali@kernel.org>
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>

Perfect!

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

I have info about some more commands but I'm not sure if they are
implement on new machines:

0x00a6  get SMBIOS version
0x22a3  get charger info (1 arg)
0x24a3  get adaptor info status (1 arg oder 0x03)
0x32a3  restore normal Fn key mode (no args)
0x33a3  put the Fn key in "raw" mode (sends the scancode e009 instead of activating the Fn- functions) (no args)
0x36a3  get hotkey scancode list (args see diags; returns number of hotkeys)
0x40a3  get docking state (no args)

> ---
>  Documentation/hwmon/dell-smm-hwmon.rst | 180 +++++++++++++++++++++++++
>  1 file changed, 180 insertions(+)
> 
> diff --git a/Documentation/hwmon/dell-smm-hwmon.rst b/Documentation/hwmon/dell-smm-hwmon.rst
> index cfaee682a245..12bba5fd1447 100644
> --- a/Documentation/hwmon/dell-smm-hwmon.rst
> +++ b/Documentation/hwmon/dell-smm-hwmon.rst
> @@ -173,3 +173,183 @@ obtain the same information and to control the fan status. The ioctl
>  interface can be accessed from C programs or from shell using the
>  i8kctl utility. See the source file of ``i8kutils`` for more
>  information on how to use the ioctl interface.
> +
> +SMM Interface
> +-------------
> +
> +.. warning:: The SMM interface was reverse-engineered by trial-and-error
> +             since Dell did not provide any Documentation,
> +             please keep that in mind.
> +
> +The driver uses the SMM interface to send commands to the system BIOS.
> +This interface is normally used by Dell's 32-bit diagnostic program or
> +on newer notebook models by the buildin BIOS diagnostics.
> +The SMM is triggered by writing to the special ioports ``0xb2`` and ``0x84``,
> +and may cause short hangs when the BIOS code is taking too long to
> +execute.
> +
> +The SMM handler inside the system BIOS looks at the contents of the
> +``eax``, ``ebx``, ``ecx``, ``edx``, ``esi`` and ``edi`` registers.
> +Each register has a special purpose:
> +
> +=============== ==================================
> +Register        Purpose
> +=============== ==================================
> +eax             Holds the command code before SMM,
> +                holds the first result after SMM.
> +ebx             Holds the arguments.
> +ecx             Unknown, set to 0.
> +edx             Holds the second result after SMM.
> +esi             Unknown, set to 0.
> +edi             Unknown, set to 0.
> +=============== ==================================
> +
> +The SMM handler can signal a failure by either:
> +
> +- setting the lower sixteen bits of ``eax`` to ``0xffff``
> +- not modifying ``eax`` at all
> +- setting the carry flag
> +
> +SMM command codes
> +-----------------
> +
> +=============== ======================= ================================================
> +Command Code    Command Name            Description
> +=============== ======================= ================================================
> +``0x0025``      Get Fn key status       Returns the Fn key pressed after SMM:
> +
> +                                        - 9th bit in ``eax`` indicates Volume up
> +                                        - 10th bit in ``eax`` indicates Volume down
> +                                        - both bits indicate Volume mute
> +
> +``0xa069``      Get power status        Returns current power status after SMM:
> +
> +                                        - 1st bit in ``eax`` indicates Battery connected
> +                                        - 3th bit in ``eax`` indicates AC connected
> +
> +``0x00a3``      Get fan state           Returns current fan state after SMM:
> +
> +                                        - 1st byte in ``eax`` holds the current
> +                                          fan state (0 - 2 or 3)
> +
> +``0x01a3``      Set fan state           Sets the fan speed:
> +
> +                                        - 1st byte in ``ebx`` holds the fan number
> +                                        - 2nd byte in ``ebx`` holds the desired
> +                                          fan state (0 - 2 or 3)
> +
> +``0x02a3``      Get fan speed           Returns the current fan speed in RPM:
> +
> +                                        - 1st byte in ``ebx`` holds the fan number
> +                                        - 1st word in ``eax`` holds the current
> +                                          fan speed in RPM (after SMM)
> +
> +``0x03a3``      Get fan type            Returns the fan type:
> +
> +                                        - 1st byte in ``ebx`` holds the fan number
> +                                        - 1st byte in ``eax`` holds the
> +                                          fan type (after SMM):
> +
> +                                          - 5th bit indicates docking fan
> +                                          - 1 indicates Processor fan
> +                                          - 2 indicates Motherboard fan
> +                                          - 3 indicates Video fan
> +                                          - 4 indicates Power supply fan
> +                                          - 5 indicates Chipset fan
> +                                          - 6 indicates other fan type
> +
> +``0x04a3``      Get nominal fan speed   Returns the nominal RPM in each fan state:
> +
> +                                        - 1st byte in ``ebx`` holds the fan number
> +                                        - 2nd byte in ``ebx`` holds the fan state
> +                                          in question (0 - 2 or 3)
> +                                        - 1st word in ``eax`` holds the nominal
> +                                          fan speed in RPM (after SMM)
> +
> +``0x05a3``      Get fan speed tolerance Returns the speed tolerance for each fan state:
> +
> +                                        - 1st byte in ``ebx`` holds the fan number
> +                                        - 2nd byte in ``ebx`` holds the fan state
> +                                          in question (0 - 2 or 3)
> +                                        - 1st byte in ``eax`` returns the speed
> +                                          tolerance
> +
> +``0x10a3``      Get sensor temperature  Returns the measured temperature:
> +
> +                                        - 1st byte in ``ebx`` holds the sensor number
> +                                        - 1st byte in ``eax`` holds the measured
> +                                          temperature (after SMM)
> +
> +``0x11a3``      Get sensor type         Returns the sensor type:
> +
> +                                        - 1st byte in ``ebx`` holds the sensor number
> +                                        - 1st byte in ``eax`` holds the
> +                                          temperature type (after SMM):
> +
> +                                          - 1 indicates CPU sensor
> +                                          - 2 indicates GPU sensor
> +                                          - 3 indicates SODIMM sensor
> +                                          - 4 indicates other sensor type
> +                                          - 5 indicates Ambient sensor
> +                                          - 6 indicates other sensor type
> +
> +``0xfea3``      Get SMM signature       Returns Dell signature if interface
> +                                        is supported (after SMM):
> +
> +                                        - ``eax`` holds 1145651527
> +                                          (0x44494147 or "DIAG")
> +                                        - ``edx`` holds 1145392204
> +                                          (0x44454c4c or "DELL")
> +
> +``0xffa3``      Get SMM signature       Same as ``0xfea3``, check both.
> +=============== ======================= ================================================
> +
> +There are additional commands for enabling (``0x31a3`` or ``0x35a3``) and
> +disabling (``0x30a3`` or ``0x34a3``) automatic fan speed control.
> +The commands are however causing severe sideeffects on many machines, so
> +they are not used by default.
> +
> +On several machines (Inspiron 3505, Precision 490, Vostro 1720, ...), the
> +fans supports a 4th "magic" state, which signals the BIOS that automatic
> +fan control should be enabled for a specific fan.
> +However there are also some machines who do support a 4th regular fan state too,
> +but in case of the "magic" state, the nominal RPM reported for this state is a
> +placeholder value, which however is not always detectable.
> +
> +Firmware Bugs
> +-------------
> +
> +The SMM calls can behave erratic on some machines:
> +
> +======================================================= =================
> +Firmware Bug                                            Affected Machines
> +======================================================= =================
> +Reading of fan states return spurious errors.           Precision 490
> +
> +Reading of fan types causes erratic fan behaviour.      Studio XPS 8000
> +
> +                                                        Studio XPS 8100
> +
> +                                                        Inspiron 580
> +
> +Fan-related SMM calls take too long (about 500ms).      Inspiron 7720
> +
> +                                                        Vostro 3360
> +
> +                                                        XPS 13 9333
> +
> +                                                        XPS 15 L502X
> +======================================================= =================
> +
> +In case you experience similar issues on your Dell machine, please
> +submit a bugreport on bugzilla to we can apply workarounds.
> +
> +Limitations
> +-----------
> +
> +The SMM calls can take too long to execute on some machines, causing
> +short hangs and/or audio glitches.
> +Also the fan state needs to be restored after suspend, as well as
> +the automatic mode settings.
> +When reading a temperature sensor, values above 127 degrees indicate
> +a BIOS read error or a deactivated sensor.
> --
> 2.30.2
> 

Re: [PATCH 7/7] hwmon: (dell-smm) Reword and mark parameter "force" as unsafe

[Pali Rohár]

On Tuesday 15 February 2022 20:11:13 Armin Wolf wrote:
> When enabling said module parameter, the driver ignores
> all feature blacklists on relevant models, which has the
> potential for strange side effects. Also there seems to
> be a slight chance for unsupported devices to behave
> badly when probed for features.
> In such cases, the kernel should be tainted to inform
> people that these issues might have been caused by
> the dell_smm_hwmon driver with "force" enabled.
> Also reword the parameter description to remind users
> that enabling "force" also enables blacklisted features.
> 
> 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 | 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 04a41d59da60..67d63932b48a 100644
> --- a/drivers/hwmon/dell-smm-hwmon.c
> +++ b/drivers/hwmon/dell-smm-hwmon.c
> @@ -87,8 +87,8 @@ MODULE_LICENSE("GPL");
>  MODULE_ALIAS("i8k");
> 
>  static bool force;
> -module_param(force, bool, 0);
> -MODULE_PARM_DESC(force, "Force loading without checking for supported models");
> +module_param_unsafe(force, bool, 0);
> +MODULE_PARM_DESC(force, "Force loading without checking for supported models and features");
> 
>  static bool ignore_dmi;
>  module_param(ignore_dmi, bool, 0);
> --
> 2.30.2
> 

Re: [PATCH 3/7] hwmon: (dell-smm) Make fan/temp sensor number a u8

[Pali Rohár]

On Tuesday 15 February 2022 20:11:09 Armin Wolf wrote:
> Right now, we only use bits 0 to 7 of the fan/temp sensor number
> by doing number & 0xff. Passing the value as a u8 makes this
> step unnecessary. Also add checks to the ioctl handler since
> users might get confused when passing 0x00000101 does the same
> as passing 0x00000001.
> 
> 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 | 68 ++++++++++++++++++++++------------
>  1 file changed, 45 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
> index 3b49e55d060f..a102034a1d38 100644
> --- a/drivers/hwmon/dell-smm-hwmon.c
> +++ b/drivers/hwmon/dell-smm-hwmon.c
> @@ -21,6 +21,7 @@
>  #include <linux/errno.h>
>  #include <linux/hwmon.h>
>  #include <linux/init.h>
> +#include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/mutex.h>
>  #include <linux/platform_device.h>
> @@ -254,46 +255,52 @@ static int i8k_smm(struct smm_regs *regs)
>  /*
>   * Read the fan status.
>   */
> -static int i8k_get_fan_status(const struct dell_smm_data *data, int fan)
> +static int i8k_get_fan_status(const struct dell_smm_data *data, u8 fan)
>  {
> -	struct smm_regs regs = { .eax = I8K_SMM_GET_FAN, };
> +	struct smm_regs regs = {
> +		.eax = I8K_SMM_GET_FAN,
> +		.ebx = fan,
> +	};
> 
>  	if (data->disallow_fan_support)
>  		return -EINVAL;
> 
> -	regs.ebx = fan & 0xff;
>  	return i8k_smm(&regs) ? : regs.eax & 0xff;
>  }
> 
>  /*
>   * Read the fan speed in RPM.
>   */
> -static int i8k_get_fan_speed(const struct dell_smm_data *data, int fan)
> +static int i8k_get_fan_speed(const struct dell_smm_data *data, u8 fan)
>  {
> -	struct smm_regs regs = { .eax = I8K_SMM_GET_SPEED, };
> +	struct smm_regs regs = {
> +		.eax = I8K_SMM_GET_SPEED,
> +		.ebx = fan,
> +	};
> 
>  	if (data->disallow_fan_support)
>  		return -EINVAL;
> 
> -	regs.ebx = fan & 0xff;
>  	return i8k_smm(&regs) ? : (regs.eax & 0xffff) * data->i8k_fan_mult;
>  }
> 
>  /*
>   * Read the fan type.
>   */
> -static int _i8k_get_fan_type(const struct dell_smm_data *data, int fan)
> +static int _i8k_get_fan_type(const struct dell_smm_data *data, u8 fan)
>  {
> -	struct smm_regs regs = { .eax = I8K_SMM_GET_FAN_TYPE, };
> +	struct smm_regs regs = {
> +		.eax = I8K_SMM_GET_FAN_TYPE,
> +		.ebx = fan,
> +	};
> 
>  	if (data->disallow_fan_support || data->disallow_fan_type_call)
>  		return -EINVAL;
> 
> -	regs.ebx = fan & 0xff;
>  	return i8k_smm(&regs) ? : regs.eax & 0xff;
>  }
> 
> -static int i8k_get_fan_type(struct dell_smm_data *data, int fan)
> +static int i8k_get_fan_type(struct dell_smm_data *data, u8 fan)
>  {
>  	/* I8K_SMM_GET_FAN_TYPE SMM call is expensive, so cache values */
>  	if (data->fan_type[fan] == INT_MIN)
> @@ -305,14 +312,16 @@ static int i8k_get_fan_type(struct dell_smm_data *data, int fan)
>  /*
>   * Read the fan nominal rpm for specific fan speed.
>   */
> -static int __init 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, u8 fan, int speed)
>  {
> -	struct smm_regs regs = { .eax = I8K_SMM_GET_NOM_SPEED, };
> +	struct smm_regs regs = {
> +		.eax = I8K_SMM_GET_NOM_SPEED,
> +		.ebx = fan | (speed << 8),
> +	};
> 
>  	if (data->disallow_fan_support)
>  		return -EINVAL;
> 
> -	regs.ebx = (fan & 0xff) | (speed << 8);
>  	return i8k_smm(&regs) ? : (regs.eax & 0xffff) * data->i8k_fan_mult;
>  }
> 
> @@ -333,7 +342,7 @@ static int i8k_enable_fan_auto_mode(const struct dell_smm_data *data, bool enabl
>  /*
>   * Set the fan speed (off, low, high, ...).
>   */
> -static int i8k_set_fan(const struct dell_smm_data *data, int fan, int speed)
> +static int i8k_set_fan(const struct dell_smm_data *data, u8 fan, int speed)
>  {
>  	struct smm_regs regs = { .eax = I8K_SMM_SET_FAN, };
> 
> @@ -341,33 +350,35 @@ static int i8k_set_fan(const struct dell_smm_data *data, int fan, int speed)
>  		return -EINVAL;
> 
>  	speed = (speed < 0) ? 0 : ((speed > data->i8k_fan_max) ? data->i8k_fan_max : speed);
> -	regs.ebx = (fan & 0xff) | (speed << 8);
> +	regs.ebx = fan | (speed << 8);
> 
>  	return i8k_smm(&regs);
>  }
> 
> -static int __init i8k_get_temp_type(int sensor)
> +static int __init i8k_get_temp_type(u8 sensor)
>  {
> -	struct smm_regs regs = { .eax = I8K_SMM_GET_TEMP_TYPE, };
> +	struct smm_regs regs = {
> +		.eax = I8K_SMM_GET_TEMP_TYPE,
> +		.ebx = sensor,
> +	};
> 
> -	regs.ebx = sensor & 0xff;
>  	return i8k_smm(&regs) ? : regs.eax & 0xff;
>  }
> 
>  /*
>   * Read the cpu temperature.
>   */
> -static int _i8k_get_temp(int sensor)
> +static int _i8k_get_temp(u8 sensor)
>  {
>  	struct smm_regs regs = {
>  		.eax = I8K_SMM_GET_TEMP,
> -		.ebx = sensor & 0xff,
> +		.ebx = sensor,
>  	};
> 
>  	return i8k_smm(&regs) ? : regs.eax & 0xff;
>  }
> 
> -static int i8k_get_temp(int sensor)
> +static int i8k_get_temp(u8 sensor)
>  {
>  	int temp = _i8k_get_temp(sensor);
> 
> @@ -500,6 +511,9 @@ static long i8k_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
>  		if (copy_from_user(&val, argp, sizeof(int)))
>  			return -EFAULT;
> 
> +		if (val > U8_MAX || val < 0)
> +			return -EINVAL;
> +
>  		val = i8k_get_fan_speed(data, val);
>  		break;
> 
> @@ -507,6 +521,9 @@ static long i8k_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
>  		if (copy_from_user(&val, argp, sizeof(int)))
>  			return -EFAULT;
> 
> +		if (val > U8_MAX || val < 0)
> +			return -EINVAL;
> +
>  		val = i8k_get_fan_status(data, val);
>  		break;
> 
> @@ -517,6 +534,9 @@ static long i8k_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
>  		if (copy_from_user(&val, argp, sizeof(int)))
>  			return -EFAULT;
> 
> +		if (val > U8_MAX || val < 0)
> +			return -EINVAL;
> +
>  		if (copy_from_user(&speed, argp + 1, sizeof(int)))
>  			return -EFAULT;
> 
> @@ -924,7 +944,8 @@ static int __init dell_smm_init_hwmon(struct device *dev)
>  {
>  	struct dell_smm_data *data = dev_get_drvdata(dev);
>  	struct device *dell_smm_hwmon_dev;
> -	int i, state, err;
> +	int state, err;
> +	u8 i;
> 
>  	for (i = 0; i < DELL_SMM_NO_TEMP; i++) {
>  		data->temp_type[i] = i8k_get_temp_type(i);
> @@ -1245,7 +1266,8 @@ static int __init dell_smm_probe(struct platform_device *pdev)
>  {
>  	struct dell_smm_data *data;
>  	const struct dmi_system_id *id, *fan_control;
> -	int fan, ret;
> +	int ret;
> +	u8 fan;
> 
>  	data = devm_kzalloc(&pdev->dev, sizeof(struct dell_smm_data), GFP_KERNEL);
>  	if (!data)
> --
> 2.30.2
> 

Re: [PATCH 1/7] hwmon: (dell-smm) Allow for specifying fan control method as module parameter

[Armin Wolf]

Am 15.02.22 um 20:19 schrieb Pali Rohár:
> On Tuesday 15 February 2022 20:11:07 Armin Wolf wrote:
>> Right now, the only way to test if setting manual/auto fan control works
>> is to edit and recompile the module, which may be too cumbersome for
>> the average user.
> There is also another way suitable for testing purposes which do not
> requires any kernel patch. Call iopl(3) syscall which changes I/O
> privilege level to 3 and which allows to poke I/O registers.
>
This is not possible under kernel lockdown/Secure Boot.

>> Allow for specifying the desired fan mode control method when loading
>> the module, but taint the kernel if so since there is the possibility
>> for strange side effects on non-whitelisted models.
>> Also update docs and kernel-parameters.txt accordingly.
>>
>> Tested on a Dell Inspiron 3505.
>>
>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>> ---
>>   .../admin-guide/kernel-parameters.txt         |  3 +++
>>   Documentation/hwmon/dell-smm-hwmon.rst        | 21 ++++++++++------
>>   drivers/hwmon/dell-smm-hwmon.c                | 25 +++++++++++++------
>>   3 files changed, 35 insertions(+), 14 deletions(-)
>>
>> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
>> index d68053db21cc..4f1b6c2b7ed1 100644
>> --- a/Documentation/admin-guide/kernel-parameters.txt
>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>> @@ -968,6 +968,9 @@
>>   	dell_smm_hwmon.fan_max=
>>   			[HW] Maximum configurable fan speed.
>>
>> +	dell_smm_hwmon.fan_mode_method=
>> +			[HW] Method to use for changing fan mode.
>> +
>>   	dfltcc=		[HW,S390]
>>   			Format: { on | off | def_only | inf_only | always }
>>   			on:       s390 zlib hardware support for compression on
>> diff --git a/Documentation/hwmon/dell-smm-hwmon.rst b/Documentation/hwmon/dell-smm-hwmon.rst
>> index beec88491171..564d99cda869 100644
>> --- a/Documentation/hwmon/dell-smm-hwmon.rst
>> +++ b/Documentation/hwmon/dell-smm-hwmon.rst
>> @@ -67,13 +67,16 @@ 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
>>   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``
>> -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
>> +As a first step, you can load the module with the module parameter
>> +``fan_mode_method`` set to 1 to test if your hardware works with
>> +an already know method for disabling automatic BIOS fan control.
>> +If ``pwm1_enable`` is now 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.
>> +Just add your model to the list ``i8k_whitelist_fan_control`` in
>> +file ``drivers/hwmon/dell-smm-hwmon.c`` in the kernel tree and use
>> +the already known code pair.
>> +Please read
>>   :ref:`Documentation/process/submitting-patches.rst <submittingpatches>`
>>   for information on submitting patches.
>>
>> @@ -120,6 +123,10 @@ Module parameters
>>                      Maximum configurable fan speed. (default:
>>                      autodetect)
>>
>> +* fan_mode_method:uint
>> +                   Method to use for changing fan mode (default:
>> +                   from whitelist)
>> +
>>   Legacy ``/proc`` interface
>>   --------------------------
>>
>> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
>> index 9949eeb79378..1c4cc516c8b2 100644
>> --- a/drivers/hwmon/dell-smm-hwmon.c
>> +++ b/drivers/hwmon/dell-smm-hwmon.c
>> @@ -111,6 +111,10 @@ static uint fan_max;
>>   module_param(fan_max, uint, 0);
>>   MODULE_PARM_DESC(fan_max, "Maximum configurable fan speed (default: autodetect)");
>>
>> +static uint fan_mode_method;
>> +module_param_unsafe(fan_mode_method, uint, 0);
>> +MODULE_PARM_DESC(fan_mode_method, "Method to use for changing fan mode (default: from whitelist)");
> No, please really do not introduce another kernel parameter for this
> driver. There are already many and we do not need to extend this list.
>
>> +
>>   struct smm_regs {
>>   	unsigned int eax;
>>   	unsigned int ebx;
>> @@ -677,7 +681,7 @@ static umode_t dell_smm_is_visible(const void *drvdata, enum hwmon_sensor_types
>>
>>   			break;
>>   		case hwmon_pwm_enable:
>> -			if (data->auto_fan)
>> +			if (data->auto_fan && data->manual_fan)
>>   				/*
>>   				 * There is no command for retrieve the current status
>>   				 * from BIOS, and userspace/firmware itself can change
>> @@ -1282,14 +1286,21 @@ static int __init dell_smm_probe(struct platform_device *pdev)
>>   	data->i8k_fan_max = fan_max ? : I8K_FAN_HIGH;	/* Must not be 0 */
>>   	data->i8k_pwm_mult = DIV_ROUND_UP(255, data->i8k_fan_max);
>>
>> -	fan_control = dmi_first_match(i8k_whitelist_fan_control);
>> -	if (fan_control && fan_control->driver_data) {
>> -		const struct i8k_fan_control_data *control = fan_control->driver_data;
>> +	/* value specified via module param overrides whitelist */
>> +	if (fan_mode_method > 0 && fan_mode_method <= ARRAY_SIZE(i8k_fan_control_data)) {
>> +		data->manual_fan = i8k_fan_control_data[fan_mode_method - 1].manual_fan;
>> +		data->auto_fan = i8k_fan_control_data[fan_mode_method - 1].auto_fan;
>> +	} else {
>> +		fan_control = dmi_first_match(i8k_whitelist_fan_control);
>> +		if (fan_control && fan_control->driver_data) {
>> +			const struct i8k_fan_control_data *control = fan_control->driver_data;
>>
>> -		data->manual_fan = control->manual_fan;
>> -		data->auto_fan = control->auto_fan;
>> -		dev_info(&pdev->dev, "enabling support for setting automatic/manual fan control\n");
>> +			data->manual_fan = control->manual_fan;
>> +			data->auto_fan = control->auto_fan;
>> +		}
>>   	}
>> +	if (data->manual_fan && data->auto_fan)
>> +		dev_info(&pdev->dev, "enabling support for setting automatic/manual fan control\n");
>>
>>   	if (!fan_mult) {
>>   		/*
>> --
>> 2.30.2
>>

Re: [PATCH 1/7] hwmon: (dell-smm) Allow for specifying fan control method as module parameter

[Pali Rohár]

On Tuesday 15 February 2022 20:44:20 Armin Wolf wrote:
> Am 15.02.22 um 20:19 schrieb Pali Rohár:
> > On Tuesday 15 February 2022 20:11:07 Armin Wolf wrote:
> > > Right now, the only way to test if setting manual/auto fan control works
> > > is to edit and recompile the module, which may be too cumbersome for
> > > the average user.
> > There is also another way suitable for testing purposes which do not
> > requires any kernel patch. Call iopl(3) syscall which changes I/O
> > privilege level to 3 and which allows to poke I/O registers.
> > 
> This is not possible under kernel lockdown/Secure Boot.

Under Secure Boot it should be still possible.

With kernel lockdown, no kernel testing/debugging at HW level is
acceptable due to security reasons, as it is against what kernel
lockdown should achieve.

> > > Allow for specifying the desired fan mode control method when loading
> > > the module, but taint the kernel if so since there is the possibility
> > > for strange side effects on non-whitelisted models.
> > > Also update docs and kernel-parameters.txt accordingly.
> > > 
> > > Tested on a Dell Inspiron 3505.
> > > 
> > > Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> > > ---
> > >   .../admin-guide/kernel-parameters.txt         |  3 +++
> > >   Documentation/hwmon/dell-smm-hwmon.rst        | 21 ++++++++++------
> > >   drivers/hwmon/dell-smm-hwmon.c                | 25 +++++++++++++------
> > >   3 files changed, 35 insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> > > index d68053db21cc..4f1b6c2b7ed1 100644
> > > --- a/Documentation/admin-guide/kernel-parameters.txt
> > > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > > @@ -968,6 +968,9 @@
> > >   	dell_smm_hwmon.fan_max=
> > >   			[HW] Maximum configurable fan speed.
> > > 
> > > +	dell_smm_hwmon.fan_mode_method=
> > > +			[HW] Method to use for changing fan mode.
> > > +
> > >   	dfltcc=		[HW,S390]
> > >   			Format: { on | off | def_only | inf_only | always }
> > >   			on:       s390 zlib hardware support for compression on
> > > diff --git a/Documentation/hwmon/dell-smm-hwmon.rst b/Documentation/hwmon/dell-smm-hwmon.rst
> > > index beec88491171..564d99cda869 100644
> > > --- a/Documentation/hwmon/dell-smm-hwmon.rst
> > > +++ b/Documentation/hwmon/dell-smm-hwmon.rst
> > > @@ -67,13 +67,16 @@ 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
> > >   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``
> > > -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
> > > +As a first step, you can load the module with the module parameter
> > > +``fan_mode_method`` set to 1 to test if your hardware works with
> > > +an already know method for disabling automatic BIOS fan control.
> > > +If ``pwm1_enable`` is now 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.
> > > +Just add your model to the list ``i8k_whitelist_fan_control`` in
> > > +file ``drivers/hwmon/dell-smm-hwmon.c`` in the kernel tree and use
> > > +the already known code pair.
> > > +Please read
> > >   :ref:`Documentation/process/submitting-patches.rst <submittingpatches>`
> > >   for information on submitting patches.
> > > 
> > > @@ -120,6 +123,10 @@ Module parameters
> > >                      Maximum configurable fan speed. (default:
> > >                      autodetect)
> > > 
> > > +* fan_mode_method:uint
> > > +                   Method to use for changing fan mode (default:
> > > +                   from whitelist)
> > > +
> > >   Legacy ``/proc`` interface
> > >   --------------------------
> > > 
> > > diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
> > > index 9949eeb79378..1c4cc516c8b2 100644
> > > --- a/drivers/hwmon/dell-smm-hwmon.c
> > > +++ b/drivers/hwmon/dell-smm-hwmon.c
> > > @@ -111,6 +111,10 @@ static uint fan_max;
> > >   module_param(fan_max, uint, 0);
> > >   MODULE_PARM_DESC(fan_max, "Maximum configurable fan speed (default: autodetect)");
> > > 
> > > +static uint fan_mode_method;
> > > +module_param_unsafe(fan_mode_method, uint, 0);
> > > +MODULE_PARM_DESC(fan_mode_method, "Method to use for changing fan mode (default: from whitelist)");
> > No, please really do not introduce another kernel parameter for this
> > driver. There are already many and we do not need to extend this list.
> > 
> > > +
> > >   struct smm_regs {
> > >   	unsigned int eax;
> > >   	unsigned int ebx;
> > > @@ -677,7 +681,7 @@ static umode_t dell_smm_is_visible(const void *drvdata, enum hwmon_sensor_types
> > > 
> > >   			break;
> > >   		case hwmon_pwm_enable:
> > > -			if (data->auto_fan)
> > > +			if (data->auto_fan && data->manual_fan)
> > >   				/*
> > >   				 * There is no command for retrieve the current status
> > >   				 * from BIOS, and userspace/firmware itself can change
> > > @@ -1282,14 +1286,21 @@ static int __init dell_smm_probe(struct platform_device *pdev)
> > >   	data->i8k_fan_max = fan_max ? : I8K_FAN_HIGH;	/* Must not be 0 */
> > >   	data->i8k_pwm_mult = DIV_ROUND_UP(255, data->i8k_fan_max);
> > > 
> > > -	fan_control = dmi_first_match(i8k_whitelist_fan_control);
> > > -	if (fan_control && fan_control->driver_data) {
> > > -		const struct i8k_fan_control_data *control = fan_control->driver_data;
> > > +	/* value specified via module param overrides whitelist */
> > > +	if (fan_mode_method > 0 && fan_mode_method <= ARRAY_SIZE(i8k_fan_control_data)) {
> > > +		data->manual_fan = i8k_fan_control_data[fan_mode_method - 1].manual_fan;
> > > +		data->auto_fan = i8k_fan_control_data[fan_mode_method - 1].auto_fan;
> > > +	} else {
> > > +		fan_control = dmi_first_match(i8k_whitelist_fan_control);
> > > +		if (fan_control && fan_control->driver_data) {
> > > +			const struct i8k_fan_control_data *control = fan_control->driver_data;
> > > 
> > > -		data->manual_fan = control->manual_fan;
> > > -		data->auto_fan = control->auto_fan;
> > > -		dev_info(&pdev->dev, "enabling support for setting automatic/manual fan control\n");
> > > +			data->manual_fan = control->manual_fan;
> > > +			data->auto_fan = control->auto_fan;
> > > +		}
> > >   	}
> > > +	if (data->manual_fan && data->auto_fan)
> > > +		dev_info(&pdev->dev, "enabling support for setting automatic/manual fan control\n");
> > > 
> > >   	if (!fan_mult) {
> > >   		/*
> > > --
> > > 2.30.2
> > > 

Re: [PATCH 1/7] hwmon: (dell-smm) Allow for specifying fan control method as module parameter

[Armin Wolf]

Am 15.02.22 um 20:49 schrieb Pali Rohár:
> On Tuesday 15 February 2022 20:44:20 Armin Wolf wrote:
>> Am 15.02.22 um 20:19 schrieb Pali Rohár:
>>> On Tuesday 15 February 2022 20:11:07 Armin Wolf wrote:
>>>> Right now, the only way to test if setting manual/auto fan control works
>>>> is to edit and recompile the module, which may be too cumbersome for
>>>> the average user.
>>> There is also another way suitable for testing purposes which do not
>>> requires any kernel patch. Call iopl(3) syscall which changes I/O
>>> privilege level to 3 and which allows to poke I/O registers.
>>>
>> This is not possible under kernel lockdown/Secure Boot.
> Under Secure Boot it should be still possible.

Afaik, access to iopl() and ioperm() is blocked in this mode too.
This for example causes dell-bios-fan-control to fail when
the machine is booted in Secure Boot mode.

> With kernel lockdown, no kernel testing/debugging at HW level is
> acceptable due to security reasons, as it is against what kernel
> lockdown should achieve.
>
That is the reason i limited the number of selectable code pairs to
two which are known to work on a wide range of machines.
So users are unable to specify SMM commands, which are controlled by
the driver. If this still is to much, then we may drop this patch.

>>>> Allow for specifying the desired fan mode control method when loading
>>>> the module, but taint the kernel if so since there is the possibility
>>>> for strange side effects on non-whitelisted models.
>>>> Also update docs and kernel-parameters.txt accordingly.
>>>>
>>>> Tested on a Dell Inspiron 3505.
>>>>
>>>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>>>> ---
>>>>    .../admin-guide/kernel-parameters.txt         |  3 +++
>>>>    Documentation/hwmon/dell-smm-hwmon.rst        | 21 ++++++++++------
>>>>    drivers/hwmon/dell-smm-hwmon.c                | 25 +++++++++++++------
>>>>    3 files changed, 35 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
>>>> index d68053db21cc..4f1b6c2b7ed1 100644
>>>> --- a/Documentation/admin-guide/kernel-parameters.txt
>>>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>>>> @@ -968,6 +968,9 @@
>>>>    	dell_smm_hwmon.fan_max=
>>>>    			[HW] Maximum configurable fan speed.
>>>>
>>>> +	dell_smm_hwmon.fan_mode_method=
>>>> +			[HW] Method to use for changing fan mode.
>>>> +
>>>>    	dfltcc=		[HW,S390]
>>>>    			Format: { on | off | def_only | inf_only | always }
>>>>    			on:       s390 zlib hardware support for compression on
>>>> diff --git a/Documentation/hwmon/dell-smm-hwmon.rst b/Documentation/hwmon/dell-smm-hwmon.rst
>>>> index beec88491171..564d99cda869 100644
>>>> --- a/Documentation/hwmon/dell-smm-hwmon.rst
>>>> +++ b/Documentation/hwmon/dell-smm-hwmon.rst
>>>> @@ -67,13 +67,16 @@ 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
>>>>    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``
>>>> -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
>>>> +As a first step, you can load the module with the module parameter
>>>> +``fan_mode_method`` set to 1 to test if your hardware works with
>>>> +an already know method for disabling automatic BIOS fan control.
>>>> +If ``pwm1_enable`` is now 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.
>>>> +Just add your model to the list ``i8k_whitelist_fan_control`` in
>>>> +file ``drivers/hwmon/dell-smm-hwmon.c`` in the kernel tree and use
>>>> +the already known code pair.
>>>> +Please read
>>>>    :ref:`Documentation/process/submitting-patches.rst <submittingpatches>`
>>>>    for information on submitting patches.
>>>>
>>>> @@ -120,6 +123,10 @@ Module parameters
>>>>                       Maximum configurable fan speed. (default:
>>>>                       autodetect)
>>>>
>>>> +* fan_mode_method:uint
>>>> +                   Method to use for changing fan mode (default:
>>>> +                   from whitelist)
>>>> +
>>>>    Legacy ``/proc`` interface
>>>>    --------------------------
>>>>
>>>> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
>>>> index 9949eeb79378..1c4cc516c8b2 100644
>>>> --- a/drivers/hwmon/dell-smm-hwmon.c
>>>> +++ b/drivers/hwmon/dell-smm-hwmon.c
>>>> @@ -111,6 +111,10 @@ static uint fan_max;
>>>>    module_param(fan_max, uint, 0);
>>>>    MODULE_PARM_DESC(fan_max, "Maximum configurable fan speed (default: autodetect)");
>>>>
>>>> +static uint fan_mode_method;
>>>> +module_param_unsafe(fan_mode_method, uint, 0);
>>>> +MODULE_PARM_DESC(fan_mode_method, "Method to use for changing fan mode (default: from whitelist)");
>>> No, please really do not introduce another kernel parameter for this
>>> driver. There are already many and we do not need to extend this list.

Since there are two code pairs, i could not reuse the force param for that.
Since the newly added pair is unused, should i remove it and reuse the
force param?

>>>> +
>>>>    struct smm_regs {
>>>>    	unsigned int eax;
>>>>    	unsigned int ebx;
>>>> @@ -677,7 +681,7 @@ static umode_t dell_smm_is_visible(const void *drvdata, enum hwmon_sensor_types
>>>>
>>>>    			break;
>>>>    		case hwmon_pwm_enable:
>>>> -			if (data->auto_fan)
>>>> +			if (data->auto_fan && data->manual_fan)
>>>>    				/*
>>>>    				 * There is no command for retrieve the current status
>>>>    				 * from BIOS, and userspace/firmware itself can change
>>>> @@ -1282,14 +1286,21 @@ static int __init dell_smm_probe(struct platform_device *pdev)
>>>>    	data->i8k_fan_max = fan_max ? : I8K_FAN_HIGH;	/* Must not be 0 */
>>>>    	data->i8k_pwm_mult = DIV_ROUND_UP(255, data->i8k_fan_max);
>>>>
>>>> -	fan_control = dmi_first_match(i8k_whitelist_fan_control);
>>>> -	if (fan_control && fan_control->driver_data) {
>>>> -		const struct i8k_fan_control_data *control = fan_control->driver_data;
>>>> +	/* value specified via module param overrides whitelist */
>>>> +	if (fan_mode_method > 0 && fan_mode_method <= ARRAY_SIZE(i8k_fan_control_data)) {
>>>> +		data->manual_fan = i8k_fan_control_data[fan_mode_method - 1].manual_fan;
>>>> +		data->auto_fan = i8k_fan_control_data[fan_mode_method - 1].auto_fan;
>>>> +	} else {
>>>> +		fan_control = dmi_first_match(i8k_whitelist_fan_control);
>>>> +		if (fan_control && fan_control->driver_data) {
>>>> +			const struct i8k_fan_control_data *control = fan_control->driver_data;
>>>>
>>>> -		data->manual_fan = control->manual_fan;
>>>> -		data->auto_fan = control->auto_fan;
>>>> -		dev_info(&pdev->dev, "enabling support for setting automatic/manual fan control\n");
>>>> +			data->manual_fan = control->manual_fan;
>>>> +			data->auto_fan = control->auto_fan;
>>>> +		}
>>>>    	}
>>>> +	if (data->manual_fan && data->auto_fan)
>>>> +		dev_info(&pdev->dev, "enabling support for setting automatic/manual fan control\n");
>>>>
>>>>    	if (!fan_mult) {
>>>>    		/*
>>>> --
>>>> 2.30.2
>>>>

Re: [PATCH 1/7] hwmon: (dell-smm) Allow for specifying fan control method as module parameter

[Pali Rohár]

On Tuesday 15 February 2022 21:19:17 Armin Wolf wrote:
> Am 15.02.22 um 20:49 schrieb Pali Rohár:
> > On Tuesday 15 February 2022 20:44:20 Armin Wolf wrote:
> > > Am 15.02.22 um 20:19 schrieb Pali Rohár:
> > > > On Tuesday 15 February 2022 20:11:07 Armin Wolf wrote:
> > > > > Right now, the only way to test if setting manual/auto fan control works
> > > > > is to edit and recompile the module, which may be too cumbersome for
> > > > > the average user.
> > > > There is also another way suitable for testing purposes which do not
> > > > requires any kernel patch. Call iopl(3) syscall which changes I/O
> > > > privilege level to 3 and which allows to poke I/O registers.
> > > > 
> > > This is not possible under kernel lockdown/Secure Boot.
> > Under Secure Boot it should be still possible.
> 
> Afaik, access to iopl() and ioperm() is blocked in this mode too.
> This for example causes dell-bios-fan-control to fail when
> the machine is booted in Secure Boot mode.

Well, then it is the cost of the usage of Secure Boot. If you need
additional functionality then disable it.

> > With kernel lockdown, no kernel testing/debugging at HW level is
> > acceptable due to security reasons, as it is against what kernel
> > lockdown should achieve.
> > 
> That is the reason i limited the number of selectable code pairs to
> two which are known to work on a wide range of machines.
> So users are unable to specify SMM commands, which are controlled by
> the driver. If this still is to much, then we may drop this patch.

This kernel driver dell-smm-hwmon should use same security model as
other parts in kernel. So if kernel disallow user to specify custom
parameters for io ports (those which are not provided by platform data)
then it should be disabled also in this driver.

It is really _bad_ practice to trying invent a security hole to the
existing security model.

> > > > > Allow for specifying the desired fan mode control method when loading
> > > > > the module, but taint the kernel if so since there is the possibility
> > > > > for strange side effects on non-whitelisted models.
> > > > > Also update docs and kernel-parameters.txt accordingly.
> > > > > 
> > > > > Tested on a Dell Inspiron 3505.
> > > > > 
> > > > > Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> > > > > ---
> > > > >    .../admin-guide/kernel-parameters.txt         |  3 +++
> > > > >    Documentation/hwmon/dell-smm-hwmon.rst        | 21 ++++++++++------
> > > > >    drivers/hwmon/dell-smm-hwmon.c                | 25 +++++++++++++------
> > > > >    3 files changed, 35 insertions(+), 14 deletions(-)
> > > > > 
> > > > > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> > > > > index d68053db21cc..4f1b6c2b7ed1 100644
> > > > > --- a/Documentation/admin-guide/kernel-parameters.txt
> > > > > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > > > > @@ -968,6 +968,9 @@
> > > > >    	dell_smm_hwmon.fan_max=
> > > > >    			[HW] Maximum configurable fan speed.
> > > > > 
> > > > > +	dell_smm_hwmon.fan_mode_method=
> > > > > +			[HW] Method to use for changing fan mode.
> > > > > +
> > > > >    	dfltcc=		[HW,S390]
> > > > >    			Format: { on | off | def_only | inf_only | always }
> > > > >    			on:       s390 zlib hardware support for compression on
> > > > > diff --git a/Documentation/hwmon/dell-smm-hwmon.rst b/Documentation/hwmon/dell-smm-hwmon.rst
> > > > > index beec88491171..564d99cda869 100644
> > > > > --- a/Documentation/hwmon/dell-smm-hwmon.rst
> > > > > +++ b/Documentation/hwmon/dell-smm-hwmon.rst
> > > > > @@ -67,13 +67,16 @@ 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
> > > > >    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``
> > > > > -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
> > > > > +As a first step, you can load the module with the module parameter
> > > > > +``fan_mode_method`` set to 1 to test if your hardware works with
> > > > > +an already know method for disabling automatic BIOS fan control.
> > > > > +If ``pwm1_enable`` is now 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.
> > > > > +Just add your model to the list ``i8k_whitelist_fan_control`` in
> > > > > +file ``drivers/hwmon/dell-smm-hwmon.c`` in the kernel tree and use
> > > > > +the already known code pair.
> > > > > +Please read
> > > > >    :ref:`Documentation/process/submitting-patches.rst <submittingpatches>`
> > > > >    for information on submitting patches.
> > > > > 
> > > > > @@ -120,6 +123,10 @@ Module parameters
> > > > >                       Maximum configurable fan speed. (default:
> > > > >                       autodetect)
> > > > > 
> > > > > +* fan_mode_method:uint
> > > > > +                   Method to use for changing fan mode (default:
> > > > > +                   from whitelist)
> > > > > +
> > > > >    Legacy ``/proc`` interface
> > > > >    --------------------------
> > > > > 
> > > > > diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
> > > > > index 9949eeb79378..1c4cc516c8b2 100644
> > > > > --- a/drivers/hwmon/dell-smm-hwmon.c
> > > > > +++ b/drivers/hwmon/dell-smm-hwmon.c
> > > > > @@ -111,6 +111,10 @@ static uint fan_max;
> > > > >    module_param(fan_max, uint, 0);
> > > > >    MODULE_PARM_DESC(fan_max, "Maximum configurable fan speed (default: autodetect)");
> > > > > 
> > > > > +static uint fan_mode_method;
> > > > > +module_param_unsafe(fan_mode_method, uint, 0);
> > > > > +MODULE_PARM_DESC(fan_mode_method, "Method to use for changing fan mode (default: from whitelist)");
> > > > No, please really do not introduce another kernel parameter for this
> > > > driver. There are already many and we do not need to extend this list.
> 
> Since there are two code pairs, i could not reuse the force param for that.
> Since the newly added pair is unused, should i remove it and reuse the
> force param?

For such things like this there is debugfs, if you really need to
specify / provide custom values.

> > > > > +
> > > > >    struct smm_regs {
> > > > >    	unsigned int eax;
> > > > >    	unsigned int ebx;
> > > > > @@ -677,7 +681,7 @@ static umode_t dell_smm_is_visible(const void *drvdata, enum hwmon_sensor_types
> > > > > 
> > > > >    			break;
> > > > >    		case hwmon_pwm_enable:
> > > > > -			if (data->auto_fan)
> > > > > +			if (data->auto_fan && data->manual_fan)
> > > > >    				/*
> > > > >    				 * There is no command for retrieve the current status
> > > > >    				 * from BIOS, and userspace/firmware itself can change
> > > > > @@ -1282,14 +1286,21 @@ static int __init dell_smm_probe(struct platform_device *pdev)
> > > > >    	data->i8k_fan_max = fan_max ? : I8K_FAN_HIGH;	/* Must not be 0 */
> > > > >    	data->i8k_pwm_mult = DIV_ROUND_UP(255, data->i8k_fan_max);
> > > > > 
> > > > > -	fan_control = dmi_first_match(i8k_whitelist_fan_control);
> > > > > -	if (fan_control && fan_control->driver_data) {
> > > > > -		const struct i8k_fan_control_data *control = fan_control->driver_data;
> > > > > +	/* value specified via module param overrides whitelist */
> > > > > +	if (fan_mode_method > 0 && fan_mode_method <= ARRAY_SIZE(i8k_fan_control_data)) {
> > > > > +		data->manual_fan = i8k_fan_control_data[fan_mode_method - 1].manual_fan;
> > > > > +		data->auto_fan = i8k_fan_control_data[fan_mode_method - 1].auto_fan;
> > > > > +	} else {
> > > > > +		fan_control = dmi_first_match(i8k_whitelist_fan_control);
> > > > > +		if (fan_control && fan_control->driver_data) {
> > > > > +			const struct i8k_fan_control_data *control = fan_control->driver_data;
> > > > > 
> > > > > -		data->manual_fan = control->manual_fan;
> > > > > -		data->auto_fan = control->auto_fan;
> > > > > -		dev_info(&pdev->dev, "enabling support for setting automatic/manual fan control\n");
> > > > > +			data->manual_fan = control->manual_fan;
> > > > > +			data->auto_fan = control->auto_fan;
> > > > > +		}
> > > > >    	}
> > > > > +	if (data->manual_fan && data->auto_fan)
> > > > > +		dev_info(&pdev->dev, "enabling support for setting automatic/manual fan control\n");
> > > > > 
> > > > >    	if (!fan_mult) {
> > > > >    		/*
> > > > > --
> > > > > 2.30.2
> > > > > 

Re: [PATCH 1/7] hwmon: (dell-smm) Allow for specifying fan control method as module parameter

[Armin Wolf]

Am 15.02.2022 um 21:31 schrieb Pali Rohár:
> On Tuesday 15 February 2022 21:19:17 Armin Wolf wrote:
>> Am 15.02.22 um 20:49 schrieb Pali Rohár:
>>> On Tuesday 15 February 2022 20:44:20 Armin Wolf wrote:
>>>> Am 15.02.22 um 20:19 schrieb Pali Rohár:
>>>>> On Tuesday 15 February 2022 20:11:07 Armin Wolf wrote:
>>>>>> Right now, the only way to test if setting manual/auto fan control works
>>>>>> is to edit and recompile the module, which may be too cumbersome for
>>>>>> the average user.
>>>>> There is also another way suitable for testing purposes which do not
>>>>> requires any kernel patch. Call iopl(3) syscall which changes I/O
>>>>> privilege level to 3 and which allows to poke I/O registers.
>>>>>
>>>> This is not possible under kernel lockdown/Secure Boot.
>>> Under Secure Boot it should be still possible.
>> Afaik, access to iopl() and ioperm() is blocked in this mode too.
>> This for example causes dell-bios-fan-control to fail when
>> the machine is booted in Secure Boot mode.
> Well, then it is the cost of the usage of Secure Boot. If you need
> additional functionality then disable it.
>
>>> With kernel lockdown, no kernel testing/debugging at HW level is
>>> acceptable due to security reasons, as it is against what kernel
>>> lockdown should achieve.
>>>
>> That is the reason i limited the number of selectable code pairs to
>> two which are known to work on a wide range of machines.
>> So users are unable to specify SMM commands, which are controlled by
>> the driver. If this still is to much, then we may drop this patch.
> This kernel driver dell-smm-hwmon should use same security model as
> other parts in kernel. So if kernel disallow user to specify custom
> parameters for io ports (those which are not provided by platform data)
> then it should be disabled also in this driver.
>
> It is really _bad_ practice to trying invent a security hole to the
> existing security model.

Understandable, i will drop this patch(es) then.

>>>>>> Allow for specifying the desired fan mode control method when loading
>>>>>> the module, but taint the kernel if so since there is the possibility
>>>>>> for strange side effects on non-whitelisted models.
>>>>>> Also update docs and kernel-parameters.txt accordingly.
>>>>>>
>>>>>> Tested on a Dell Inspiron 3505.
>>>>>>
>>>>>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>>>>>> ---
>>>>>>     .../admin-guide/kernel-parameters.txt         |  3 +++
>>>>>>     Documentation/hwmon/dell-smm-hwmon.rst        | 21 ++++++++++------
>>>>>>     drivers/hwmon/dell-smm-hwmon.c                | 25 +++++++++++++------
>>>>>>     3 files changed, 35 insertions(+), 14 deletions(-)
>>>>>>
>>>>>> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
>>>>>> index d68053db21cc..4f1b6c2b7ed1 100644
>>>>>> --- a/Documentation/admin-guide/kernel-parameters.txt
>>>>>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>>>>>> @@ -968,6 +968,9 @@
>>>>>>     	dell_smm_hwmon.fan_max=
>>>>>>     			[HW] Maximum configurable fan speed.
>>>>>>
>>>>>> +	dell_smm_hwmon.fan_mode_method=
>>>>>> +			[HW] Method to use for changing fan mode.
>>>>>> +
>>>>>>     	dfltcc=		[HW,S390]
>>>>>>     			Format: { on | off | def_only | inf_only | always }
>>>>>>     			on:       s390 zlib hardware support for compression on
>>>>>> diff --git a/Documentation/hwmon/dell-smm-hwmon.rst b/Documentation/hwmon/dell-smm-hwmon.rst
>>>>>> index beec88491171..564d99cda869 100644
>>>>>> --- a/Documentation/hwmon/dell-smm-hwmon.rst
>>>>>> +++ b/Documentation/hwmon/dell-smm-hwmon.rst
>>>>>> @@ -67,13 +67,16 @@ 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
>>>>>>     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``
>>>>>> -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
>>>>>> +As a first step, you can load the module with the module parameter
>>>>>> +``fan_mode_method`` set to 1 to test if your hardware works with
>>>>>> +an already know method for disabling automatic BIOS fan control.
>>>>>> +If ``pwm1_enable`` is now 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.
>>>>>> +Just add your model to the list ``i8k_whitelist_fan_control`` in
>>>>>> +file ``drivers/hwmon/dell-smm-hwmon.c`` in the kernel tree and use
>>>>>> +the already known code pair.
>>>>>> +Please read
>>>>>>     :ref:`Documentation/process/submitting-patches.rst <submittingpatches>`
>>>>>>     for information on submitting patches.
>>>>>>
>>>>>> @@ -120,6 +123,10 @@ Module parameters
>>>>>>                        Maximum configurable fan speed. (default:
>>>>>>                        autodetect)
>>>>>>
>>>>>> +* fan_mode_method:uint
>>>>>> +                   Method to use for changing fan mode (default:
>>>>>> +                   from whitelist)
>>>>>> +
>>>>>>     Legacy ``/proc`` interface
>>>>>>     --------------------------
>>>>>>
>>>>>> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
>>>>>> index 9949eeb79378..1c4cc516c8b2 100644
>>>>>> --- a/drivers/hwmon/dell-smm-hwmon.c
>>>>>> +++ b/drivers/hwmon/dell-smm-hwmon.c
>>>>>> @@ -111,6 +111,10 @@ static uint fan_max;
>>>>>>     module_param(fan_max, uint, 0);
>>>>>>     MODULE_PARM_DESC(fan_max, "Maximum configurable fan speed (default: autodetect)");
>>>>>>
>>>>>> +static uint fan_mode_method;
>>>>>> +module_param_unsafe(fan_mode_method, uint, 0);
>>>>>> +MODULE_PARM_DESC(fan_mode_method, "Method to use for changing fan mode (default: from whitelist)");
>>>>> No, please really do not introduce another kernel parameter for this
>>>>> driver. There are already many and we do not need to extend this list.
>> Since there are two code pairs, i could not reuse the force param for that.
>> Since the newly added pair is unused, should i remove it and reuse the
>> force param?
> For such things like this there is debugfs, if you really need to
> specify / provide custom values.
>
>>>>>> +
>>>>>>     struct smm_regs {
>>>>>>     	unsigned int eax;
>>>>>>     	unsigned int ebx;
>>>>>> @@ -677,7 +681,7 @@ static umode_t dell_smm_is_visible(const void *drvdata, enum hwmon_sensor_types
>>>>>>
>>>>>>     			break;
>>>>>>     		case hwmon_pwm_enable:
>>>>>> -			if (data->auto_fan)
>>>>>> +			if (data->auto_fan && data->manual_fan)
>>>>>>     				/*
>>>>>>     				 * There is no command for retrieve the current status
>>>>>>     				 * from BIOS, and userspace/firmware itself can change
>>>>>> @@ -1282,14 +1286,21 @@ static int __init dell_smm_probe(struct platform_device *pdev)
>>>>>>     	data->i8k_fan_max = fan_max ? : I8K_FAN_HIGH;	/* Must not be 0 */
>>>>>>     	data->i8k_pwm_mult = DIV_ROUND_UP(255, data->i8k_fan_max);
>>>>>>
>>>>>> -	fan_control = dmi_first_match(i8k_whitelist_fan_control);
>>>>>> -	if (fan_control && fan_control->driver_data) {
>>>>>> -		const struct i8k_fan_control_data *control = fan_control->driver_data;
>>>>>> +	/* value specified via module param overrides whitelist */
>>>>>> +	if (fan_mode_method > 0 && fan_mode_method <= ARRAY_SIZE(i8k_fan_control_data)) {
>>>>>> +		data->manual_fan = i8k_fan_control_data[fan_mode_method - 1].manual_fan;
>>>>>> +		data->auto_fan = i8k_fan_control_data[fan_mode_method - 1].auto_fan;
>>>>>> +	} else {
>>>>>> +		fan_control = dmi_first_match(i8k_whitelist_fan_control);
>>>>>> +		if (fan_control && fan_control->driver_data) {
>>>>>> +			const struct i8k_fan_control_data *control = fan_control->driver_data;
>>>>>>
>>>>>> -		data->manual_fan = control->manual_fan;
>>>>>> -		data->auto_fan = control->auto_fan;
>>>>>> -		dev_info(&pdev->dev, "enabling support for setting automatic/manual fan control\n");
>>>>>> +			data->manual_fan = control->manual_fan;
>>>>>> +			data->auto_fan = control->auto_fan;
>>>>>> +		}
>>>>>>     	}
>>>>>> +	if (data->manual_fan && data->auto_fan)
>>>>>> +		dev_info(&pdev->dev, "enabling support for setting automatic/manual fan control\n");
>>>>>>
>>>>>>     	if (!fan_mult) {
>>>>>>     		/*
>>>>>> --
>>>>>> 2.30.2
>>>>>>

Re: [PATCH 5/7] hwmon: (dell-smm) Improve assembly code

[kernel test robot]

Hi Armin,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on groeck-staging/hwmon-next]
[also build test ERROR on next-20220215]
[cannot apply to linus/master v5.17-rc4]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Armin-Wolf/hwmon-dell-smm-Miscellaneous-improvements/20220216-031722
base:   https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
config: i386-randconfig-a004-20220214 (https://download.01.org/0day-ci/archive/20220216/202202160842.6EYIVsJn-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 37f422f4ac31c8b8041c6b62065263314282dab6)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/e03bd707be4885b219afdfd7a24778fb0a8129e1
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Armin-Wolf/hwmon-dell-smm-Miscellaneous-improvements/20220216-031722
        git checkout e03bd707be4885b219afdfd7a24778fb0a8129e1
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/hwmon/dell-smm-hwmon.c:178:15: error: inline assembly requires more registers than available
           asm volatile("out %%al,$0xb2\n\t"
                        ^
   1 error generated.


vim +178 drivers/hwmon/dell-smm-hwmon.c

   161	
   162	/*
   163	 * Call the System Management Mode BIOS. Code provided by Jonathan Buzzard.
   164	 */
   165	static int i8k_smm_func(void *par)
   166	{
   167		ktime_t calltime = ktime_get();
   168		struct smm_regs *regs = par;
   169		int eax = regs->eax;
   170		int ebx = regs->ebx;
   171		long long duration;
   172		int rc;
   173	
   174		/* SMM requires CPU 0 */
   175		if (smp_processor_id() != 0)
   176			return -EBUSY;
   177	
 > 178		asm volatile("out %%al,$0xb2\n\t"
   179			     "out %%al,$0x84\n"
   180			     : "=a" (regs->eax),
   181			       "=b" (regs->ebx),
   182			       "=c" (regs->ecx),
   183			       "=d" (regs->edx),
   184			       "=S" (regs->esi),
   185			       "=D" (regs->edi),
   186			       CC_OUT(c) (rc)
   187			     : "a" (regs->eax),
   188			       "b" (regs->ebx),
   189			       "c" (regs->ecx),
   190			       "d" (regs->edx),
   191			       "S" (regs->esi),
   192			       "D" (regs->edi));
   193	
   194		if (rc != 0 || (regs->eax & 0xffff) == 0xffff || regs->eax == eax)
   195			rc = -EINVAL;
   196	
   197		duration = ktime_us_delta(ktime_get(), calltime);
   198		pr_debug("smm(0x%.4x 0x%.4x) = 0x%.4x  (took %7lld usecs)\n", eax, ebx,
   199			 (rc ? 0xffff : regs->eax & 0xffff), duration);
   200	
   201		return rc;
   202	}
   203	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Re: [PATCH 5/7] hwmon: (dell-smm) Improve assembly code

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3433 bytes --]

Hi Armin,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on groeck-staging/hwmon-next]
[also build test ERROR on next-20220215]
[cannot apply to linus/master v5.17-rc4]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Armin-Wolf/hwmon-dell-smm-Miscellaneous-improvements/20220216-031722
base:   https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
config: i386-randconfig-a004-20220214 (https://download.01.org/0day-ci/archive/20220216/202202160842.6EYIVsJn-lkp(a)intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 37f422f4ac31c8b8041c6b62065263314282dab6)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/e03bd707be4885b219afdfd7a24778fb0a8129e1
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Armin-Wolf/hwmon-dell-smm-Miscellaneous-improvements/20220216-031722
        git checkout e03bd707be4885b219afdfd7a24778fb0a8129e1
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/hwmon/dell-smm-hwmon.c:178:15: error: inline assembly requires more registers than available
           asm volatile("out %%al,$0xb2\n\t"
                        ^
   1 error generated.


vim +178 drivers/hwmon/dell-smm-hwmon.c

   161	
   162	/*
   163	 * Call the System Management Mode BIOS. Code provided by Jonathan Buzzard.
   164	 */
   165	static int i8k_smm_func(void *par)
   166	{
   167		ktime_t calltime = ktime_get();
   168		struct smm_regs *regs = par;
   169		int eax = regs->eax;
   170		int ebx = regs->ebx;
   171		long long duration;
   172		int rc;
   173	
   174		/* SMM requires CPU 0 */
   175		if (smp_processor_id() != 0)
   176			return -EBUSY;
   177	
 > 178		asm volatile("out %%al,$0xb2\n\t"
   179			     "out %%al,$0x84\n"
   180			     : "=a" (regs->eax),
   181			       "=b" (regs->ebx),
   182			       "=c" (regs->ecx),
   183			       "=d" (regs->edx),
   184			       "=S" (regs->esi),
   185			       "=D" (regs->edi),
   186			       CC_OUT(c) (rc)
   187			     : "a" (regs->eax),
   188			       "b" (regs->ebx),
   189			       "c" (regs->ecx),
   190			       "d" (regs->edx),
   191			       "S" (regs->esi),
   192			       "D" (regs->edi));
   193	
   194		if (rc != 0 || (regs->eax & 0xffff) == 0xffff || regs->eax == eax)
   195			rc = -EINVAL;
   196	
   197		duration = ktime_us_delta(ktime_get(), calltime);
   198		pr_debug("smm(0x%.4x 0x%.4x) = 0x%.4x  (took %7lld usecs)\n", eax, ebx,
   199			 (rc ? 0xffff : regs->eax & 0xffff), duration);
   200	
   201		return rc;
   202	}
   203	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

Re: [PATCH 7/7] hwmon: (dell-smm) Reword and mark parameter "force" as unsafe

[Guenter Roeck]

On Tue, Feb 15, 2022 at 08:11:13PM +0100, Armin Wolf wrote:
> When enabling said module parameter, the driver ignores
> all feature blacklists on relevant models, which has the
> potential for strange side effects. Also there seems to
> be a slight chance for unsupported devices to behave
> badly when probed for features.
> In such cases, the kernel should be tainted to inform
> people that these issues might have been caused by
> the dell_smm_hwmon driver with "force" enabled.
> Also reword the parameter description to remind users
> that enabling "force" also enables blacklisted features.
> 
> Tested on a Dell Inspiron 3505.
> 
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> Reviewed-by: Pali Rohár <pali@kernel.org>

Applied to hwmon-next.

Thanks,
Guenter

> ---
>  drivers/hwmon/dell-smm-hwmon.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> --
> 2.30.2
> 
> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
> index 04a41d59da60..67d63932b48a 100644
> --- a/drivers/hwmon/dell-smm-hwmon.c
> +++ b/drivers/hwmon/dell-smm-hwmon.c
> @@ -87,8 +87,8 @@ MODULE_LICENSE("GPL");
>  MODULE_ALIAS("i8k");
> 
>  static bool force;
> -module_param(force, bool, 0);
> -MODULE_PARM_DESC(force, "Force loading without checking for supported models");
> +module_param_unsafe(force, bool, 0);
> +MODULE_PARM_DESC(force, "Force loading without checking for supported models and features");
> 
>  static bool ignore_dmi;
>  module_param(ignore_dmi, bool, 0);

Re: [PATCH 6/7] hwmon: (dell-smm) Add SMM interface documentation

[Guenter Roeck]

On Tue, Feb 15, 2022 at 08:11:12PM +0100, Armin Wolf wrote:
> Document the SMM interface as requested by Pali Rohar.
> Since Dell does not offer any offical documentation
> regarding the SMM interface, the necessary information
> was extracted from the dell_smm_hwmon driver and other
> sources.
> 
> Suggested-by: Pali Rohár <pali@kernel.org>
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> Reviewed-by: Pali Rohár <pali@kernel.org>

Applied to hwmon-next.

Thanks,
Guenter

> ---
>  Documentation/hwmon/dell-smm-hwmon.rst | 180 +++++++++++++++++++++++++
>  1 file changed, 180 insertions(+)
> 
> --
> 2.30.2
> 
> diff --git a/Documentation/hwmon/dell-smm-hwmon.rst b/Documentation/hwmon/dell-smm-hwmon.rst
> index cfaee682a245..12bba5fd1447 100644
> --- a/Documentation/hwmon/dell-smm-hwmon.rst
> +++ b/Documentation/hwmon/dell-smm-hwmon.rst
> @@ -173,3 +173,183 @@ obtain the same information and to control the fan status. The ioctl
>  interface can be accessed from C programs or from shell using the
>  i8kctl utility. See the source file of ``i8kutils`` for more
>  information on how to use the ioctl interface.
> +
> +SMM Interface
> +-------------
> +
> +.. warning:: The SMM interface was reverse-engineered by trial-and-error
> +             since Dell did not provide any Documentation,
> +             please keep that in mind.
> +
> +The driver uses the SMM interface to send commands to the system BIOS.
> +This interface is normally used by Dell's 32-bit diagnostic program or
> +on newer notebook models by the buildin BIOS diagnostics.
> +The SMM is triggered by writing to the special ioports ``0xb2`` and ``0x84``,
> +and may cause short hangs when the BIOS code is taking too long to
> +execute.
> +
> +The SMM handler inside the system BIOS looks at the contents of the
> +``eax``, ``ebx``, ``ecx``, ``edx``, ``esi`` and ``edi`` registers.
> +Each register has a special purpose:
> +
> +=============== ==================================
> +Register        Purpose
> +=============== ==================================
> +eax             Holds the command code before SMM,
> +                holds the first result after SMM.
> +ebx             Holds the arguments.
> +ecx             Unknown, set to 0.
> +edx             Holds the second result after SMM.
> +esi             Unknown, set to 0.
> +edi             Unknown, set to 0.
> +=============== ==================================
> +
> +The SMM handler can signal a failure by either:
> +
> +- setting the lower sixteen bits of ``eax`` to ``0xffff``
> +- not modifying ``eax`` at all
> +- setting the carry flag
> +
> +SMM command codes
> +-----------------
> +
> +=============== ======================= ================================================
> +Command Code    Command Name            Description
> +=============== ======================= ================================================
> +``0x0025``      Get Fn key status       Returns the Fn key pressed after SMM:
> +
> +                                        - 9th bit in ``eax`` indicates Volume up
> +                                        - 10th bit in ``eax`` indicates Volume down
> +                                        - both bits indicate Volume mute
> +
> +``0xa069``      Get power status        Returns current power status after SMM:
> +
> +                                        - 1st bit in ``eax`` indicates Battery connected
> +                                        - 3th bit in ``eax`` indicates AC connected
> +
> +``0x00a3``      Get fan state           Returns current fan state after SMM:
> +
> +                                        - 1st byte in ``eax`` holds the current
> +                                          fan state (0 - 2 or 3)
> +
> +``0x01a3``      Set fan state           Sets the fan speed:
> +
> +                                        - 1st byte in ``ebx`` holds the fan number
> +                                        - 2nd byte in ``ebx`` holds the desired
> +                                          fan state (0 - 2 or 3)
> +
> +``0x02a3``      Get fan speed           Returns the current fan speed in RPM:
> +
> +                                        - 1st byte in ``ebx`` holds the fan number
> +                                        - 1st word in ``eax`` holds the current
> +                                          fan speed in RPM (after SMM)
> +
> +``0x03a3``      Get fan type            Returns the fan type:
> +
> +                                        - 1st byte in ``ebx`` holds the fan number
> +                                        - 1st byte in ``eax`` holds the
> +                                          fan type (after SMM):
> +
> +                                          - 5th bit indicates docking fan
> +                                          - 1 indicates Processor fan
> +                                          - 2 indicates Motherboard fan
> +                                          - 3 indicates Video fan
> +                                          - 4 indicates Power supply fan
> +                                          - 5 indicates Chipset fan
> +                                          - 6 indicates other fan type
> +
> +``0x04a3``      Get nominal fan speed   Returns the nominal RPM in each fan state:
> +
> +                                        - 1st byte in ``ebx`` holds the fan number
> +                                        - 2nd byte in ``ebx`` holds the fan state
> +                                          in question (0 - 2 or 3)
> +                                        - 1st word in ``eax`` holds the nominal
> +                                          fan speed in RPM (after SMM)
> +
> +``0x05a3``      Get fan speed tolerance Returns the speed tolerance for each fan state:
> +
> +                                        - 1st byte in ``ebx`` holds the fan number
> +                                        - 2nd byte in ``ebx`` holds the fan state
> +                                          in question (0 - 2 or 3)
> +                                        - 1st byte in ``eax`` returns the speed
> +                                          tolerance
> +
> +``0x10a3``      Get sensor temperature  Returns the measured temperature:
> +
> +                                        - 1st byte in ``ebx`` holds the sensor number
> +                                        - 1st byte in ``eax`` holds the measured
> +                                          temperature (after SMM)
> +
> +``0x11a3``      Get sensor type         Returns the sensor type:
> +
> +                                        - 1st byte in ``ebx`` holds the sensor number
> +                                        - 1st byte in ``eax`` holds the
> +                                          temperature type (after SMM):
> +
> +                                          - 1 indicates CPU sensor
> +                                          - 2 indicates GPU sensor
> +                                          - 3 indicates SODIMM sensor
> +                                          - 4 indicates other sensor type
> +                                          - 5 indicates Ambient sensor
> +                                          - 6 indicates other sensor type
> +
> +``0xfea3``      Get SMM signature       Returns Dell signature if interface
> +                                        is supported (after SMM):
> +
> +                                        - ``eax`` holds 1145651527
> +                                          (0x44494147 or "DIAG")
> +                                        - ``edx`` holds 1145392204
> +                                          (0x44454c4c or "DELL")
> +
> +``0xffa3``      Get SMM signature       Same as ``0xfea3``, check both.
> +=============== ======================= ================================================
> +
> +There are additional commands for enabling (``0x31a3`` or ``0x35a3``) and
> +disabling (``0x30a3`` or ``0x34a3``) automatic fan speed control.
> +The commands are however causing severe sideeffects on many machines, so
> +they are not used by default.
> +
> +On several machines (Inspiron 3505, Precision 490, Vostro 1720, ...), the
> +fans supports a 4th "magic" state, which signals the BIOS that automatic
> +fan control should be enabled for a specific fan.
> +However there are also some machines who do support a 4th regular fan state too,
> +but in case of the "magic" state, the nominal RPM reported for this state is a
> +placeholder value, which however is not always detectable.
> +
> +Firmware Bugs
> +-------------
> +
> +The SMM calls can behave erratic on some machines:
> +
> +======================================================= =================
> +Firmware Bug                                            Affected Machines
> +======================================================= =================
> +Reading of fan states return spurious errors.           Precision 490
> +
> +Reading of fan types causes erratic fan behaviour.      Studio XPS 8000
> +
> +                                                        Studio XPS 8100
> +
> +                                                        Inspiron 580
> +
> +Fan-related SMM calls take too long (about 500ms).      Inspiron 7720
> +
> +                                                        Vostro 3360
> +
> +                                                        XPS 13 9333
> +
> +                                                        XPS 15 L502X
> +======================================================= =================
> +
> +In case you experience similar issues on your Dell machine, please
> +submit a bugreport on bugzilla to we can apply workarounds.
> +
> +Limitations
> +-----------
> +
> +The SMM calls can take too long to execute on some machines, causing
> +short hangs and/or audio glitches.
> +Also the fan state needs to be restored after suspend, as well as
> +the automatic mode settings.
> +When reading a temperature sensor, values above 127 degrees indicate
> +a BIOS read error or a deactivated sensor.

Re: [PATCH 3/7] hwmon: (dell-smm) Make fan/temp sensor number a u8

[Guenter Roeck]

On Tue, Feb 15, 2022 at 08:11:09PM +0100, Armin Wolf wrote:
> Right now, we only use bits 0 to 7 of the fan/temp sensor number
> by doing number & 0xff. Passing the value as a u8 makes this
> step unnecessary. Also add checks to the ioctl handler since
> users might get confused when passing 0x00000101 does the same
> as passing 0x00000001.
> 
> Tested on a Dell Inspiron 3505.
> 
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> Reviewed-by: Pali Rohár <pali@kernel.org>

Applied to hwmon-next.

Thanks,
Guenter

> ---
>  drivers/hwmon/dell-smm-hwmon.c | 68 ++++++++++++++++++++++------------
>  1 file changed, 45 insertions(+), 23 deletions(-)
> 
> --
> 2.30.2
> 
> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
> index 3b49e55d060f..a102034a1d38 100644
> --- a/drivers/hwmon/dell-smm-hwmon.c
> +++ b/drivers/hwmon/dell-smm-hwmon.c
> @@ -21,6 +21,7 @@
>  #include <linux/errno.h>
>  #include <linux/hwmon.h>
>  #include <linux/init.h>
> +#include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/mutex.h>
>  #include <linux/platform_device.h>
> @@ -254,46 +255,52 @@ static int i8k_smm(struct smm_regs *regs)
>  /*
>   * Read the fan status.
>   */
> -static int i8k_get_fan_status(const struct dell_smm_data *data, int fan)
> +static int i8k_get_fan_status(const struct dell_smm_data *data, u8 fan)
>  {
> -	struct smm_regs regs = { .eax = I8K_SMM_GET_FAN, };
> +	struct smm_regs regs = {
> +		.eax = I8K_SMM_GET_FAN,
> +		.ebx = fan,
> +	};
> 
>  	if (data->disallow_fan_support)
>  		return -EINVAL;
> 
> -	regs.ebx = fan & 0xff;
>  	return i8k_smm(&regs) ? : regs.eax & 0xff;
>  }
> 
>  /*
>   * Read the fan speed in RPM.
>   */
> -static int i8k_get_fan_speed(const struct dell_smm_data *data, int fan)
> +static int i8k_get_fan_speed(const struct dell_smm_data *data, u8 fan)
>  {
> -	struct smm_regs regs = { .eax = I8K_SMM_GET_SPEED, };
> +	struct smm_regs regs = {
> +		.eax = I8K_SMM_GET_SPEED,
> +		.ebx = fan,
> +	};
> 
>  	if (data->disallow_fan_support)
>  		return -EINVAL;
> 
> -	regs.ebx = fan & 0xff;
>  	return i8k_smm(&regs) ? : (regs.eax & 0xffff) * data->i8k_fan_mult;
>  }
> 
>  /*
>   * Read the fan type.
>   */
> -static int _i8k_get_fan_type(const struct dell_smm_data *data, int fan)
> +static int _i8k_get_fan_type(const struct dell_smm_data *data, u8 fan)
>  {
> -	struct smm_regs regs = { .eax = I8K_SMM_GET_FAN_TYPE, };
> +	struct smm_regs regs = {
> +		.eax = I8K_SMM_GET_FAN_TYPE,
> +		.ebx = fan,
> +	};
> 
>  	if (data->disallow_fan_support || data->disallow_fan_type_call)
>  		return -EINVAL;
> 
> -	regs.ebx = fan & 0xff;
>  	return i8k_smm(&regs) ? : regs.eax & 0xff;
>  }
> 
> -static int i8k_get_fan_type(struct dell_smm_data *data, int fan)
> +static int i8k_get_fan_type(struct dell_smm_data *data, u8 fan)
>  {
>  	/* I8K_SMM_GET_FAN_TYPE SMM call is expensive, so cache values */
>  	if (data->fan_type[fan] == INT_MIN)
> @@ -305,14 +312,16 @@ static int i8k_get_fan_type(struct dell_smm_data *data, int fan)
>  /*
>   * Read the fan nominal rpm for specific fan speed.
>   */
> -static int __init 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, u8 fan, int speed)
>  {
> -	struct smm_regs regs = { .eax = I8K_SMM_GET_NOM_SPEED, };
> +	struct smm_regs regs = {
> +		.eax = I8K_SMM_GET_NOM_SPEED,
> +		.ebx = fan | (speed << 8),
> +	};
> 
>  	if (data->disallow_fan_support)
>  		return -EINVAL;
> 
> -	regs.ebx = (fan & 0xff) | (speed << 8);
>  	return i8k_smm(&regs) ? : (regs.eax & 0xffff) * data->i8k_fan_mult;
>  }
> 
> @@ -333,7 +342,7 @@ static int i8k_enable_fan_auto_mode(const struct dell_smm_data *data, bool enabl
>  /*
>   * Set the fan speed (off, low, high, ...).
>   */
> -static int i8k_set_fan(const struct dell_smm_data *data, int fan, int speed)
> +static int i8k_set_fan(const struct dell_smm_data *data, u8 fan, int speed)
>  {
>  	struct smm_regs regs = { .eax = I8K_SMM_SET_FAN, };
> 
> @@ -341,33 +350,35 @@ static int i8k_set_fan(const struct dell_smm_data *data, int fan, int speed)
>  		return -EINVAL;
> 
>  	speed = (speed < 0) ? 0 : ((speed > data->i8k_fan_max) ? data->i8k_fan_max : speed);
> -	regs.ebx = (fan & 0xff) | (speed << 8);
> +	regs.ebx = fan | (speed << 8);
> 
>  	return i8k_smm(&regs);
>  }
> 
> -static int __init i8k_get_temp_type(int sensor)
> +static int __init i8k_get_temp_type(u8 sensor)
>  {
> -	struct smm_regs regs = { .eax = I8K_SMM_GET_TEMP_TYPE, };
> +	struct smm_regs regs = {
> +		.eax = I8K_SMM_GET_TEMP_TYPE,
> +		.ebx = sensor,
> +	};
> 
> -	regs.ebx = sensor & 0xff;
>  	return i8k_smm(&regs) ? : regs.eax & 0xff;
>  }
> 
>  /*
>   * Read the cpu temperature.
>   */
> -static int _i8k_get_temp(int sensor)
> +static int _i8k_get_temp(u8 sensor)
>  {
>  	struct smm_regs regs = {
>  		.eax = I8K_SMM_GET_TEMP,
> -		.ebx = sensor & 0xff,
> +		.ebx = sensor,
>  	};
> 
>  	return i8k_smm(&regs) ? : regs.eax & 0xff;
>  }
> 
> -static int i8k_get_temp(int sensor)
> +static int i8k_get_temp(u8 sensor)
>  {
>  	int temp = _i8k_get_temp(sensor);
> 
> @@ -500,6 +511,9 @@ static long i8k_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
>  		if (copy_from_user(&val, argp, sizeof(int)))
>  			return -EFAULT;
> 
> +		if (val > U8_MAX || val < 0)
> +			return -EINVAL;
> +
>  		val = i8k_get_fan_speed(data, val);
>  		break;
> 
> @@ -507,6 +521,9 @@ static long i8k_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
>  		if (copy_from_user(&val, argp, sizeof(int)))
>  			return -EFAULT;
> 
> +		if (val > U8_MAX || val < 0)
> +			return -EINVAL;
> +
>  		val = i8k_get_fan_status(data, val);
>  		break;
> 
> @@ -517,6 +534,9 @@ static long i8k_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
>  		if (copy_from_user(&val, argp, sizeof(int)))
>  			return -EFAULT;
> 
> +		if (val > U8_MAX || val < 0)
> +			return -EINVAL;
> +
>  		if (copy_from_user(&speed, argp + 1, sizeof(int)))
>  			return -EFAULT;
> 
> @@ -924,7 +944,8 @@ static int __init dell_smm_init_hwmon(struct device *dev)
>  {
>  	struct dell_smm_data *data = dev_get_drvdata(dev);
>  	struct device *dell_smm_hwmon_dev;
> -	int i, state, err;
> +	int state, err;
> +	u8 i;
> 
>  	for (i = 0; i < DELL_SMM_NO_TEMP; i++) {
>  		data->temp_type[i] = i8k_get_temp_type(i);
> @@ -1245,7 +1266,8 @@ static int __init dell_smm_probe(struct platform_device *pdev)
>  {
>  	struct dell_smm_data *data;
>  	const struct dmi_system_id *id, *fan_control;
> -	int fan, ret;
> +	int ret;
> +	u8 fan;
> 
>  	data = devm_kzalloc(&pdev->dev, sizeof(struct dell_smm_data), GFP_KERNEL);
>  	if (!data)

Re: [PATCH 4/7] hwmon: (dell-smm) Improve temperature sensors detection

[Guenter Roeck]

On Tue, Feb 15, 2022 at 08:11:10PM +0100, Armin Wolf wrote:
> On the Dell Inspiron 3505, three temperature sensors are
> available through the SMM interface. However since they
> do not have an associated type, they are not detected.
> Probe for those sensors in case no type was detected.
> _i8k_get_temp() is used instead of i8k_get_temp()
> since it is sometimes faster and the result is
> easier to check (no -ENODATA) since we do not
> care about the actual temp value.
> 
> Tested on a Dell Inspiron 3505.
> 
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>

Applied to hwmon-next.

Thanks,
Guenter

> ---
>  drivers/hwmon/dell-smm-hwmon.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> --
> 2.30.2
> 
> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
> index a102034a1d38..b7016971bb2e 100644
> --- a/drivers/hwmon/dell-smm-hwmon.c
> +++ b/drivers/hwmon/dell-smm-hwmon.c
> @@ -655,6 +655,11 @@ static umode_t dell_smm_is_visible(const void *drvdata, enum hwmon_sensor_types
>  	case hwmon_temp:
>  		switch (attr) {
>  		case hwmon_temp_input:
> +			/* _i8k_get_temp() is fine since we do not care about the actual value */
> +			if (data->temp_type[channel] >= 0 || _i8k_get_temp(channel) >= 0)
> +				return 0444;
> +
> +			break;
>  		case hwmon_temp_label:
>  			if (data->temp_type[channel] >= 0)
>  				return 0444;