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

From: Jonathan Cameron <jic23@kernel.org>
To: "Nuno Sá" <noname.nuno@gmail.com>
Cc: Nuno Sa <nuno.sa@analog.com>,
	linux-iio@vger.kernel.org, Lars-Peter Clausen <lars@metafoo.de>
Subject: Re: [PATCH v3 4/4] iio: inkern: move to the cleanup.h magic
Date: Sat, 9 Mar 2024 17:41:45 +0000	[thread overview]
Message-ID: <20240309174145.0834de04@jic23-huawei> (raw)
In-Reply-To: <740c98678f943c761623800544a8ba998aa4aa93.camel@gmail.com>

On Mon, 04 Mar 2024 09:04:49 +0100
Nuno Sá <noname.nuno@gmail.com> wrote:

> On Sun, 2024-03-03 at 14:24 +0000, Jonathan Cameron wrote:
> > On Thu, 29 Feb 2024 16:10:28 +0100
> > Nuno Sa <nuno.sa@analog.com> wrote:
> >   
> > > Use the new cleanup magic for handling mutexes in IIO. This allows us to
> > > greatly simplify some code paths.
> > > 
> > > While at it, also use __free(kfree) where allocations are done and drop
> > > obvious comment in iio_channel_read_min().
> > > 
> > > Signed-off-by: Nuno Sa <nuno.sa@analog.com>  
> > 
> > Hi Nuno
> > 
> > Series looks very nice. One trivial thing inline - I can tidy that up whilst
> > applying if nothing else comes up.
> > 
> > Given this obviously touches a lot of core code, so even though simple it's
> > high risk for queuing up late. I also have a complex mess already queued up
> > for the coming merge window. Hence I'm going to hold off on applying this
> > series until the start of the next cycle.
> > 
> > Nothing outside IIO is going to depend on it, so it's rather simpler decision
> > to hold it than for the ones that add new general purpose infrastructure.
> > 
> >   
> 
> Seems reasonable... It may even give us some time to see how the cond_guard()
> and scoped_cond_guard() will end up.

Absolutely - thankfully converting to the suggestions Linus made will be straight
forwards, so hopefully the worst that happens is a complex merge, or some
fixing up to do afterwards.

> 
> > 
> >   
> > >  EXPORT_SYMBOL_GPL(iio_read_channel_attribute);
> > >  
> > > @@ -757,29 +711,24 @@ int iio_read_channel_processed_scale(struct
> > > iio_channel *chan, int *val,
> > >  	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(chan-  
> > > >indio_dev);  
> > >  	int ret;
> > >  
> > > -	mutex_lock(&iio_dev_opaque->info_exist_lock);
> > > -	if (!chan->indio_dev->info) {
> > > -		ret = -ENODEV;
> > > -		goto err_unlock;
> > > -	}
> > > +	guard(mutex)(&iio_dev_opaque->info_exist_lock);
> > > +	if (!chan->indio_dev->info)
> > > +		return -ENODEV;
> > >  
> > >  	if (iio_channel_has_info(chan->channel, IIO_CHAN_INFO_PROCESSED)) {
> > >  		ret = iio_channel_read(chan, val, NULL,
> > >  				       IIO_CHAN_INFO_PROCESSED);
> > >  		if (ret < 0)
> > > -			goto err_unlock;
> > > +			return ret;
> > >  		*val *= scale;  
> > 
> > 		return 0;
> >   
> > >  	} else {  
> > could drop the else.
> >   
> > >  		ret = iio_channel_read(chan, val, NULL, IIO_CHAN_INFO_RAW);
> > >  		if (ret < 0)
> > > -			goto err_unlock;
> > > +			return ret;
> > >  		ret = iio_convert_raw_to_processed_unlocked(chan, *val,
> > > val,
> > >  							    scale);  
> > 		return iio_convert_raw_to_proc...
> >   
> 
> Hmm, unless I completely misunderstood your comments on v2, this was exactly
> what I had but you recommended to leave the else branch :).
> 
That was a younger me :)  Either way is fine.

Jonathan

> - Nuno Sá
> 