On Tue, Mar 3, 2015 at 7:46 PM, Lars-Peter Clausen <lars@xxxxxxxxxx> wrote:
> On 03/03/2015 05:21 PM, Octavian Purdila wrote:
> [...]
Hi Lars,
Thank you for the review!
>>
>> @@ -61,26 +64,48 @@ ssize_t iio_buffer_read_first_n_outer(struct file
>> *filp, char __user *buf,
>> if (!rb || !rb->access->read_first_n)
>> return -EINVAL;
>>
>> - do {
>> - if (!iio_buffer_data_available(rb)) {
>> - if (filp->f_flags & O_NONBLOCK)
>> - return -EAGAIN;
>> + /*
>> + * 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;
>>
>> + to_read = min_t(size_t, n / datum_size, rb->watermark);
>
>
> I'd maybe call it wakeup_threshold.
>
>> +
>> + do {
>> + if (filp->f_flags & O_NONBLOCK) {
>> + if (!iio_buffer_data_available(rb)) {
>> + ret = -EAGAIN;
>> + break;
>> + }
>> + } else {
>> ret = wait_event_interruptible(rb->pollq,
>> - iio_buffer_data_available(rb) ||
>> - indio_dev->info == NULL);
>> + iio_buffer_data_available(rb) >= to_read ||
>> + indio_dev->info ==
>> NULL);
>
>
> This needs also to evaluate to true if the buffer is disabled so we have a
> chance to read any amount residue sample that are less than watermark.
>
Good point.
>> if (ret)
>> return ret;
>> - if (indio_dev->info == NULL)
>> - return -ENODEV;
>> + if (indio_dev->info == NULL) {
>> + ret = -ENODEV;
>> + break;
>> + }
>> }
>>
>> - ret = rb->access->read_first_n(rb, n, buf);
>> - if (ret == 0 && (filp->f_flags & O_NONBLOCK))
>> - ret = -EAGAIN;
>> - } while (ret == 0);
>> + ret = rb->access->read_first_n(rb, n, buf + count);
>> + if (ret < 0)
>> + break;
>>
>> - return ret;
>> + count += ret;
>> + n -= ret;
>> + to_read -= ret / datum_size;
>
>
>
> This will underflow if there are more than watermark sample in the buffer
> and and more than watermark samples have been requested by userspace...
>
>
>> + } while (to_read > 0);
>
>
> ... and then we get trapped in a very long loop.
>
> In my opinion there is no need to change the loop at all, only update the
> wakeup condition.
>
Correct, how did I miss that :/ I will rewrite the code to keep the
loop unchanged.
>> +
>> + if (count)
>> + return count;
>> + if (ret < 0)
>> + return ret;
>> +
>> + return -EAGAIN;
>> }
>>
>> /**
>> @@ -96,9 +121,8 @@ unsigned int iio_buffer_poll(struct file *filp,
>> return -ENODEV;
>>
>> poll_wait(filp, &rb->pollq, wait);
>> - if (iio_buffer_data_available(rb))
>> + if (iio_buffer_data_available(rb) >= rb->watermark)
>
>
> Same here needs to wakeup if the buffer is disabled and there is at least
> one sample.
>
Ok.
>> return POLLIN | POLLRDNORM;
>> - /* need a way of knowing if there may be enough data... */
>> return 0;
>> }
>>
> [...]
>>
>> @@ -418,7 +443,16 @@ static ssize_t iio_buffer_write_length(struct device
>> *dev,
>> }
>> mutex_unlock(&indio_dev->mlock);
>>
>> - return ret ? ret : len;
>> + if (ret)
>> + return ret;
>> +
>> + if (buffer->length)
>> + val = buffer->length;
>> +
>> + if (val < buffer->watermark)
>> + buffer->watermark = val;
>
>
> Needs to be inside the locked section.
>
Ok.
>> +
>> + return len;
>> }
>
> [...]
>>
>> +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 > buffer->length)
>
>
> Same here.
Ok.
>
>> + return -EINVAL;
>> +
>> + mutex_lock(&indio_dev->mlock);
>> + if (iio_buffer_is_active(indio_dev->buffer)) {
>> + ret = -EBUSY;
>> + goto out;
>> + }
>> +
>> + buffer->watermark = val;
>
>
> We should probably reject 0.
>
I agree.
>> + ret = 0;
>> +out:
>> + mutex_unlock(&indio_dev->mlock);
>> + return ret ? ret : len;
>> +}
>> +
>
> [...]
>>
>> int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
>> @@ -944,8 +1022,18 @@ static const void *iio_demux(struct iio_buffer
>> *buffer,
>> static int iio_push_to_buffer(struct iio_buffer *buffer, const void
>> *data)
>> {
>> const void *dataout = iio_demux(buffer, data);
>> + int ret;
>> +
>> + ret = buffer->access->store_to(buffer, dataout);
>> + if (ret)
>> + return ret;
>>
>> - return buffer->access->store_to(buffer, dataout);
>> + /*
>> + * We can't just test for watermark to decide if we wake the poll
>> queue
>> + * because read may request less samples than the watermark.
>> + */
>> + wake_up_interruptible(&buffer->pollq);
>
>
> What happened to poll parameters?
>
I don't understand you question, can you please elaborate?
--
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
