[PATCH] iio: buffer-dma: Expose data available

[PATCH] iio: buffer-dma: Expose data available

[mfornero]

From: Matt Fornero <matt.fornero@mathworks.com>

Add a sysfs attribute that exposes the buffer data available to
userspace. This attribute can be checked at runtime to determine the
overall buffer fill level (across all allocated DMA buffers).

Signed-off-by: Matt Fornero <matt.fornero@mathworks.com>
---
 drivers/iio/buffer/industrialio-buffer-dma.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/iio/buffer/industrialio-buffer-dma.c b/drivers/iio/buffer/industrialio-buffer-dma.c
index ff03324..8739a41 100644
--- a/drivers/iio/buffer/industrialio-buffer-dma.c
+++ b/drivers/iio/buffer/industrialio-buffer-dma.c
@@ -16,6 +16,7 @@
 #include <linux/iio/buffer.h>
 #include <linux/iio/buffer_impl.h>
 #include <linux/iio/buffer-dma.h>
+#include <linux/iio/sysfs.h>
 #include <linux/dma-mapping.h>
 #include <linux/sizes.h>
 
@@ -599,6 +600,28 @@ int iio_dma_buffer_set_length(struct iio_buffer *buffer, int length)
 }
 EXPORT_SYMBOL_GPL(iio_dma_buffer_set_length);
 
