Re: [PATCH v6 3/5] counter: Add character device interface

On Thu, 21 Jan 2021 17:03:11 +0900
William Breathitt Gray [off-list ref] wrote:

On Tue, Jan 19, 2021 at 10:20:22AM +0100, Oleksij Rempel wrote:

quoted

On Sun, Nov 22, 2020 at 03:29:54PM -0500, William Breathitt Gray wrote:  

quoted

This patch introduces a character device interface for the Counter
subsystem. Device data is exposed through standard character device read
operations. Device data is gathered when a Counter event is pushed by
the respective Counter device driver. Configuration is handled via ioctl
operations on the respective Counter character device node.

Cc: David Lechner <david@lechnology.com>
Cc: Gwendal Grignou <redacted>
Cc: Dan Carpenter <redacted>
Signed-off-by: William Breathitt Gray <redacted>
---  

Hello William,

the series looks quite interesting, we have some thoughts... see below:

[...]  

quoted

+/**
+ * counter_push_event - queue event for userspace reading
+ * @counter:	pointer to Counter structure
+ * @event:	triggered event
+ * @channel:	event channel
+ *
+ * Note: If no one is watching for the respective event, it is silently
+ * discarded.
+ *
+ * RETURNS:
+ * 0 on success, negative error number on failure.
+ */
+int counter_push_event(struct counter_device *const counter, const u8 event,
+		       const u8 channel)
+{
+	struct counter_event ev = {0};
+	unsigned int copied = 0;
+	unsigned long flags;
+	struct counter_event_node *event_node;
+	struct counter_comp_node *comp_node;
+	int err = 0;
+
+	ev.timestamp = ktime_get_ns();
+	ev.watch.event = event;
+	ev.watch.channel = channel;
+
+	raw_spin_lock_irqsave(&counter->events_lock, flags);
+
+	/* Search for event in the list */
+	list_for_each_entry(event_node, &counter->events_list, l)
+		if (event_node->event == event &&
+		    event_node->channel == channel)
+			break;
+
+	/* If event is not in the list */
+	if (&event_node->l == &counter->events_list)
+		goto exit_early;
+
+	/* Read and queue relevant comp for userspace */
+	list_for_each_entry(comp_node, &event_node->comp_list, l) {
+		err = counter_get_data(counter, comp_node, &ev.value);
+		if (err)
+			goto exit_early;
+
+		ev.watch.component = comp_node->component;
+
+		copied += kfifo_put(&counter->events, ev);  

We want to calculate the frequency of some IRQ pulses in user space and
counter values with time stamps really fits well here. As the pulses are
from a physical system (rotating wheel), they will only change at a
certain rate. We want to have the possibility to read from the counter
device less often, we intentionally want to skip (meaning miss)
events.

When reading we're interested in the newest events. The kfifo implements
a "tail" drop FIFO, which means new values are added at the end, and if
the FIFO is full, they are dropped. We need a "head" drop FIFO which
discards the oldest events, keeping only the recent ones.

As far as we know, kfifo doesn't offer a head drop mode, but I think
this can be added.  

I'm not sure if kfifo has this kind of mode, but it seems like something
that should be there if it is not already -- I imagine this kind of
operation mode would be pretty common. Perhaps someone knows how to
achieve this and will share here.

I don't think it does.  In IIO we started with a ring (effectively this)
- hopefully no one else remembers the horrible mess I made of it ;),
but eventually decided that there were very few usecases that
actually demanded it so just switched over to kfifo.

It's actually pretty rare that you aren't better off draining to
a backing store of some type.

Not had any demand to bring a ring back to IIO since we dropped it.

quoted

[...]
  

quoted

 struct counter_device {
 	const char *name;

@@ -270,12 +270,20 @@ struct counter_device {
 
 	int id;
 	struct device dev;
+	struct cdev chrdev;
+	raw_spinlock_t events_lock;
+	struct list_head events_list;
+	struct list_head next_events_list;
+	DECLARE_KFIFO(events, struct counter_event, 64);  

Do you plan to make the size of the FIFO configurable?

regards,
Oleksij & Marc  

I suppose it wouldn't be a problem to make this a configurable setting;
I think I can implement this via kfifo_alloc() and kfifo_free(). How
would users control this -- maybe using a sysfs attribute?

Would make sense to do it from sysfs.

Jonathan

William Breathitt Gray