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

["Nuno Sá" <noname.nuno@gmail.com>]

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@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.

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á