Re: [PATCH] hwmon: pmbus: protect read-modify-write with lock

Re: [PATCH] hwmon: pmbus: protect read-modify-write with lock

[Guenter Roeck]

Hi,

On Thu, May 30, 2019 at 06:45:48AM +0000, Adamski, Krzysztof (Nokia - PL/Wroclaw) wrote:
> The operation done in the pmbus_update_fan() function is a
> read-modify-write operation but it lacks any kind of lock protection
> which may cause problems if run more than once simultaneously. This
> patch uses an existing update_lock mutex to fix this problem.
> 
> Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com>
> ---
> 
> I'm resending this patch to proper recipients this time. Sorry if the
> previous submission confused anybody.
> 
>  drivers/hwmon/pmbus/pmbus_core.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index ef7ee90ee785..94adbede7912 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -268,6 +268,7 @@ int pmbus_update_fan(struct i2c_client *client, int page, int id,
>  	int rv;
>  	u8 to;
>  
> +	mutex_lock(&data->update_lock);
>  	from = pmbus_read_byte_data(client, page,
>  				    pmbus_fan_config_registers[id]);
>  	if (from < 0)
> @@ -278,11 +279,15 @@ int pmbus_update_fan(struct i2c_client *client, int page, int id,
>  		rv = pmbus_write_byte_data(client, page,
>  					   pmbus_fan_config_registers[id], to);
>  		if (rv < 0)
> -			return rv;
> +			goto out;
>  	}
>  
> -	return _pmbus_write_word_data(client, page,
> -				      pmbus_fan_command_registers[id], command);
> +	rv = _pmbus_write_word_data(client, page,
> +				    pmbus_fan_command_registers[id], command);
> +
> +out:
> +	mutex_lock(&data->update_lock);

Should be mutex_unlock(), meaning you have not tested this ;-).

Either case, I think this is unnecessary. The function is (or should be)
always called with the lock already taken (ie with pmbus_set_sensor()
in the call path). If not, we would need a locked and an unlocked version
of this function to avoid lock recursion.

Thanks,
Guenter

> +	return rv;
>  }
>  EXPORT_SYMBOL_GPL(pmbus_update_fan);
>  
> -- 
> 2.20.1
> 

Re: [PATCH] hwmon: pmbus: protect read-modify-write with lock

[Adamski, Krzysztof (Nokia - PL/Wroclaw)]

On Thu, May 30, 2019 at 10:21:20AM -0700, Guenter Roeck wrote:
>Hi,
>
>On Thu, May 30, 2019 at 06:45:48AM +0000, Adamski, Krzysztof (Nokia - PL/Wroclaw) wrote:
>> The operation done in the pmbus_update_fan() function is a
>> read-modify-write operation but it lacks any kind of lock protection
>> which may cause problems if run more than once simultaneously. This
>> patch uses an existing update_lock mutex to fix this problem.
>>
>> Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com>
>> ---
>>
>> I'm resending this patch to proper recipients this time. Sorry if the
>> previous submission confused anybody.
>>
>>  drivers/hwmon/pmbus/pmbus_core.c | 11 ++++++++---
>>  1 file changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
>> index ef7ee90ee785..94adbede7912 100644
>> --- a/drivers/hwmon/pmbus/pmbus_core.c
>> +++ b/drivers/hwmon/pmbus/pmbus_core.c
>> @@ -268,6 +268,7 @@ int pmbus_update_fan(struct i2c_client *client, int page, int id,
>>  	int rv;
>>  	u8 to;
>>
>> +	mutex_lock(&data->update_lock);
>>  	from = pmbus_read_byte_data(client, page,
>>  				    pmbus_fan_config_registers[id]);
>>  	if (from < 0)
>> @@ -278,11 +279,15 @@ int pmbus_update_fan(struct i2c_client *client, int page, int id,
>>  		rv = pmbus_write_byte_data(client, page,
>>  					   pmbus_fan_config_registers[id], to);
>>  		if (rv < 0)
>> -			return rv;
>> +			goto out;
>>  	}
>>
>> -	return _pmbus_write_word_data(client, page,
>> -				      pmbus_fan_command_registers[id], command);
>> +	rv = _pmbus_write_word_data(client, page,
>> +				    pmbus_fan_command_registers[id], command);
>> +
>> +out:
>> +	mutex_lock(&data->update_lock);
>
>Should be mutex_unlock(), meaning you have not tested this ;-).
>
>Either case, I think this is unnecessary. The function is (or should be)
>always called with the lock already taken (ie with pmbus_set_sensor()
>in the call path). If not, we would need a locked and an unlocked version
>of this function to avoid lock recursion.

