Message-ID: <53F3A535.9040703@kernel.org>
Date:	Tue, 19 Aug 2014 20:27:49 +0100
From:	Jonathan Cameron <jic23@...nel.org>
To:	Aniroop Mathur <aniroop.mathur@...il.com>
CC:	Lars-Peter Clausen <lars@...afoo.de>, a.mathur@...sung.com,
	"cpgs ." <cpgs@...sung.com>, linux-kernel@...r.kernel.org,
	linux-iio@...r.kernel.org
Subject: Re: [PATCH 1/1] IIO: Added write function in iio_buffer_fileops

On 18/08/14 16:29, Aniroop Mathur wrote:
> Dear Mr. Jonathan, Mr. Lars,
> Greetings !
> 
> Kindly provide feedback upon the comments below.
> 
Sorry, bit busy at the moment, so might be another day or two before I get
a chance to address this...

Jonathan
> 
> On Thu, Aug 14, 2014 at 8:08 PM, Jonathan Cameron <jic23@...nel.org> wrote:
>> On 14/08/14 10:41, Lars-Peter Clausen wrote:
>>> On 08/13/2014 06:33 PM, Aniroop Mathur wrote:
>>>> On Wed, Aug 13, 2014 at 8:18 PM, Lars-Peter Clausen <lars@...afoo.de> wrote:
>>>>> On 08/13/2014 03:42 PM, Aniroop Mathur wrote:
>>>>>>
>>>>>> On Wed, Aug 13, 2014 at 2:38 PM, Lars-Peter Clausen <lars@...afoo.de>
>>>>>> wrote:
>>>>>>>
>>>>>>> On 08/13/2014 08:29 AM, a.mathur@...sung.com wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> From: Aniroop Mathur <a.mathur@...sung.com>
>>>>>>>>
>>>>>>>> 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 inclined to agree with Lars.  As an additional point this will cause
>> confusion when we have buffered writing for output devices (DACs).
>>
>>
>>>>>>> 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 ?
>> Why should there be?
> 
> 
> Below is the use case for which write function is required:
> 
> I am working on Sensor Simulator application for android phones.
> This android application will simulate the sensor values
> for any other already developed 3rd party/organization application/s
> and not for simulator application itself.
> In other words, I need to check the working of other sensor applications by
> using my simulator application and along with it, checking of HAL and
> Framework too.
> 
> For simulation, below is the data flow
> by using user-space libiio and by directly writing to iio buffer.
> 
> Hardware--Driver--IIO--General_IIO_HAL--Framework--Other_Sensor_Application/s
>                                        |
>                                        | --> libiio <-->
> Simulator_Sensor_Application
>                                        |
>                                        | <--> Simulator_Sensor_Application
> 
> 
> If we use libiio,
> Sensor_Simulator_Application will not be able to send sensor data to
> Other_Sensor_Application/s because it can only write and read data for itself.
> Reading and writing data for itself is not what is required. There is a need
> to send data to other application/s.
> Also, we could not check HAL and Framework here.
> 
> If we directly write to iio buffer,
> Sensor_Simulator_Application will be able to send sensor data to
> Other_Sensor_Application/s and this in turn will also check whether there
> are any problems in hal and framework for Other_Application to work properly.
> So, if some Other_Sensor_Application is behaving differently
> for same sensor data it means there is some problem in
> HAL/Framework/Application itself.
> 
> Also importantly, we can change the recorded data and check Other_Application
> working with that new data.
> As an example,
> There is an application, which uses accel+gyro+pressure to calculate
> amount of calories burnt of an individual.
> So once recorded the data physically by going in outside environment,
> we can change just the pressure data and measure the amount of calories burnt
> without actually physically going outside at new height for new pressure.
> 
> Now to write directly to IIO buffer, there is a need of two things
> that should be present in IIO subsystem:
> 1. Write function in iio_buffer file_ops.
> 2. Multiple file descriptor support for same device node.
>    because both Simulator_Sensor_Application and Other_Sensor_Application/s
>    need to open device node for their use.
> 
> Currently, both are not present in IIO subsystem.
> So first, I am hoping to add write function and
> after that adding multiple fd support.
> 
> In input subsystem, both facilities are available.
> 
> How can we achieve this task without directly writing to iio buffer ?
> How will user-space libiio help us for this task ?
> 
>>>>>
>>>>>
>>>>> 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().
>>>>>
>>>>>
>>>>
>>>> If there is no buffer, then there is clearly a chance of data loss/miss. :)
>>>> Because if one application is reading the data with frequency 5 Hz
>>>> and other application is writing the data at frequency 50 Hz (20 ms delay)
>>>> so this reading application will miss reading a lot of data.
>>>> Like in this case, after every 200 ms, 9 out of 10 data will be missed.
>>>
>>> Not if implemented correctly. Even with the current kernel implementation you'll have this issues as the buffer will
>>> simply overflow when you write faster than you read.
>>>
>>>
> 
> Generally, there is less difference between read and write frequency.
> But there is difference.
> So, in case of a buffer, there is very very less chance of data loss.
> But in case of no buffer, there will be data loss/miss.
> 
> Generally, as you know, application requests for frequency at which
> hardware should send data for their reading.
> But driver frequency to write data in buffer
> and application frequency to read data does not match exactly.
> So either fast or slow, we end with either data loss
> or same data being sent again to app.
> 
> Therefore, buffer is required to avoid such cases.
> 
> 
>>> [...]
>> If we have a usecase for playback functionality like this I would prefer it to
>> be on a separate IIO device.  When Lars' DAC buffered code is ready it will
>> be relatively easy to string together a fake DAC with a fake ADC to get
>> this sort of functionality.   It will be rather more interesting to
>> work out how to setup an arbitary device but it could be done,
>> most likely using configfs.
>>
>> This would be more similar to uinput than the writing to the chardevs
>> directly as you are suggesting here.
>>
>>>>>>> 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.
>>>>>
>>>>>
>>>>
>>>> This is the code flow below which I checked:
>>>>
>>>> kfifo_in(&kf->kf, data, 1);
>>>> so len=1
>>>> kfifo_copy_in(fifo, buf, len, fifo->in);
>>>> l = min(len, size - off);
>>>> memcpy(fifo->data + off, src, l);
>>>>
>>>> In memcpy, if l is 1, so it will copy one byte only.
>>>> So, how it is writing one record and not one byte ?
>>> You missed this part:
>>>
>>>     if (esize != 1) {
>>>         off *= esize;
>>>         size *= esize;
>>>         len *= esize;
>>>     }
>>>
>>> so len gets multiplied by the record size. len=1 means 1 record.
>>>
> 
> Oh, I really missed this part.
> Thank you Mr. Lars for the correction.
> 
> So, in the write function,
> we can just replace 1 by rb->bytes_per_datum.
> And add the check like below:
> + if(!rb || rb->bytes_per_datum==0)
> + return -1;
> 
> I initially write the code with bytes_per_datum only.
> But changed to 1 after seeing the latest kernel and
> got confused with "1" value.
> 
> Is there anything else need to be changed ?
> 
>>>>
>>>>>>
>>>>>> 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?
>>>>>
>>>> I disabled the hardware by powering off the chip.
>>>> And after writing is complete, chip is powered on again. :)
>>>
>>> But how do you disable the device when the buffer is still active?
>>> IIO expects the device to be active when the buffer is active.
>> Agreed. This sounds like a pretty uggly hack.  I would not be happy with
>> having this in any driver.
>>
> 
> Taking example of pressure sensor hardware chip,
> 
> Case1: Normal case,
> 1. We enabled the iio software buffer.
> 2. Pressure Hardware chip generates an interrupt.
> 3. Driver receives the interrupt call and make
>    a call to push data to iio buffer
> 
> But if there is no change in pressure i.e. there is no new data
> hardware chip will not generate any interrupt and hence
> no data is pushed to iio software buffer.
> And iio software buffer is still active and waiting if there is any data comes.
> 
> Case 2: My case,
> 1. We enable the iio software buffer.
> 2. We record the pressure real data if there is.
> 3. We power off the pressure hardware chip
>    or second way, we disable the irq only.
> 
> In this case too, there will be no interrupt call received by driver,
> and hence no data is pushed to iio buffer.
> And iio software buffer is still active and waiting if there is any data comes.
> 
> In both cases, from iio buffer perspective,
> iio software buffer is active and waiting for some data.
> 
> I cannot see any difference for IIO buffer in both cases.
> So, I am not able to understand the problem here with IIO buffer ?
> 
> Later in case 2, as buffer is active,
> Sensor_Simulator_Application can write data to it
> and Other_Sensor_Application/s can read data from it.
> And after simulation is over,
> we power on the hardware chip again or second way, enable the irq again,
> everything gets back to normal as before.
> 
> Thanks,
> Aniroop
> 
>>>
>>> - Lars
>>>
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>>> the body of a message to majordomo@...r.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists