Subject: Re: [PATCH 1/2] hwmon: (dell-smm) Simplify ioctl handler
To:     =?UTF-8?Q?Pali_Roh=c3=a1r?= <pali@kernel.org>
Cc:     jdelvare@suse.com, linux@roeck-us.net, linux-hwmon@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20211120170319.72369-1-W_Armin@gmx.de>
 <20211120170319.72369-2-W_Armin@gmx.de>
 <20211123161332.discv3bfx4rkowah@pali>
From:   Armin Wolf <W_Armin@gmx.de>
Message-ID: <5024959a-772a-ebde-089d-0668e1e188f7@gmx.de>
Date:   Mon, 29 Nov 2021 21:57:40 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.13.0
MIME-Version: 1.0
In-Reply-To: <20211123161332.discv3bfx4rkowah@pali>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: quoted-printable
Content-Language: en-US
Precedence: bulk

Am 23.11.21 um 17:13 schrieb Pali Roh=C3=A1r:

> 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-hw=
mon.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 =3D 0;
>>   	int speed, err;
>> -	unsigned char buff[16];
>>   	int __user *argp =3D (int __user *)arg;
>>
>>   	if (!argp)
>> @@ -468,15 +467,19 @@ i8k_ioctl_unlocked(struct file *fp, struct dell_s=
mm_data *data, unsigned int cmd
>>
>>   		val =3D (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 =3D i8k_get_fn_status();
>>   		break;
>> @@ -527,23 +530,8 @@ i8k_ioctl_unlocked(struct file *fp, struct dell_sm=
m_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
>>