LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH 0/2] hwmon: (dell-smm) Improve ioctl handler
@ 2021-11-20 17:03 Armin Wolf
  2021-11-20 17:03 ` [PATCH 1/2] hwmon: (dell-smm) Simplify " Armin Wolf
  2021-11-20 17:03 ` [PATCH 2/2] hwmon: (dell-smm) Unify i8k_ioctl() and i8k_ioctl_unlocked() Armin Wolf
  0 siblings, 2 replies; 15+ messages in thread
From: Armin Wolf @ 2021-11-20 17:03 UTC (permalink / raw)
  To: pali; +Cc: jdelvare, linux, linux-hwmon, linux-kernel

This patch series improves the ioctl handler in dell_smm_hwmon.
The first patch is simplifying the ioctl handler by removing
an unnecessary switch case, while the second patch is unifying
both i8k_ioctl() and i8k_ioctl_unlocked(), resulting in better
performance since i8k_mutex is only acquired when needed
(during fan status change) instead of being acquired for
every ioctl call.

Tested on a Dell Inspiron 3505.

Armin Wolf (2):
  hwmon: (dell-smm) Simplify ioctl handler
  hwmon: (dell-smm) Unify i8k_ioctl() and i8k_ioctl_unlocked()

 drivers/hwmon/dell-smm-hwmon.c | 57 +++++++++++-----------------------
 1 file changed, 18 insertions(+), 39 deletions(-)

--
2.30.2


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 1/2] hwmon: (dell-smm) Simplify ioctl handler
  2021-11-20 17:03 [PATCH 0/2] hwmon: (dell-smm) Improve ioctl handler Armin Wolf
@ 2021-11-20 17:03 ` Armin Wolf
  2021-11-23 16:13   ` Pali Rohár
  2021-11-20 17:03 ` [PATCH 2/2] hwmon: (dell-smm) Unify i8k_ioctl() and i8k_ioctl_unlocked() Armin Wolf
  1 sibling, 1 reply; 15+ messages in thread
From: Armin Wolf @ 2021-11-20 17:03 UTC (permalink / raw)
  To: pali; +Cc: jdelvare, linux, linux-hwmon, linux-kernel

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

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;
 }
--
2.30.2


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 2/2] hwmon: (dell-smm) Unify i8k_ioctl() and i8k_ioctl_unlocked()
  2021-11-20 17:03 [PATCH 0/2] hwmon: (dell-smm) Improve ioctl handler Armin Wolf
  2021-11-20 17:03 ` [PATCH 1/2] hwmon: (dell-smm) Simplify " Armin Wolf
