| Message ID | 20240223-iio-use-cleanup-magic-v2-2-f6b4848c1f34@analog.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: move IIO to the cleanup.h magic | expand |
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; > } > >
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á
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) { + 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); + } 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; }
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(-)