Re: [PATCH v3 2/4] iio: trigger: move to the cleanup.h magic

[Andy Shevchenko <andy.shevchenko@gmail.com>]

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...

-- 
With Best Regards,
Andy Shevchenko