@ 2021-11-20 17:03 ` Armin Wolf
  2021-11-22 16:01   ` Pali Rohár
  2021-11-23 16:04   ` Pali Rohár
  1 sibling, 2 replies; 15+ messages in thread
From: Armin Wolf @ 2021-11-20 17:03 UTC (permalink / raw)
  To: pali; +Cc: jdelvare, linux, linux-hwmon, linux-kernel

The only purpose of i8k_ioctl() is to call i8k_ioctl_unlocked()
with i8k_mutex held. Judging from the hwmon code, this mutex
only needs to be held when setting the fan speed/mode.
Unify both functions and reduce the locking of i8k_mutex
to I8K_SET_FAN.

Tested on a Dell Inspiron 3505.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/hwmon/dell-smm-hwmon.c | 27 +++++++++------------------
 1 file changed, 9 insertions(+), 18 deletions(-)

diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
index b5d1703faa62..e2d388f7360e 100644
--- a/drivers/hwmon/dell-smm-hwmon.c
+++ b/drivers/hwmon/dell-smm-hwmon.c
@@ -449,12 +449,12 @@ static int i8k_get_power_status(void)
  * Procfs interface
  */

-static int
-i8k_ioctl_unlocked(struct file *fp, struct dell_smm_data *data, unsigned int cmd, unsigned long arg)
+static long i8k_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
 {
-	int val = 0;
-	int speed, err;
+	struct dell_smm_data *data = PDE_DATA(file_inode(fp));
 	int __user *argp = (int __user *)arg;
+	int speed, err;
+	int val = 0;

 	if (!argp)
 		return -EINVAL;
@@ -516,11 +516,14 @@ i8k_ioctl_unlocked(struct file *fp, struct dell_smm_data *data, unsigned int cmd
 		if (copy_from_user(&speed, argp + 1, sizeof(int)))
 			return -EFAULT;

+		mutex_lock(&data->i8k_mutex);
 		err = i8k_set_fan(data, val, speed);
-		if (err < 0)
+		if (err < 0) {
+			mutex_unlock(&data->i8k_mutex);
 			return err;
-
+		}
 		val = i8k_get_fan_status(data, val);
+		mutex_unlock(&data->i8k_mutex);
 		break;

 	default:
@@ -536,18 +539,6 @@ i8k_ioctl_unlocked(struct file *fp, struct dell_smm_data *data, unsigned int cmd
 	return 0;
 }

-static long i8k_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
-{
-	struct dell_smm_data *data = PDE_DATA(file_inode(fp));
-	long ret;
-
-	mutex_lock(&data->i8k_mutex);
-	ret = i8k_ioctl_unlocked(fp, data, cmd, arg);
-	mutex_unlock(&data->i8k_mutex);
-
-	return ret;
-}
-
 /*
  * Print the information for /proc/i8k.
  */
--
2.30.2


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/2] hwmon: (dell-smm) Unify i8k_ioctl() and i8k_ioctl_unlocked()
  2021-11-20 17:03 ` [PATCH 2/2] hwmon: (dell-smm) Unify i8k_ioctl() and i8k_ioctl_unlocked() Armin Wolf
@ 2021-11-22 16:01   ` Pali Rohár
  2021-11-22 17:55     ` Guenter Roeck
  2021-11-23 16:04   ` Pali Rohár
  1 sibling, 1 reply; 15+ messages in thread
From: Pali Rohár @ 2021-11-22 16:01 UTC (permalink / raw)
  To: Armin Wolf; +Cc: jdelvare, linux, linux-hwmon, linux-kernel

On Saturday 20 November 2021 18:03:19 Armin Wolf wrote:
> The only purpose of i8k_ioctl() is to call i8k_ioctl_unlocked()
> with i8k_mutex held. Judging from the hwmon code, this mutex
> only needs to be held when setting the fan speed/mode.

Really? I think that there is no difference between setting and getting
fan speed/mode. At least I do not see why 'set' needs mutex and 'get' do
not need it. Some more explanation is needed...

I'm not sure here but SMM call consist of two 'out' instructions and we
probably need to ensure that interrupt does not happen between them.

> Unify both functions and reduce the locking of i8k_mutex
> to I8K_SET_FAN.
> 
> Tested on a Dell Inspiron 3505.
> 
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> ---
>  drivers/hwmon/dell-smm-hwmon.c | 27 +++++++++------------------
>  1 file changed, 9 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
> index b5d1703faa62..e2d388f7360e 100644
> --- a/drivers/hwmon/dell-smm-hwmon.c
> +++ b/drivers/hwmon/dell-smm-hwmon.c
> @@ -449,12 +449,12 @@ static int i8k_get_power_status(void)
>   * Procfs interface
>   */
> 
> -static int
> -i8k_ioctl_unlocked(struct file *fp, struct dell_smm_data *data, unsigned int cmd, unsigned long arg)
> +static long i8k_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
>  {
> -	int val = 0;
> -	int speed, err;
> +	struct dell_smm_data *data = PDE_DATA(file_inode(fp));
>  	int __user *argp = (int __user *)arg;
> +	int speed, err;
> +	int val = 0;
> 
>  	if (!argp)
>  		return -EINVAL;
> @@ -516,11 +516,14 @@ i8k_ioctl_unlocked(struct file *fp, struct dell_smm_data *data, unsigned int cmd
>  		if (copy_from_user(&speed, argp + 1, sizeof(int)))
>  			return -EFAULT;
> 
> +		mutex_lock(&data->i8k_mutex);
>  		err = i8k_set_fan(data, val, speed);
> -		if (err < 0)
> +		if (err < 0) {
> +			mutex_unlock(&data->i8k_mutex);
>  			return err;
> -
> +		}
>  		val = i8k_get_fan_status(data, val);
> +		mutex_unlock(&data->i8k_mutex);
>  		break;
> 
>  	default:
> @@ -536,18 +539,6 @@ i8k_ioctl_unlocked(struct file *fp, struct dell_smm_data *data, unsigned int cmd
>  	return 0;
>  }
> 
> -static long i8k_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
> -{
> -	struct dell_smm_data *data = PDE_DATA(file_inode(fp));
> -	long ret;
> -
> -	mutex_lock(&data->i8k_mutex);
> -	ret = i8k_ioctl_unlocked(fp, data, cmd, arg);
> -	mutex_unlock(&data->i8k_mutex);
> -
> -	return ret;
> -}
> -
>  /*
>   * Print the information for /proc/i8k.
>   */
> --
> 2.30.2
> 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/2] hwmon: (dell-smm) Unify i8k_ioctl() and i8k_ioctl_unlocked()
  2021-11-22 16:01   ` Pali Rohár
