Messages in this thread | | | Subject | Re: [PATCH 1/2] hwmon: (dell-smm) Simplify ioctl handler | From | Armin Wolf <> | Date | Mon, 29 Nov 2021 21:57:40 +0100 |
| |
Am 23.11.21 um 17:13 schrieb Pali Rohár:
> 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@gmx.de> >> --- >> 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?
For the string yes, but maybe i should change the int to an u32?
>> + 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 >>
| |