Date:   Mon, 13 Apr 2020 16:58:16 +0100
From:   Jonathan Cameron <jic23@...nel.org>
To:     Alexandru Ardelean <alexandru.ardelean@...log.com>
Cc:     <linux-iio@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <lars@...afoo.de>
Subject: Re: [PATCH v3 3/5] iio: buffer: move iio buffer chrdev in
 industrialio-buffer.c

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

> This change makes the first jump to move the buffer chardev from indio_dev
> to the buffer attached to indio_dev (i.e. indio_dev->buffer).
> 
> This requires that some functions that are shared between
> 'industrialio-core.c' & industrialio-buffer.c be re-shuffled.
> The 'iio_buffer_fileops' is now moved to 'industrialio-buffer.c'. It's also
> more appropriate to have it here, but the old 'legacy' iio_ioctl() must be
> passed to it.
> 
> What happens now is:
> * if IS_ENABLED(CONFIG_IIO_BUFFER) and 'indio_dev->buffer != NULL' the
>   indio_dev->buffer->chrdev will be initialized (as the old legacy style
>   chardev)
> * if -ENOTSUPP is returned for either of the conditions above (not being
>   met), the chardev will be created directly via 'indio_dev->chrdev', same
>   as before; a new field is required now 'indio_dev->chrdev_initialized' to
>   mark it true, so that 'indio_dev->chrdev' gets deleted if initialized;
> 
> 'indio_dev->chrdev_initialized' is of type 'int', because recently
> checkpatch complains that 'bool' types on structs can cause alignment
> issues.
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@...log.com>

I wonder if it would be possible to make the ioctl mess a bit cleaner.
Have the buffer ioctl function as here, but have that call a generic
iio_device_ioctl function in industrialio-core.c which can in theory
handle 'device wide' ioctls.  Then have the events code register into
that as a consumer of a particular device wide ioctl.

That way the layering is a violated a little less.

What do you think?

Jonathan