You've got me :) I did not test that as I do not have a workflow using
this. I just stumbled opon this when looking at the code related to my
other patches. So it was more like a - "hey, shouldn't there be a lock
here?". But I was wrong, thanks.

Krzysztof

[PATCH] hwmon: pmbus: protect read-modify-write with lock

[Adamski, Krzysztof (Nokia - PL/Wroclaw)]

The operation done in the pmbus_update_fan() function is a
read-modify-write operation but it lacks any kind of lock protection
which may cause problems if run more than once simultaneously. This
patch uses an existing update_lock mutex to fix this problem.

Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com>
---

I'm resending this patch to proper recipients this time. Sorry if the
previous submission confused anybody.

 drivers/hwmon/pmbus/pmbus_core.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index ef7ee90ee785..94adbede7912 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -268,6 +268,7 @@ int pmbus_update_fan(struct i2c_client *client, int page, int id,
 	int rv;
 	u8 to;
 
+	mutex_lock(&data->update_lock);
 	from = pmbus_read_byte_data(client, page,
 				    pmbus_fan_config_registers[id]);
 	if (from < 0)
@@ -278,11 +279,15 @@ int pmbus_update_fan(struct i2c_client *client, int page, int id,
 		rv = pmbus_write_byte_data(client, page,
 					   pmbus_fan_config_registers[id], to);
 		if (rv < 0)
-			return rv;
+			goto out;
 	}
 
-	return _pmbus_write_word_data(client, page,
-				      pmbus_fan_command_registers[id], command);
+	rv = _pmbus_write_word_data(client, page,
+				    pmbus_fan_command_registers[id], command);
+
+out:
+	mutex_lock(&data->update_lock);
+	return rv;
 }
 EXPORT_SYMBOL_GPL(pmbus_update_fan);
 
-- 
2.20.1

Re: [PATCH] hwmon: pmbus: protect read-modify-write with lock

[Sverdlin, Alexander (Nokia - DE/Ulm)]

Hi!

On 30/05/2019 08:45, Adamski, Krzysztof (Nokia - PL/Wroclaw) wrote:
> The operation done in the pmbus_update_fan() function is a
> read-modify-write operation but it lacks any kind of lock protection
> which may cause problems if run more than once simultaneously. This
> patch uses an existing update_lock mutex to fix this problem.
> 
> Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com>
> ---
> 
> I'm resending this patch to proper recipients this time. Sorry if the
> previous submission confused anybody.
> 
>  drivers/hwmon/pmbus/pmbus_core.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index ef7ee90ee785..94adbede7912 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -268,6 +268,7 @@ int pmbus_update_fan(struct i2c_client *client, int page, int id,
>  	int rv;
>  	u8 to;
>  
> +	mutex_lock(&data->update_lock);
>  	from = pmbus_read_byte_data(client, page,
>  				    pmbus_fan_config_registers[id]);
>  	if (from < 0)
> @@ -278,11 +279,15 @@ int pmbus_update_fan(struct i2c_client *client, int page, int id,
>  		rv = pmbus_write_byte_data(client, page,
>  					   pmbus_fan_config_registers[id], to);
>  		if (rv < 0)
> -			return rv;
> +			goto out;
>  	}
>  
> -	return _pmbus_write_word_data(client, page,
> -				      pmbus_fan_command_registers[id], command);
> +	rv = _pmbus_write_word_data(client, page,
> +				    pmbus_fan_command_registers[id], command);
> +
> +out:
> +	mutex_lock(&data->update_lock);

This has to be mutex_unlock()...

> +	return rv;
>  }
>  EXPORT_SYMBOL_GPL(pmbus_update_fan);

-- 
Best regards,
Alexander Sverdlin.

Re: [PATCH] hwmon: pmbus: protect read-modify-write with lock

[Guenter Roeck]

