[PATCH] iio: temperature: mlx90614: use guard(mutex) for EEPROM access locking

[PATCH] iio: temperature: mlx90614: use guard(mutex) for EEPROM access locking

[Luiz Mugnaini]

From: Luiz Mugnaini <luizmugnaini@usp.br>

Replace mutex_lock()/mutex_unlock() pairs with guard(mutex) from
cleanup.h for cleaner and safer mutex handling. The lock protects
EEPROM access across five call sites in mlx90614_read_raw(),
mlx90614_write_raw(), and mlx90614_sleep().

In all cases, the code between mutex_unlock() and the end of scope
is either trivial computation, a return statement, or a call to
mlx90614_power_put() (which only schedules a deferred autosuspend
via pm_runtime_put_autosuspend()). The slightly extended lock scope
is negligible compared to the existing hold times, which include
EEPROM write cycles with 40ms sleeps.

Signed-off-by: Luiz Mugnaini <luizmugnaini@usp.br>
---
 drivers/iio/temperature/mlx90614.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/drivers/iio/temperature/mlx90614.c b/drivers/iio/temperature/mlx90614.c
index 1ad21b73e..adf100756 100644
--- a/drivers/iio/temperature/mlx90614.c
+++ b/drivers/iio/temperature/mlx90614.c
@@ -23,6 +23,7 @@
  */
 
 #include <linux/bitfield.h>
+#include <linux/cleanup.h>
 #include <linux/delay.h>
 #include <linux/err.h>
 #include <linux/gpio/consumer.h>
