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;
> 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?
> }
>
