Re: [PATCH v2 2/5] iio: events: move to the cleanup.h magic

From: Jonathan Cameron <jic23@kernel.org>
To: Nuno Sa <nuno.sa@analog.com>
Cc: <linux-iio@vger.kernel.org>, Lars-Peter Clausen <lars@metafoo.de>,
	"Fabio M. De Francesco"
	<fabio.maria.de.francesco@linux.intel.com>
Subject: Re: [PATCH v2 2/5] iio: events: move to the cleanup.h magic
Date: Sun, 25 Feb 2024 12:35:31 +0000	[thread overview]
Message-ID: <20240225123531.020e875f@jic23-huawei> (raw)
In-Reply-To: <20240223-iio-use-cleanup-magic-v2-2-f6b4848c1f34@analog.com>

On Fri, 23 Feb 2024 13:43:45 +0100
Nuno Sa <nuno.sa@analog.com> wrote:

> Use the new cleanup magic for handling mutexes in IIO. This allows us to
> greatly simplify some code paths.
> 
> Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> ---
>  drivers/iio/industrialio-event.c | 42 +++++++++++++++++-----------------------
>  1 file changed, 18 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/iio/industrialio-event.c b/drivers/iio/industrialio-event.c
> index 910c1f14abd5..ef3cecbce915 100644
> --- a/drivers/iio/industrialio-event.c
> +++ b/drivers/iio/industrialio-event.c
> @@ -7,6 +7,7 @@
>   */
>  
>  #include <linux/anon_inodes.h>
> +#include <linux/cleanup.h>
>  #include <linux/device.h>
>  #include <linux/fs.h>
>  #include <linux/kernel.h>
> @@ -146,11 +147,10 @@ static ssize_t iio_event_chrdev_read(struct file *filep,
>  				return -ENODEV;
>  		}
>  
> -		if (mutex_lock_interruptible(&ev_int->read_lock))
> -			return -ERESTARTSYS;
> -		ret = kfifo_to_user(&ev_int->det_events, buf, count, &copied);
> -		mutex_unlock(&ev_int->read_lock);
> -
> +		scoped_cond_guard(mutex_intr, return -ERESTARTSYS,
> +				  &ev_int->read_lock)
> +			ret = kfifo_to_user(&ev_int->det_events, buf, count,
> +					    &copied);

>  		if (ret)
>  			return ret;
>  
> @@ -198,28 +198,22 @@ static int iio_event_getfd(struct iio_dev *indio_dev)
>  	if (ev_int == NULL)
>  		return -ENODEV;
>  
> -	fd = mutex_lock_interruptible(&iio_dev_opaque->mlock);
> -	if (fd)
> -		return fd;
> +	scoped_cond_guard(mutex_intr, return -EINTR, &iio_dev_opaque->mlock) {

Maybe we want to wait for
	cond_guard() that Fabio has been working on to land
https://lore.kernel.org/all/20240217105904.1912368-2-fabio.maria.de.francesco@linux.intel.com/
Not sure if it will make the merge window and I don't want to hold this a whole
cycle if it doesn't.

In many cases I find the scoped version easier to read, but sometimes it makes things
a little messy and for cases like this where it's taken for nearly the whole function
(other than some input parameter checks) the guard() form is nice and
cond_guard() as similar advantages.

If I didn't have a few other requests for changes I'd just have picked this
and we could have coped with the slightly less elegant change set.

> +		if (test_and_set_bit(IIO_BUSY_BIT_POS, &ev_int->flags))
> +			return -EBUSY;
>  
> -	if (test_and_set_bit(IIO_BUSY_BIT_POS, &ev_int->flags)) {
> -		fd = -EBUSY;
> -		goto unlock;
> +		iio_device_get(indio_dev);
> +
> +		fd = anon_inode_getfd("iio:event", &iio_event_chrdev_fileops,
> +				      indio_dev, O_RDONLY | O_CLOEXEC);
> +		if (fd < 0) {
> +			clear_bit(IIO_BUSY_BIT_POS, &ev_int->flags);
> +			iio_device_put(indio_dev);
Given this is an error path, I think it would now be nicer to do
			return fd;
		}

		kfifo_reset_out(&ev_int->dev_events);

		return fd;

That was avoided in original code as it was nicer without the gotos
but now those are gone I think the refactor makes sense.

> +		} else {
> +			kfifo_reset_out(&ev_int->det_events);
> +		}
>  	}
>  
> -	iio_device_get(indio_dev);
> -
> -	fd = anon_inode_getfd("iio:event", &iio_event_chrdev_fileops,
> -				indio_dev, O_RDONLY | O_CLOEXEC);
> -	if (fd < 0) {
> -		clear_bit(IIO_BUSY_BIT_POS, &ev_int->flags);
> -		iio_device_put(indio_dev);
> -	} else {
> -		kfifo_reset_out(&ev_int->det_events);
> -	}
> -
> -unlock:
> -	mutex_unlock(&iio_dev_opaque->mlock);
>  	return fd;
>  }
>  
> 