On Mon, Jun 03, 2019 at 01:11:45PM +0000, Sverdlin, Alexander (Nokia - DE/Ulm) wrote:
> Hi!
> 
> On 30/05/2019 08:45, Adamski, Krzysztof (Nokia - PL/Wroclaw) wrote:
> > The operation done in the pmbus_update_fan() function is a
> > read-modify-write operation but it lacks any kind of lock protection
> > which may cause problems if run more than once simultaneously. This
> > patch uses an existing update_lock mutex to fix this problem.
> > 
> > Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com>
> > ---
> > 
> > I'm resending this patch to proper recipients this time. Sorry if the
> > previous submission confused anybody.
> > 
> >  drivers/hwmon/pmbus/pmbus_core.c | 11 ++++++++---
> >  1 file changed, 8 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> > index ef7ee90ee785..94adbede7912 100644
> > --- a/drivers/hwmon/pmbus/pmbus_core.c
> > +++ b/drivers/hwmon/pmbus/pmbus_core.c
> > @@ -268,6 +268,7 @@ int pmbus_update_fan(struct i2c_client *client, int page, int id,
> >  	int rv;
> >  	u8 to;
> >  
> > +	mutex_lock(&data->update_lock);
> >  	from = pmbus_read_byte_data(client, page,
> >  				    pmbus_fan_config_registers[id]);
> >  	if (from < 0)
> > @@ -278,11 +279,15 @@ int pmbus_update_fan(struct i2c_client *client, int page, int id,
> >  		rv = pmbus_write_byte_data(client, page,
> >  					   pmbus_fan_config_registers[id], to);
> >  		if (rv < 0)
> > -			return rv;
> > +			goto out;
> >  	}
> >  
> > -	return _pmbus_write_word_data(client, page,
> > -				      pmbus_fan_command_registers[id], command);
> > +	rv = _pmbus_write_word_data(client, page,
> > +				    pmbus_fan_command_registers[id], command);
> > +
> > +out:
> > +	mutex_lock(&data->update_lock);
> 
> This has to be mutex_unlock()...
> 

Not only that - it is also recursive.

Guenter

[PATCH] hwmon: pmbus: protect read-modify-write with lock

[Adamski, Krzysztof (Nokia - PL/Wroclaw)]

The operation done in the pmbus_update_fan() function is a
read-modify-write operation but it lacks any kind of lock protection
which may cause problems if run more than once simultaneously. This
patch uses an existing update_lock mutex to fix this problem.

Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com>
---
 drivers/hwmon/pmbus/pmbus_core.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index ef7ee90ee785..94adbede7912 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -268,6 +268,7 @@ int pmbus_update_fan(struct i2c_client *client, int page, int id,
 	int rv;
 	u8 to;
 
+	mutex_lock(&data->update_lock);
 	from = pmbus_read_byte_data(client, page,
 				    pmbus_fan_config_registers[id]);
 	if (from < 0)
@@ -278,11 +279,15 @@ int pmbus_update_fan(struct i2c_client *client, int page, int id,
 		rv = pmbus_write_byte_data(client, page,
 					   pmbus_fan_config_registers[id], to);
 		if (rv < 0)
-			return rv;
+			goto out;
 	}
 
-	return _pmbus_write_word_data(client, page,
-				      pmbus_fan_command_registers[id], command);
+	rv = _pmbus_write_word_data(client, page,
+				    pmbus_fan_command_registers[id], command);
+
+out:
+	mutex_lock(&data->update_lock);
+	return rv;
 }
 EXPORT_SYMBOL_GPL(pmbus_update_fan);
 
-- 
2.20.1

[PATCH] hwmon: pmbus: protect read-modify-write with lock

[Adamski, Krzysztof (Nokia - PL/Wroclaw)]

The operation done in the pmbus_update_fan() function is a
read-modify-write operation but it lacks any kind of lock protection
which may cause problems if run more than once simultaneously. This
patch uses an existing update_lock mutex to fix this problem.

Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com>
---
 drivers/hwmon/pmbus/pmbus_core.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index ef7ee90ee785..94adbede7912 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -268,6 +268,7 @@ int pmbus_update_fan(struct i2c_client *client, int page, int id,
 	int rv;
 	u8 to;
 
+	mutex_lock(&data->update_lock);
 	from = pmbus_read_byte_data(client, page,
 				    pmbus_fan_config_registers[id]);
 	if (from < 0)
@@ -278,11 +279,15 @@ int pmbus_update_fan(struct i2c_client *client, int page, int id,
 		rv = pmbus_write_byte_data(client, page,
 					   pmbus_fan_config_registers[id], to);
 		if (rv < 0)
-			return rv;
+			goto out;
 	}
 
-	return _pmbus_write_word_data(client, page,
-				      pmbus_fan_command_registers[id], command);
+	rv = _pmbus_write_word_data(client, page,
+				    pmbus_fan_command_registers[id], command);
+
+out:
+	mutex_lock(&data->update_lock);
+	return rv;
 }
 EXPORT_SYMBOL_GPL(pmbus_update_fan);
 
-- 
2.20.1

Re: [PATCH] hwmon: pmbus: protect read-modify-write with lock

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 661 bytes --]

