On Fri, 23 Feb 2024 13:43:45 +0100
Nuno Sa <nuno.sa@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxx/
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;
> }
>
>
