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

From: Jonathan Cameron <jic23@kernel.org>
To: Andy Shevchenko <andy.shevchenko@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, 23 Mar 2024 18:09:51 +0000	[thread overview]
Message-ID: <20240323180951.5e990e11@jic23-huawei> (raw)
In-Reply-To: <ZfX3gnbwYcZlGpBq@surfacebook.localdomain>

On Sat, 16 Mar 2024 21:48:18 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> Thu, Feb 29, 2024 at 04:10:28PM +0100, Nuno Sa kirjoitti:
> > 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().  
> 
> ...
> 
> >  int iio_map_array_register(struct iio_dev *indio_dev, struct iio_map *maps)
> >  {
> > -	int i = 0, ret = 0;
> > +	int i = 0, ret;
> >  	struct iio_map_internal *mapi;  
> 
> Why not making it reversed xmas tree order at the same time?
> 

I tweaked this. Went a bit further as mixing declarations that
set values and ones that don't is a bad pattern for readability.

	struct iio_map_internal *mapi;
	int i = 0;
	int ret;

> >  	if (!maps)
> >  		return 0;  
> 
> ...
> 
> > -error_ret:
> > -	if (ret)
> > -		iio_map_array_unregister_locked(indio_dev);
> > -	mutex_unlock(&iio_map_list_lock);
> >  
> > +	return 0;
> > +error_ret:
> > +	iio_map_array_unregister_locked(indio_dev);
> >  	return ret;
> >  }  
> 
> Do we really need to split this? (I'm fine with a new code, but seems to me as
> unneeded churn.)

I much prefer not to have the
	if (ret) // error case
		do something.

	//back to both good and bad paths.
	return ret;

pattern - so I've very keen to have this spit as I disliked the original
code and there is even less reason to combine the paths now we don't need
the mutex_unlock.

> 
> ...
> 
> > +	struct iio_channel *channel __free(kfree) = kzalloc(sizeof(*channel),
> > +							    GFP_KERNEL);  
> 
> I would indent a bit differently:
> 
> 	struct iio_channel *channel __free(kfree) =
> 					kzalloc(sizeof(*channel), GFP_KERNEL);
> 
> (maybe less TABs, but you got the idea)
Given I was rebasing anyway, tidied this up (in 4 places) as well (fewer tabs ;)

> 
> >  	if (!channel)
> >  		return ERR_PTR(-ENOMEM);  
> 
> ...
> 
> > +	struct iio_channel *chans __free(kfree) = kcalloc(nummaps + 1,
> > +							  sizeof(*chans),
> > +							  GFP_KERNEL);  
> 
> Ditto.
> 
> >  	if (!chans)
> >  		return ERR_PTR(-ENOMEM);  
> 
> ...
> 
> >  	/* 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))  
> 
> I would kill these ' != 0' pieces, but I see they are in the original code.

Don't mind a change doing that, but not in this patch.

> 
> > +				continue;
> > +			c = c_i;
> > +			iio_device_get(c->indio_dev);
> > +			break;
> > +		}
> >  	}  
> 
> ...
> 
> > -	channel = kzalloc(sizeof(*channel), GFP_KERNEL);
> > +	struct iio_channel *channel __free(kfree) = kzalloc(sizeof(*channel),
> > +							    GFP_KERNEL);  
> 
> Indentation?
> 
> ...
> 
> > -error_no_chan:
> > -	kfree(channel);
> >  error_no_mem:
> >  	iio_device_put(c->indio_dev);
> >  	return ERR_PTR(err);  
> 
> Effectively you move kfree after device put, would it be a problem?
It's not immediately obvious what that put pairs with so we should probably
address that a bit more clearly anyway - but the change should be safe.

> 
> ...
> 
> >  struct iio_channel *iio_channel_get_all(struct device *dev)
> >  {
> >  	const char *name;
> > -	struct iio_channel *chans;
> > +	struct iio_channel *fw_chans;
> >  	struct iio_map_internal *c = NULL;  
> 
> Move it here for better ordering?
Trivial, but meh, I'm tweaking anyway so done.
> 
> >  	int nummaps = 0;
> >  	int mapind = 0;  
> 
> ...
> 
> > -	chans = fwnode_iio_channel_get_all(dev);
> > +	fw_chans = fwnode_iio_channel_get_all(dev);  
> 
> I would move it before conditional...
> 
> >  	/*
> >  	 * We only want to carry on if the error is -ENODEV.  Anything else
> >  	 * should be reported up the stack.
> >  	 */
> > -	if (!IS_ERR(chans) || PTR_ERR(chans) != -ENODEV)
> > -		return chans;  
> 
> ...here.
> 
> > +	if (!IS_ERR(fw_chans) || PTR_ERR(fw_chans) != -ENODEV)
> > +		return fw_chans;  
> 
> ...
> 
> > +	struct iio_channel *chans __free(kfree) = kcalloc(nummaps + 1,
> > +							  sizeof(*chans),
> > +							  GFP_KERNEL);  
> 
> Indentation?
> 
> > +	if (!chans)
> > +		return ERR_PTR(-ENOMEM);  
> 