On Sun, 2024-02-25 at 12:35 +0000, Jonathan Cameron wrote:
> 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.
Oh nice, I was wondering about something like this as my first reaction was also to
have something like guard().
>
> 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.
Agreed. I'll hold a bit to see if it get's merged...
>
> 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;
Alright...
- Nuno Sá
