* [PATCH] iio: temperature: mlx90614: use guard(mutex) for EEPROM access locking @ 2026-04-18 20:51 Luiz Mugnaini 2026-04-19 11:43 ` Jonathan Cameron 2026-04-20 7:59 ` Andy Shevchenko 0 siblings, 2 replies; 5+ messages in thread From: Luiz Mugnaini @ 2026-04-18 20:51 UTC (permalink / raw) To: cmo, jic23, dlechner, nuno.sa, andy; +Cc: Luiz Mugnaini, linux-iio 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] iio: temperature: mlx90614: use guard(mutex) for EEPROM access locking 2026-04-18 20:51 [PATCH] iio: temperature: mlx90614: use guard(mutex) for EEPROM access locking Luiz Mugnaini @ 2026-04-19 11:43 ` Jonathan Cameron 2026-04-20 14:11 ` Luiz Mugnaini 2026-04-20 7:59 ` Andy Shevchenko 1 sibling, 1 reply; 5+ messages in thread From: Jonathan Cameron @ 2026-04-19 11:43 UTC (permalink / raw) To: Luiz Mugnaini; +Cc: cmo, dlechner, nuno.sa, andy, linux-iio 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(); > } ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] iio: temperature: mlx90614: use guard(mutex) for EEPROM access locking 2026-04-19 11:43 ` Jonathan Cameron @ 2026-04-20 14:11 ` Luiz Mugnaini 0 siblings, 0 replies; 5+ messages in thread From: Luiz Mugnaini @ 2026-04-20 14:11 UTC (permalink / raw) To: Jonathan Cameron; +Cc: cmo, dlechner, nuno.sa, andy, linux-iio 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(); > >> } ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] iio: temperature: mlx90614: use guard(mutex) for EEPROM access locking 2026-04-18 20:51 [PATCH] iio: temperature: mlx90614: use guard(mutex) for EEPROM access locking Luiz Mugnaini 2026-04-19 11:43 ` Jonathan Cameron @ 2026-04-20 7:59 ` Andy Shevchenko 2026-04-20 14:12 ` Luiz Mugnaini 1 sibling, 1 reply; 5+ messages in thread From: Andy Shevchenko @ 2026-04-20 7:59 UTC (permalink / raw) To: Luiz Mugnaini; +Cc: cmo, jic23, dlechner, nuno.sa, andy, linux-iio 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] iio: temperature: mlx90614: use guard(mutex) for EEPROM access locking 2026-04-20 7:59 ` Andy Shevchenko @ 2026-04-20 14:12 ` Luiz Mugnaini 0 siblings, 0 replies; 5+ messages in thread From: Luiz Mugnaini @ 2026-04-20 14:12 UTC (permalink / raw) To: Andy Shevchenko; +Cc: cmo, jic23, dlechner, nuno.sa, andy, linux-iio 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? > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2026-04-20 14:12 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-04-18 20:51 [PATCH] iio: temperature: mlx90614: use guard(mutex) for EEPROM access locking Luiz Mugnaini 2026-04-19 11:43 ` Jonathan Cameron 2026-04-20 14:11 ` Luiz Mugnaini 2026-04-20 7:59 ` Andy Shevchenko 2026-04-20 14:12 ` Luiz Mugnaini
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox