Re: [PATCH v2 5/5] iio: inkern: move to the cleanup.h magic

From: "Nuno Sá" <noname.nuno@gmail.com>
To: Jonathan Cameron <jic23@kernel.org>, Nuno Sa <nuno.sa@analog.com>
Cc: linux-iio@vger.kernel.org, Lars-Peter Clausen <lars@metafoo.de>
Subject: Re: [PATCH v2 5/5] iio: inkern: move to the cleanup.h magic
Date: Mon, 26 Feb 2024 09:57:06 +0100	[thread overview]
Message-ID: <51dc0e063c835c5c851fbc0b9bb8acfc03e8f6b4.camel@gmail.com> (raw)
In-Reply-To: <20240225131202.165d9c34@jic23-huawei>

On Sun, 2024-02-25 at 13:12 +0000, Jonathan Cameron wrote:
> On Fri, 23 Feb 2024 13:43:48 +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.
> > 
> > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> > ---
> >  drivers/iio/inkern.c | 224 ++++++++++++++++-----------------------------------
> >  1 file changed, 71 insertions(+), 153 deletions(-)
> > 
> > diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> > index 7a1f6713318a..6a1d6ff8eb97 100644
> > --- a/drivers/iio/inkern.c
> > +++ b/drivers/iio/inkern.c
> > @@ -3,6 +3,7 @@
> >   *
> >   * Copyright (c) 2011 Jonathan Cameron
> >   */
> > +#include <linux/cleanup.h>
> >  #include <linux/err.h>
> >  #include <linux/export.h>
> >  #include <linux/minmax.h>
> > @@ -43,30 +44,26 @@ static int iio_map_array_unregister_locked(struct iio_dev
> > *indio_dev)
> >  
> >  int iio_map_array_register(struct iio_dev *indio_dev, struct iio_map *maps)
> >  {
> > -	int i = 0, ret = 0;
> > +	int i = 0;
> >  	struct iio_map_internal *mapi;
> >  
> >  	if (!maps)
> >  		return 0;
> >  
> > -	mutex_lock(&iio_map_list_lock);
> > +	guard(mutex)(&iio_map_list_lock);
> >  	while (maps[i].consumer_dev_name) {
> >  		mapi = kzalloc(sizeof(*mapi), GFP_KERNEL);
> >  		if (!mapi) {
> > -			ret = -ENOMEM;
> > -			goto error_ret;
> > +			iio_map_array_unregister_locked(indio_dev);
> > +			return -ENOMEM;
> 
> break this out to a separate error path via a goto.
> The cleanup is not totally obvious so I'd like it to stand out more
> than being burried here.  This wasn't good in original code either
> as that should just have duplicated the mutex_unlock.
> 
> 
> >  		}
> >  		mapi->map = &maps[i];
> >  		mapi->indio_dev = indio_dev;
> >  		list_add_tail(&mapi->l, &iio_map_list);
> >  		i++;
> >  	}
> > -error_ret:
> > -	if (ret)
> > -		iio_map_array_unregister_locked(indio_dev);
> > -	mutex_unlock(&iio_map_list_lock);
> >  
> > -	return ret;
> > +	return 0;
> >  }
> >  EXPORT_SYMBOL_GPL(iio_map_array_register);
> 
> ...
> 
> >  EXPORT_SYMBOL_GPL(iio_map_array_unregister);
> >  
> > @@ -337,17 +329,17 @@ static struct iio_channel *iio_channel_get_sys(const char
> > *name,
> >  		return ERR_PTR(-ENODEV);
> >  
> >  	/* first find matching entry the channel map */
> > -	mutex_lock(&iio_map_list_lock);
> > -	list_for_each_entry(c_i, &iio_map_list, l) {
> > -		if ((name && strcmp(name, c_i->map->consumer_dev_name) != 0) ||
> > -		    (channel_name &&
> > -		     strcmp(channel_name, c_i->map->consumer_channel) != 0))
> > -			continue;
> > -		c = c_i;
> > -		iio_device_get(c->indio_dev);
> > -		break;
> > +	scoped_guard(mutex, &iio_map_list_lock) {
> > +		list_for_each_entry(c_i, &iio_map_list, l) {
> > +			if ((name && strcmp(name, c_i->map->consumer_dev_name)
> > != 0) ||
> > +			    (channel_name &&
> > +			     strcmp(channel_name, c_i->map->consumer_channel) !=
> > 0))
> > +				continue;
> > +			c = c_i;
> > +			iio_device_get(c->indio_dev);
> > +			break;
> This mix of continue and break is odd. But not something to cleanup in this patch.
> It's based on assumption we either have name or channel_name which is checked
> above.
> 			if ((name && strcmp(name, c_i->map->consumer_dev_name) ==
> 0) ||
> 			    (!name && stcmp(channel_name, c_i->map-
> >consumer_channel == 0)) {
> 				c = c_i;
> 				iio_device_get(c->indio_dev);
> 				break;
> 			}
> is I think equivalent. Still too complex for this patch I think.
> 
> > +		}
> >  	}
> > -	mutex_unlock(&iio_map_list_lock);
> >  	if (!c)
> >  		return ERR_PTR(-ENODEV);
> >  
> > @@ -469,7 +461,7 @@ struct iio_channel *iio_channel_get_all(struct device *dev)
> >  
> >  	name = dev_name(dev);
> >  
> > -	mutex_lock(&iio_map_list_lock);
> > +	guard(mutex)(&iio_map_list_lock);
> >  	/* first count the matching maps */
> >  	list_for_each_entry(c, &iio_map_list, l)
> >  		if (name && strcmp(name, c->map->consumer_dev_name) != 0)
> > @@ -477,17 +469,13 @@ struct iio_channel *iio_channel_get_all(struct device *dev)
> >  		else
> >  			nummaps++;
> >  
> > -	if (nummaps == 0) {
> > -		ret = -ENODEV;
> > -		goto error_ret;
> > -	}
> > +	if (nummaps == 0)
> > +		return ERR_PTR(-ENODEV);
> >  
> >  	/* NULL terminated array to save passing size */
> >  	chans = kcalloc(nummaps + 1, sizeof(*chans), GFP_KERNEL);
> 
> as below, consider dragging the instantiation down here and use __free(kfree) for
> this
> plus make sure to return_ptr() at the good exit path.
> 
> > -	if (!chans) {
> > -		ret = -ENOMEM;
> > -		goto error_ret;
> > -	}
> > +	if (!chans)
> > +		return ERR_PTR(-ENOMEM);
> >  
> >  	/* for each map fill in the chans element */
> >  	list_for_each_entry(c, &iio_map_list, l) {
> > @@ -509,7 +497,6 @@ struct iio_channel *iio_channel_get_all(struct device *dev)
> >  		ret = -ENODEV;
> >  		goto error_free_chans;
> >  	}
> > -	mutex_unlock(&iio_map_list_lock);
> >  
> >  	return chans;
> >  
> > @@ -517,9 +504,6 @@ struct iio_channel *iio_channel_get_all(struct device *dev)
> >  	for (i = 0; i < nummaps; i++)
> >  		iio_device_put(chans[i].indio_dev);
> >  	kfree(chans);
> 
> Could use __free(kfree) and return_ptr(chans);  Not a huge gain though
> so maybe not worth it.
> 

I'll see how it looks like. Even though not a huge gain, I guess it makes the code
more consistent as we move to the cleanup "idiom"...

- Nuno Sá 

> 