Message ID | 20220215191113.16640-4-W_Armin@gmx.de (mailing list archive) |
---|---|
State | Accepted |
Headers | show |
Series | hwmon: (dell-smm) Miscellaneous improvements | expand |
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(®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 <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(®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)
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)
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(-) -- 2.30.2