On Saturday 20 November 2021 18:03:18 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->machineid already contains the string s/->machineid/->bios_machineid/ > with the necessary zero padding. data is allocated by devm_kzalloc() so data->bios_machineid is really zero padded. > Tested on a Dell Inspiron 3505. > > Signed-off-by: Armin Wolf <W_Armin@xxxxxx> > --- > 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..b5d1703faa62 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, 4)) > + 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, 16)) What about usage of sizeof(data->bios_machineid) instead of hardcoded constant 16? And maybe same for constant 4? > + 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 >