+
+static ssize_t iio_dma_get_data_available(struct device *dev,
+					       struct device_attribute *attr,
+					       char *buf)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	size_t bytes;
+
+	bytes = iio_dma_buffer_data_available(indio_dev->buffer);
+
+	return sprintf(buf, "%llu\n", (unsigned long long)bytes);
+}
+
+static IIO_DEVICE_ATTR(data_available, S_IRUGO,
+		       iio_dma_get_data_available, NULL, 0);
+
+static const struct attribute *iio_dma_buffer_attributes[] = {
+	&iio_dev_attr_data_available.dev_attr.attr,
+	NULL,
+};
+
+
 /**
  * iio_dma_buffer_init() - Initialize DMA buffer queue
  * @queue: Buffer to initialize
@@ -615,6 +638,7 @@ int iio_dma_buffer_init(struct iio_dma_buffer_queue *queue,
 	iio_buffer_init(&queue->buffer);
 	queue->buffer.length = PAGE_SIZE;
 	queue->buffer.watermark = queue->buffer.length / 2;
+	queue->buffer.attrs = iio_dma_buffer_attributes;
 	queue->dev = dev;
 	queue->ops = ops;
 
-- 
2.1.4

Re: [PATCH] iio: buffer-dma: Expose data available

[Jonathan Cameron]

On Fri,  1 Dec 2017 15:46:20 -0500
mfornero@gmail.com wrote:

> From: Matt Fornero <matt.fornero@mathworks.com>
> 
> Add a sysfs attribute that exposes the buffer data available to
> userspace. This attribute can be checked at runtime to determine the
> overall buffer fill level (across all allocated DMA buffers).
> 
> Signed-off-by: Matt Fornero <matt.fornero@mathworks.com>

Seems sensible to me.

Lars?

Thanks,

Jonathan
> ---
>  drivers/iio/buffer/industrialio-buffer-dma.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/drivers/iio/buffer/industrialio-buffer-dma.c b/drivers/iio/buffer/industrialio-buffer-dma.c
> index ff03324..8739a41 100644
> --- a/drivers/iio/buffer/industrialio-buffer-dma.c
> +++ b/drivers/iio/buffer/industrialio-buffer-dma.c
> @@ -16,6 +16,7 @@
>  #include <linux/iio/buffer.h>
>  #include <linux/iio/buffer_impl.h>
>  #include <linux/iio/buffer-dma.h>
> +#include <linux/iio/sysfs.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/sizes.h>
>  
> @@ -599,6 +600,28 @@ int iio_dma_buffer_set_length(struct iio_buffer *buffer, int length)
>  }
>  EXPORT_SYMBOL_GPL(iio_dma_buffer_set_length);
>  
> +
> +static ssize_t iio_dma_get_data_available(struct device *dev,
> +					       struct device_attribute *attr,
> +					       char *buf)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	size_t bytes;
> +
> +	bytes = iio_dma_buffer_data_available(indio_dev->buffer);
> +
> +	return sprintf(buf, "%llu\n", (unsigned long long)bytes);
> +}
> +
> +static IIO_DEVICE_ATTR(data_available, S_IRUGO,
> +		       iio_dma_get_data_available, NULL, 0);
> +
> +static const struct attribute *iio_dma_buffer_attributes[] = {
> +	&iio_dev_attr_data_available.dev_attr.attr,
> +	NULL,
> +};
> +
> +
>  /**
>   * iio_dma_buffer_init() - Initialize DMA buffer queue
>   * @queue: Buffer to initialize
> @@ -615,6 +638,7 @@ int iio_dma_buffer_init(struct iio_dma_buffer_queue *queue,
>  	iio_buffer_init(&queue->buffer);
>  	queue->buffer.length = PAGE_SIZE;
>  	queue->buffer.watermark = queue->buffer.length / 2;
> +	queue->buffer.attrs = iio_dma_buffer_attributes;
>  	queue->dev = dev;
>  	queue->ops = ops;
>  

Re: [PATCH] iio: buffer-dma: Expose data available

[Lars-Peter Clausen]

On 12/02/2017 12:52 PM, Jonathan Cameron wrote:
> On Fri,  1 Dec 2017 15:46:20 -0500
> mfornero@gmail.com wrote:
> 
>> From: Matt Fornero <matt.fornero@mathworks.com>
>>
>> Add a sysfs attribute that exposes the buffer data available to
>> userspace. This attribute can be checked at runtime to determine the
>> overall buffer fill level (across all allocated DMA buffers).
>>
>> Signed-off-by: Matt Fornero <matt.fornero@mathworks.com>
> 
> Seems sensible to me.
> 
> Lars?

I guess one question is whether this should be generic.
iio_dma_buffer_data_available() is a generic function and not specific to
the DMA buffers, so it would work just fine for FIFO based buffers as well.

Re: [PATCH] iio: buffer-dma: Expose data available

[Matthew Fornero]

[-- Attachment #1: Type: text/plain, Size: 988 bytes --]

On Tue, Dec 5, 2017 at 1:32 AM Lars-Peter Clausen <lars@metafoo.de> wrote:
> I guess one question is whether this should be generic.
> iio_dma_buffer_data_available() is a generic function and not specific to
the
> DMA buffers, so it would work just fine for FIFO based buffers as well.

So it looks like we could implement this generically as a member of
iio_buffer_attrs, and have it simply use iio_buffer_data_available()
instead of iio_dma_buffer_data_available().

This would also be a bit cleaner, as the iio_buffer_attrs logic already
has code to append existings attrs, allowing device-specific attrs to be
easily added.

What about the case when the data_available element in
iio_buffer_access_funcs is undefined (e.g. industrialio-buffer-cb)?
Do we modify iio_buffer_data_available() to return 0, do we make the
sysfs function return -ENOENT or -EINVAL, or simply not expose the sysfs
interface for this case?
-- 

--

Matthew Fornero

matthew.fornero@gmail.com

(734)-846-4968

[-- Attachment #2: Type: text/html, Size: 1459 bytes --]

Re: [PATCH] iio: buffer-dma: Expose data available

[Matthew Fornero]

On Tue, Dec 5, 2017 at 1:32 AM Lars-Peter Clausen <lars@metafoo.de> wrote:
> I guess one question is whether this should be generic.
> iio_dma_buffer_data_available() is a generic function and not specific to
> the DMA buffers, so it would work just fine for FIFO based buffers as well.

So it looks like we could implement this generically as a member of
iio_buffer_attrs, and have it simply use iio_buffer_data_available()
instead of iio_dma_buffer_data_available().

This would also be a bit cleaner, as the iio_buffer_attrs logic already
has code to append existings attrs, allowing device-specific attrs to be
easily added.

What about the case when the data_available element in
iio_buffer_access_funcs is undefined (e.g. industrialio-buffer-cb)?
Do we modify iio_buffer_data_available() to return 0, do we make the
sysfs function return -ENOENT or -EINVAL (or simply not expose the sysfs
interface for this case)?

Re: [PATCH] iio: buffer-dma: Expose data available

[Lars-Peter Clausen]

On 12/05/2017 07:01 PM, Matthew Fornero wrote:
> On Tue, Dec 5, 2017 at 1:32 AM Lars-Peter Clausen <lars@metafoo.de
> <mailto:lars@metafoo.de>> wrote:
>> I guess one question is whether this should be generic.
>> iio_dma_buffer_data_available() is a generic function and not specific to the
>> DMA buffers, so it would work just fine for FIFO based buffers as well.
> 
> So it looks like we could implement this generically as a member of
> iio_buffer_attrs, and have it simply use iio_buffer_data_available()
> instead of iio_dma_buffer_data_available().
> 
> This would also be a bit cleaner, as the iio_buffer_attrs logic already
> has code to append existings attrs, allowing device-specific attrs to be
> easily added.
> 
> What about the case when the data_available element in 
> iio_buffer_access_funcs is undefined (e.g. industrialio-buffer-cb)? 
> Do we modify iio_buffer_data_available() to return 0, do we make the
> sysfs function return -ENOENT or -EINVAL, or simply not expose the sysfs
> interface for this case?

The callback buffer does not support any of the interfaces required for the
userspace facing side. E.g. there is no read() callback.

For proper support of the userspace interfaces data_available() is required,
otherwise read() would never return. So I think this is fine, we'd never
register the userspacing interface if it didn't have the data_available
callback.

Re: [PATCH] iio: buffer-dma: Expose data available

[Matthew Fornero]

On Tue, Dec 5, 2017 at 10:08 AM Lars-Peter Clausen <lars@metafoo.de> wrote:
> > What about the case when the data_available element in
> > iio_buffer_access_funcs is undefined (e.g. industrialio-buffer-cb)?
> > Do we modify iio_buffer_data_available() to return 0, do we make the
> > sysfs function return -ENOENT or -EINVAL, or simply not expose the sysfs
> > interface for this case?
>
> The callback buffer does not support any of the interfaces required for the
> userspace facing side. E.g. there is no read() callback.
>
> For proper support of the userspace interfaces data_available() is required,
> otherwise read() would never return. So I think this is fine, we'd never
> register the userspacing interface if it didn't have the data_available
> callback.

So there should already be provisions in place for *not* registering the
cb buffer with user-space, meaning no additional logic would be required
for exposing / not exposing a "data_available" sysfs attribute, correct?

Assuming the above is correct, I'll come back with a generic v2 of this.

Re: [PATCH] iio: buffer-dma: Expose data available

[Lars-Peter Clausen]

On 12/05/2017 07:11 PM, Matthew Fornero wrote:
> On Tue, Dec 5, 2017 at 10:08 AM Lars-Peter Clausen <lars@metafoo.de> wrote:
>>> What about the case when the data_available element in
>>> iio_buffer_access_funcs is undefined (e.g. industrialio-buffer-cb)?
>>> Do we modify iio_buffer_data_available() to return 0, do we make the
>>> sysfs function return -ENOENT or -EINVAL, or simply not expose the sysfs
>>> interface for this case?
>>
>> The callback buffer does not support any of the interfaces required for the
>> userspace facing side. E.g. there is no read() callback.
>>
>> For proper support of the userspace interfaces data_available() is required,
>> otherwise read() would never return. So I think this is fine, we'd never
>> register the userspacing interface if it didn't have the data_available
>> callback.
> 
> So there should already be provisions in place for *not* registering the
> cb buffer with user-space, meaning no additional logic would be required
> for exposing / not exposing a "data_available" sysfs attribute, correct?

Yes.

[PATCH v2] iio: buffer: Expose data available

[mfornero]

From: Matt Fornero <matt.fornero@mathworks.com>

Add a sysfs attribute that exposes buffer data available to userspace.
This attribute can be checked at runtime to determine the overall buffer
fill level (across all allocated buffers).

Signed-off-by: Matt Fornero <matt.fornero@mathworks.com>
---
v1 -> v2:
 - Use iio_buffer_data_available() for all buffers instead
   of only DMA buffers

 drivers/iio/industrialio-buffer.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index d80f830..e7bdfa3 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -1121,6 +1121,18 @@ static ssize_t iio_buffer_store_watermark(struct device *dev,
 	return ret ? ret : len;
 }
 
+static ssize_t iio_dma_show_data_available(struct device *dev,
+						struct device_attribute *attr,
+						char *buf)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	size_t bytes;
+
+	bytes = iio_buffer_data_available(indio_dev->buffer);
+
+	return sprintf(buf, "%llu\n", (unsigned long long)bytes);
+}
+
 static DEVICE_ATTR(length, S_IRUGO | S_IWUSR, iio_buffer_read_length,
 	iio_buffer_write_length);
 static struct device_attribute dev_attr_length_ro = __ATTR(length,
@@ -1131,11 +1143,14 @@ static DEVICE_ATTR(watermark, S_IRUGO | S_IWUSR,
 		   iio_buffer_show_watermark, iio_buffer_store_watermark);
 static struct device_attribute dev_attr_watermark_ro = __ATTR(watermark,
 	S_IRUGO, iio_buffer_show_watermark, NULL);
+static DEVICE_ATTR(data_available, S_IRUGO,
+		iio_dma_show_data_available, NULL);
 
 static struct attribute *iio_buffer_attrs[] = {
 	&dev_attr_length.attr,
 	&dev_attr_enable.attr,
 	&dev_attr_watermark.attr,
+	&dev_attr_data_available.attr,
 };
 
 int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
-- 
1.8.3.1

Re: [PATCH v2] iio: buffer: Expose data available

[Lars-Peter Clausen]

On 12/05/2017 09:56 PM, mfornero@gmail.com wrote:
> From: Matt Fornero <matt.fornero@mathworks.com>
> 
> Add a sysfs attribute that exposes buffer data available to userspace.
> This attribute can be checked at runtime to determine the overall buffer
> fill level (across all allocated buffers).
> 
> Signed-off-by: Matt Fornero <matt.fornero@mathworks.com>

Looks sensible to me. I think we need an update to the ABI documentation as
well for this in Documentation/ABI/testing/sysfs-bus-iio

> ---
> v1 -> v2:
>  - Use iio_buffer_data_available() for all buffers instead
>    of only DMA buffers
> 
>  drivers/iio/industrialio-buffer.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index d80f830..e7bdfa3 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -1121,6 +1121,18 @@ static ssize_t iio_buffer_store_watermark(struct device *dev,
>  	return ret ? ret : len;
>  }
>  
> +static ssize_t iio_dma_show_data_available(struct device *dev,
> +						struct device_attribute *attr,
> +						char *buf)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	size_t bytes;
> +
> +	bytes = iio_buffer_data_available(indio_dev->buffer);
> +
> +	return sprintf(buf, "%llu\n", (unsigned long long)bytes);

Since this is the kernel we shouldn't need those MSVC workarounds ;)
"%zu" should work

> +}
[..]

[PATCH v3] iio: buffer: Expose data available

[mfornero]

From: Matt Fornero <matt.fornero@mathworks.com>

Add a sysfs attribute that exposes buffer data available to userspace.
This attribute can be checked at runtime to determine the overall buffer
fill level (across all allocated buffers).

Signed-off-by: Matt Fornero <matt.fornero@mathworks.com>
---
v2 -> v3:
 - Use %zu for sprintf instead of %llu
 - Add documentation

 Documentation/ABI/testing/sysfs-bus-iio | 10 ++++++++++
 drivers/iio/industrialio-buffer.c       | 15 +++++++++++++++
 2 files changed, 25 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
index a478740..9cc0ea1 100644
--- a/Documentation/ABI/testing/sysfs-bus-iio
+++ b/Documentation/ABI/testing/sysfs-bus-iio
@@ -1413,6 +1413,16 @@ Description:
 		the available samples after the timeout expires and thus have a
 		maximum delay guarantee.
 
+What:		/sys/bus/iio/devices/iio:deviceX/buffer/data_available
+KernelVersion: 4.16
+Contact:	linux-iio@vger.kernel.org
+Description:
+		A read-only value indicating the bytes of data available in the
+		buffer. In the case of an output buffer, this indicates the
+		amount of empty space available to write data to. In the case of
+		an input buffer, this indicates the amount of data available for
+		reading.
+
 What:		/sys/bus/iio/devices/iio:deviceX/buffer/hwfifo_enabled
 KernelVersion: 4.2
 Contact:	linux-iio@vger.kernel.org
diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index d2b4651..eda2a0f 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -1198,6 +1198,18 @@ static ssize_t iio_buffer_store_watermark(struct device *dev,
 	return ret ? ret : len;
 }
 
+static ssize_t iio_dma_show_data_available(struct device *dev,
+						struct device_attribute *attr,
+						char *buf)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	size_t bytes;
+
+	bytes = iio_buffer_data_available(indio_dev->buffer);
+
+	return sprintf(buf, "%zu\n", bytes);
+}
+
 static DEVICE_ATTR(length, S_IRUGO | S_IWUSR, iio_buffer_read_length,
 		   iio_buffer_write_length);
 static struct device_attribute dev_attr_length_ro = __ATTR(length,
@@ -1208,11 +1220,14 @@ static DEVICE_ATTR(watermark, S_IRUGO | S_IWUSR,
 		   iio_buffer_show_watermark, iio_buffer_store_watermark);
 static struct device_attribute dev_attr_watermark_ro = __ATTR(watermark,
 	S_IRUGO, iio_buffer_show_watermark, NULL);
+static DEVICE_ATTR(data_available, S_IRUGO,
+		iio_dma_show_data_available, NULL);
 
 static struct attribute *iio_buffer_attrs[] = {
 	&dev_attr_length.attr,
 	&dev_attr_enable.attr,
 	&dev_attr_watermark.attr,
+	&dev_attr_data_available.attr,
 };
 
 int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
-- 
1.8.3.1

Re: [PATCH v3] iio: buffer: Expose data available

[Jonathan Cameron]

On Wed,  6 Dec 2017 14:43:30 -0500
mfornero@gmail.com wrote:

> From: Matt Fornero <matt.fornero@mathworks.com>
> 
> Add a sysfs attribute that exposes buffer data available to userspace.
> This attribute can be checked at runtime to determine the overall buffer
> fill level (across all allocated buffers).

There are a few odd cases where we don't actually know the right answer
to this case, but we can 'estimate it'.  The classic would be a hardware
fifo like the sca3000 where (IIRC) you don't have the ability to query the
fill level of the buffer, but only whether it is empty.  You also have
watershed interrupts that let you know there is 'at least' this much in the
buffer.  Currently we route that one through a software kfifo anyway so
it doesn't matter here  - just thought I'd mention it ;)

Anyhow, looks fine to me.  The description including output buffers
 is good future proofing but that code isn't yet upstream.

I don't have an issue with having it in the description though!

Applied to the togreg branch of iio.git and pushed out as testing.
Lars, if you would like to add a tag let me know as I won't be pushing this
out in non rebasing form until at least next weekend.  Obviously I can
also back it out of change things if you have comments that require it
as well!

Thanks,

Jonathan

> 
> Signed-off-by: Matt Fornero <matt.fornero@mathworks.com>
> ---
> v2 -> v3:
>  - Use %zu for sprintf instead of %llu
>  - Add documentation
> 
>  Documentation/ABI/testing/sysfs-bus-iio | 10 ++++++++++
>  drivers/iio/industrialio-buffer.c       | 15 +++++++++++++++
>  2 files changed, 25 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
> index a478740..9cc0ea1 100644
> --- a/Documentation/ABI/testing/sysfs-bus-iio
> +++ b/Documentation/ABI/testing/sysfs-bus-iio
> @@ -1413,6 +1413,16 @@ Description:
>  		the available samples after the timeout expires and thus have a
>  		maximum delay guarantee.
>  
> +What:		/sys/bus/iio/devices/iio:deviceX/buffer/data_available
> +KernelVersion: 4.16
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		A read-only value indicating the bytes of data available in the
> +		buffer. In the case of an output buffer, this indicates the
> +		amount of empty space available to write data to. In the case of
> +		an input buffer, this indicates the amount of data available for
> +		reading.
> +
>  What:		/sys/bus/iio/devices/iio:deviceX/buffer/hwfifo_enabled
>  KernelVersion: 4.2
>  Contact:	linux-iio@vger.kernel.org
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index d2b4651..eda2a0f 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -1198,6 +1198,18 @@ static ssize_t iio_buffer_store_watermark(struct device *dev,
>  	return ret ? ret : len;
>  }
>  
> +static ssize_t iio_dma_show_data_available(struct device *dev,
> +						struct device_attribute *attr,
> +						char *buf)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	size_t bytes;
> +
> +	bytes = iio_buffer_data_available(indio_dev->buffer);
> +
> +	return sprintf(buf, "%zu\n", bytes);
> +}
> +
>  static DEVICE_ATTR(length, S_IRUGO | S_IWUSR, iio_buffer_read_length,
>  		   iio_buffer_write_length);
>  static struct device_attribute dev_attr_length_ro = __ATTR(length,
> @@ -1208,11 +1220,14 @@ static DEVICE_ATTR(watermark, S_IRUGO | S_IWUSR,
>  		   iio_buffer_show_watermark, iio_buffer_store_watermark);
>  static struct device_attribute dev_attr_watermark_ro = __ATTR(watermark,
>  	S_IRUGO, iio_buffer_show_watermark, NULL);
> +static DEVICE_ATTR(data_available, S_IRUGO,
> +		iio_dma_show_data_available, NULL);
>  
>  static struct attribute *iio_buffer_attrs[] = {
>  	&dev_attr_length.attr,
>  	&dev_attr_enable.attr,
>  	&dev_attr_watermark.attr,
> +	&dev_attr_data_available.attr,
>  };
>  
>  int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)