@ 2021-11-22 17:55     ` Guenter Roeck
  2021-11-22 18:50       ` Armin Wolf
  0 siblings, 1 reply; 15+ messages in thread
From: Guenter Roeck @ 2021-11-22 17:55 UTC (permalink / raw)
  To: Pali Rohár, Armin Wolf; +Cc: jdelvare, linux-hwmon, linux-kernel

On 11/22/21 8:01 AM, Pali Rohár wrote:
> On Saturday 20 November 2021 18:03:19 Armin Wolf wrote:
>> The only purpose of i8k_ioctl() is to call i8k_ioctl_unlocked()
>> with i8k_mutex held. Judging from the hwmon code, this mutex
>> only needs to be held when setting the fan speed/mode.
> 
> Really? I think that there is no difference between setting and getting
> fan speed/mode. At least I do not see why 'set' needs mutex and 'get' do
> not need it. Some more explanation is needed...
> 
I8K_SET_FAN sets the fan speed and returns the current status. Without
locking, the returned status may not match or be associated with the previous
set operation.

Maybe that doesn't matter, and the synchronization is not needed. If so,
you can probably remove the locking entirely.

Guenter

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/2] hwmon: (dell-smm) Unify i8k_ioctl() and i8k_ioctl_unlocked()
  2021-11-22 17:55     ` Guenter Roeck
@ 2021-11-22 18:50       ` Armin Wolf
  2021-11-22 19:10         ` Pali Rohár
  0 siblings, 1 reply; 15+ messages in thread
From: Armin Wolf @ 2021-11-22 18:50 UTC (permalink / raw)
  To: Guenter Roeck, Pali Rohár; +Cc: jdelvare, linux-hwmon, linux-kernel


Am 22.11.21 um 18:55 schrieb Guenter Roeck:
> On 11/22/21 8:01 AM, Pali Rohár wrote:
>> On Saturday 20 November 2021 18:03:19 Armin Wolf wrote:
>>> The only purpose of i8k_ioctl() is to call i8k_ioctl_unlocked()
>>> with i8k_mutex held. Judging from the hwmon code, this mutex
>>> only needs to be held when setting the fan speed/mode.
>>
>> Really? I think that there is no difference between setting and getting
>> fan speed/mode. At least I do not see why 'set' needs mutex and 'get' do
>> not need it. Some more explanation is needed...
>>
> I8K_SET_FAN sets the fan speed and returns the current status. Without
> locking, the returned status may not match or be associated with the
> previous
> set operation.
>
> Maybe that doesn't matter, and the synchronization is not needed. If so,
> you can probably remove the locking entirely.
>
> Guenter

That is the reason i kept the locking code. Since i do not want to break
the ioctl interfacein any way, removing the locking code seems too risky
to me.

Armin


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/2] hwmon: (dell-smm) Unify i8k_ioctl() and i8k_ioctl_unlocked()
  2021-11-22 18:50       ` Armin Wolf
