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.
>> + 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
