Re: read /dev/iio:device0 return -1 (Invalid argument)

[Jonathan Cameron <jic23@kernel.org>]

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@kernel.org> 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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>