Re: [PATCH 1/1] IIO: Added write function in iio_buffer_fileops

[Lars-Peter Clausen]

On 08/13/2014 03:42 PM, Aniroop Mathur wrote:
> On Wed, Aug 13, 2014 at 2:38 PM, Lars-Peter Clausen <lars@xxxxxxxxxx> wrote:
> > On 08/13/2014 08:29 AM, a.mathur@xxxxxxxxxxx wrote:
> > > From: Aniroop Mathur <a.mathur@xxxxxxxxxxx>
> > >
> > > Earlier, user space can only read from iio device node but cannot write to
> > > it.
> > > This patch adds write function in iio buffer file operations,
> > > which will allow user-space applications/HAL to write the data
> > > to iio device node.
> > > So now there will be two way communication between IIO subsystem
> > > and user space. (userspace <--> kernel)
> > >
> > > It can be used by HAL or any user-space application which wants to
> > > write data to iio device node/buffer upon receiving some data from it.
> > > As an example,
> > > It is useful for iio device simulator application which need to record
> > > the data by reading from iio device node and replay the recorded data
> > > by writing back to iio device node.
> >
> > I'm not convinced that this is something that should be added to the kernel.
> > I'm wondering why can't this be done in userspace, e.g. by having a
> > simulator mode for the application or by using LD_PRELOAD. Having this in
> > userspace will be much more flexible and will be easier to implement
> > correctly and you'll most likely want to simulate more than just buffer
> > access, for example setting/getting properties of the device or channel. For
> > the libiio[1] we are planning to implement a test backend that when
> > activated will allow to simulate a whole device rather than just buffer
> > support.
> >
> > [1] https://github.com/analogdevicesinc/libiio
>
> In normal Input Subsystem, there is two way communication between
> kernel and userpace. It has both read and write functionality. :)
> Why there is only one way communication in case of IIO ?

I've not seen a compelling reason yet why this must be implemented in kernel 
space. In my opinion, as outlined above, userspace if both easier and more 
flexible.

> For Input devices, I completed simulation just by reading and writing at
> input device node /dev/input/eventX.
> But for IIO devices, I am stuck as there is no write function available
> for iio device node /dev/iio:device0.
>
> As per my understanding, if we do the simulation in userspace
> then we need to create one more buffer in userpace. This will lead to
> extra memory usage. With write functionality in IIO just like
> in Input subsystem, we can avoid extra memory space. :)

No, you don't actually have to create a additional buffer. Just return the 
data that you'd otherwise have passed to write().