@ 2021-11-22 19:10         ` Pali Rohár
  2021-11-22 19:28           ` Guenter Roeck
  0 siblings, 1 reply; 15+ messages in thread
From: Pali Rohár @ 2021-11-22 19:10 UTC (permalink / raw)
  To: Armin Wolf; +Cc: Guenter Roeck, jdelvare, linux-hwmon, linux-kernel

On Monday 22 November 2021 19:50:14 Armin Wolf wrote:
> Am 22.11.21 um 18:55 schrieb Guenter Roeck:
> > On 11/22/21 8:01 AM, Pali Rohár wrote:
> > > On Saturday 20 November 2021 18:03:19 Armin Wolf wrote:
> > > > The only purpose of i8k_ioctl() is to call i8k_ioctl_unlocked()
> > > > with i8k_mutex held. Judging from the hwmon code, this mutex
> > > > only needs to be held when setting the fan speed/mode.
> > > 
> > > Really? I think that there is no difference between setting and getting
> > > fan speed/mode. At least I do not see why 'set' needs mutex and 'get' do
> > > not need it. Some more explanation is needed...
> > > 
> > I8K_SET_FAN sets the fan speed and returns the current status. Without
> > locking, the returned status may not match or be associated with the
> > previous
> > set operation.
> > 
> > Maybe that doesn't matter, and the synchronization is not needed. If so,
> > you can probably remove the locking entirely.
> > 
> > Guenter
> 
> That is the reason i kept the locking code. Since i do not want to break
> the ioctl interfacein any way, removing the locking code seems too risky
> to me.

I see. That is a good point.

But there is same race condition also when at the same time going to
change speed via ioctl and also via hwmon sysfs.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/2] hwmon: (dell-smm) Unify i8k_ioctl() and i8k_ioctl_unlocked()
  2021-11-22 19:10         ` Pali Rohár
@ 2021-11-22 19:28           ` Guenter Roeck
  2021-11-22 19:43             ` Pali Rohár
  0 siblings, 1 reply; 15+ messages in thread
From: Guenter Roeck @ 2021-11-22 19:28 UTC (permalink / raw)
  To: Pali Rohár, Armin Wolf; +Cc: jdelvare, linux-hwmon, linux-kernel

On 11/22/21 11:10 AM, Pali Rohár wrote:
> On Monday 22 November 2021 19:50:14 Armin Wolf wrote:
>> Am 22.11.21 um 18:55 schrieb Guenter Roeck:
>>> On 11/22/21 8:01 AM, Pali Rohár wrote:
>>>> On Saturday 20 November 2021 18:03:19 Armin Wolf wrote:
>>>>> The only purpose of i8k_ioctl() is to call i8k_ioctl_unlocked()
>>>>> with i8k_mutex held. Judging from the hwmon code, this mutex
>>>>> only needs to be held when setting the fan speed/mode.
>>>>
>>>> Really? I think that there is no difference between setting and getting
>>>> fan speed/mode. At least I do not see why 'set' needs mutex and 'get' do
>>>> not need it. Some more explanation is needed...
>>>>
>>> I8K_SET_FAN sets the fan speed and returns the current status. Without
>>> locking, the returned status may not match or be associated with the
>>> previous
>>> set operation.
>>>
>>> Maybe that doesn't matter, and the synchronization is not needed. If so,
>>> you can probably remove the locking entirely.
>>>
>>> Guenter
>>
>> That is the reason i kept the locking code. Since i do not want to break
>> the ioctl interfacein any way, removing the locking code seems too risky
>> to me.
> 
> I see. That is a good point.
> 
> But there is same race condition also when at the same time going to
> change speed via ioctl and also via hwmon sysfs.
> 

I thought the sysfs code does not change the fan speed and report the
fan status in the same request. Did I miss something ?

Guenter

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/2] hwmon: (dell-smm) Unify i8k_ioctl() and i8k_ioctl_unlocked()
  2021-11-22 19:28           ` Guenter Roeck