@@ -296,10 +297,9 @@ static int mlx90614_read_raw(struct iio_dev *indio_dev,
 		if (ret < 0)
 			return ret;
 
-		mutex_lock(&data->lock);
+		guard(mutex)(&data->lock);
 		ret = i2c_smbus_read_word_data(data->client,
 					       chip_info->op_eeprom_emissivity);
-		mutex_unlock(&data->lock);
 		mlx90614_power_put(data);
 
 		if (ret < 0)
@@ -319,10 +319,9 @@ static int mlx90614_read_raw(struct iio_dev *indio_dev,
 		if (ret < 0)
 			return ret;
 
-		mutex_lock(&data->lock);
+		guard(mutex)(&data->lock);
 		ret = i2c_smbus_read_word_data(data->client,
 					       chip_info->op_eeprom_config1);
-		mutex_unlock(&data->lock);
 		mlx90614_power_put(data);
 
 		if (ret < 0)
@@ -358,10 +357,9 @@ static int mlx90614_write_raw(struct iio_dev *indio_dev,
 		if (ret < 0)
 			return ret;
 
-		mutex_lock(&data->lock);
+		guard(mutex)(&data->lock);
 		ret = mlx90614_write_word(data->client,
 					  chip_info->op_eeprom_emissivity, val);
-		mutex_unlock(&data->lock);
 		mlx90614_power_put(data);
 
 		return ret;
@@ -373,10 +371,9 @@ static int mlx90614_write_raw(struct iio_dev *indio_dev,
 		if (ret < 0)
 			return ret;
 
-		mutex_lock(&data->lock);
+		guard(mutex)(&data->lock);
 		ret = mlx90614_iir_search(data->client,
 					  val * 100 + val2 / 10000);
-		mutex_unlock(&data->lock);
 		mlx90614_power_put(data);
 
 		return ret;
@@ -476,12 +473,11 @@ static int mlx90614_sleep(struct mlx90614_data *data)
 
 	dev_dbg(&data->client->dev, "Requesting sleep");
 
-	mutex_lock(&data->lock);
+	guard(mutex)(&data->lock);
 	ret = i2c_smbus_xfer(data->client->adapter, data->client->addr,
 			     data->client->flags | I2C_CLIENT_PEC,
 			     I2C_SMBUS_WRITE, chip_info->op_sleep,
 			     I2C_SMBUS_BYTE, NULL);
-	mutex_unlock(&data->lock);
 
 	return ret;
 }
-- 
2.53.0

Re: [PATCH] iio: temperature: mlx90614: use guard(mutex) for EEPROM access locking

[Jonathan Cameron]

On Sat, 18 Apr 2026 17:51:36 -0300
Luiz Mugnaini <luizmugnaini@usp.br> wrote:

> From: Luiz Mugnaini <luizmugnaini@usp.br>
> 
> Replace mutex_lock()/mutex_unlock() pairs with guard(mutex) from
> cleanup.h for cleaner and safer mutex handling. The lock protects
> EEPROM access across five call sites in mlx90614_read_raw(),
> mlx90614_write_raw(), and mlx90614_sleep().
> 
> In all cases, the code between mutex_unlock() and the end of scope
> is either trivial computation, a return statement, or a call to
> mlx90614_power_put() (which only schedules a deferred autosuspend
> via pm_runtime_put_autosuspend()). The slightly extended lock scope
> is negligible compared to the existing hold times, which include
> EEPROM write cycles with 40ms sleeps.
> 
> Signed-off-by: Luiz Mugnaini <luizmugnaini@usp.br>
Hi Luiz,

When making this sort of modernization change I'd suggest taking
a look at the feedback and final form of other similar patches.
Pretty much everyone gets the same feedback on these and you'd have
saved everyone time if you'd realized it applies here as well.

I'd also consider if having a locked reader helper might be
worth doing here as well

Something like

static xxx mlx90614_read_word_data_locked(xxxx)
{
	guard(mutex)(&data->lock);
	return i2c_smbus_read_word_data();
}
> ---
>  drivers/iio/temperature/mlx90614.c | 16 ++++++----------
>  1 file changed, 6 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/iio/temperature/mlx90614.c b/drivers/iio/temperature/mlx90614.c
> index 1ad21b73e..adf100756 100644
> --- a/drivers/iio/temperature/mlx90614.c
> +++ b/drivers/iio/temperature/mlx90614.c
> @@ -23,6 +23,7 @@
>   */
>  
>  #include <linux/bitfield.h>
> +#include <linux/cleanup.h>
>  #include <linux/delay.h>
>  #include <linux/err.h>
>  #include <linux/gpio/consumer.h>
> @@ -296,10 +297,9 @@ static int mlx90614_read_raw(struct iio_dev *indio_dev,
>  		if (ret < 0)
>  			return ret;
>  
> -		mutex_lock(&data->lock);
> +		guard(mutex)(&data->lock);

What is the scope of this guard?  Hint, that gets 'interesting' when in a switch
case statement and there is a standard solution for that...

>  		ret = i2c_smbus_read_word_data(data->client,
>  					       chip_info->op_eeprom_emissivity);
> -		mutex_unlock(&data->lock);
>  		mlx90614_power_put(data);
>  
>  		if (ret < 0)
> @@ -319,10 +319,9 @@ static int mlx90614_read_raw(struct iio_dev *indio_dev,
>  		if (ret < 0)
>  			return ret;
>  
> -		mutex_lock(&data->lock);
> +		guard(mutex)(&data->lock);
Same applies here and probably a load more cases below.

>  		ret = i2c_smbus_read_word_data(data->client,
>  					       chip_info->op_eeprom_config1);
> -		mutex_unlock(&data->lock);
>  		mlx90614_power_put(data);
It may make sense to take a closer look at whether we can use cleanup magic
to handle the power management as well though that would need more substantial
refactors so wouldn't belong in this patch.


>  
>  		if (ret < 0)
> @@ -358,10 +357,9 @@ static int mlx90614_write_raw(struct iio_dev *indio_dev,
>  		if (ret < 0)
>  			return ret;
>  
> -		mutex_lock(&data->lock);
> +		guard(mutex)(&data->lock);
>  		ret = mlx90614_write_word(data->client,
>  					  chip_info->op_eeprom_emissivity, val);
> -		mutex_unlock(&data->lock);
>  		mlx90614_power_put(data);
>  
>  		return ret;
> @@ -373,10 +371,9 @@ static int mlx90614_write_raw(struct iio_dev *indio_dev,
>  		if (ret < 0)
>  			return ret;
>  
> -		mutex_lock(&data->lock);
> +		guard(mutex)(&data->lock);
>  		ret = mlx90614_iir_search(data->client,
>  					  val * 100 + val2 / 10000);
> -		mutex_unlock(&data->lock);
>  		mlx90614_power_put(data);
>  
>  		return ret;
> @@ -476,12 +473,11 @@ static int mlx90614_sleep(struct mlx90614_data *data)
>  
>  	dev_dbg(&data->client->dev, "Requesting sleep");
>  
> -	mutex_lock(&data->lock);
> +	guard(mutex)(&data->lock);
>  	ret = i2c_smbus_xfer(data->client->adapter, data->client->addr,
>  			     data->client->flags | I2C_CLIENT_PEC,
>  			     I2C_SMBUS_WRITE, chip_info->op_sleep,
>  			     I2C_SMBUS_BYTE, NULL);
> -	mutex_unlock(&data->lock);
>  
>  	return ret;
	reutrn i2c_smbus_xfer();

>  }

Re: [PATCH] iio: temperature: mlx90614: use guard(mutex) for EEPROM access locking

[Andy Shevchenko]

On Sat, Apr 18, 2026 at 05:51:36PM -0300, Luiz Mugnaini wrote:

> Replace mutex_lock()/mutex_unlock() pairs with guard(mutex) from
> cleanup.h for cleaner and safer mutex handling. The lock protects
> EEPROM access across five call sites in mlx90614_read_raw(),
> mlx90614_write_raw(), and mlx90614_sleep().
> 
> In all cases, the code between mutex_unlock() and the end of scope
> is either trivial computation, a return statement, or a call to
> mlx90614_power_put() (which only schedules a deferred autosuspend
> via pm_runtime_put_autosuspend()). The slightly extended lock scope
> is negligible compared to the existing hold times, which include
> EEPROM write cycles with 40ms sleeps.

...

> -		mutex_lock(&data->lock);
> +		guard(mutex)(&data->lock);
>  		ret = i2c_smbus_read_word_data(data->client,
>  					       chip_info->op_eeprom_emissivity);
> -		mutex_unlock(&data->lock);
>  		mlx90614_power_put(data);

How do you sure that the code:
- is not slowed down due to unneeded things in the critical section
- doesn't introduce no deadlocks
?

How did you test this, please?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] iio: temperature: mlx90614: use guard(mutex) for EEPROM access locking

[Luiz Mugnaini]

Hi Jonathan,

Thanks for the feedback. I'll be sending a v2 shortly. About your points:

 > I'd also consider if having a locked reader helper might be
 > worth doing here as well

I would prefer just using scoped_guard() in this specific case as there are
only two affected call sites and no real code duplication. But I agree that
if this proliferates, a helper procedure might be the way to go.

 > It may make sense to take a closer look at whether we can use cleanup
 > magic to handle the power management as well though that would need more
 > substantial refactors so wouldn't belong in this patch.

Agreed. That would indeed entail a new patch.

 > > @@ -476,12 +473,11 @@ static int mlx90614_sleep(struct 
mlx90614_data *data)
 > >
 > >      dev_dbg(&data->client->dev, "Requesting sleep");
 > >
 > > -    mutex_lock(&data->lock);
 > > +    guard(mutex)(&data->lock);
 > >      ret = i2c_smbus_xfer(data->client->adapter, data->client->addr,
 > >                   data->client->flags | I2C_CLIENT_PEC,
 > >                   I2C_SMBUS_WRITE, chip_info->op_sleep,
 > >                   I2C_SMBUS_BYTE, NULL);
 > > -    mutex_unlock(&data->lock);
 > >
 > >      return ret;
 >
 > return i2c_smbus_xfer();

I'll remove that intermediate ret variable, thanks for the tip.

Best regards,
Luiz


On 19/04/2026 08:43, Jonathan Cameron wrote:
> On Sat, 18 Apr 2026 17:51:36 -0300
> Luiz Mugnaini <luizmugnaini@usp.br> wrote:
>
>> From: Luiz Mugnaini <luizmugnaini@usp.br>
>>
>> Replace mutex_lock()/mutex_unlock() pairs with guard(mutex) from
>> cleanup.h for cleaner and safer mutex handling. The lock protects
>> EEPROM access across five call sites in mlx90614_read_raw(),
>> mlx90614_write_raw(), and mlx90614_sleep().
>>
>> In all cases, the code between mutex_unlock() and the end of scope
>> is either trivial computation, a return statement, or a call to
>> mlx90614_power_put() (which only schedules a deferred autosuspend
>> via pm_runtime_put_autosuspend()). The slightly extended lock scope
>> is negligible compared to the existing hold times, which include
>> EEPROM write cycles with 40ms sleeps.
>>
>> Signed-off-by: Luiz Mugnaini <luizmugnaini@usp.br>
> Hi Luiz,
>
> When making this sort of modernization change I'd suggest taking
> a look at the feedback and final form of other similar patches.
> Pretty much everyone gets the same feedback on these and you'd have
> saved everyone time if you'd realized it applies here as well.
>
> I'd also consider if having a locked reader helper might be
> worth doing here as well
>
> Something like
>
> static xxx mlx90614_read_word_data_locked(xxxx)
> {
> 	guard(mutex)(&data->lock);
> 	return i2c_smbus_read_word_data();
> }
>> ---
>>   drivers/iio/temperature/mlx90614.c | 16 ++++++----------
>>   1 file changed, 6 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/iio/temperature/mlx90614.c b/drivers/iio/temperature/mlx90614.c
>> index 1ad21b73e..adf100756 100644
>> --- a/drivers/iio/temperature/mlx90614.c
>> +++ b/drivers/iio/temperature/mlx90614.c
>> @@ -23,6 +23,7 @@
>>    */
>>   
>>   #include <linux/bitfield.h>
>> +#include <linux/cleanup.h>
>>   #include <linux/delay.h>
>>   #include <linux/err.h>
>>   #include <linux/gpio/consumer.h>
>> @@ -296,10 +297,9 @@ static int mlx90614_read_raw(struct iio_dev *indio_dev,
>>   		if (ret < 0)
>>   			return ret;
>>   
>> -		mutex_lock(&data->lock);
>> +		guard(mutex)(&data->lock);
> What is the scope of this guard?  Hint, that gets 'interesting' when in a switch
> case statement and there is a standard solution for that...
>
>>   		ret = i2c_smbus_read_word_data(data->client,
>>   					       chip_info->op_eeprom_emissivity);
>> -		mutex_unlock(&data->lock);
>>   		mlx90614_power_put(data);
>>   
>>   		if (ret < 0)
>> @@ -319,10 +319,9 @@ static int mlx90614_read_raw(struct iio_dev *indio_dev,
>>   		if (ret < 0)
>>   			return ret;
>>   
>> -		mutex_lock(&data->lock);
>> +		guard(mutex)(&data->lock);
> Same applies here and probably a load more cases below.
>
>>   		ret = i2c_smbus_read_word_data(data->client,
>>   					       chip_info->op_eeprom_config1);
>> -		mutex_unlock(&data->lock);
>>   		mlx90614_power_put(data);
> It may make sense to take a closer look at whether we can use cleanup magic
> to handle the power management as well though that would need more substantial
> refactors so wouldn't belong in this patch.
>
>
>>   
>>   		if (ret < 0)
>> @@ -358,10 +357,9 @@ static int mlx90614_write_raw(struct iio_dev *indio_dev,
>>   		if (ret < 0)
>>   			return ret;
>>   
>> -		mutex_lock(&data->lock);
>> +		guard(mutex)(&data->lock);
>>   		ret = mlx90614_write_word(data->client,
>>   					  chip_info->op_eeprom_emissivity, val);
>> -		mutex_unlock(&data->lock);
>>   		mlx90614_power_put(data);
>>   
>>   		return ret;
>> @@ -373,10 +371,9 @@ static int mlx90614_write_raw(struct iio_dev *indio_dev,
>>   		if (ret < 0)
>>   			return ret;
>>   
>> -		mutex_lock(&data->lock);
>> +		guard(mutex)(&data->lock);
>>   		ret = mlx90614_iir_search(data->client,
>>   					  val * 100 + val2 / 10000);
>> -		mutex_unlock(&data->lock);
>>   		mlx90614_power_put(data);
>>   
>>   		return ret;
>> @@ -476,12 +473,11 @@ static int mlx90614_sleep(struct mlx90614_data *data)
>>   
>>   	dev_dbg(&data->client->dev, "Requesting sleep");
>>   
>> -	mutex_lock(&data->lock);
>> +	guard(mutex)(&data->lock);
>>   	ret = i2c_smbus_xfer(data->client->adapter, data->client->addr,
>>   			     data->client->flags | I2C_CLIENT_PEC,
>>   			     I2C_SMBUS_WRITE, chip_info->op_sleep,
>>   			     I2C_SMBUS_BYTE, NULL);
>> -	mutex_unlock(&data->lock);
>>   
>>   	return ret;
> 	reutrn i2c_smbus_xfer();
>
>>   }

Re: [PATCH] iio: temperature: mlx90614: use guard(mutex) for EEPROM access locking

[Luiz Mugnaini]

Hi Andy,

Thanks for the feedback.

Addressing your points, the extended scope only adds mlx90614_power_put(),
which calls pm_runtime_put_autosuspend() - a non-blocking and non-locking
operation, posing no dead-lock risks. However, I do understand the raised
concerns and I think it would be better to use a scoped_guard() to preserve
the semantics of the original code. I'll be changing that soon in a v2.

Best Regards,
Luiz

On 20/04/2026 04:59, Andy Shevchenko wrote:
> On Sat, Apr 18, 2026 at 05:51:36PM -0300, Luiz Mugnaini wrote:
>
>> Replace mutex_lock()/mutex_unlock() pairs with guard(mutex) from
>> cleanup.h for cleaner and safer mutex handling. The lock protects
>> EEPROM access across five call sites in mlx90614_read_raw(),
>> mlx90614_write_raw(), and mlx90614_sleep().
>>
>> In all cases, the code between mutex_unlock() and the end of scope
>> is either trivial computation, a return statement, or a call to
>> mlx90614_power_put() (which only schedules a deferred autosuspend
>> via pm_runtime_put_autosuspend()). The slightly extended lock scope
>> is negligible compared to the existing hold times, which include
>> EEPROM write cycles with 40ms sleeps.
> ...
>
>> -		mutex_lock(&data->lock);
>> +		guard(mutex)(&data->lock);
>>   		ret = i2c_smbus_read_word_data(data->client,
>>   					       chip_info->op_eeprom_emissivity);
>> -		mutex_unlock(&data->lock);
>>   		mlx90614_power_put(data);
> How do you sure that the code:
> - is not slowed down due to unneeded things in the critical section
> - doesn't introduce no deadlocks
> ?
>
> How did you test this, please?
>