lkml.org 
[lkml]   [2021]   [Nov]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 1/2] hwmon: (dell-smm) Simplify ioctl handler
From
Date
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
>>

\
 
 \ /
  Last update: 2021-11-29 22:01    [W:0.876 / U:0.320 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site