This patch series improves the ioctl handler in dell_smm_hwmon.
The first patch is simplifying the ioctl handler by removing
an unnecessary switch case, while the second patch is unifying
both i8k_ioctl() and i8k_ioctl_unlocked(), resulting in better
performance since i8k_mutex is only acquired when needed
(during fan status change) instead of being acquired for
every ioctl call.
Tested on a Dell Inspiron 3505.
changes in v2:
- simplify logic between mutex_lock()/_unlock()
- replace hardcoded values with sizeof statements
Armin Wolf (2):
hwmon: (dell-smm) Simplify ioctl handler
hwmon: (dell-smm) Unify i8k_ioctl() and i8k_ioctl_unlocked()
drivers/hwmon/dell-smm-hwmon.c | 58 +++++++++++-----------------------
1 file changed, 18 insertions(+), 40 deletions(-)
--
2.30.2
The second switch-case has no real purpose:
- for I8K_BIOS_VERSION, val does not represent a return value,
making the check for error values unnecessary.
- for I8K_MACHINE_ID, val remains zero, so the error check is
unnecessary too.
Remove the switch-case and move the calls to copy_to_user()
into the first switch-case for I8K_BIOS_VERSION/_MACHINE_ID.
Omit buff[] since data->bios_machineid already contains the string
with the necessary zero padding through devm_kzalloc().
Tested on a Dell Inspiron 3505.
Signed-off-by: Armin Wolf <[email protected]>
---
drivers/hwmon/dell-smm-hwmon.c | 30 +++++++++---------------------
1 file changed, 9 insertions(+), 21 deletions(-)
diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
index 5596c211f38d..186d40938036 100644
--- a/drivers/hwmon/dell-smm-hwmon.c
+++ b/drivers/hwmon/dell-smm-hwmon.c
@@ -454,7 +454,6 @@ i8k_ioctl_unlocked(struct file *fp, struct dell_smm_data *data, unsigned int cmd
{
int val = 0;
int speed, err;
- unsigned char buff[16];
int __user *argp = (int __user *)arg;
if (!argp)
@@ -468,15 +467,19 @@ i8k_ioctl_unlocked(struct file *fp, struct dell_smm_data *data, unsigned int cmd
val = (data->bios_version[0] << 16) |
(data->bios_version[1] << 8) | data->bios_version[2];
- break;
+ if (copy_to_user(argp, &val, sizeof(val)))
+ return -EFAULT;
+
+ return 0;
case I8K_MACHINE_ID:
if (restricted && !capable(CAP_SYS_ADMIN))
return -EPERM;
- strscpy_pad(buff, data->bios_machineid, sizeof(buff));
- break;
+ if (copy_to_user(argp, data->bios_machineid, sizeof(data->bios_machineid)))
+ return -EFAULT;
+ return 0;
case I8K_FN_STATUS:
val = i8k_get_fn_status();
break;
@@ -527,23 +530,8 @@ i8k_ioctl_unlocked(struct file *fp, struct dell_smm_data *data, unsigned int cmd
if (val < 0)
return val;
- switch (cmd) {
- case I8K_BIOS_VERSION:
- if (copy_to_user(argp, &val, 4))
- return -EFAULT;
-
- break;
- case I8K_MACHINE_ID:
- if (copy_to_user(argp, buff, 16))
- return -EFAULT;
-
- break;
- default:
- if (copy_to_user(argp, &val, sizeof(int)))
- return -EFAULT;
-
- break;
- }
+ if (copy_to_user(argp, &val, sizeof(int)))
+ return -EFAULT;
return 0;
}
--
2.30.2
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 <[email protected]>
---
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
On Saturday 11 December 2021 16:54:21 Armin Wolf wrote:
> The second switch-case has no real purpose:
>
> - for I8K_BIOS_VERSION, val does not represent a return value,
> making the check for error values unnecessary.
> - for I8K_MACHINE_ID, val remains zero, so the error check is
> unnecessary too.
>
> Remove the switch-case and move the calls to copy_to_user()
> into the first switch-case for I8K_BIOS_VERSION/_MACHINE_ID.
> Omit buff[] since data->bios_machineid already contains the string
> with the necessary zero padding through devm_kzalloc().
>
> Tested on a Dell Inspiron 3505.
>
> Signed-off-by: Armin Wolf <[email protected]>
Reviewed-by: Pali Rohár <[email protected]>
> ---
> drivers/hwmon/dell-smm-hwmon.c | 30 +++++++++---------------------
> 1 file changed, 9 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
> index 5596c211f38d..186d40938036 100644
> --- a/drivers/hwmon/dell-smm-hwmon.c
> +++ b/drivers/hwmon/dell-smm-hwmon.c
> @@ -454,7 +454,6 @@ i8k_ioctl_unlocked(struct file *fp, struct dell_smm_data *data, unsigned int cmd
> {
> int val = 0;
> int speed, err;
> - unsigned char buff[16];
> int __user *argp = (int __user *)arg;
>
> if (!argp)
> @@ -468,15 +467,19 @@ i8k_ioctl_unlocked(struct file *fp, struct dell_smm_data *data, unsigned int cmd
>
> val = (data->bios_version[0] << 16) |
> (data->bios_version[1] << 8) | data->bios_version[2];
> - break;
>
> + if (copy_to_user(argp, &val, sizeof(val)))
> + return -EFAULT;
> +
> + return 0;
> case I8K_MACHINE_ID:
> if (restricted && !capable(CAP_SYS_ADMIN))
> return -EPERM;
>
> - strscpy_pad(buff, data->bios_machineid, sizeof(buff));
> - break;
> + if (copy_to_user(argp, data->bios_machineid, sizeof(data->bios_machineid)))
> + return -EFAULT;
>
> + return 0;
> case I8K_FN_STATUS:
> val = i8k_get_fn_status();
> break;
> @@ -527,23 +530,8 @@ i8k_ioctl_unlocked(struct file *fp, struct dell_smm_data *data, unsigned int cmd
> if (val < 0)
> return val;
>
> - switch (cmd) {
> - case I8K_BIOS_VERSION:
> - if (copy_to_user(argp, &val, 4))
> - return -EFAULT;
> -
> - break;
> - case I8K_MACHINE_ID:
> - if (copy_to_user(argp, buff, 16))
> - return -EFAULT;
> -
> - break;
> - default:
> - if (copy_to_user(argp, &val, sizeof(int)))
> - return -EFAULT;
> -
> - break;
> - }
> + if (copy_to_user(argp, &val, sizeof(int)))
> + return -EFAULT;
>
> return 0;
> }
> --
> 2.30.2
>
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 <[email protected]>
Reviewed-by: Pali Rohár <[email protected]>
> ---
> 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
>
On Sat, Dec 11, 2021 at 04:54:21PM +0100, Armin Wolf wrote:
> The second switch-case has no real purpose:
>
> - for I8K_BIOS_VERSION, val does not represent a return value,
> making the check for error values unnecessary.
> - for I8K_MACHINE_ID, val remains zero, so the error check is
> unnecessary too.
>
> Remove the switch-case and move the calls to copy_to_user()
> into the first switch-case for I8K_BIOS_VERSION/_MACHINE_ID.
> Omit buff[] since data->bios_machineid already contains the string
> with the necessary zero padding through devm_kzalloc().
>
> Tested on a Dell Inspiron 3505.
>
> Signed-off-by: Armin Wolf <[email protected]>
> Reviewed-by: Pali Roh?r <[email protected]>
Series applied.
Thanks,
Guenter
> ---
> drivers/hwmon/dell-smm-hwmon.c | 30 +++++++++---------------------
> 1 file changed, 9 insertions(+), 21 deletions(-)
>
> --
> 2.30.2
>
> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
> index 5596c211f38d..186d40938036 100644
> --- a/drivers/hwmon/dell-smm-hwmon.c
> +++ b/drivers/hwmon/dell-smm-hwmon.c
> @@ -454,7 +454,6 @@ i8k_ioctl_unlocked(struct file *fp, struct dell_smm_data *data, unsigned int cmd
> {
> int val = 0;
> int speed, err;
> - unsigned char buff[16];
> int __user *argp = (int __user *)arg;
>
> if (!argp)
> @@ -468,15 +467,19 @@ i8k_ioctl_unlocked(struct file *fp, struct dell_smm_data *data, unsigned int cmd
>
> val = (data->bios_version[0] << 16) |
> (data->bios_version[1] << 8) | data->bios_version[2];
> - break;
>
> + if (copy_to_user(argp, &val, sizeof(val)))
> + return -EFAULT;
> +
> + return 0;
> case I8K_MACHINE_ID:
> if (restricted && !capable(CAP_SYS_ADMIN))
> return -EPERM;
>
> - strscpy_pad(buff, data->bios_machineid, sizeof(buff));
> - break;
> + if (copy_to_user(argp, data->bios_machineid, sizeof(data->bios_machineid)))
> + return -EFAULT;
>
> + return 0;
> case I8K_FN_STATUS:
> val = i8k_get_fn_status();
> break;
> @@ -527,23 +530,8 @@ i8k_ioctl_unlocked(struct file *fp, struct dell_smm_data *data, unsigned int cmd
> if (val < 0)
> return val;
>
> - switch (cmd) {
> - case I8K_BIOS_VERSION:
> - if (copy_to_user(argp, &val, 4))
> - return -EFAULT;
> -
> - break;
> - case I8K_MACHINE_ID:
> - if (copy_to_user(argp, buff, 16))
> - return -EFAULT;
> -
> - break;
> - default:
> - if (copy_to_user(argp, &val, sizeof(int)))
> - return -EFAULT;
> -
> - break;
> - }
> + if (copy_to_user(argp, &val, sizeof(int)))
> + return -EFAULT;
>
> return 0;
> }