| Message ID | 20240229-iio-use-cleanup-magic-v3-2-c3d34889ae3c@analog.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | iio: move IIO to the cleanup.h magic | expand |
Thu, Feb 29, 2024 at 04:10:26PM +0100, Nuno Sa kirjoitti: > Use the new cleanup magic for handling mutexes in IIO. This allows us to > greatly simplify some code paths. ... > /* 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; > + goto error_device_del; With scoped_guard() it can't be achived, but in the original code the unlocking can be potentially done before printing the message. Here are pros and cons of printing messages under the lock: + the timestamp and appearance in the log probably more accurate - the lock maybe taken for much longer time (if there is a slow console is involved) That said, always consider where to put message printing when it's a critical section. > + } > + list_add_tail(&trig_info->list, &iio_trigger_list); > } ... > 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); In this case the outer {} better to have. ... > - ret = bitmap_find_free_region(trig->pool, > - CONFIG_IIO_CONSUMERS_PER_TRIGGER, > - ilog2(1)); > + ret = bitmap_find_free_region(trig->pool, > + CONFIG_IIO_CONSUMERS_PER_TRIGGER, > + ilog2(1)); Despite being in the original code, this is funny magic constant...
Thu, Feb 29, 2024 at 04:10:26PM +0100, Nuno Sa kirjoitti: > Use the new cleanup magic for handling mutexes in IIO. This allows us to > greatly simplify some code paths. ... > static void iio_trigger_put_irq(struct iio_trigger *trig, int irq) > { > - mutex_lock(&trig->pool_lock); > + guard(mutex)(&trig->pool_lock); > clear_bit(irq - trig->subirq_base, trig->pool); Another side note: Why do we need atomic bit operation(s)? > - mutex_unlock(&trig->pool_lock); > }
On Sat, 2024-03-16 at 21:39 +0200, Andy Shevchenko wrote: > Thu, Feb 29, 2024 at 04:10:26PM +0100, Nuno Sa kirjoitti: > > Use the new cleanup magic for handling mutexes in IIO. This allows us to > > greatly simplify some code paths. > > ... > > > static void iio_trigger_put_irq(struct iio_trigger *trig, int irq) > > { > > - mutex_lock(&trig->pool_lock); > > + guard(mutex)(&trig->pool_lock); > > clear_bit(irq - trig->subirq_base, trig->pool); > > Another side note: Why do we need atomic bit operation(s)? > Did not checked the code but my guess is that the lock is always grabbed so we likely don't need the atomic variants. But that's something for a future patch. - Nuno Sá
On Sat, 16 Mar 2024 21:32:56 +0200 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > Thu, Feb 29, 2024 at 04:10:26PM +0100, Nuno Sa kirjoitti: > > Use the new cleanup magic for handling mutexes in IIO. This allows us to > > greatly simplify some code paths. > > ... > > > /* 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; > > + goto error_device_del; > > With scoped_guard() it can't be achived, but in the original code the unlocking > can be potentially done before printing the message. > Here are pros and cons of printing messages under the lock: > + the timestamp and appearance in the log probably more accurate > - the lock maybe taken for much longer time (if there is a slow > console is involved) > > That said, always consider where to put message printing when it's a critical > section It's an unlikely to occur error message. We almost certainly don't care. > > > + } > > + list_add_tail(&trig_info->list, &iio_trigger_list); > > } > > ... > > > 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); > > In this case the outer {} better to have. > > ... > > > - ret = bitmap_find_free_region(trig->pool, > > - CONFIG_IIO_CONSUMERS_PER_TRIGGER, > > - ilog2(1)); > > > + ret = bitmap_find_free_region(trig->pool, > > + CONFIG_IIO_CONSUMERS_PER_TRIGGER, > > + ilog2(1)); > > Despite being in the original code, this is funny magic constant... Not that magic, build time config variable to avoid adding complexity of dynamic expansion of various structures. We could have picked a big number but someone will always want a bigger one and from what I recall actually make it expandable was nasty to do. Been a long time, though so I'm open to patches that get rid of this in favor of a dynamic solution. Jonathan >
On Mon, Mar 18, 2024 at 2:33 PM Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote: > On Sat, 16 Mar 2024 21:32:56 +0200 > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > Thu, Feb 29, 2024 at 04:10:26PM +0100, Nuno Sa kirjoitti: ... > > > - ret = bitmap_find_free_region(trig->pool, > > > - CONFIG_IIO_CONSUMERS_PER_TRIGGER, > > > - ilog2(1)); > > > > > + ret = bitmap_find_free_region(trig->pool, > > > + CONFIG_IIO_CONSUMERS_PER_TRIGGER, > > > + ilog2(1)); > > > > Despite being in the original code, this is funny magic constant... > > Not that magic, build time config variable to avoid adding complexity > of dynamic expansion of various structures. We could have picked a big > number but someone will always want a bigger one and from what I recall > actually make it expandable was nasty to do. Been a long time, though > so I'm open to patches that get rid of this in favor of a dynamic solution. I didn't get you, sorry. Logarithm (by any base) from 1 is 0. Writing it as arithmetic expression seems funny to me.
On Mon, 18 Mar 2024 15:12:20 +0200 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Mon, Mar 18, 2024 at 2:33 PM Jonathan Cameron > <Jonathan.Cameron@huawei.com> wrote: > > On Sat, 16 Mar 2024 21:32:56 +0200 > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > Thu, Feb 29, 2024 at 04:10:26PM +0100, Nuno Sa kirjoitti: > > ... > > > > > - ret = bitmap_find_free_region(trig->pool, > > > > - CONFIG_IIO_CONSUMERS_PER_TRIGGER, > > > > - ilog2(1)); > > > > > > > + ret = bitmap_find_free_region(trig->pool, > > > > + CONFIG_IIO_CONSUMERS_PER_TRIGGER, > > > > + ilog2(1)); > > > > > > Despite being in the original code, this is funny magic constant... > > > > Not that magic, build time config variable to avoid adding complexity > > of dynamic expansion of various structures. We could have picked a big > > number but someone will always want a bigger one and from what I recall > > actually make it expandable was nasty to do. Been a long time, though > > so I'm open to patches that get rid of this in favor of a dynamic solution. > > I didn't get you, sorry. Logarithm (by any base) from 1 is 0. Writing > it as arithmetic expression seems funny to me. Ah. I was looking at the line above ;) That one is because it lines up with the docs for bitmap_find_free_region() Last parameter is order, but seems more natural to express it in number of bits hence the 1 rather that superficially looking like we are asking for a region of length 0. >
diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-trigger.c index 18f83158f637..16de57846bd9 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; + 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); @@ -102,9 +102,8 @@ EXPORT_SYMBOL(iio_trigger_register); void iio_trigger_unregister(struct iio_trigger *trig_info) { - mutex_lock(&iio_trigger_list_lock); - list_del(&trig_info->list); - mutex_unlock(&iio_trigger_list_lock); + scoped_guard(mutex, &iio_trigger_list_lock) + list_del(&trig_info->list); ida_free(&iio_trigger_ida, trig_info->id); /* Possible issue in here */ @@ -120,12 +119,11 @@ int iio_trigger_set_immutable(struct iio_dev *indio_dev, struct iio_trigger *tri return -EINVAL; iio_dev_opaque = to_iio_dev_opaque(indio_dev); - mutex_lock(&iio_dev_opaque->mlock); + guard(mutex)(&iio_dev_opaque->mlock); WARN_ON(iio_dev_opaque->trig_readonly); indio_dev->trig = iio_trigger_get(trig); iio_dev_opaque->trig_readonly = true; - mutex_unlock(&iio_dev_opaque->mlock); return 0; } @@ -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); - return trig; + return NULL; } static void iio_reenable_work_fn(struct work_struct *work) @@ -259,22 +253,21 @@ 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); - if (ret >= 0) - ret += trig->subirq_base; + 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 ret; + return ret + trig->subirq_base; } static void iio_trigger_put_irq(struct iio_trigger *trig, int irq) { - mutex_lock(&trig->pool_lock); + guard(mutex)(&trig->pool_lock); clear_bit(irq - trig->subirq_base, trig->pool); - mutex_unlock(&trig->pool_lock); } /* Complexity in here. With certain triggers (datardy) an acknowledgement @@ -451,16 +444,12 @@ static ssize_t current_trigger_store(struct device *dev, struct iio_trigger *trig; int ret; - mutex_lock(&iio_dev_opaque->mlock); - if (iio_dev_opaque->currentmode == INDIO_BUFFER_TRIGGERED) { - mutex_unlock(&iio_dev_opaque->mlock); - return -EBUSY; + scoped_guard(mutex, &iio_dev_opaque->mlock) { + if (iio_dev_opaque->currentmode == INDIO_BUFFER_TRIGGERED) + return -EBUSY; + if (iio_dev_opaque->trig_readonly) + return -EPERM; } - if (iio_dev_opaque->trig_readonly) { - mutex_unlock(&iio_dev_opaque->mlock); - return -EPERM; - } - mutex_unlock(&iio_dev_opaque->mlock); trig = iio_trigger_acquire_by_name(buf); if (oldtrig == trig) {
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> --- drivers/iio/industrialio-trigger.c | 71 ++++++++++++++++---------------------- 1 file changed, 30 insertions(+), 41 deletions(-)