> ---
>  drivers/iio/iio_core.h            |  25 ++++---
>  drivers/iio/industrialio-buffer.c | 119 ++++++++++++++++++++++++++++--
>  drivers/iio/industrialio-core.c   |  59 ++++++++-------
>  include/linux/iio/buffer_impl.h   |   7 ++
>  include/linux/iio/iio.h           |   2 +
>  5 files changed, 170 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/iio/iio_core.h b/drivers/iio/iio_core.h
> index fd9a5f1d5e51..4bdadeac2710 100644
> --- a/drivers/iio/iio_core.h
> +++ b/drivers/iio/iio_core.h
> @@ -40,24 +40,31 @@ ssize_t iio_format_value(char *buf, unsigned int type, int size, int *vals);
>  #ifdef CONFIG_IIO_BUFFER
>  struct poll_table_struct;
>  
> -__poll_t iio_buffer_poll(struct file *filp,
> -			     struct poll_table_struct *wait);
> -ssize_t iio_buffer_read_outer(struct file *filp, char __user *buf,
> -			      size_t n, loff_t *f_ps);
> +long iio_device_event_ioctl(struct iio_dev *indio_dev, struct file *filp,
> +			    unsigned int cmd, unsigned long arg);
> +
> +int iio_device_buffers_init(struct iio_dev *indio_dev, struct module *this_mod);
> +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);
>  
> -#define iio_buffer_poll_addr (&iio_buffer_poll)
> -#define iio_buffer_read_outer_addr (&iio_buffer_read_outer)
> -
>  void iio_disable_all_buffers(struct iio_dev *indio_dev);
>  void iio_buffer_wakeup_poll(struct iio_dev *indio_dev);
>  
>  #else
>  
> -#define iio_buffer_poll_addr NULL
> -#define iio_buffer_read_outer_addr NULL
> +static inline int iio_device_buffers_init(struct iio_dev *indio_dev,
> +					  struct module *this_mod)
> +{
> +	return -ENOTSUPP;
> +}
> +
> +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)
>  {
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index f9ffc7762f6c..4b5c3baadaab 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -99,11 +99,11 @@ static bool iio_buffer_ready(struct iio_dev *indio_dev, struct iio_buffer *buf,
>   * Return: negative values corresponding to error codes or ret != 0
>   *	   for ending the reading activity
>   **/
> -ssize_t iio_buffer_read_outer(struct file *filp, char __user *buf,
> -			      size_t n, loff_t *f_ps)
> +static ssize_t iio_buffer_read_outer(struct file *filp, char __user *buf,
> +				     size_t n, loff_t *f_ps)
>  {
> -	struct iio_dev *indio_dev = filp->private_data;
> -	struct iio_buffer *rb = indio_dev->buffer;
> +	struct iio_buffer *rb = filp->private_data;
> +	struct iio_dev *indio_dev = rb->indio_dev;
>  	DEFINE_WAIT_FUNC(wait, woken_wake_function);
>  	size_t datum_size;
>  	size_t to_wait;
> @@ -165,11 +165,11 @@ ssize_t iio_buffer_read_outer(struct file *filp, char __user *buf,
>   * Return: (EPOLLIN | EPOLLRDNORM) if data is available for reading
>   *	   or 0 for other cases
>   */
> -__poll_t iio_buffer_poll(struct file *filp,
> +static __poll_t iio_buffer_poll(struct file *filp,
>  			     struct poll_table_struct *wait)
>  {
> -	struct iio_dev *indio_dev = filp->private_data;
> -	struct iio_buffer *rb = indio_dev->buffer;
> +	struct iio_buffer *rb = filp->private_data;
> +	struct iio_dev *indio_dev = rb->indio_dev;
>  
>  	if (!indio_dev->info || rb == NULL)
>  		return 0;
> @@ -180,6 +180,48 @@ __poll_t iio_buffer_poll(struct file *filp,
>  	return 0;
>  }
>  
> +/**
> + * iio_buffer_chrdev_open() - chrdev file open for buffer access
> + * @inode:	Inode structure for identifying the device in the file system
> + * @filp:	File structure for iio device used to keep and later access
> + *		private data
> + *
> + * Return: 0 on success or -EBUSY if the device is already opened
> + **/
> +static int iio_buffer_chrdev_open(struct inode *inode, struct file *filp)
> +{
> +	struct iio_buffer *buffer = container_of(inode->i_cdev,
> +						 struct iio_buffer, chrdev);
> +
> +	if (test_and_set_bit(IIO_BUSY_BIT_POS, &buffer->file_ops_flags))
> +		return -EBUSY;
> +
> +	iio_buffer_get(buffer);
> +
> +	filp->private_data = buffer;
> +
> +	return 0;
> +}
> +
> +/**
> + * iio_buffer_chrdev_release() - chrdev file close for buffer access
> + * @inode:	Inode structure pointer for the char device
> + * @filp:	File structure pointer for the char device
> + *
> + * Return: 0 for successful release
> + */
> +static int iio_buffer_chrdev_release(struct inode *inode, struct file *filp)
> +{
> +	struct iio_buffer *buffer = container_of(inode->i_cdev,
> +						 struct iio_buffer, chrdev);
> +
> +	clear_bit(IIO_BUSY_BIT_POS, &buffer->file_ops_flags);
> +
> +	iio_buffer_put(buffer);
> +
> +	return 0;
> +}
> +
>  /**
>   * iio_buffer_wakeup_poll - Wakes up the buffer waitqueue
>   * @indio_dev: The IIO device
> @@ -1121,6 +1163,14 @@ void iio_disable_all_buffers(struct iio_dev *indio_dev)
>  	iio_buffer_deactivate_all(indio_dev);
>  }
>  
> +long iio_buffer_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
> +{
> +	struct iio_buffer *buffer = filep->private_data;
> +	struct iio_dev *indio_dev = buffer->indio_dev;
> +
> +	return iio_device_event_ioctl(indio_dev, filep, cmd, arg);
> +}
> +
>  static ssize_t iio_buffer_store_enable(struct device *dev,
>  				       struct device_attribute *attr,
>  				       const char *buf,
> @@ -1356,6 +1406,61 @@ void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev)
>  	iio_free_chan_devattr_list(&indio_dev->buffer->scan_el_dev_attr_list);
>  }
>  
> +static const struct file_operations iio_buffer_fileops = {
> +	.read = iio_buffer_read_outer,
> +	.release = iio_buffer_chrdev_release,
> +	.open = iio_buffer_chrdev_open,
> +	.poll = iio_buffer_poll,
> +	.owner = THIS_MODULE,
> +	.llseek = noop_llseek,
> +	.unlocked_ioctl = iio_buffer_ioctl,
> +	.compat_ioctl = compat_ptr_ioctl,
> +};
> +
> +int iio_device_buffers_init(struct iio_dev *indio_dev, struct module *this_mod)
> +{
> +	struct iio_buffer *buffer = indio_dev->buffer;
> +	int ret;
> +
> +	if (!buffer)
> +		return -ENOTSUPP;
> +
> +	cdev_init(&buffer->chrdev, &iio_buffer_fileops);
> +
> +	buffer->chrdev.owner = this_mod;
> +
> +	ret = cdev_device_add(&buffer->chrdev, &indio_dev->dev);
> +	if (ret)
> +		return ret;
> +
> +	iio_device_get(indio_dev);
> +	iio_buffer_get(buffer);
> +
> +	return 0;
> +}
> +
> +void iio_device_buffers_put(struct iio_dev *indio_dev)
> +{
> +	struct iio_buffer *buffer = indio_dev->buffer;
> +
> +	if (!buffer)
> +		return;
> +
> +	iio_buffer_put(buffer);
> +}
> +
> +void iio_device_buffers_uninit(struct iio_dev *indio_dev)
> +{
> +	struct iio_buffer *buffer = indio_dev->buffer;
> +
> +	if (!buffer)
> +		return;
> +
> +	cdev_device_del(&buffer->chrdev, &indio_dev->dev);
> +	iio_buffer_put(buffer);
> +	iio_device_put(indio_dev);
> +}
> +
>  /**
>   * iio_validate_scan_mask_onehot() - Validates that exactly one channel is selected
>   * @indio_dev: the iio device
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index c8c074602709..2158aeab0bd2 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -1475,7 +1475,7 @@ static void iio_dev_release(struct device *device)
>  	iio_device_unregister_eventset(indio_dev);
>  	iio_device_unregister_sysfs(indio_dev);
>  
> -	iio_buffer_put(indio_dev->buffer);
> +	iio_device_buffers_put(indio_dev);
>  
>  	ida_simple_remove(&iio_ida, indio_dev->id);
>  	kfree(indio_dev);
> @@ -1610,7 +1610,7 @@ void devm_iio_device_free(struct device *dev, struct iio_dev *iio_dev)
>  EXPORT_SYMBOL_GPL(devm_iio_device_free);
>  
>  /**
> - * iio_chrdev_open() - chrdev file open for buffer access and ioctls
> + * iio_chrdev_open() - chrdev file open for event ioctls
>   * @inode:	Inode structure for identifying the device in the file system
>   * @filp:	File structure for iio device used to keep and later access
>   *		private data
> @@ -1633,7 +1633,7 @@ static int iio_chrdev_open(struct inode *inode, struct file *filp)
>  }
>  
>  /**
> - * iio_chrdev_release() - chrdev file close buffer access and ioctls
> + * iio_chrdev_release() - chrdev file close for event ioctls
>   * @inode:	Inode structure pointer for the char device
>   * @filp:	File structure pointer for the char device
>   *
> @@ -1649,11 +1649,9 @@ static int iio_chrdev_release(struct inode *inode, struct file *filp)
>  	return 0;
>  }
>  
> -/* Somewhat of a cross file organization violation - ioctls here are actually
> - * event related */
> -static long iio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> +long iio_device_event_ioctl(struct iio_dev *indio_dev, struct file *filp,
> +			    unsigned int cmd, unsigned long arg)
>  {
> -	struct iio_dev *indio_dev = filp->private_data;
>  	int __user *ip = (int __user *)arg;
>  	int fd;
>  
> @@ -1671,16 +1669,15 @@ static long iio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  	return -EINVAL;
>  }
>  
> -static const struct file_operations iio_buffer_fileops = {
> -	.read = iio_buffer_read_outer_addr,
> -	.release = iio_chrdev_release,
> -	.open = iio_chrdev_open,
> -	.poll = iio_buffer_poll_addr,
> -	.owner = THIS_MODULE,
> -	.llseek = noop_llseek,
> -	.unlocked_ioctl = iio_ioctl,
> -	.compat_ioctl = compat_ptr_ioctl,
> -};
> +/* Somewhat of a cross file organization violation - ioctls here are actually
> + * event related */
> +static long iio_event_ioctl_wrapper(struct file *filp, unsigned int cmd,
> +				    unsigned long arg)
> +{
> +	struct iio_dev *indio_dev = filp->private_data;
> +

Hmm. I wonder if we keep a level of indirection in here.  So actually call
something like: 'iio_device_ioctl' in industrialio-core.c and have the
event registration register a callback with the core.  Thus no events and
nothing is registered so that core function simply returns an error
- right now I suspect it returns a chardev that is completely useless
(if no events supported by the device).

> +	return iio_device_event_ioctl(indio_dev, filp, cmd, arg);
> +}
>  
>  static int iio_check_unique_scan_index(struct iio_dev *indio_dev)
>  {
> @@ -1712,7 +1709,7 @@ static const struct file_operations iio_event_fileops = {
>  	.open = iio_chrdev_open,
>  	.owner = THIS_MODULE,
>  	.llseek = noop_llseek,
> -	.unlocked_ioctl = iio_ioctl,
> +	.unlocked_ioctl = iio_event_ioctl_wrapper,
>  	.compat_ioctl = compat_ptr_ioctl,
>  };
>  
> @@ -1766,16 +1763,21 @@ int __iio_device_register(struct iio_dev *indio_dev, struct module *this_mod)
>  		indio_dev->setup_ops == NULL)
>  		indio_dev->setup_ops = &noop_ring_setup_ops;
>  
> -	if (indio_dev->buffer)
> -		cdev_init(&indio_dev->chrdev, &iio_buffer_fileops);
> -	else
> +	ret = iio_device_buffers_init(indio_dev, this_mod);
> +	if (ret) {
> +		if (ret != -ENOTSUPP)
> +			goto error_unreg_eventset;
> +
>  		cdev_init(&indio_dev->chrdev, &iio_event_fileops);
>  
> -	indio_dev->chrdev.owner = this_mod;
> +		indio_dev->chrdev.owner = this_mod;
>  
> -	ret = cdev_device_add(&indio_dev->chrdev, &indio_dev->dev);
> -	if (ret < 0)
> -		goto error_unreg_eventset;
> +		ret = cdev_device_add(&indio_dev->chrdev, &indio_dev->dev);
> +		if (ret < 0)
> +			goto error_unreg_eventset;
> +
> +		indio_dev->chrdev_initialized = true;
> +	}
>  
>  	return 0;
>  
> @@ -1797,7 +1799,12 @@ EXPORT_SYMBOL(__iio_device_register);
>   **/
>  void iio_device_unregister(struct iio_dev *indio_dev)
>  {
> -	cdev_device_del(&indio_dev->chrdev, &indio_dev->dev);
> +	if (indio_dev->chrdev_initialized)
> +		cdev_device_del(&indio_dev->chrdev, &indio_dev->dev);
> +
> +	indio_dev->chrdev_initialized = false;
> +
> +	iio_device_buffers_uninit(indio_dev);
>  
>  	mutex_lock(&indio_dev->info_exist_lock);
>  
> diff --git a/include/linux/iio/buffer_impl.h b/include/linux/iio/buffer_impl.h
> index 8fb92250a190..b4a55c3f556b 100644
> --- a/include/linux/iio/buffer_impl.h
> +++ b/include/linux/iio/buffer_impl.h
> @@ -1,6 +1,7 @@
>  /* SPDX-License-Identifier: GPL-2.0 */
>  #ifndef _IIO_BUFFER_GENERIC_IMPL_H_
>  #define _IIO_BUFFER_GENERIC_IMPL_H_
> +#include <linux/cdev.h>
>  #include <linux/sysfs.h>
>  #include <linux/kref.h>
>  
> @@ -72,6 +73,12 @@ struct iio_buffer {
>  	/** @indio_dev: IIO device to which this buffer belongs to. */
>  	struct iio_dev *indio_dev;
>  
> +	/** @chrdev: associated character device. */
> +	struct cdev chrdev;
> +
> +	/** @file_ops_flags: file ops related flags including busy flag. */
> +	unsigned long file_ops_flags;
> +
>  	/** @length: Number of datums in buffer. */
>  	unsigned int length;
>  
> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> index e975020abaa6..e93497f483f7 100644
> --- a/include/linux/iio/iio.h
> +++ b/include/linux/iio/iio.h
> @@ -517,6 +517,7 @@ struct iio_buffer_setup_ops {
>   * @setup_ops:		[DRIVER] callbacks to call before and after buffer
>   *			enable/disable
>   * @chrdev:		[INTERN] associated character device
> + * @chrdev_initialized:	[INTERN] true if @chrdev device has been initialized
>   * @groups:		[INTERN] attribute groups
>   * @groupcounter:	[INTERN] index of next attribute group
>   * @flags:		[INTERN] file ops related flags including busy flag.
> @@ -560,6 +561,7 @@ struct iio_dev {
>  	struct mutex			info_exist_lock;
>  	const struct iio_buffer_setup_ops	*setup_ops;
>  	struct cdev			chrdev;
> +	int				chrdev_initialized;
>  #define IIO_MAX_GROUPS 6
>  	const struct attribute_group	*groups[IIO_MAX_GROUPS + 1];
>  	int				groupcounter;

Powered by blists - more mailing lists