On Wed, Dec 30, 2020 at 03:04:40PM +0000, Jonathan Cameron wrote:

On Fri, 25 Dec 2020 19:15:36 -0500
William Breathitt Gray [off-list ref] 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>

There are a few things in here that could profitably be pulled out as precursor
patches.  I don't really understand the connection of extension_name to the
addition of a chardev for example.  Might be needed to provide enough
info to actually use the chardev, but does it have meaning without that?
Either way, definitely feels like it can be done in a separate patch.

The extension_name attributes are needed so chrdev users have enough
info to identify which extension number corresponds to which extension.
I'll move this to change to a separate patch and provide an appropriate
explanation there to make things clearer.

quoted

+static long counter_chrdev_ioctl(struct file *filp, unsigned int cmd,
+				 unsigned long arg)
+{
+	struct counter_device *const counter = filp->private_data;
+	unsigned long flags;
+	int err = 0;
+
+	switch (cmd) {
+	case COUNTER_CLEAR_WATCHES_IOCTL:
+		return counter_clear_watches(counter);
+	case COUNTER_ADD_WATCH_IOCTL:
+		return counter_add_watch(counter, arg);
+	case COUNTER_LOAD_WATCHES_IOCTL:
+		raw_spin_lock_irqsave(&counter->events_list_lock, flags);
+
+		counter_events_list_free(&counter->events_list);
+		list_replace_init(&counter->next_events_list,
+				  &counter->events_list);
+
+		if (counter->ops->events_configure)
+			err = counter->ops->events_configure(counter);
+
+		raw_spin_unlock_irqrestore(&counter->events_list_lock, flags);
+		break;

return here. 

Ack.

quoted

+static int counter_get_data(struct counter_device *const counter,
+			    const struct counter_comp_node *const comp_node,
+			    u64 *const value)
+{
+	const struct counter_comp *const comp = &comp_node->comp;
+	void *const parent = comp_node->parent;
+	int err = 0;
+	u8 value_u8 = 0;
+	u32 value_u32 = 0;
+
+	if (comp_node->component.type == COUNTER_COMPONENT_NONE)
+		return 0;
+
+	switch (comp->type) {
+	case COUNTER_COMP_U8:
+	case COUNTER_COMP_BOOL:
+		switch (comp_node->component.scope) {
+		case COUNTER_SCOPE_DEVICE:
+			err = comp->device_u8_read(counter, &value_u8);
+			break;
+		case COUNTER_SCOPE_SIGNAL:
+			err = comp->signal_u8_read(counter, parent, &value_u8);
+			break;
+		case COUNTER_SCOPE_COUNT:
+			err = comp->count_u8_read(counter, parent, &value_u8);
+			break;
+		}
+		*value = value_u8;
+		break;
+	case COUNTER_COMP_SIGNAL_LEVEL:
+	case COUNTER_COMP_FUNCTION:
+	case COUNTER_COMP_ENUM:
+	case COUNTER_COMP_COUNT_DIRECTION:
+	case COUNTER_COMP_COUNT_MODE:
+		switch (comp_node->component.scope) {
+		case COUNTER_SCOPE_DEVICE:
+			err = comp->device_u32_read(counter, &value_u32);
+			break;
+		case COUNTER_SCOPE_SIGNAL:
+			err = comp->signal_u32_read(counter, parent,
+						    &value_u32);
+			break;
+		case COUNTER_SCOPE_COUNT:
+			err = comp->count_u32_read(counter, parent, &value_u32);
+			break;
+		}
+		*value = value_u32;

Seems like a return here would make more sense as no shared stuff to do at
end of the switch. Same in other similar cases.

Ack.

quoted

+		break;
+	case COUNTER_COMP_U64:
+		switch (comp_node->component.scope) {
+		case COUNTER_SCOPE_DEVICE:
+			return comp->device_u64_read(counter, value);
+		case COUNTER_SCOPE_SIGNAL:
+			return comp->signal_u64_read(counter, parent, value);
+		case COUNTER_SCOPE_COUNT:
+			return comp->count_u64_read(counter, parent, value);
+		}
+		break;
+	case COUNTER_COMP_SYNAPSE_ACTION:
+		err = comp->action_read(counter, parent, comp->priv,
+					&value_u32);
+		*value = value_u32;
+		break;
+	}
+
+	return err;
+}
+
+/**
+ * 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.
+ */
+void 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;
+
+	ev.timestamp = ktime_get_ns();
+	ev.watch.event = event;
+	ev.watch.channel = channel;
+
+	raw_spin_lock_irqsave(&counter->events_list_lock, flags);

For a raw spin lock, we definitely want to see comments on why it
is necessary.

Ack.

quoted

@@ -650,7 +670,7 @@ static int counter_count_attrs_create(struct counter_device *const counter,
 		return err;
 
 	/* Create Count name attribute */
-	err = counter_name_attr_create(dev, group, count->name);
+	err = counter_name_attr_create(dev, group, "name", count->name);

This refactoring could also be pulled out to a precusor patch.

Ack. This will be part of the extension_name patch.

quoted

@@ -319,12 +315,21 @@ struct counter_device {
 
 	int id;
 	struct device dev;
+	struct cdev chrdev;
+	struct list_head events_list;
+	raw_spinlock_t events_list_lock;
+	struct list_head next_events_list;
+	DECLARE_KFIFO(events, struct counter_event, 64);

Why 64?  Probably want that to be somewhat dynamic, even if only at build time.

Ack. This will be dynamically configurable via sysfs attribute in v8.

quoted

+	wait_queue_head_t events_wait;
+	struct mutex events_lock;
 };
 
 int counter_register(struct counter_device *const counter);
 void counter_unregister(struct counter_device *const counter);
 int devm_counter_register(struct device *dev,
 			  struct counter_device *const counter);
+void counter_push_event(struct counter_device *const counter, const u8 event,
+			const u8 channel);
 
 #define COUNTER_COMP_DEVICE_U8(_name, _read, _write) \
 { \

diff --git a/include/uapi/linux/counter.h b/include/uapi/linux/counter.h
new file mode 100644
index 000000000000..7585dc9db19d
--- /dev/null
+++ b/include/uapi/linux/counter.h

Small thing but I would have been tempted to do a precursor patch to the
main change simply putting in place the userspace header.

Classic Nop patch that makes it easier to focus on the real stuff in this
patch by getting that noise out of the way!

Jonathan

Ack.

William Breathitt Gray