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
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 <[email protected]>
---
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
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 <[email protected]>
---
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(®s) ? : 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(®s) ? : (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(®s) ? : 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(®s) ? : (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(®s);
}
-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(®s) ? : 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(®s) ? : 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
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 <[email protected]>
> Reviewed-by: Pali Roh?r <[email protected]>
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(®s) ? : 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(®s) ? : (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(®s) ? : 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(®s) ? : (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(®s);
> }
>
> -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(®s) ? : 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(®s) ? : 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)
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 <[email protected]>
> Reviewed-by: Pali Roh?r <[email protected]>
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);