linux-kernel - Re: [PATCH v3 2/4] iio: mpl3115: use guards from cleanup.h

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250927173621.09bc9f39@jic23-huawei>
Date: Sat, 27 Sep 2025 17:36:21 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Antoni Pokusinski <apokusinski01@...il.com>
Cc: dlechner@...libre.com, nuno.sa@...log.com, andy@...nel.org,
 robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org,
 linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
 linux-iio@...r.kernel.org, linux@...ck-us.net, rodrigo.gobbi.7@...il.com,
 naresh.solanki@...ements.com, michal.simek@....com,
 grantpeltier93@...il.com, farouk.bouabid@...rry.de,
 marcelo.schmitt1@...il.com
Subject: Re: [PATCH v3 2/4] iio: mpl3115: use guards from cleanup.h

On Sat, 27 Sep 2025 00:01:48 +0200
Antoni Pokusinski <apokusinski01@...il.com> wrote:

> Include linux/cleanup.h and use the scoped_guard() to simplify the code.
See below. I'm not sure this is in general a good idea in this driver, but
see the comments below.  I think more traditional factoring out of the code
under the lock into a helper function should be the main change here.
That might or might not make sense combined with a scoped_guard().


> 
> Signed-off-by: Antoni Pokusinski <apokusinski01@...il.com>
> ---
>  drivers/iio/pressure/mpl3115.c | 42 +++++++++++++++-------------------
>  1 file changed, 19 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/iio/pressure/mpl3115.c b/drivers/iio/pressure/mpl3115.c
> index 579da60ef441..80af672f65c6 100644
> --- a/drivers/iio/pressure/mpl3115.c
> +++ b/drivers/iio/pressure/mpl3115.c
> @@ -10,14 +10,16 @@
>   * interrupts, user offset correction, raw mode
>   */
>  
> -#include <linux/module.h>
> +#include <linux/cleanup.h>
> +#include <linux/delay.h>
>  #include <linux/i2c.h>
> +#include <linux/module.h>
> +
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
>  #include <linux/iio/trigger_consumer.h>
>  #include <linux/iio/buffer.h>
>  #include <linux/iio/triggered_buffer.h>
> -#include <linux/delay.h>
>  
>  #define MPL3115_STATUS 0x00
>  #define MPL3115_OUT_PRESS 0x01 /* MSB first, 20 bit */
> @@ -163,32 +165,26 @@ static irqreturn_t mpl3115_trigger_handler(int irq, void *p)
>  	u8 buffer[16] __aligned(8) = { };
>  	int ret, pos = 0;
>  
> -	mutex_lock(&data->lock);
> -	ret = mpl3115_request(data);
> -	if (ret < 0) {
> -		mutex_unlock(&data->lock);
> -		goto done;
> -	}
> -
> -	if (test_bit(0, indio_dev->active_scan_mask)) {
> -		ret = i2c_smbus_read_i2c_block_data(data->client,
> -			MPL3115_OUT_PRESS, 3, &buffer[pos]);
> -		if (ret < 0) {
> -			mutex_unlock(&data->lock);
> +	scoped_guard(mutex, &data->lock) {
> +		ret = mpl3115_request(data);
> +		if (ret < 0)
>  			goto done;
Read the guidance in cleanup.h.  Whilst what you have here is actually not
a bug, it is considered fragile to combine gotos and scoped cleanup in a function.
Sometimes that means that if we are using guards() we need to also duplicate
some error handling.

So, the way to avoid that is to factor out the stuff under the goto to a helper
function.  That function than then return directly on errors like this.

Looks something like

	scoped_guard(mutex, &data->lock) {
		ret = mpl3115_fill_buffer(data, buffer);
		if (ret) {
			iio_trigger_notify_done(indio_dev->trig);
			return IRQ_HANDLED;
		}
	}

	iio_push_to_buffers_with_ts...
	iio_trigger_notify_done(indio_dev->trig);
	return IRQ_HANDLED;


However, it is also worth keeping in mind that sometimes scoped cleanup
of which guards are a special case is not the right solution for a whole
driver. I'm not sure if it is worth while in this case, but try the approach
mentioned above and see how it looks.

Alternative would still be to factor out the helper, but instead just have
	mutex_lock(&data->lock);
	ret = mpl3115_fill_buffer(data, buffer);
	mutex_unlock(&data->lock);
	if (ret)
		goto...


Jonathan

> +
> +		if (test_bit(0, indio_dev->active_scan_mask)) {
> +			ret = i2c_smbus_read_i2c_block_data(data->client,
> +				MPL3115_OUT_PRESS, 3, &buffer[pos]);
> +			if (ret < 0)
> +				goto done;
> +			pos += 4;
>  		}
> -		pos += 4;
> -	}
>  
> -	if (test_bit(1, indio_dev->active_scan_mask)) {
> -		ret = i2c_smbus_read_i2c_block_data(data->client,
> -			MPL3115_OUT_TEMP, 2, &buffer[pos]);
> -		if (ret < 0) {
> -			mutex_unlock(&data->lock);
> -			goto done;
> +		if (test_bit(1, indio_dev->active_scan_mask)) {
> +			ret = i2c_smbus_read_i2c_block_data(data->client,
> +				MPL3115_OUT_TEMP, 2, &buffer[pos]);
> +			if (ret < 0)
> +				goto done;
>  		}
>  	}
> -	mutex_unlock(&data->lock);
>  
>  	iio_push_to_buffers_with_ts(indio_dev, buffer, sizeof(buffer),
>  				    iio_get_time_ns(indio_dev));