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

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

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?

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

...

> +	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)

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

> +				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?

...

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

>  	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);

-- 
With Best Regards,
Andy Shevchenko