diff mbox series

[1/2] hwmon: (dell-smm) Simplify ioctl handler

Message ID 20211120170319.72369-2-W_Armin@gmx.de (mailing list archive)
State Changes Requested
Headers show
Series hwmon: (dell-smm) Improve ioctl handler | expand

Commit Message

Armin Wolf Nov. 20, 2021, 5:03 p.m. UTC 
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
with the necessary zero padding.

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(-)

--
2.30.2

Comments

Pali Rohár Nov. 23, 2021, 4:13 p.m. UTC | #1 
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?

> +			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
>
Armin Wolf Nov. 29, 2021, 8:57 p.m. UTC | #2 
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
>>
Pali Rohár Dec. 9, 2021, 4:52 p.m. UTC | #3 
On Monday 29 November 2021 21:57:40 Armin Wolf wrote:
> 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?

I do not know if changing int to u32 should be done or not...

> > > +			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
> > >
diff mbox series

Patch

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))
+			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;
 }