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

[Lars-Peter Clausen <lars@metafoo.de>]

Looks good mostly, two things.

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.

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

> +
> +	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;
> +}