On Fri, Feb 26, 2021 at 06:14:12PM -0600, David Lechner wrote:

On 2/25/21 6:03 PM, William Breathitt Gray wrote:

quoted

On Sun, Feb 21, 2021 at 03:51:40PM +0000, Jonathan Cameron wrote:

quoted

On Thu, 18 Feb 2021 19:32:16 +0900
William Breathitt Gray [off-list ref] wrote:

quoted

On Sun, Feb 14, 2021 at 06:11:46PM +0000, Jonathan Cameron wrote:

quoted

On Fri, 12 Feb 2021 21:13:44 +0900
William Breathitt Gray [off-list ref] wrote:
   

quoted

The events_queue_size sysfs attribute provides a way for users to
dynamically configure the Counter events queue size for the Counter
character device interface. The size is in number of struct
counter_event data structures. The number of elements will be rounded-up
to a power of 2 due to a requirement of the kfifo_alloc function called
during reallocation of the queue.

Cc: Oleksij Rempel <o.rempel@pengutronix.de>
Signed-off-by: William Breathitt Gray <redacted>
---
  Documentation/ABI/testing/sysfs-bus-counter |  8 +++++++
  drivers/counter/counter-chrdev.c            | 23 +++++++++++++++++++
  drivers/counter/counter-chrdev.h            |  2 ++
  drivers/counter/counter-sysfs.c             | 25 +++++++++++++++++++++
  4 files changed, 58 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-counter b/Documentation/ABI/testing/sysfs-bus-counter
index 847e96f19d19..f6cb2a8b08a7 100644
--- a/Documentation/ABI/testing/sysfs-bus-counter
+++ b/Documentation/ABI/testing/sysfs-bus-counter

@@ -212,6 +212,14 @@ Description:
  		both edges:
  			Any state transition.
  
+What:		/sys/bus/counter/devices/counterX/events_queue_size
+KernelVersion:	5.13
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Size of the Counter events queue in number of struct
+		counter_event data structures. The number of elements will be
+		rounded-up to a power of 2.
+
  What:		/sys/bus/counter/devices/counterX/name
  KernelVersion:	5.2
  Contact:	linux-iio@vger.kernel.org

diff --git a/drivers/counter/counter-chrdev.c b/drivers/counter/counter-chrdev.c
index 16f02df7f73d..53eea894e13f 100644
--- a/drivers/counter/counter-chrdev.c
+++ b/drivers/counter/counter-chrdev.c

@@ -375,6 +375,29 @@ void counter_chrdev_remove(struct counter_device *const counter)
  	cdev_del(&counter->chrdev);
  }
  
+int counter_chrdev_realloc_queue(struct counter_device *const counter,
+				 size_t queue_size)
+{
+	int err;
+	DECLARE_KFIFO_PTR(events, struct counter_event);
+	unsigned long flags;
+
+	/* Allocate new events queue */
+	err = kfifo_alloc(&events, queue_size, GFP_ATOMIC);

Is there any potential for losing events?

We take the events_list_lock down below so we're safe against missing an
event, but past events currently unread in the queue will be lost.

Shortening the size of the queue is inherently a destructive process if
we have more events in the current queue than can fit in the new queue.
Because we a liable to lose some events in such a case, I think it's
best to keep the behavior of this reallocation consistent and have it
provide a fresh empty queue every time, as opposed to sometimes dropping
events and sometimes not.

I also suspect an actual user would be setting the size of their queue
to the required amount before they begin watching events, rather than
adjusting it sporadically during a live operation.

Absolutely agree.   As such I wonder if you are better off enforcing this
behaviour?  If the cdev is open for reading, don't allow the fifo to be
resized.

Jonathan

I can't really think of a good reason not to, so let's enforce it: if
the cdev is open, then we'll return an EINVAL if the user attempts to
resize the queue.

What is a good way to check for this condition? Should I just call
kref_read() and see if it's greater than 1? For example, in
counter_chrdev_realloc_queue():

	if (kref_read(&counter->dev.kobj.kref) > 1)
		return -EINVAL;

William Breathitt Gray

Wouldn't EBUSY make more sense?

Yes, EBUSY would be better here because the operation isn't necessarily
invalid, just unavailable because someone else has the cdev open.

William Breathitt Gray