| Message ID | 20211211155422.16830-3-W_Armin@gmx.de (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | hwmon: (dell-smm) Improve ioctl handler | expand |
On Saturday 11 December 2021 16:54:22 Armin Wolf wrote: > The only purpose of i8k_ioctl() is to call i8k_ioctl_unlocked() > with i8k_mutex held. Judging from the hwmon code, this mutex > only needs to be held when setting the fan speed/mode, so > the operation of I8K_SET_FAN is guaranteed to be atomic. > Unify both functions and reduce the locking of i8k_mutex > to I8K_SET_FAN. > > 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 | 28 +++++++++------------------- > 1 file changed, 9 insertions(+), 19 deletions(-) > > diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c > index 186d40938036..d8c6e75bb374 100644 > --- a/drivers/hwmon/dell-smm-hwmon.c > +++ b/drivers/hwmon/dell-smm-hwmon.c > @@ -449,12 +449,12 @@ static int i8k_get_power_status(void) > * Procfs interface > */ > > -static int > -i8k_ioctl_unlocked(struct file *fp, struct dell_smm_data *data, unsigned int cmd, unsigned long arg) > +static long i8k_ioctl(struct file *fp, unsigned int cmd, unsigned long arg) > { > - int val = 0; > - int speed, err; > + struct dell_smm_data *data = PDE_DATA(file_inode(fp)); > int __user *argp = (int __user *)arg; > + int speed, err; > + int val = 0; > > if (!argp) > return -EINVAL; > @@ -516,11 +516,13 @@ i8k_ioctl_unlocked(struct file *fp, struct dell_smm_data *data, unsigned int cmd > if (copy_from_user(&speed, argp + 1, sizeof(int))) > return -EFAULT; > > + mutex_lock(&data->i8k_mutex); > err = i8k_set_fan(data, val, speed); > if (err < 0) > - return err; > - > - val = i8k_get_fan_status(data, val); > + val = err; > + else > + val = i8k_get_fan_status(data, val); > + mutex_unlock(&data->i8k_mutex); > break; > > default: > @@ -536,18 +538,6 @@ i8k_ioctl_unlocked(struct file *fp, struct dell_smm_data *data, unsigned int cmd > return 0; > } > > -static long i8k_ioctl(struct file *fp, unsigned int cmd, unsigned long arg) > -{ > - struct dell_smm_data *data = PDE_DATA(file_inode(fp)); > - long ret; > - > - mutex_lock(&data->i8k_mutex); > - ret = i8k_ioctl_unlocked(fp, data, cmd, arg); > - mutex_unlock(&data->i8k_mutex); > - > - return ret; > -} > - > /* > * Print the information for /proc/i8k. > */ > -- > 2.30.2 >
diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c index 186d40938036..d8c6e75bb374 100644 --- a/drivers/hwmon/dell-smm-hwmon.c +++ b/drivers/hwmon/dell-smm-hwmon.c @@ -449,12 +449,12 @@ static int i8k_get_power_status(void) * Procfs interface */ -static int -i8k_ioctl_unlocked(struct file *fp, struct dell_smm_data *data, unsigned int cmd, unsigned long arg) +static long i8k_ioctl(struct file *fp, unsigned int cmd, unsigned long arg) { - int val = 0; - int speed, err; + struct dell_smm_data *data = PDE_DATA(file_inode(fp)); int __user *argp = (int __user *)arg; + int speed, err; + int val = 0; if (!argp) return -EINVAL; @@ -516,11 +516,13 @@ i8k_ioctl_unlocked(struct file *fp, struct dell_smm_data *data, unsigned int cmd if (copy_from_user(&speed, argp + 1, sizeof(int))) return -EFAULT; + mutex_lock(&data->i8k_mutex); err = i8k_set_fan(data, val, speed); if (err < 0) - return err; - - val = i8k_get_fan_status(data, val); + val = err; + else + val = i8k_get_fan_status(data, val); + mutex_unlock(&data->i8k_mutex); break; default: @@ -536,18 +538,6 @@ i8k_ioctl_unlocked(struct file *fp, struct dell_smm_data *data, unsigned int cmd return 0; } -static long i8k_ioctl(struct file *fp, unsigned int cmd, unsigned long arg) -{ - struct dell_smm_data *data = PDE_DATA(file_inode(fp)); - long ret; - - mutex_lock(&data->i8k_mutex); - ret = i8k_ioctl_unlocked(fp, data, cmd, arg); - mutex_unlock(&data->i8k_mutex); - - return ret; -} - /* * Print the information for /proc/i8k. */
The only purpose of i8k_ioctl() is to call i8k_ioctl_unlocked() with i8k_mutex held. Judging from the hwmon code, this mutex only needs to be held when setting the fan speed/mode, so the operation of I8K_SET_FAN is guaranteed to be atomic. Unify both functions and reduce the locking of i8k_mutex to I8K_SET_FAN. Tested on a Dell Inspiron 3505. Signed-off-by: Armin Wolf <W_Armin@gmx.de> --- drivers/hwmon/dell-smm-hwmon.c | 28 +++++++++------------------- 1 file changed, 9 insertions(+), 19 deletions(-) -- 2.30.2