On Sun, 2020-04-26 at 11:00 +0100, Jonathan Cameron wrote: > [External] > > On Sun, 26 Apr 2020 10:38:15 +0300 > Alexandru Ardelean <alexandru.ardelean@xxxxxxxxxx> wrote: > > > This patch moves the chardev creation into industrialio-buffer.c & > > industrialio-event.c. The move has to be in a single step (which makes the > > patch a bit big), in order to pass the reference of the chardev to > > 'indio_dev->chrdev'. > > > > For structure purposes, industrialio-core.c should be the place where > > cdev_device_add()/device_add() gets called, and the deletion function as > > well. > > > > What happens after this patch is: > > - 'indio_dev->chrdev' is converted to a pointer > > - if there is an IIO buffer, iio_device_buffer_attach_chrdev() will attach > > it's chardev to 'indio_chrdev' > > - if it doesn't, the event interface will attach a reference to it's > > chardev (if it is instantiated) via iio_device_event_attach_chrdev() > > > > That way, the control of the 'legacy' chardev is still visible in > > 'industrialio-core.c'. So, each logic file (for buffers & events) shouldn't > > hide things too much away from the core file. > > > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@xxxxxxxxxx> > > A few really minor suggestions inline. At least half of them are tidying up > things whilst we are here that were ugly in the code we are duplicating. > > Looks good to me. However, I'd like more eyes on this set before I apply it > so will give it at least a week or two on the list! > > Jonathan > > > --- > > drivers/iio/iio_core.h | 16 ++--- > > drivers/iio/industrialio-buffer.c | 90 +++++++++++++++++++++++-- > > drivers/iio/industrialio-core.c | 105 ++++-------------------------- > > drivers/iio/industrialio-event.c | 98 +++++++++++++++++++++++++++- > > include/linux/iio/buffer_impl.h | 7 ++ > > include/linux/iio/iio.h | 6 +- > > 6 files changed, 208 insertions(+), 114 deletions(-) > > > > diff --git a/drivers/iio/iio_core.h b/drivers/iio/iio_core.h > > index 39ec0344fb68..a527a66be9e5 100644 > > --- a/drivers/iio/iio_core.h > > +++ b/drivers/iio/iio_core.h > > @@ -40,17 +40,14 @@ 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); > > + > > +void iio_device_buffer_attach_chrdev(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_device_buffers_put(struct iio_dev *indio_dev); > > > > void iio_disable_all_buffers(struct iio_dev *indio_dev); > > @@ -58,8 +55,7 @@ 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 void iio_device_buffer_attach_chrdev(struct iio_dev > > *indio_dev) {} > > > > static inline int iio_buffer_alloc_sysfs_and_mask(struct iio_dev > > *indio_dev) > > { > > @@ -77,8 +73,8 @@ static inline void iio_buffer_wakeup_poll(struct iio_dev > > *indio_dev) {} > > > > int iio_device_register_eventset(struct iio_dev *indio_dev); > > void iio_device_unregister_eventset(struct iio_dev *indio_dev); > > +void iio_device_event_attach_chrdev(struct iio_dev *indio_dev); > > void iio_device_wakeup_eventset(struct iio_dev *indio_dev); > > -int iio_event_getfd(struct iio_dev *indio_dev); > > > > struct iio_event_interface; > > bool iio_event_enabled(const struct iio_event_interface *ev_int); > > diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio- > > buffer.c > > index a66d3fbc2905..f5a975079bf4 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 > > @@ -1129,6 +1171,17 @@ void iio_disable_all_buffers(struct iio_dev > > *indio_dev) > > iio_buffer_deactivate_all(indio_dev); > > } > > > > +static long iio_buffer_ioctl(struct file *filep, unsigned int cmd, > > + unsigned long arg) > > +{ > > + struct iio_buffer *buffer = filep->private_data; > > + > > + if (!buffer || !buffer->access) > > + return -ENODEV; > > + > > + return iio_device_event_ioctl(buffer->indio_dev, filep, cmd, arg); > > +} > > + > > static ssize_t iio_buffer_store_enable(struct device *dev, > > struct device_attribute *attr, > > const char *buf, > > @@ -1355,6 +1408,29 @@ void iio_buffer_free_sysfs_and_mask(struct iio_dev > > *indio_dev) > > iio_free_chan_devattr_list(&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, > > +}; > > + > > +void iio_device_buffer_attach_chrdev(struct iio_dev *indio_dev) > > +{ > > + struct iio_buffer *buffer = indio_dev->buffer; > > + > > + if (!buffer) > > + return; > > + > > + cdev_init(&buffer->chrdev, &iio_buffer_fileops); > > + > > + indio_dev->chrdev = &buffer->chrdev; > > +} > > + > > void iio_device_buffers_put(struct iio_dev *indio_dev) > > { > > struct iio_buffer *buffer = indio_dev->buffer; > > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio- > > core.c > > index a65022396329..aec585cc8453 100644 > > --- a/drivers/iio/industrialio-core.c > > +++ b/drivers/iio/industrialio-core.c > > @@ -1584,79 +1584,6 @@ struct iio_dev *devm_iio_device_alloc(struct device > > *dev, int sizeof_priv) > > } > > EXPORT_SYMBOL_GPL(devm_iio_device_alloc); > > > > -/** > > - * iio_chrdev_open() - chrdev file open for buffer access and 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 > > - * > > - * Return: 0 on success or -EBUSY if the device is already opened > > - **/ > > -static int iio_chrdev_open(struct inode *inode, struct file *filp) > > -{ > > - struct iio_dev *indio_dev = container_of(inode->i_cdev, > > - struct iio_dev, chrdev); > > - > > - if (test_and_set_bit(IIO_BUSY_BIT_POS, &indio_dev->flags)) > > - return -EBUSY; > > - > > - iio_device_get(indio_dev); > > - > > - filp->private_data = indio_dev; > > - > > - return 0; > > -} > > - > > -/** > > - * iio_chrdev_release() - chrdev file close buffer access and ioctls > > - * @inode: Inode structure pointer for the char device > > - * @filp: File structure pointer for the char device > > - * > > - * Return: 0 for successful release > > - */ > > -static int iio_chrdev_release(struct inode *inode, struct file *filp) > > -{ > > - struct iio_dev *indio_dev = container_of(inode->i_cdev, > > - struct iio_dev, chrdev); > > - clear_bit(IIO_BUSY_BIT_POS, &indio_dev->flags); > > - iio_device_put(indio_dev); > > - > > - 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) > > -{ > > - struct iio_dev *indio_dev = filp->private_data; > > - int __user *ip = (int __user *)arg; > > - int fd; > > - > > - if (!indio_dev->info) > > - return -ENODEV; > > - > > - if (cmd == IIO_GET_EVENT_FD_IOCTL) { > > - fd = iio_event_getfd(indio_dev); > > - if (fd < 0) > > - return fd; > > - if (copy_to_user(ip, &fd, sizeof(fd))) > > - return -EFAULT; > > - return 0; > > - } > > - 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, > > -}; > > - > > static int iio_check_unique_scan_index(struct iio_dev *indio_dev) > > { > > int i, j; > > @@ -1682,15 +1609,6 @@ static int iio_check_unique_scan_index(struct iio_dev > > *indio_dev) > > > > static const struct iio_buffer_setup_ops noop_ring_setup_ops; > > > > -static const struct file_operations iio_event_fileops = { > > - .release = iio_chrdev_release, > > - .open = iio_chrdev_open, > > - .owner = THIS_MODULE, > > - .llseek = noop_llseek, > > - .unlocked_ioctl = iio_ioctl, > > - .compat_ioctl = compat_ptr_ioctl, > > -}; > > - > > int __iio_device_register(struct iio_dev *indio_dev, struct module > > *this_mod) > > { > > int ret; > > @@ -1741,16 +1659,17 @@ 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 if (indio_dev->event_interface) > > - cdev_init(&indio_dev->chrdev, &iio_event_fileops); > > + iio_device_buffer_attach_chrdev(indio_dev); > > > > - indio_dev->chrdev.owner = this_mod; > > + /* no chrdev attached from buffer, we go with event-only chrdev */ > > + if (!indio_dev->chrdev) > > + iio_device_event_attach_chrdev(indio_dev); > > > > - if (indio_dev->buffer || indio_dev->event_interface) > > - ret = cdev_device_add(&indio_dev->chrdev, &indio_dev->dev); > > - else > > + if (indio_dev->chrdev) { > > + indio_dev->chrdev->owner = this_mod; > > + > > + ret = cdev_device_add(indio_dev->chrdev, &indio_dev->dev); > > + } else > > ret = device_add(&indio_dev->dev); > > > > if (ret < 0) > > @@ -1776,13 +1695,15 @@ EXPORT_SYMBOL(__iio_device_register); > > **/ > > void iio_device_unregister(struct iio_dev *indio_dev) > > { > > - if (indio_dev->buffer || indio_dev->event_interface) > > - cdev_device_del(&indio_dev->chrdev, &indio_dev->dev); > > + if (indio_dev->chrdev) > > + cdev_device_del(indio_dev->chrdev, &indio_dev->dev); > > else > > device_del(&indio_dev->dev); > > > > mutex_lock(&indio_dev->info_exist_lock); > > > > + indio_dev->chrdev = NULL; > > + > > Why? I can't immediately see the reason and I'm too lazy to figure it out > on a Sunday morning ;) Paranoid code. Initially I just thought I'd add it as a similar mechanism for 'indio_dev->info' being made NULL later. But I can't think of a good reason. I guess I added it in the middle of coding, without much thought. Especially, since there isn't any code to check this being NULL to do anything. Will remove it. 'indio_dev->info = NULL' should be enough. > > > iio_device_unregister_debugfs(indio_dev); > > > > iio_disable_all_buffers(indio_dev); > > diff --git a/drivers/iio/industrialio-event.c b/drivers/iio/industrialio- > > event.c > > index 5b17c92d3b50..0674b2117c98 100644 > > --- a/drivers/iio/industrialio-event.c > > +++ b/drivers/iio/industrialio-event.c > > @@ -30,6 +30,8 @@ > > * @flags: file operations related flags including busy flag. > > * @group: event interface sysfs attribute group > > * @read_lock: lock to protect kfifo read operations > > + * @chrdev: associated chardev for this event > > + * @indio_dev: IIO device to which this event interface belongs > > to > > */ > > struct iio_event_interface { > > wait_queue_head_t wait; > > @@ -39,6 +41,9 @@ struct iio_event_interface { > > unsigned long flags; > > struct attribute_group group; > > struct mutex read_lock; > > + > > + struct cdev chrdev; > > + struct iio_dev *indio_dev; > > }; > > > > bool iio_event_enabled(const struct iio_event_interface *ev_int) > > @@ -182,7 +187,7 @@ static const struct file_operations > > iio_event_chrdev_fileops = { > > .llseek = noop_llseek, > > }; > > > > -int iio_event_getfd(struct iio_dev *indio_dev) > > +static int iio_event_getfd(struct iio_dev *indio_dev) > > { > > struct iio_event_interface *ev_int = indio_dev->event_interface; > > int fd; > > @@ -215,6 +220,97 @@ int iio_event_getfd(struct iio_dev *indio_dev) > > return fd; > > } > > > > +/** > > + * 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 > > + * > > + * Return: 0 on success or -EBUSY if the device is already opened > > + **/ > > Don't we use the */ termination or kernel-doc through out IIO. Nice to be > consistent. Ah. I see we don't. Tidy that up whilst you are here :) ack > > > > +static int iio_chrdev_open(struct inode *inode, struct file *filp) > > +{ > > + struct iio_event_interface *ev = > > + container_of(inode->i_cdev, struct iio_event_interface, chrdev); > > + > > + if (test_and_set_bit(IIO_BUSY_BIT_POS, &ev->flags)) > > + return -EBUSY; > > + > > + iio_device_get(ev->indio_dev); > > + > > + filp->private_data = ev; > > + > > + return 0; > > +} > > + > > +/** > > + * 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 > > + * > > + * Return: 0 for successful release > > + */ > > +static int iio_chrdev_release(struct inode *inode, struct file *filp) > > +{ > > + struct iio_event_interface *ev = > > + container_of(inode->i_cdev, struct iio_event_interface, chrdev); > > + > > + clear_bit(IIO_BUSY_BIT_POS, &ev->flags); > > + iio_device_put(ev->indio_dev); > > + > > + return 0; > > +} > > + > > +long iio_device_event_ioctl(struct iio_dev *indio_dev, struct file *filp, > > + unsigned int cmd, unsigned long arg) > > +{ > > + int __user *ip = (int __user *)arg; > > + int fd; > > + > > Given we've has some confusion on this recently perhaps a note that > this is protection on driver 'going away'. We could even > add a little wrapper around it to make it explicit in the code. > What do you think? Maybe a comment is enough. Comment sounds good enough. This code should get called via 2 potential code-paths: 1 via events chardev & 1 via buffer chardev. > > > + if (!indio_dev->info) > > + return -ENODEV; > > + > > + if (cmd == IIO_GET_EVENT_FD_IOCTL) { > > + fd = iio_event_getfd(indio_dev); > > + if (fd < 0) > > + return fd; > > + if (copy_to_user(ip, &fd, sizeof(fd))) > > + return -EFAULT; > > + return 0; > > + } > > + return -EINVAL; > > +} > > + > > +static long iio_event_ioctl_wrapper(struct file *filp, unsigned int cmd, > > + unsigned long arg) > > +{ > > + struct iio_event_interface *ev = filp->private_data; > > + > > + return iio_device_event_ioctl(ev->indio_dev, filp, cmd, arg); > > +} > > + > > +static const struct file_operations iio_event_fileops = { > > + .release = iio_chrdev_release, > > + .open = iio_chrdev_open, > > + .owner = THIS_MODULE, > > As we are here. Might be nice to order this to match the file_operations > structure > ordering. Certainly seems odd to have .owner in the middle.. > lalala for where you might have copied this from ;) ack > > > + .llseek = noop_llseek, > > + .unlocked_ioctl = iio_event_ioctl_wrapper, > > + .compat_ioctl = compat_ptr_ioctl, > > +}; > > + > > +void iio_device_event_attach_chrdev(struct iio_dev *indio_dev) > > +{ > > + struct iio_event_interface *ev = indio_dev->event_interface; > > + > > + if (!ev) > > + return; > > + > > + cdev_init(&ev->chrdev, &iio_event_fileops); > > + > > + ev->indio_dev = indio_dev; > > + indio_dev->chrdev = &ev->chrdev; > > +} > > + > > static const char * const iio_ev_type_text[] = { > > [IIO_EV_TYPE_THRESH] = "thresh", > > [IIO_EV_TYPE_MAG] = "mag", > > diff --git a/include/linux/iio/buffer_impl.h > > b/include/linux/iio/buffer_impl.h > > index 67d73d465e02..46fc977deae3 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 5f9f439a4f01..52992be44e9e 100644 > > --- a/include/linux/iio/iio.h > > +++ b/include/linux/iio/iio.h > > @@ -516,10 +516,9 @@ struct iio_buffer_setup_ops { > > * @info_exist_lock: [INTERN] lock to prevent use during removal > > * @setup_ops: [DRIVER] callbacks to call before and after > > buffer > > * enable/disable > > - * @chrdev: [INTERN] associated character device > > + * @chrdev: [INTERN] reference to associated character > > device > > * @groups: [INTERN] attribute groups > > * @groupcounter: [INTERN] index of next attribute group > > - * @flags: [INTERN] file ops related flags including busy flag. > > * @debugfs_dentry: [INTERN] device specific debugfs dentry. > > * @cached_reg_addr: [INTERN] cached register address for debugfs > > reads. > > */ > > @@ -559,12 +558,11 @@ struct iio_dev { > > clockid_t clock_id; > > struct mutex info_exist_lock; > > const struct iio_buffer_setup_ops *setup_ops; > > - struct cdev chrdev; > > + struct cdev *chrdev; > > #define IIO_MAX_GROUPS 6 > > const struct attribute_group *groups[IIO_MAX_GROUPS + 1]; > > int groupcounter; > > > > - unsigned long flags; > > #if defined(CONFIG_DEBUG_FS) > > struct dentry *debugfs_dentry; > > unsigned cached_reg_addr;