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