> > > Signed-off-by: Aniroop Mathur <a.mathur@xxxxxxxxxxx>
> > > ---
> > >    drivers/iio/iio_core.h            |    5 ++++-
> > >    drivers/iio/industrialio-buffer.c |   34
> > > ++++++++++++++++++++++++++++++++++
> > >    drivers/iio/industrialio-core.c   |    1 +
> > >    3 files changed, 39 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/iio/iio_core.h b/drivers/iio/iio_core.h
> > > index 5f0ea77..ba3fe53 100644
> > > --- a/drivers/iio/iio_core.h
> > > +++ b/drivers/iio/iio_core.h
> > > @@ -47,10 +47,12 @@ unsigned int iio_buffer_poll(struct file *filp,
> > >                               struct poll_table_struct *wait);
> > >    ssize_t iio_buffer_read_first_n_outer(struct file *filp, char __user
> > > *buf,
> > >                                        size_t n, loff_t *f_ps);
> > > -
> > > +ssize_t iio_buffer_write_first_n_outer(struct file *filp,
> > > +                            const char __user *buf, size_t n, loff_t
> > > *f_ps);
> > >
> > >    #define iio_buffer_poll_addr (&iio_buffer_poll)
> > >    #define iio_buffer_read_first_n_outer_addr
> > > (&iio_buffer_read_first_n_outer)
> > > +#define iio_buffer_write_first_n_outer_addr
> > > (&iio_buffer_write_first_n_outer)
> > >
> > >    void iio_disable_all_buffers(struct iio_dev *indio_dev);
> > >    void iio_buffer_wakeup_poll(struct iio_dev *indio_dev);
> > > @@ -59,6 +61,7 @@ void iio_buffer_wakeup_poll(struct iio_dev *indio_dev);
> > >
> > >    #define iio_buffer_poll_addr NULL
> > >    #define iio_buffer_read_first_n_outer_addr NULL
> > > +#define iio_buffer_write_first_n_outer_addr NULL
> > >
> > >    static inline void iio_disable_all_buffers(struct iio_dev *indio_dev) {}
> > >    static inline void iio_buffer_wakeup_poll(struct iio_dev *indio_dev) {}
> > > diff --git a/drivers/iio/industrialio-buffer.c
> > > b/drivers/iio/industrialio-buffer.c
> > > index 9f1a140..ef889af 100644
> > > --- a/drivers/iio/industrialio-buffer.c
> > > +++ b/drivers/iio/industrialio-buffer.c
> > > @@ -21,6 +21,7 @@
> > >    #include <linux/slab.h>
> > >    #include <linux/poll.h>
> > >    #include <linux/sched.h>
> > > +#include <asm/uaccess.h>
> >
> >
> > linux/uaccess.h
> >
> >
> > >    #include <linux/iio/iio.h>
> > >    #include "iio_core.h"
> > > @@ -87,6 +88,39 @@ ssize_t iio_buffer_read_first_n_outer(struct file
> > > *filp, char __user *buf,
> > >    }
> > >
> > >    /**
> > > + * iio_buffer_write_first_n_outer() - chrdev write to buffer
> > > + *
> > > + * This function pushes the user space data to kernel iio buffer
> > > + **/
> > > +ssize_t iio_buffer_write_first_n_outer(struct file *filp,
> > > +                               const 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;
> > > +       int ret = -1;
> > > +       unsigned char *data = NULL;
> > > +
> > > +       if (!indio_dev->info)
> > > +               return -ENODEV;
> > > +
> > > +       if (n != 1)
> > > +               return -EINVAL;
> > > +
> > > +       data = kzalloc(1, GFP_KERNEL);
> > > +       if (!data)
> > > +               return -ENOMEM;
> > > +
> > > +       if (copy_from_user(data, buf, 1)) {
> > > +               kfree(data);
> > > +               return -EFAULT;
> > > +       }
> > > +
> > > +       ret = iio_push_to_buffer(rb, data);
> >
> >
> > Are you sure that this works? iio_push_to_buffer() expects a data buffer of
> > size rb->bytes_per_datum bytes. On the other hand rb->bytes_per_datum is
> > only valid when the buffer is enabled, so for this to work the buffer would
> > need to be enabled. Which means you'd inject the fake data in the middle of
> > the real data stream.
>
> Yes, It works :)
> In one patch, bytes_per_datum has been removed from kifo_in.
> Patch - iio:kfifo_buf Take advantage of the fixed record size used in IIO
> commit c559afbfb08c7eac215ba417251225d3a8e01062
> - ret = kfifo_in(&kf->kf, data, r->bytes_per_datum);
> + ret = kfifo_in(&kf->kf, data, 1);
> So, I think we can now write only one byte of data.

No, we need to write 1 record and the size of one record is bytes_per_datum. 
If you only write one byte you'll cause a out of bounds access.

> Initially, I wrote the code for write functionality in kernel version 3.6
> using bytes_per_datum instead of fixed size of 1 byte.
> It worked fine. :)
> For this, we just need to replace size 1 by r->bytes_per_datum.
>
> We are not injecting data in middle of real data stream.
> When we inject the recorded data, we disabled the hardware chip,
> so no new/real data is pushed to the buffer during that time.
>
> To record, we enabled the buffer, read the real data and save it.
> To replay, we disabled the hardware chip and injected saved data by
> writing back to iio device node.
> So, Buffer is still enabled at time of writing to iio device node. :)

How do you disable the hardware without disabling the buffer?

- Lars
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/