Re: [PATCH v2] iio: make blocking read wait for data

[Josselin Costanzi <josselin.costanzi@mobile-devices.fr>]

Jonathan Cameron wrote:

> On 10/06/14 17:26, Josselin Costanzi wrote:
>> Currently the IIO buffer blocking read only wait until at least one
>> data element is available.
>> This patch adds the possibility for the userspace to to blocking calls
>> for multiple elements. This should limit the read() calls count when
>> trying to get data in batches.
>> This commit also fix a bug where data is lost if an error happens
>> after some data is already read.
>>
>> Signed-off-by: Josselin Costanzi <josselin.costanzi@mobile-devices.fr>
> Hi Josselin,
> 
> Thanks for taking this on. Definitely a useful little bit of functionality.
> I'll take a close look once all the bits Lars picked up on are sorted.
> 
> Right now, why watermark_low?  (rather than simply watermark?).
It was to match the SO_RCVLOWAT socket option wich I understand as
"socket_receive_low_watermark".

> And you know what comes with adding new ABI.  Documentation please :)
Yes, I'll have to work on that! Once we all agree on the interface we want 
to provide.

>> ---
>>   drivers/iio/industrialio-buffer.c | 138 ++++++++++++++++++++++++++++++++++----
>>   drivers/iio/kfifo_buf.c           |   4 ++
>>   include/linux/iio/buffer.h        |  40 +++++++++++
>>   3 files changed, 169 insertions(+), 13 deletions(-)
>>
>> Changelog:
>> v2: thanks to Lars-Peter Clausen and Jonathan Cameron
>> - Avoid breaking default ABI
>> - Add watermark and timeout properties to buffers
>>
>> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
>> index 36b1ae9..1fe0116 100644
>> --- a/drivers/iio/industrialio-buffer.c
>> +++ b/drivers/iio/industrialio-buffer.c
>> @@ -56,6 +56,10 @@ ssize_t iio_buffer_read_first_n_outer(struct file *filp, char __user *buf,
>>   {
>>   	struct iio_dev *indio_dev = filp->private_data;
>>   	struct iio_buffer *rb = indio_dev->buffer;
>> +	size_t datum_size = rb->access->get_bytes_per_datum(rb);
>> +	size_t watermark_bytes = rb->low_watermark * datum_size;
>> +	size_t count = 0;
>> +	long timeout = rb->timeout;
>>   	int ret;
>>
>>   	if (!indio_dev->info)
>> @@ -66,24 +70,35 @@ ssize_t iio_buffer_read_first_n_outer(struct file *filp, char __user *buf,
>>
>>   	do {
>>   		if (!iio_buffer_data_available(rb)) {
>> -			if (filp->f_flags & O_NONBLOCK)
>> -				return -EAGAIN;
>> +			if (filp->f_flags & O_NONBLOCK) {
>> +				ret = -EAGAIN;
>> +				break;
>> +			}
>>
>> -			ret = wait_event_interruptible(rb->pollq,
>> +			timeout = wait_event_interruptible_timeout(rb->pollq,
>>   					iio_buffer_data_available(rb) ||
>> -					indio_dev->info == NULL);
>> -			if (ret)
>> -				return ret;
>> -			if (indio_dev->info == NULL)
>> -				return -ENODEV;
>> +					indio_dev->info == NULL,
>> +					timeout);
>> +			if (timeout <= 0) {
>> +				ret = timeout;
>> +				break;
>> +			}
>> +
>> +			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;
>> +		n -= ret;
>> +		count += ret;
>> +	 } while (n > 0 && count < watermark_bytes);
>> +
>> +	return count ? count : ret;
>>   }
>>
>>   /**
>> @@ -126,6 +141,8 @@ void iio_buffer_init(struct iio_buffer *buffer)
>>   	INIT_LIST_HEAD(&buffer->buffer_list);
>>   	init_waitqueue_head(&buffer->pollq);
>>   	kref_init(&buffer->ref);
>> +	buffer->low_watermark = 1;
>> +	buffer->timeout = MAX_SCHEDULE_TIMEOUT;
>>   }
>>   EXPORT_SYMBOL(iio_buffer_init);
>>
>> @@ -795,6 +812,101 @@ done:
>>   }
>>   EXPORT_SYMBOL(iio_buffer_store_enable);
>>
>> +ssize_t iio_buffer_show_watermark(struct device *dev,
>> +				   struct device_attribute *attr,
>> +				   char *buf)
>> +{
>> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>> +	struct iio_buffer *buffer = indio_dev->buffer;
>> +
>> +	return sprintf(buf, "%u\n", buffer->low_watermark);
>> +}
>> +EXPORT_SYMBOL(iio_buffer_show_watermark);
>> +
>> +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;
>> +
>> +	mutex_lock(&indio_dev->mlock);
>> +	if (iio_buffer_is_active(indio_dev->buffer)) {
>> +		ret = -EBUSY;
>> +		goto out;
>> +	}
>> +
>> +	buffer->low_watermark = val;
>> +	ret = 0;
>> +out:
>> +	mutex_unlock(&indio_dev->mlock);
>> +	return ret ? ret : len;
>> +}
>> +EXPORT_SYMBOL(iio_buffer_store_watermark);
>> +
>> +static int iio_buffer_timeout_set(struct iio_buffer *buffer, long timeout_us)
>> +{
>> +	if (timeout_us >= 0) {
>> +		buffer->timeout = usecs_to_jiffies(timeout_us);
>> +	} else if (timeout_us == -1){
>> +		buffer->timeout = MAX_SCHEDULE_TIMEOUT;
>> +	} else {
>> +		return -EINVAL;
>> +	}
>> +	return 0;
>> +}
>> +
>> +static long iio_buffer_timeout_get(struct iio_buffer *buffer)
>> +{
>> +	if (buffer->timeout != MAX_SCHEDULE_TIMEOUT)
>> +		return jiffies_to_usecs(buffer->timeout);
>> +	return -1;
>> +}
>> +
>> +ssize_t iio_buffer_show_timeout(struct device *dev,
>> +				   struct device_attribute *attr,
>> +				   char *buf)
>> +{
>> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>> +	struct iio_buffer *buffer = indio_dev->buffer;
>> +
>> +	return sprintf(buf, "%ld\n", iio_buffer_timeout_get(buffer));
>> +}
>> +EXPORT_SYMBOL(iio_buffer_show_timeout);
>> +
>> +ssize_t iio_buffer_store_timeout(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;
>> +	long val;
>> +	int ret;
>> +
>> +	ret = kstrtol(buf, 10, &val);
>> +	if (ret)
>> +		return ret;
>> +
>> +	mutex_lock(&indio_dev->mlock);
>> +	if (iio_buffer_is_active(indio_dev->buffer)) {
>> +		ret = -EBUSY;
>> +		goto out;
>> +	}
>> +	ret = iio_buffer_timeout_set(buffer, val);
>> +out:
>> +	mutex_unlock(&indio_dev->mlock);
>> +	return ret ? ret : len;
>> +}
>> +EXPORT_SYMBOL(iio_buffer_store_timeout);
>> +
>>   /**
>>    * iio_validate_scan_mask_onehot() - Validates that exactly one channel is selected
>>    * @indio_dev: the iio device
>> diff --git a/drivers/iio/kfifo_buf.c b/drivers/iio/kfifo_buf.c
>> index 7134e8a..ab7ea54 100644
>> --- a/drivers/iio/kfifo_buf.c
>> +++ b/drivers/iio/kfifo_buf.c
>> @@ -54,10 +54,14 @@ static int iio_get_length_kfifo(struct iio_buffer *r)
>>
>>   static IIO_BUFFER_ENABLE_ATTR;
>>   static IIO_BUFFER_LENGTH_ATTR;
>> +static IIO_BUFFER_WATERMARK_ATTR;
>> +static IIO_BUFFER_TIMEOUT_ATTR;
>>
>>   static struct attribute *iio_kfifo_attributes[] = {
>>   	&dev_attr_length.attr,
>>   	&dev_attr_enable.attr,
>> +	&dev_attr_low_watermark.attr,
>> +	&dev_attr_timeout.attr,
>>   	NULL,
>>   };
>>
>> diff --git a/include/linux/iio/buffer.h b/include/linux/iio/buffer.h
>> index 5193927..41c8f11 100644
>> --- a/include/linux/iio/buffer.h
>> +++ b/include/linux/iio/buffer.h
>> @@ -61,6 +61,8 @@ struct iio_buffer_access_funcs {
>>    * struct iio_buffer - general buffer structure
>>    * @length:		[DEVICE] number of datums in buffer
>>    * @bytes_per_datum:	[DEVICE] size of individual datum including timestamp
>> + * @low_watermark:	[DEVICE] blocking read granularity in datums
>> + * @timeout:		[DEVICE] blocking read timeout in jiffies
>>    * @scan_el_attrs:	[DRIVER] control of scan elements if that scan mode
>>    *			control method is used
>>    * @scan_mask:		[INTERN] bitmask used in masking scan mode elements
>> @@ -80,6 +82,8 @@ struct iio_buffer_access_funcs {
>>   struct iio_buffer {
>>   	int					length;
>>   	int					bytes_per_datum;
>> +	unsigned int				low_watermark;
>> +	long					timeout;
>>   	struct attribute_group			*scan_el_attrs;
>>   	long					*scan_mask;
>>   	bool					scan_timestamp;
>> @@ -201,6 +205,34 @@ ssize_t iio_buffer_store_enable(struct device *dev,
>>   ssize_t iio_buffer_show_enable(struct device *dev,
>>   			       struct device_attribute *attr,
>>   			       char *buf);
>> +/**
>> + * iio_buffer_show_watermark() - attr to see the read watermark
>> + **/
>> +ssize_t iio_buffer_show_watermark(struct device *dev,
>> +				struct device_attribute *attr,
>> +				char *buf);
>> +/**
>> + * iio_buffer_store_watermark() - attr to configure the read watermark
>> + **/
>> +ssize_t iio_buffer_store_watermark(struct device *dev,
>> +				struct device_attribute *attr,
>> +				const char *buf,
>> +				size_t len);
>> +/**
>> + * iio_buffer_show_timeout() - attr to see the read timeout (microseconds)
>> + * Note: if no timeout is set, returns -1
>> + **/
>> +ssize_t iio_buffer_show_timeout(struct device *dev,
>> +				struct device_attribute *attr,
>> +				char *buf);
>> +/**
>> + * iio_buffer_store_timeout() - attr to configure read timeout (microseconds)
>> + * Note: to disable the timeout, write -1
>> + **/
>> +ssize_t iio_buffer_store_timeout(struct device *dev,
>> +				struct device_attribute *attr,
>> +				const char *buf,
>> +				size_t len);
>>   #define IIO_BUFFER_LENGTH_ATTR DEVICE_ATTR(length, S_IRUGO | S_IWUSR,	\
>>   					   iio_buffer_read_length,	\
>>   					   iio_buffer_write_length)
>> @@ -209,6 +241,14 @@ ssize_t iio_buffer_show_enable(struct device *dev,
>>   					   iio_buffer_show_enable,	\
>>   					   iio_buffer_store_enable)
>>
>> +#define IIO_BUFFER_WATERMARK_ATTR DEVICE_ATTR(low_watermark, S_IRUGO | S_IWUSR,	\
>> +					   iio_buffer_show_watermark,	\
>> +					   iio_buffer_store_watermark)
>> +
>> +#define IIO_BUFFER_TIMEOUT_ATTR DEVICE_ATTR(timeout, S_IRUGO | S_IWUSR,	\
>> +					   iio_buffer_show_timeout,	\
>> +					   iio_buffer_store_timeout)
>> +
>>   bool iio_validate_scan_mask_onehot(struct iio_dev *indio_dev,
>>   	const unsigned long *mask);
>>
>>