| Message ID | 20200410141729.82834-4-alexandru.ardelean@analog.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iio: core,buffer: re-organize chardev creation | expand |
On Fri, 10 Apr 2020 17:17:27 +0300 Alexandru Ardelean <alexandru.ardelean@analog.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@analog.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;
On Mon, 2020-04-13 at 16:58 +0100, Jonathan Cameron wrote: > [External] > > On Fri, 10 Apr 2020 17:17:27 +0300 > Alexandru Ardelean <alexandru.ardelean@analog.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@analog.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? I like it. Will create a patch for that. I'll see if I can make it neat into this series somehow. Otherwise, would it be ok to do it on top of this series? I currently don't have a clear idea of how clean the patchset would look. > > 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;
On Tue, 14 Apr 2020 05:43:40 +0000 "Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote: > On Mon, 2020-04-13 at 16:58 +0100, Jonathan Cameron wrote: > > [External] > > > > On Fri, 10 Apr 2020 17:17:27 +0300 > > Alexandru Ardelean <alexandru.ardelean@analog.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@analog.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? > > I like it. > Will create a patch for that. > > I'll see if I can make it neat into this series somehow. > Otherwise, would it be ok to do it on top of this series? > I currently don't have a clear idea of how clean the patchset would look. Ideally as part of this series. I don't 'think' it should be too bad but if it proves horrible I'll cope with it as patches on top of this series. Jonathan > > > > > > 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;
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; + + 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;
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@analog.com> --- 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(-)