On Tue, May 28, 2019 at 09:08:21AM +0000, Adamski, Krzysztof (Nokia - PL/Wroclaw) wrote:
> The operation done in the pmbus_update_fan() function is a
> read-modify-write operation but it lacks any kind of lock protection
> which may cause problems if run more than once simultaneously. This
> patch uses an existing update_lock mutex to fix this problem.
> 
> Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com>

Please use get_maintainer to find the people responsible for this file.
It is not my realm.

> +out:
> +	mutex_lock(&data->update_lock);

Despite the above, have you tested the code? This likely should be
mutex_unlock?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] hwmon: pmbus: protect read-modify-write with lock

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 661 bytes --]

On Tue, May 28, 2019 at 09:08:21AM +0000, Adamski, Krzysztof (Nokia - PL/Wroclaw) wrote:
> The operation done in the pmbus_update_fan() function is a
> read-modify-write operation but it lacks any kind of lock protection
> which may cause problems if run more than once simultaneously. This
> patch uses an existing update_lock mutex to fix this problem.
> 
> Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com>

Please use get_maintainer to find the people responsible for this file.
It is not my realm.

> +out:
> +	mutex_lock(&data->update_lock);

Despite the above, have you tested the code? This likely should be
mutex_unlock?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] hwmon: pmbus: protect read-modify-write with lock

[Adamski, Krzysztof (Nokia - PL/Wroclaw)]

On Sat, Jun 08, 2019 at 12:32:18AM +0200, Wolfram Sang wrote:
>On Tue, May 28, 2019 at 09:08:21AM +0000, Adamski, Krzysztof (Nokia - PL/Wroclaw) wrote:
>> The operation done in the pmbus_update_fan() function is a
>> read-modify-write operation but it lacks any kind of lock protection
>> which may cause problems if run more than once simultaneously. This
>> patch uses an existing update_lock mutex to fix this problem.
>>
>> Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com>
>
>Please use get_maintainer to find the people responsible for this file.
>It is not my realm.
>
>> +out:
>> +	mutex_lock(&data->update_lock);
>
>Despite the above, have you tested the code? This likely should be
>mutex_unlock?
>

Sorry for that, I made a mistake and used wrong command to generate the
patch and send it to the wrong list of people. You can ignore this
patchset, it was resent to the proper mailing list instead.

Krzysztof

Re: [PATCH] hwmon: pmbus: protect read-modify-write with lock

[Adamski, Krzysztof (Nokia - PL/Wroclaw)]

On Sat, Jun 08, 2019 at 12:32:18AM +0200, Wolfram Sang wrote:
>On Tue, May 28, 2019 at 09:08:21AM +0000, Adamski, Krzysztof (Nokia - PL/Wroclaw) wrote:
>> The operation done in the pmbus_update_fan() function is a
>> read-modify-write operation but it lacks any kind of lock protection
>> which may cause problems if run more than once simultaneously. This
>> patch uses an existing update_lock mutex to fix this problem.
>>
>> Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com>
>
>Please use get_maintainer to find the people responsible for this file.
>It is not my realm.
>
>> +out:
>> +	mutex_lock(&data->update_lock);
>
>Despite the above, have you tested the code? This likely should be
>mutex_unlock?
>

Sorry for that, I made a mistake and used wrong command to generate the
patch and send it to the wrong list of people. You can ignore this
patchset, it was resent to the proper mailing list instead.

Krzysztof

Re: [PATCH] hwmon: pmbus: protect read-modify-write with lock

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 195 bytes --]

Hi Krzysztof,

> patch and send it to the wrong list of people. You can ignore this
> patchset, it was resent to the proper mailing list instead.

Thanks for the heads up.

Regards,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] hwmon: pmbus: protect read-modify-write with lock

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 195 bytes --]

Hi Krzysztof,

> patch and send it to the wrong list of people. You can ignore this
> patchset, it was resent to the proper mailing list instead.

Thanks for the heads up.

Regards,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]