On Sat, 16 Mar 2024 21:32:56 +0200
Andy Shevchenko <andy.shevchenko@xxxxxxxxx> 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
>
