On Sun, 2024-02-25 at 12:53 +0000, Jonathan Cameron wrote:
> On Fri, 23 Feb 2024 13:43:47 +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>
>
> I think we can do more in here as a result of early returns being available.
>
> > ---
> > drivers/iio/industrialio-buffer.c | 105 ++++++++++++++------------------------
> > 1 file changed, 38 insertions(+), 67 deletions(-)
> >
> > diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-
> > buffer.c
> > index b581a7e80566..ec6bc881cf13 100644
> > --- a/drivers/iio/industrialio-buffer.c
> > +++ b/drivers/iio/industrialio-buffer.c
> > @@ -10,6 +10,7 @@
> > * - Alternative access techniques?
> > */
> > #include <linux/anon_inodes.h>
> > +#include <linux/cleanup.h>
> > #include <linux/kernel.h>
> > #include <linux/export.h>
> > #include <linux/device.h>
> > @@ -533,28 +534,25 @@ static ssize_t iio_scan_el_store(struct device *dev,
> > ret = kstrtobool(buf, &state);
> > if (ret < 0)
> > return ret;
> > - mutex_lock(&iio_dev_opaque->mlock);
> > - if (iio_buffer_is_active(buffer)) {
> > - ret = -EBUSY;
> > - goto error_ret;
> > - }
> > +
> > + guard(mutex)(&iio_dev_opaque->mlock);
> > + if (iio_buffer_is_active(buffer))
> > + return -EBUSY;
> > +
> > ret = iio_scan_mask_query(indio_dev, buffer, this_attr->address);
> > if (ret < 0)
> > - goto error_ret;
> > + return ret;
>
> We could short cut this I think and end up with a simpler flow.
> The early returns allow something like
>
> if (state && ret) /* Nothing to do */
> return len;
>
> if (state)
> ret = iio_scan_mask_set(indio_dev, buffer, this_attr->address);
> else
> ret = iio_scan_mask_clear(buffer, this_attr->address);
> if (ret)
> return ret;
>
> return len;
Nice...
>
> > if (!state && ret) {
> > ret = iio_scan_mask_clear(buffer, this_attr->address);
> > if (ret)
> > - goto error_ret;
> > + return ret;
> > } else if (state && !ret) {
> > ret = iio_scan_mask_set(indio_dev, buffer, this_attr->address);
> > if (ret)
> > - goto error_ret;
> > + return ret;
> > }
> >
> > -error_ret:
> > - mutex_unlock(&iio_dev_opaque->mlock);
> > -
> > - return ret < 0 ? ret : len;
> > + return len;
> > }
> >
>
>
>
> ...
>
> >
> > @@ -1326,21 +1305,19 @@ static ssize_t enable_store(struct device *dev, struct
> > device_attribute *attr,
> > if (ret < 0)
> > return ret;
> >
> > - mutex_lock(&iio_dev_opaque->mlock);
> > + guard(mutex)(&iio_dev_opaque->mlock);
> >
> > /* Find out if it is in the list */
> > inlist = iio_buffer_is_active(buffer);
> > /* Already in desired state */
> > if (inlist == requested_state)
> > - goto done;
> > + return len;
> >
> > if (requested_state)
> > ret = __iio_update_buffers(indio_dev, buffer, NULL);
> > else
> > ret = __iio_update_buffers(indio_dev, NULL, buffer);
> >
> > -done:
> > - mutex_unlock(&iio_dev_opaque->mlock);
> > return (ret < 0) ? ret : len;
> Maybe just switch this for
>
> if (ret < 0)
> return ret;
>
> return len;
>
> So it looks more like the new return len above?
>
Ok, I typically prefer that form anyways :)
- Nuno Sá
