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

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 23 Feb 2024 13:43:46 +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>
Hi Nuno,

Thanks for doing these.

A few minor comments inline.

Trick with these cleanup.h series (that I am still perfecting)
is spotting the places the code can be improved that are exposed by
the simplifications. Often we've gone through an elaborate dance with
error handling etc that is no longer necessary.

> ---
>  drivers/iio/industrialio-trigger.c | 64 ++++++++++++++++----------------------
>  1 file changed, 26 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-trigger.c
> index 18f83158f637..e4f0802fdd1d 100644
> --- a/drivers/iio/industrialio-trigger.c
> +++ b/drivers/iio/industrialio-trigger.c
> @@ -4,6 +4,7 @@
>   * Copyright (c) 2008 Jonathan Cameron
>   */
>  
> +#include <linux/cleanup.h>
>  #include <linux/kernel.h>
>  #include <linux/idr.h>
>  #include <linux/err.h>
> @@ -80,19 +81,18 @@ int iio_trigger_register(struct iio_trigger *trig_info)
>  		goto error_unregister_id;
>  
>  	/* Add to list of available triggers held by the IIO core */
> -	mutex_lock(&iio_trigger_list_lock);
> -	if (__iio_trigger_find_by_name(trig_info->name)) {
> -		pr_err("Duplicate trigger name '%s'\n", trig_info->name);
> -		ret = -EEXIST;
> -		goto error_device_del;
> +	scoped_guard(mutex, &iio_trigger_list_lock) {
> +		if (__iio_trigger_find_by_name(trig_info->name)) {
> +			pr_err("Duplicate trigger name '%s'\n", trig_info->name);
> +			ret =  -EEXIST;

Bonus space after =

> +			goto error_device_del;
> +		}
> +		list_add_tail(&trig_info->list, &iio_trigger_list);
>  	}
> -	list_add_tail(&trig_info->list, &iio_trigger_list);
> -	mutex_unlock(&iio_trigger_list_lock);
>  
>  	return 0;
>  
>  error_device_del:
> -	mutex_unlock(&iio_trigger_list_lock);
>  	device_del(&trig_info->dev);
>  error_unregister_id:
>  	ida_free(&iio_trigger_ida, trig_info->id);


> @@ -145,18 +143,14 @@ static struct iio_trigger *__iio_trigger_find_by_name(const char *name)
>  
>  static struct iio_trigger *iio_trigger_acquire_by_name(const char *name)
>  {
> -	struct iio_trigger *trig = NULL, *iter;
> +	struct iio_trigger *iter;
>  
> -	mutex_lock(&iio_trigger_list_lock);
> +	guard(mutex)(&iio_trigger_list_lock);
>  	list_for_each_entry(iter, &iio_trigger_list, list)
> -		if (sysfs_streq(iter->name, name)) {
> -			trig = iter;
> -			iio_trigger_get(trig);
> -			break;
> -		}
> -	mutex_unlock(&iio_trigger_list_lock);
> +		if (sysfs_streq(iter->name, name))
> +			return iio_trigger_get(iter);

Nice :)
>  
> -	return trig;
> +	return NULL;
>  }
>  
>  static void iio_reenable_work_fn(struct work_struct *work)
> @@ -259,11 +253,10 @@ static int iio_trigger_get_irq(struct iio_trigger *trig)
>  {
>  	int ret;
>  
> -	mutex_lock(&trig->pool_lock);
> -	ret = bitmap_find_free_region(trig->pool,
> -				      CONFIG_IIO_CONSUMERS_PER_TRIGGER,
> -				      ilog2(1));
> -	mutex_unlock(&trig->pool_lock);
> +	scoped_guard(mutex, &trig->pool_lock)
> +		ret = bitmap_find_free_region(trig->pool,
> +					      CONFIG_IIO_CONSUMERS_PER_TRIGGER,
> +					      ilog2(1));

This presents an opportunity make this more idiomatic as a result
	scoped_guard(mutex, &trig->pool_lock) {
		ret = bitmap_find_free_region(trig->pool,
					      CONFIG_IIO_CONSUMERS_PER_TRIGGER,
					      ilog2(1));
		if (ret < 0)
			return ret;
	}

	return trig->subirq_base + ret;

Getting rid of 'non-standard' error handling conditions is one of the nicest
things this cleanup.h stuff enables as we don't dance around to avoid lots
of unlock paths.

Jonathan
>  	if (ret >= 0)
>  		ret += trig->subirq_base;
>