@ 2021-11-22 19:43             ` Pali Rohár
  2021-11-23 16:00               ` Guenter Roeck
  0 siblings, 1 reply; 15+ messages in thread
From: Pali Rohár @ 2021-11-22 19:43 UTC (permalink / raw)
  To: Guenter Roeck; +Cc: Armin Wolf, jdelvare, linux-hwmon, linux-kernel

On Monday 22 November 2021 11:28:30 Guenter Roeck wrote:
> On 11/22/21 11:10 AM, Pali Rohár wrote:
> > On Monday 22 November 2021 19:50:14 Armin Wolf wrote:
> > > Am 22.11.21 um 18:55 schrieb Guenter Roeck:
> > > > On 11/22/21 8:01 AM, Pali Rohár wrote:
> > > > > On Saturday 20 November 2021 18:03:19 Armin Wolf wrote:
> > > > > > The only purpose of i8k_ioctl() is to call i8k_ioctl_unlocked()
> > > > > > with i8k_mutex held. Judging from the hwmon code, this mutex
> > > > > > only needs to be held when setting the fan speed/mode.
> > > > > 
> > > > > Really? I think that there is no difference between setting and getting
> > > > > fan speed/mode. At least I do not see why 'set' needs mutex and 'get' do
> > > > > not need it. Some more explanation is needed...
> > > > > 
> > > > I8K_SET_FAN sets the fan speed and returns the current status. Without
> > > > locking, the returned status may not match or be associated with the
> > > > previous
> > > > set operation.
> > > > 
> > > > Maybe that doesn't matter, and the synchronization is not needed. If so,
> > > > you can probably remove the locking entirely.
> > > > 
> > > > Guenter
> > > 
> > > That is the reason i kept the locking code. Since i do not want to break
> > > the ioctl interfacein any way, removing the locking code seems too risky
> > > to me.
> > 
> > I see. That is a good point.
> > 
> > But there is same race condition also when at the same time going to
> > change speed via ioctl and also via hwmon sysfs.
> > 
> 
> I thought the sysfs code does not change the fan speed and report the
> fan status in the same request. Did I miss something ?

No. I mean something different. Let me to write trace call:

CPU 0:                          CPU 1:
                                1. dell_smm_write()
1. ioctl(I8K_SET_FAN)
2. i8k_set_fan()
                                2. i8k_set_fan()
3. i8k_get_fan_status()

So to ensure that i8k_get_fan_status() on CPU 0 returns value which
belongs to i8k_set_fan() from CPU 0 it is needed to still use mutex.

