Re: [PATCH v3 4/5] iio: buffer: move sysfs alloc/free in industrialio-buffer.c

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 10 Apr 2020 17:17:28 +0300
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> Since we now have the 'iio_device_buffers_{un}init()' entry-points into the
> industrialio-buffer.c we can now move the sysfs alloc & free in there as
> part of that init/uninit.
> 
> This changes the order of iio_device_register()/iio_device_unregister() a
> bit, but overall this shouldn't matter.
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
Seems fine at first glance.

Thanks,

Jonathan

> ---
>  drivers/iio/iio_core.h            | 10 ----------
>  drivers/iio/industrialio-buffer.c | 29 +++++++++++++++--------------
>  drivers/iio/industrialio-core.c   | 14 +-------------
>  3 files changed, 16 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/iio/iio_core.h b/drivers/iio/iio_core.h
> index 4bdadeac2710..1d69071e1426 100644
> --- a/drivers/iio/iio_core.h
> +++ b/drivers/iio/iio_core.h
> @@ -48,9 +48,6 @@ void iio_device_buffers_uninit(struct iio_dev *indio_dev);
>  
>  void iio_device_buffers_put(struct iio_dev *indio_dev);
>  
> -int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev);
> -void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev);
> -
>  void iio_disable_all_buffers(struct iio_dev *indio_dev);
>  void iio_buffer_wakeup_poll(struct iio_dev *indio_dev);
>  
> @@ -66,13 +63,6 @@ static inline void iio_device_buffers_uninit(struct iio_dev *indio_dev) {}
>  
>  static inline void iio_device_buffers_put(struct iio_dev *indio_dev) {}
>  
> -static inline int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
> -{
> -	return 0;
> -}
> -
> -static inline void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev) {}
> -
>  static inline void iio_disable_all_buffers(struct iio_dev *indio_dev) {}
>  static inline void iio_buffer_wakeup_poll(struct iio_dev *indio_dev) {}
>  
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index 4b5c3baadaab..8ab089a9c3bc 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -1284,11 +1284,11 @@ static struct attribute *iio_buffer_attrs[] = {
>  	&dev_attr_data_available.attr,
>  };
>  
> -int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
> +static int iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer)
>  {
> +	struct iio_dev *indio_dev = buffer->indio_dev;
>  	struct iio_dev_attr *p;
>  	struct attribute **attr;
> -	struct iio_buffer *buffer = indio_dev->buffer;
>  	int ret, i, attrn, attrcount, attrcount_orig = 0;
>  	const struct iio_chan_spec *channels;
>  
> @@ -1301,9 +1301,6 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
>  		indio_dev->masklength = ml;
>  	}
>  
> -	if (!buffer)
> -		return 0;
> -
>  	attrcount = 0;
>  	if (buffer->attrs) {
>  		while (buffer->attrs[attrcount] != NULL)
> @@ -1395,15 +1392,12 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
>  	return ret;
>  }
>  
> -void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev)
> +static void iio_buffer_free_sysfs_and_mask(struct iio_buffer *buffer)
>  {
> -	if (!indio_dev->buffer)
> -		return;
> -
> -	bitmap_free(indio_dev->buffer->scan_mask);
> -	kfree(indio_dev->buffer->buffer_group.attrs);
> -	kfree(indio_dev->buffer->scan_el_group.attrs);
> -	iio_free_chan_devattr_list(&indio_dev->buffer->scan_el_dev_attr_list);
> +	bitmap_free(buffer->scan_mask);
> +	kfree(buffer->buffer_group.attrs);
> +	kfree(buffer->scan_el_group.attrs);
> +	iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list);
>  }
>  
>  static const struct file_operations iio_buffer_fileops = {
> @@ -1425,13 +1419,19 @@ int iio_device_buffers_init(struct iio_dev *indio_dev, struct module *this_mod)
>  	if (!buffer)
>  		return -ENOTSUPP;
>  
> +	ret = iio_buffer_alloc_sysfs_and_mask(buffer);
> +	if (ret)
> +		return ret;
> +
>  	cdev_init(&buffer->chrdev, &iio_buffer_fileops);
>  
>  	buffer->chrdev.owner = this_mod;
>  
>  	ret = cdev_device_add(&buffer->chrdev, &indio_dev->dev);
> -	if (ret)
> +	if (ret) {
> +		iio_buffer_free_sysfs_and_mask(buffer);
>  		return ret;
> +	}
>  
>  	iio_device_get(indio_dev);
>  	iio_buffer_get(buffer);
> @@ -1457,6 +1457,7 @@ void iio_device_buffers_uninit(struct iio_dev *indio_dev)
>  		return;
>  
>  	cdev_device_del(&buffer->chrdev, &indio_dev->dev);
> +	iio_buffer_free_sysfs_and_mask(buffer);
>  	iio_buffer_put(buffer);
>  	iio_device_put(indio_dev);
>  }
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index 2158aeab0bd2..794aaa4be832 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -1737,18 +1737,11 @@ int __iio_device_register(struct iio_dev *indio_dev, struct module *this_mod)
>  
>  	iio_device_register_debugfs(indio_dev);
>  
> -	ret = iio_buffer_alloc_sysfs_and_mask(indio_dev);
> -	if (ret) {
> -		dev_err(indio_dev->dev.parent,
> -			"Failed to create buffer sysfs interfaces\n");
> -		goto error_unreg_debugfs;
> -	}
> -
>  	ret = iio_device_register_sysfs(indio_dev);
>  	if (ret) {
>  		dev_err(indio_dev->dev.parent,
>  			"Failed to register sysfs interfaces\n");
> -		goto error_buffer_free_sysfs;
> +		goto error_free_sysfs;
>  	}
>  	ret = iio_device_register_eventset(indio_dev);
>  	if (ret) {
> @@ -1785,9 +1778,6 @@ int __iio_device_register(struct iio_dev *indio_dev, struct module *this_mod)
>  	iio_device_unregister_eventset(indio_dev);
>  error_free_sysfs:
>  	iio_device_unregister_sysfs(indio_dev);
> -error_buffer_free_sysfs:
> -	iio_buffer_free_sysfs_and_mask(indio_dev);
> -error_unreg_debugfs:
>  	iio_device_unregister_debugfs(indio_dev);
>  	return ret;
>  }
> @@ -1818,8 +1808,6 @@ void iio_device_unregister(struct iio_dev *indio_dev)
>  	iio_buffer_wakeup_poll(indio_dev);
>  
>  	mutex_unlock(&indio_dev->info_exist_lock);
> -
> -	iio_buffer_free_sysfs_and_mask(indio_dev);
>  }
>  EXPORT_SYMBOL(iio_device_unregister);
>