All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
To: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Nuno Sa <nuno.sa@analog.com>, <linux-iio@vger.kernel.org>,
	"Jonathan Cameron" <jic23@kernel.org>,
	Lars-Peter Clausen <lars@metafoo.de>
Subject: Re: [PATCH v3 2/4] iio: trigger: move to the cleanup.h magic
Date: Mon, 18 Mar 2024 12:33:20 +0000	[thread overview]
Message-ID: <20240318123320.000030d3@Huawei.com> (raw)
In-Reply-To: <ZfXz6E086KPWUn8Q@surfacebook.localdomain>

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

> 


  reply	other threads:[~2024-03-18 12:33 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-02-29 15:10 [PATCH v3 0/4] iio: move IIO to the cleanup.h magic Nuno Sa
2024-02-29 15:10 ` [PATCH v3 1/4] iio: core: move to " Nuno Sa
2024-02-29 15:10 ` [PATCH v3 2/4] iio: trigger: move to the " Nuno Sa
2024-03-16 19:32   ` Andy Shevchenko
2024-03-18 12:33     ` Jonathan Cameron [this message]
2024-03-18 13:12       ` Andy Shevchenko
2024-03-18 14:15         ` Jonathan Cameron
2024-03-16 19:39   ` Andy Shevchenko
2024-03-18  9:22     ` Nuno Sá
2024-02-29 15:10 ` [PATCH v3 3/4] iio: buffer: iio: core: " Nuno Sa
2024-03-16 19:38   ` Andy Shevchenko
2024-03-18  9:23     ` Nuno Sá
2024-03-18 12:35     ` Jonathan Cameron
2024-03-16 19:49   ` Andy Shevchenko
2024-02-29 15:10 ` [PATCH v3 4/4] iio: inkern: " Nuno Sa
2024-03-03 14:24   ` Jonathan Cameron
2024-03-04  8:04     ` Nuno Sá
2024-03-09 17:41       ` Jonathan Cameron
2024-03-16 13:26         ` Jonathan Cameron
2024-03-16 19:48   ` Andy Shevchenko
2024-03-18  9:20     ` Nuno Sá
2024-03-23 18:09     ` Jonathan Cameron

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20240318123320.000030d3@Huawei.com \
    --to=jonathan.cameron@huawei.com \
    --cc=andy.shevchenko@gmail.com \
    --cc=jic23@kernel.org \
    --cc=lars@metafoo.de \
    --cc=linux-iio@vger.kernel.org \
    --cc=nuno.sa@analog.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.