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@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>
> 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á
