Re: [PATCH v5 1/3] iio: add watermark logic to iio read and poll

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Mar 18, 2015 at 11:29 AM, Octavian Purdila
<octavian.purdila@xxxxxxxxx> wrote:
> On Wed, Mar 18, 2015 at 11:19 AM, Lars-Peter Clausen <lars@xxxxxxxxxx> wrote:
>> Looks good mostly, two things.
>>
>
> Thanks for the review Lars !
>
>> On 03/04/2015 06:56 PM, Octavian Purdila wrote:
>> [...]
>>>
>>> @@ -61,19 +80,24 @@ ssize_t iio_buffer_read_first_n_outer(struct file
>>> *filp, char __user *buf,
>>>         if (!rb || !rb->access->read_first_n)
>>>                 return -EINVAL;
>>>
>>> +       /*
>>> +        * If datum_size is 0 there will never be anything to read from
>>> the
>>> +        * buffer, so signal end of file now.
>>> +        */
>>> +       if (!datum_size)
>>> +               return 0;
>>> +
>>> +       if (!(filp->f_flags & O_NONBLOCK))
>>> +               to_wait = min_t(size_t, n / datum_size, rb->watermark);
>>> +
>>>         do {
>>> -               if (!iio_buffer_data_available(rb)) {
>>> -                       if (filp->f_flags & O_NONBLOCK)
>>> -                               return -EAGAIN;
>>
>>
>> If I understand this correctly we no longer return -EAGAIN if no data is
>> available in non blocking mode, but rather return 0. Which means
>> end-of-file, so it is not correct.
>>
>
> You are right, I'll add have to add back the if
> (!iio_buffer_data_available(rb) && (filp->f_flags & O_NONBLOCK))
> statement.
>

Actually this case is already taken care of below the patch context:

                ret = rb->access->read_first_n(rb, n, buf);
                if (ret == 0 && (filp->f_flags & O_NONBLOCK))
                        ret = -EAGAIN;


And sorry for the previous spurious email :)

>>> +               ret = wait_event_interruptible(rb->pollq,
>>> +                                     iio_buffer_ready(indio_dev, rb,
>>> to_wait));
>>> +               if (ret)
>>> +                       return ret;
>>>
>>> -                       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;
>>> -               }
>>> +               if (!indio_dev->info)
>>> +                       return -ENODEV;
>>>   [...]
>>> +
>>> +static ssize_t iio_buffer_store_watermark(struct device *dev,
>>> +                                         struct device_attribute *attr,
>>> +                                         const char *buf,
>>> +                                         size_t len)
>>> +{
>>> +       struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>>> +       struct iio_buffer *buffer = indio_dev->buffer;
>>> +       unsigned int val;
>>> +       int ret;
>>> +
>>> +       ret = kstrtouint(buf, 10, &val);
>>> +       if (ret)
>>> +               return ret;
>>> +       if (!val)
>>> +               return -EINVAL;
>>> +
>>> +       mutex_lock(&indio_dev->mlock);
>>> +
>>> +       if (val > buffer->length) {
>>> +               ret = -EINVAL;
>>> +               goto out;
>>> +       }
>>
>>
>> This is missing the check for val == 0.
>>
>
> Unless I misunderstand you, the check is done right after kstrtouint()
> before taking the lock.
>
>>
>>> +
>>> +       if (iio_buffer_is_active(indio_dev->buffer)) {
>>> +               ret = -EBUSY;
>>> +               goto out;
>>> +       }
>>> +
>>> +       buffer->watermark = val;
>>> +out:
>>> +       mutex_unlock(&indio_dev->mlock);
>>> +
>>> +       return ret ? ret : len;
>>> +}
>>
>>
--
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




[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux