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

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

On Sun, 2024-02-25 at 12:45 +0000, Jonathan Cameron wrote:
> 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.
> 

Will do.

- Nuno Sá