On 05/01/16 11:57, Julio Cruz wrote:
> Dear All,
>
> Thanks to your comments, I found out that the function in my kernel
> iio_buffer_read_first_n_outer is different to the one that you
> mentioned.
>
> I was debugging a kernel version 3.10, that's include another
> implementation of the function that we are talking. After some extra
> effort, I could update to version 3.14 and effectively, the behavior
> is as expected.
Cool and thanks for letting us know - back then it seems we didn't support
blocking reads which explains the problem. Btw, you have my sympathies
with older kernels, I'm trying to bring up a board for the first time since
I ran 3.7 on it and it's proving 'interesting'... Unfortunately it's
the only source of lis3l02dq that I have and I'd like to merge the old
staging driver into the more recent generic st_sensors driver. Fun fun fun!
>
> Very sorry for this misunderstanding.
That's fine. None of us managed to remember that this was the case
for older kernels!
>
> BTW, in case of future problems, would be fine
>
> Thanks
>
> Julio
>
>
> kernel 3.10:
> ---------------
> ssize_t iio_buffer_read_first_n_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;
>
> i if (!rb || !rb->access->read_first_n)
> return -EINVAL;
> return rb->access->read_first_n(rb, n, buf);
> }
>
> kernel 3.14:
> ---------------
> ssize_t iio_buffer_read_first_n_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;
> int ret;
>
> if (!indio_dev->info)
> return -ENODEV;
>
> if (!rb || !rb->access->read_first_n)
> return -EINVAL;
>
> do {
> if (!iio_buffer_data_available(rb)) {
> if (filp->f_flags & O_NONBLOCK)
> return -EAGAIN;
>
> ret = wait_event_interruptible(rb->pollq,
> iio_buffer_data_available(rb) ||
> indio_dev->info == NULL);
> if (ret)
> return ret;
> if (indio_dev->info == NULL)
> return -ENODEV;
> }
>
> ret = rb->access->read_first_n(rb, n, buf);
> if (ret == 0 && (filp->f_flags & O_NONBLOCK))
> ret = -EAGAIN;
> } while (ret == 0);
>
> return ret;
> }
>
> On Tue, Jan 5, 2016 at 2:22 AM, Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
>> On 04/01/16 12:46, Lars-Peter Clausen wrote:
>>> On 01/04/2016 12:34 PM, Jonathan Cameron wrote:
>>>> On 04/01/16 04:59, Julio Cruz wrote:
>>>>> Hi Jonathan,
>>>>>
>>>>> Previously, you help me about an issue related with data loss. You suggest
>>>>> me to debug deep in the core elements. I will try to summarize the results
>>>>> below for future reference.
>>>>>
>>>>> When there is not data available in the buffer (kfifo), and the application
>>>>> try to read data (using "read" function), it return zero (0).
>>>>>
>>>>> If libiio will be used to read the data, there is a problem (detailed at
>>>>> https://github.com/analogdevicesinc/libiio/issues/23). In brief, Paul
>>>>> (pcercuei) suggest me that this issue must be manage by the driver, in this
>>>>> case, return -EAGAIN when there is not data available [Resource temporarily
>>>>> unavailable (POSIX.1)].
>>>>>
>>>>> After review the core elements as suggested, I changed the line (in
>>>>> function iio_read_first_n_kfifo of kfifo_buf.c) as below:
>>>>>
>>>>> - return copied;
>>>>> + return copied == 0 ? -EAGAIN: copied;
>>>>>
>>>>> Do you think will be OK like this?
>>>> Hmm.. This is an interesting one (thanks for tracking it down)
>>>>
>>>> The man page for read indeed allows for this to occur.
>>>>
>>>> When attempting to read a file (other than a pipe or FIFO) that sup‐
>>>> ports non-blocking reads and has no data currently available:
>>>>
>>>> * If O_NONBLOCK is set, read() shall return −1 and set errno to
>>>> [EAGAIN].
>>>>
>>>>
>>>> However the issue here is that this is an ABI change and there may
>>>> unfortunately be code out there relying on it returning 0.
>>>
>>> We never propagate 0 to userspace though. The referenced function is
>>> iio_read_first_n_kfifo() which is an internal function. The function that
>>> handles the userspace ABI is iio_buffer_read_first_n_outer() and here, as
>>> Daniel pointed out, there are two things that can happen.
>>>
>>> We are in non-blocking mode and iio_read_first_n_kfifo() returns 0. In that
>>> case we'll return -EAGAIN as mandated by the specification.
>>>
>>> We are in blocking mode and iio_read_first_n_kfifo() returns 0. In that case
>>> we'll go back to waiting for more data and we'll only return if either data
>>> was received or the application was interrupted by a signal. In the former
>>> case we'll return the number of received bytes in the later case -ERESTARTSYS.
>>>
>>> So either way we should never return 0, something else must be going on.
>>>
>>>
>>> Btw. letting iio_read_first_n_kfifo() return -EAGAIN will break blocking mode.
>> That's what I get for thinking I remembered how this code works ;)
>> Completely forgot the outer function did anything non trivial.
>>
>> Thanks Daniel / Lars for picking up on this!
>>
>> Oops.
>>
>> Jonathan
>>>
>>> - Lars
>>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