Armin is right here and I think that patch is correct.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/2] hwmon: (dell-smm) Unify i8k_ioctl() and i8k_ioctl_unlocked()
  2021-11-22 19:43             ` Pali Rohár
@ 2021-11-23 16:00               ` Guenter Roeck
  2021-11-23 16:02                 ` Pali Rohár
  0 siblings, 1 reply; 15+ messages in thread
From: Guenter Roeck @ 2021-11-23 16:00 UTC (permalink / raw)
  To: Pali Rohár; +Cc: Armin Wolf, jdelvare, linux-hwmon, linux-kernel

On Mon, Nov 22, 2021 at 08:43:10PM +0100, Pali Rohár wrote:
> On Monday 22 November 2021 11:28:30 Guenter Roeck wrote:
> > On 11/22/21 11:10 AM, Pali Rohár wrote:
> > > On Monday 22 November 2021 19:50:14 Armin Wolf wrote:
> > > > Am 22.11.21 um 18:55 schrieb Guenter Roeck:
> > > > > On 11/22/21 8:01 AM, Pali Rohár wrote:
> > > > > > On Saturday 20 November 2021 18:03:19 Armin Wolf wrote:
> > > > > > > The only purpose of i8k_ioctl() is to call i8k_ioctl_unlocked()
> > > > > > > with i8k_mutex held. Judging from the hwmon code, this mutex
> > > > > > > only needs to be held when setting the fan speed/mode.
> > > > > > 
> > > > > > Really? I think that there is no difference between setting and getting
> > > > > > fan speed/mode. At least I do not see why 'set' needs mutex and 'get' do
> > > > > > not need it. Some more explanation is needed...
> > > > > > 
> > > > > I8K_SET_FAN sets the fan speed and returns the current status. Without
> > > > > locking, the returned status may not match or be associated with the
> > > > > previous
> > > > > set operation.
> > > > > 
> > > > > Maybe that doesn't matter, and the synchronization is not needed. If so,
> > > > > you can probably remove the locking entirely.
> > > > > 
> > > > > Guenter
> > > > 
> > > > That is the reason i kept the locking code. Since i do not want to break
> > > > the ioctl interfacein any way, removing the locking code seems too risky
> > > > to me.
> > > 
> > > I see. That is a good point.
> > > 
> > > But there is same race condition also when at the same time going to
> > > change speed via ioctl and also via hwmon sysfs.
> > > 
> > 
> > I thought the sysfs code does not change the fan speed and report the
> > fan status in the same request. Did I miss something ?
> 
> No. I mean something different. Let me to write trace call:
> 
> CPU 0:                          CPU 1:
>                                 1. dell_smm_write()
> 1. ioctl(I8K_SET_FAN)
> 2. i8k_set_fan()
>                                 2. i8k_set_fan()
> 3. i8k_get_fan_status()
> 
> So to ensure that i8k_get_fan_status() on CPU 0 returns value which
> belongs to i8k_set_fan() from CPU 0 it is needed to still use mutex.
> 
> Armin is right here and I think that patch is correct.

Quoting your earlier reply:

> Really? I think that there is no difference between setting and getting
> fan speed/mode. At least I do not see why 'set' needs mutex and 'get' do
> not need it. Some more explanation is needed...

This was the reason for my comment. Your latest reply is leaving me a bit
puzzled. If you are ok with the patch as-is, please provide a Reviewed-by:
or Acked-by: tag.

Thanks,
Guenter


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/2] hwmon: (dell-smm) Unify i8k_ioctl() and i8k_ioctl_unlocked()
  2021-11-23 16:00               ` Guenter Roeck
@ 2021-11-23 16:02                 ` Pali Rohár
  0 siblings, 0 replies; 15+ messages in thread
From: Pali Rohár @ 2021-11-23 16:02 UTC (permalink / raw)
  To: Guenter Roeck; +Cc: Armin Wolf, jdelvare, linux-hwmon, linux-kernel

On Tuesday 23 November 2021 08:00:51 Guenter Roeck wrote:
> On Mon, Nov 22, 2021 at 08:43:10PM +0100, Pali Rohár wrote:
> > On Monday 22 November 2021 11:28:30 Guenter Roeck wrote:
> > > On 11/22/21 11:10 AM, Pali Rohár wrote:
> > > > On Monday 22 November 2021 19:50:14 Armin Wolf wrote:
> > > > > Am 22.11.21 um 18:55 schrieb Guenter Roeck:
> > > > > > On 11/22/21 8:01 AM, Pali Rohár wrote:
> > > > > > > On Saturday 20 November 2021 18:03:19 Armin Wolf wrote:
> > > > > > > > The only purpose of i8k_ioctl() is to call i8k_ioctl_unlocked()
> > > > > > > > with i8k_mutex held. Judging from the hwmon code, this mutex
> > > > > > > > only needs to be held when setting the fan speed/mode.
> > > > > > > 
> > > > > > > Really? I think that there is no difference between setting and getting
> > > > > > > fan speed/mode. At least I do not see why 'set' needs mutex and 'get' do
> > > > > > > not need it. Some more explanation is needed...
> > > > > > > 
> > > > > > I8K_SET_FAN sets the fan speed and returns the current status. Without
> > > > > > locking, the returned status may not match or be associated with the
> > > > > > previous
> > > > > > set operation.
> > > > > > 
> > > > > > Maybe that doesn't matter, and the synchronization is not needed. If so,
> > > > > > you can probably remove the locking entirely.
> > > > > > 
> > > > > > Guenter
> > > > > 
> > > > > That is the reason i kept the locking code. Since i do not want to break
> > > > > the ioctl interfacein any way, removing the locking code seems too risky
> > > > > to me.
> > > > 
> > > > I see. That is a good point.
> > > > 
> > > > But there is same race condition also when at the same time going to
> > > > change speed via ioctl and also via hwmon sysfs.
> > > > 
> > > 
> > > I thought the sysfs code does not change the fan speed and report the
> > > fan status in the same request. Did I miss something ?
> > 
> > No. I mean something different. Let me to write trace call:
> > 
> > CPU 0:                          CPU 1:
> >                                 1. dell_smm_write()
> > 1. ioctl(I8K_SET_FAN)
> > 2. i8k_set_fan()
> >                                 2. i8k_set_fan()
> > 3. i8k_get_fan_status()
> > 
> > So to ensure that i8k_get_fan_status() on CPU 0 returns value which
> > belongs to i8k_set_fan() from CPU 0 it is needed to still use mutex.
> > 
> > Armin is right here and I think that patch is correct.
> 
> Quoting your earlier reply:
> 
> > Really? I think that there is no difference between setting and getting
> > fan speed/mode. At least I do not see why 'set' needs mutex and 'get' do
> > not need it. Some more explanation is needed...
> 
> This was the reason for my comment. Your latest reply is leaving me a bit
> puzzled. If you are ok with the patch as-is, please provide a Reviewed-by:
> or Acked-by: tag.

Sorry for that. I should have explicitly wrote that I realized how it
works after Armin explained it.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/2] hwmon: (dell-smm) Unify i8k_ioctl() and i8k_ioctl_unlocked()
  2021-11-20 17:03 ` [PATCH 2/2] hwmon: (dell-smm) Unify i8k_ioctl() and i8k_ioctl_unlocked() Armin Wolf
  2021-11-22 16:01   ` Pali Rohár
@ 2021-11-23 16:04   ` Pali Rohár
  1 sibling, 0 replies; 15+ messages in thread
From: Pali Rohár @ 2021-11-23 16:04 UTC (permalink / raw)
  To: Armin Wolf; +Cc: jdelvare, linux, linux-hwmon, linux-kernel

On Saturday 20 November 2021 18:03:19 Armin Wolf wrote:
> The only purpose of i8k_ioctl() is to call i8k_ioctl_unlocked()
> with i8k_mutex held. Judging from the hwmon code, this mutex
> only needs to be held when setting the fan speed/mode.
> Unify both functions and reduce the locking of i8k_mutex
> to I8K_SET_FAN.
> 
> Tested on a Dell Inspiron 3505.
> 
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>

Maybe some more information in commit message could be useful, but now
it is in this email thread.

Anyway, change is correct, so:

Reviewed-by: Pali Rohár <pali@kernel.org>

> ---
>  drivers/hwmon/dell-smm-hwmon.c | 27 +++++++++------------------
>  1 file changed, 9 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
> index b5d1703faa62..e2d388f7360e 100644
> --- a/drivers/hwmon/dell-smm-hwmon.c
> +++ b/drivers/hwmon/dell-smm-hwmon.c
> @@ -449,12 +449,12 @@ static int i8k_get_power_status(void)
>   * Procfs interface
>   */
> 
> -static int
> -i8k_ioctl_unlocked(struct file *fp, struct dell_smm_data *data, unsigned int cmd, unsigned long arg)
> +static long i8k_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
>  {
> -	int val = 0;
> -	int speed, err;
> +	struct dell_smm_data *data = PDE_DATA(file_inode(fp));
>  	int __user *argp = (int __user *)arg;
> +	int speed, err;
> +	int val = 0;
> 
>  	if (!argp)
>  		return -EINVAL;
> @@ -516,11 +516,14 @@ i8k_ioctl_unlocked(struct file *fp, struct dell_smm_data *data, unsigned int cmd
>  		if (copy_from_user(&speed, argp + 1, sizeof(int)))
>  			return -EFAULT;
> 
> +		mutex_lock(&data->i8k_mutex);
>  		err = i8k_set_fan(data, val, speed);
> -		if (err < 0)
> +		if (err < 0) {
> +			mutex_unlock(&data->i8k_mutex);
>  			return err;
> -
> +		}
>  		val = i8k_get_fan_status(data, val);
> +		mutex_unlock(&data->i8k_mutex);
>  		break;
> 
>  	default:
> @@ -536,18 +539,6 @@ i8k_ioctl_unlocked(struct file *fp, struct dell_smm_data *data, unsigned int cmd
>  	return 0;
>  }
> 
> -static long i8k_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
> -{
> -	struct dell_smm_data *data = PDE_DATA(file_inode(fp));
> -	long ret;
> -
> -	mutex_lock(&data->i8k_mutex);
> -	ret = i8k_ioctl_unlocked(fp, data, cmd, arg);
> -	mutex_unlock(&data->i8k_mutex);
> -
> -	return ret;
> -}
> -
>  /*
>   * Print the information for /proc/i8k.
>   */
> --
> 2.30.2
> 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/2] hwmon: (dell-smm) Simplify ioctl handler
  2021-11-20 17:03 ` [PATCH 1/2] hwmon: (dell-smm) Simplify " Armin Wolf
@ 2021-11-23 16:13   ` Pali Rohár
  2021-11-29 20:57     ` Armin Wolf
  0 siblings, 1 reply; 15+ messages in thread
From: Pali Rohár @ 2021-11-23 16:13 UTC (permalink / raw)
  To: Armin Wolf; +Cc: jdelvare, linux, linux-hwmon, linux-kernel

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
> 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/2] hwmon: (dell-smm) Simplify ioctl handler
  2021-11-23 16:13   ` Pali Rohár
@ 2021-11-29 20:57     ` Armin Wolf
  2021-12-09 16:52       ` Pali Rohár
  0 siblings, 1 reply; 15+ messages in thread
From: Armin Wolf @ 2021-11-29 20:57 UTC (permalink / raw)
  To: Pali Rohár; +Cc: jdelvare, linux, linux-hwmon, linux-kernel

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/2] hwmon: (dell-smm) Simplify ioctl handler
  2021-11-29 20:57     ` Armin Wolf
@ 2021-12-09 16:52       ` Pali Rohár
  0 siblings, 0 replies; 15+ messages in thread
From: Pali Rohár @ 2021-12-09 16:52 UTC (permalink / raw)
  To: Armin Wolf; +Cc: jdelvare, linux, linux-hwmon, linux-kernel

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2021-12-09 16:52 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-20 17:03 [PATCH 0/2] hwmon: (dell-smm) Improve ioctl handler Armin Wolf
2021-11-20 17:03 ` [PATCH 1/2] hwmon: (dell-smm) Simplify " Armin Wolf
2021-11-23 16:13   ` Pali Rohár
2021-11-29 20:57     ` Armin Wolf
2021-12-09 16:52       ` Pali Rohár
2021-11-20 17:03 ` [PATCH 2/2] hwmon: (dell-smm) Unify i8k_ioctl() and i8k_ioctl_unlocked() Armin Wolf
2021-11-22 16:01   ` Pali Rohár
2021-11-22 17:55     ` Guenter Roeck
2021-11-22 18:50       ` Armin Wolf
2021-11-22 19:10         ` Pali Rohár
2021-11-22 19:28           ` Guenter Roeck
2021-11-22 19:43             ` Pali Rohár
2021-11-23 16:00               ` Guenter Roeck
2021-11-23 16:02                 ` Pali Rohár
2021-11-23 16:04   ` Pali Rohár

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).