counter: fix, improvements and stm32 timer events support

When trying to watch a component array extension, and the array isn't the first extended element, it fails as the type comparison is always done on the 1st element. Fix it by indexing the 'ext' array.

Example on a dummy struct counter_comp:

static struct counter_comp dummy[] = {
	COUNTER_COMP_DIRECTION(..),
	...,
	COUNTER_COMP_ARRAY_CAPTURE(...),
};

static struct counter_count dummy_cnt = {
...
.ext = dummy,
.num_ext = ARRAY_SIZE(dummy),
}

Currently, counter_get_ext() returns -EINVAL when trying to add a watch event on one of the capture array element in such example.

Fixes d2011be1e22f ("counter: Introduce the COUNTER_COMP_ARRAY component type")

Signed-off-by Fabrice Gasnier <[email protected]>

Replace COUNTER_COUNT_SCOPE that doesn't exist by the defined COUNTER_SCOPE_COUNT.

Signed-off-by Fabrice Gasnier <[email protected]>

This adds a new counter tool to be able to test various watch events. A flexible watch array can be populated from command line, each field may be tuned with a dedicated command line argument.
Each argument can be repeated several times: each time it gets repeated, a corresponding new watch element is allocated.

It also comes with a simple default watch (to monitor overflows), used when no watch parameters are provided.

The print_usage() routine proposes another example, from the command line, which generates a 2 elements watch array, to monitor:
- overflow events
- capture events, on channel 3, that reads read captured data by specifying the component id (capture3_component_id being 7 here).

Signed-off-by Fabrice Gasnier <[email protected]>

---
 tools/counter/Build                  |   1 +
 tools/counter/Makefile               |   8 +-
 tools/counter/counter_watch_events.c | 348 +++++++++++++++++++++++++++
 3 files changed, 356 insertions(+), 1 deletion(-)
 create mode 100644 tools/counter/counter_watch_events.c

tools/counter/Build +1 -0

index 33f4a51d715e..4bbadb7ec93a 100644
@@ -1 +1,2 @@
 counter_example-y += counter_example.o
+counter_watch_events-y += counter_watch_events.o

tools/counter/Makefile +1 -1

index b2c2946f44c9..00e211edd768 100644
@@ -14,7 +14,7 @@ MAKEFLAGS += -r
 
 override CFLAGS += -O2 -Wall -g -D_GNU_SOURCE -I$(OUTPUT)include
 
-ALL_TARGETS := counter_example
+ALL_TARGETS := counter_example counter_watch_events
 ALL_PROGRAMS := $(patsubst %,$(OUTPUT)%,$(ALL_TARGETS))
 
 all: $(ALL_PROGRAMS)

tools/counter/counter_watch_events.c new file +8 -0

+		/* Device event channel 0 */
+		.channel = 0,
+	},
+};
+
+static int find_match_or_number_from_array(char *arg, const char * const str[], int sz, __u8 *val)
+{
+	unsigned int i;

tools/counter/counter_watch_events.c new file +77 -0

+	idx = strtoul(optarg, &dummy, 10);
+	if (!errno) {
+		if (idx >= sz)
+			return -EINVAL;
+		*val = idx;
+		return 0;
+	}
+
+	return -errno;
+}
+
+static const char * const counter_event_type_name[] = {
+	"COUNTER_EVENT_OVERFLOW",
+	"COUNTER_EVENT_UNDERFLOW",
+	"COUNTER_EVENT_OVERFLOW_UNDERFLOW",
+	"COUNTER_EVENT_THRESHOLD",
+	"COUNTER_EVENT_INDEX",
+	"COUNTER_EVENT_CHANGE_OF_STATE",
+	"COUNTER_EVENT_CAPTURE",
+};
+
+static int counter_arg_to_event_type(char *arg, __u8 *event)
+{
+	return find_match_or_number_from_array(arg, counter_event_type_name,
+					       ARRAY_SIZE(counter_event_type_name), event);
+}
+
+static const char * const counter_component_type_name[] = {
+	"COUNTER_COMPONENT_NONE",
+	"COUNTER_COMPONENT_SIGNAL",
+	"COUNTER_COMPONENT_COUNT",
+	"COUNTER_COMPONENT_FUNCTION",
+	"COUNTER_COMPONENT_SYNAPSE_ACTION",
+	"COUNTER_COMPONENT_EXTENSION",
+};
+
+static int counter_arg_to_component_type(char *arg, __u8 *type)
+{
+	return find_match_or_number_from_array(arg, counter_component_type_name,
+					       ARRAY_SIZE(counter_component_type_name), type);
+}
+
+static const char * const counter_scope_name[] = {
+	"COUNTER_SCOPE_DEVICE",
+	"COUNTER_SCOPE_SIGNAL",
+	"COUNTER_SCOPE_COUNT",
+};
+
+static int counter_arg_to_scope(char *arg, __u8 *type)
+{
+	return find_match_or_number_from_array(arg, counter_scope_name,
+					       ARRAY_SIZE(counter_scope_name), type);
+}
+
+static void print_usage(void)
+{
+	fprintf(stderr, "Usage: counter_watch_events [options]...\n"
+		"Test various watch events for given counter device\n"
+		"  --channel -c <n>\n"
+		"        Set watch.channel\n"
+		"  --debug -d\n"
+		"        Prints debug information\n"
+		"  --event -e <number or counter_event_type string>\n"
+		"        Sets watch.event\n"
+		"  --help -h\n"
+		"        Prints usage\n"
+		"  --device-num -n <n>\n"
+		"        Set device number (/dev/counter<n>, default to 0)\n"
+		"  --id -i <n>\n"
+		"        Set watch.component.id\n"
+		"  --loop -l <n>\n"
+		"        Loop for a number of events (forever if n < 0)\n"
+		"  --parent -p <n>\n"
+		"        Set watch.component.parent number\n"
+		"  --scope -s <number or counter_scope string>\n"
+		"        Set watch.component.scope\n"
+		"  --type -t <number or counter_component_type string>\n"

tools/counter/counter_watch_events.c new file +47 -0

+
+static void print_watch(struct counter_watch *watch, int nwatch)
+{
+	int i;
+
+	/* prints the watch array in C-like structure */
+	printf("watch[%d] = {\n", nwatch);
+	for (i = 0; i < nwatch; i++) {
+		printf(" [%d] =\t{\n"
+		       "\t\t.component.type = %s\n"
+		       "\t\t.component.scope = %s\n"
+		       "\t\t.component.parent = %d\n"
+		       "\t\t.component.id = %d\n"
+		       "\t\t.event = %s\n"
+		       "\t\t.channel = %d\n"
+		       "\t},\n",
+		       i,
+		       counter_component_type_name[watch[i].component.type],
+		       counter_scope_name[watch[i].component.scope],
+		       watch[i].component.parent,
+		       watch[i].component.id,
+		       counter_event_type_name[watch[i].event],
+		       watch[i].channel);
+	}
+	printf("};\n");
+}
+
+static const struct option longopts[] = {
+	{ "channel",		required_argument, 0, 'c' },
+	{ "debug",		no_argument,       0, 'd' },
+	{ "event",		required_argument, 0, 'e' },
+	{ "help",		no_argument,       0, 'h' },
+	{ "device-num",		required_argument, 0, 'n' },
+	{ "id",			required_argument, 0, 'i' },
+	{ "loop",		required_argument, 0, 'l' },
+	{ "parent",		required_argument, 0, 'p' },
+	{ "scope",		required_argument, 0, 's' },
+	{ "type",		required_argument, 0, 't' },
+	{ },
+};
+
+int main(int argc, char **argv)
+{
+	int c, fd, i, ret;
+	struct counter_event event_data;
+	char *device_name = NULL;
+	int debug = 0, loop = -1;

tools/counter/counter_watch_events.c new file +19 -0

+	while ((c = getopt_long(argc, argv, "c:de:hn:i:l:p:s:t:", longopts, NULL)) != -1) {
+		switch (c) {
+		case 'c':
+			ncfg[0]++;
+			break;
+		case 'e':
+			ncfg[1]++;
+			break;
+		case 'i':
+			ncfg[2]++;
+			break;
+		case 'p':
+			ncfg[3]++;
+			break;
+		case 's':
+			ncfg[4]++;
+			break;
+		case 't':
+			ncfg[5]++;

Quoted 15 lines · Fabrice Gasnier

This adds a new counter tool to be able to test various watch events. A flexible watch array can be populated from command line, each field may be tuned with a dedicated command line argument.
Each argument can be repeated several times: each time it gets repeated, a corresponding new watch element is allocated.

It also comes with a simple default watch (to monitor overflows), used when no watch parameters are provided.

The print_usage() routine proposes another example, from the command line, which generates a 2 elements watch array, to monitor:
- overflow events
- capture events, on channel 3, that reads read captured data by specifying the component id (capture3_component_id being 7 here).

Quoted 1 line

Signed-off-by Fabrice Gasnier <[email protected]>

Hi Fabrice,

This is great idea, it'll make it so much easier to test out drivers so I'm excited! :-)

This is a new tool, so would you add a MAINTAINERS entry for the counter_watch_events.c file?

More comments inline below.

Quoted 31 lines

---
 tools/counter/Build                  |   1 +
 tools/counter/Makefile               |   8 +-
 tools/counter/counter_watch_events.c | 348 +++++++++++++++++++++++++++
 3 files changed, 356 insertions(+), 1 deletion(-)
 create mode 100644 tools/counter/counter_watch_events.c

tools/counter/Build +1 -0

index 33f4a51d715e..4bbadb7ec93a 100644
@@ -1 +1,2 @@
 counter_example-y += counter_example.o
+counter_watch_events-y += counter_watch_events.o

tools/counter/Makefile +1 -1

index b2c2946f44c9..00e211edd768 100644
@@ -14,7 +14,7 @@ MAKEFLAGS += -r
 
 override CFLAGS += -O2 -Wall -g -D_GNU_SOURCE -I$(OUTPUT)include
 
-ALL_TARGETS := counter_example
+ALL_TARGETS := counter_example counter_watch_events
 ALL_PROGRAMS := $(patsubst %,$(OUTPUT)%,$(ALL_TARGETS))
 
 all: $(ALL_PROGRAMS)

@@ -31,6 +31,12 @@ $(OUTPUT)include/linux/counter.h: ../../include/uapi/linux/counter.h
 
 prepare: $(OUTPUT)include/linux/counter.h
 

tools/counter/Makefile $(OUTPUT)include/linux/counter.h: ../../include/uapi/linux/counter.h · line 39

Quoted 6 lines

tools/counter/Makefile +6 -0

+COUNTER_WATCH_EVENTS := $(OUTPUT)counter_watch_events.o
+$(COUNTER_WATCH_EVENTS): prepare FORCE
+	$(Q)$(MAKE) $(build)=counter_watch_events
+$(OUTPUT)counter_watch_events: $(COUNTER_WATCH_EVENTS)
+	$(QUIET_LINK)$(CC) $(CFLAGS) $(LDFLAGS) $< -o $@
+

Move this below the COUNTER_EXAMPLE block just so we can keep the recipes in alphabetical order.

tools/counter/counter_watch_events.c line 2

Quoted 12 lines

tools/counter/Makefile +0 -0

 COUNTER_EXAMPLE := $(OUTPUT)counter_example.o
 $(COUNTER_EXAMPLE): prepare FORCE
 	$(Q)$(MAKE) $(build)=counter_example

tools/counter/counter_watch_events.c new file +2 -0

new file mode 100644
index 000000000000..7f73a1519d8e
@@ -0,0 +1,348 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/* Counter - Test various watch events in a userspace application

"Counter" should be "Counter Watch Events" (or "counter_watch_events").

tools/counter/counter_watch_events.c line 3

Quoted 1 line

tools/counter/counter_watch_events.c new file +1 -0

+ * inspired by counter_example.c

No need to mention counter_example.c, this utility does far more than and bares little resemblance at this point to counter_example.c which is really just a bare minimal example of watching Counter events.

Quoted 12 lines

tools/counter/counter_watch_events.c new file +11 -0

+ */
+
+#include <errno.h>
+#include <fcntl.h>
+#include <getopt.h>
+#include <linux/counter.h>
+#include <stdlib.h>
+#include <stdio.h>
+#include <string.h>
+#include <sys/ioctl.h>
+#include <unistd.h>

tools/counter/counter_watch_events.c line 16

Quoted 2 lines

tools/counter/counter_watch_events.c new file +2 -0

+
+#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))

My initial reaction was that this macro is already exposed in some header for us, but my local /usr/include/linux/kernel.h file doesn't appear to bare it so I guess not. Perhaps it'll be fine for our needs -- I think the only difference between this ARRAY_SIZE and the Linux kernel one is the addition of __must_be_array(x).

tools/counter/counter_watch_events.c line 25

Quoted 10 lines

tools/counter/counter_watch_events.c new file +9 -0

+
+static struct counter_watch simple_watch[] = {
+	{
+		/* Component data: Count 0 count */
+		.component.type = COUNTER_COMPONENT_COUNT,
+		.component.scope = COUNTER_SCOPE_COUNT,
+		.component.parent = 0,
+		/* Event type: Index */
+		.event = COUNTER_EVENT_OVERFLOW_UNDERFLOW,

There's a bit of confusion here. The comment says the event type is INDEX, but the structure event type is set for OVERFLOW_UNDERFLOW; also, the commit description states that we're monitoring "overflows" which implies to me the OVERFLOW event type. So which of the three is it?

Quoted 9 lines

tools/counter/counter_watch_events.c new file +8 -0

+		/* Device event channel 0 */
+		.channel = 0,
+	},
+};
+
+static int find_match_or_number_from_array(char *arg, const char * const str[], int sz, __u8 *val)
+{
+	unsigned int i;

tools/counter/counter_watch_events.c line 43

Quoted 10 lines

tools/counter/counter_watch_events.c new file +10 -0

+	char *dummy;
+	unsigned long idx;
+	int ret;
+
+	for (i = 0; i < sz; i++) {
+		ret = strncmp(arg, str[i], strlen(str[i]));
+		if (!ret && strlen(str[i]) == strlen(arg)) {
+			*val = i;
+			return 0;
+		}

This has several strlen calls so I wonder if it's more wasteful than it needs to me. I suppose the compiler would optimize this away, but I think there is an alternative solution.

We're checking for an exact match, so you don't need the string length. Instead, you can compare each character, break when characters differ, or return 0 when you reached the null byte for both. So something like this:

    for (j = 0; arg[j] == str[i][j]; j++) {
            /* If we reached the end of the strings */
            if (arg[j] == '\0') {
                    *val = i;
                    return 0;
            }
    }
    /* Strings do not match; continue to the next string */

We end up with the same number of lines, so I'll leave it up to you whether you want to use this solution, or if you consider the existing code clearer read.

tools/counter/counter_watch_events.c line 46

Quoted 4 lines

tools/counter/counter_watch_events.c new file +3 -0

+	}
+
+	/* fallback to number */

I'm not sure it makes sense to support numbers. Although it's true that the component type, component scope, and event type are passed as __u8 values, users are expected to treat those values are opaque and pass them via the respective enum constants. Since we don't guarantee that the specific enum constant values will remain consistent between kernel versions, I don't think it's a good idea to give to users that sort of implication by allowing them to use raw numbers for configuration selection.

Quoted 78 lines

tools/counter/counter_watch_events.c new file +77 -0

+	idx = strtoul(optarg, &dummy, 10);
+	if (!errno) {
+		if (idx >= sz)
+			return -EINVAL;
+		*val = idx;
+		return 0;
+	}
+
+	return -errno;
+}
+
+static const char * const counter_event_type_name[] = {
+	"COUNTER_EVENT_OVERFLOW",
+	"COUNTER_EVENT_UNDERFLOW",
+	"COUNTER_EVENT_OVERFLOW_UNDERFLOW",
+	"COUNTER_EVENT_THRESHOLD",
+	"COUNTER_EVENT_INDEX",
+	"COUNTER_EVENT_CHANGE_OF_STATE",
+	"COUNTER_EVENT_CAPTURE",
+};
+
+static int counter_arg_to_event_type(char *arg, __u8 *event)
+{
+	return find_match_or_number_from_array(arg, counter_event_type_name,
+					       ARRAY_SIZE(counter_event_type_name), event);
+}
+
+static const char * const counter_component_type_name[] = {
+	"COUNTER_COMPONENT_NONE",
+	"COUNTER_COMPONENT_SIGNAL",
+	"COUNTER_COMPONENT_COUNT",
+	"COUNTER_COMPONENT_FUNCTION",
+	"COUNTER_COMPONENT_SYNAPSE_ACTION",
+	"COUNTER_COMPONENT_EXTENSION",
+};
+
+static int counter_arg_to_component_type(char *arg, __u8 *type)
+{
+	return find_match_or_number_from_array(arg, counter_component_type_name,
+					       ARRAY_SIZE(counter_component_type_name), type);
+}
+
+static const char * const counter_scope_name[] = {
+	"COUNTER_SCOPE_DEVICE",
+	"COUNTER_SCOPE_SIGNAL",
+	"COUNTER_SCOPE_COUNT",
+};
+
+static int counter_arg_to_scope(char *arg, __u8 *type)
+{
+	return find_match_or_number_from_array(arg, counter_scope_name,
+					       ARRAY_SIZE(counter_scope_name), type);
+}
+
+static void print_usage(void)
+{
+	fprintf(stderr, "Usage: counter_watch_events [options]...\n"
+		"Test various watch events for given counter device\n"
+		"  --channel -c <n>\n"
+		"        Set watch.channel\n"
+		"  --debug -d\n"
+		"        Prints debug information\n"
+		"  --event -e <number or counter_event_type string>\n"
+		"        Sets watch.event\n"
+		"  --help -h\n"
+		"        Prints usage\n"
+		"  --device-num -n <n>\n"
+		"        Set device number (/dev/counter<n>, default to 0)\n"
+		"  --id -i <n>\n"
+		"        Set watch.component.id\n"
+		"  --loop -l <n>\n"
+		"        Loop for a number of events (forever if n < 0)\n"
+		"  --parent -p <n>\n"
+		"        Set watch.component.parent number\n"
+		"  --scope -s <number or counter_scope string>\n"
+		"        Set watch.component.scope\n"
+		"  --type -t <number or counter_component_type string>\n"

tools/counter/counter_watch_events.c line 133

Quoted 10 lines

tools/counter/counter_watch_events.c new file +10 -0

+		"        Set watch.component.type\n"
+		"\n"
+		"Example with two watched events:\n\n"
+		"counter_watch_events -d \\\n"
+		"\t-t COUNTER_COMPONENT_COUNT -s COUNTER_SCOPE_COUNT"
+		" -e COUNTER_EVENT_OVERFLOW_UNDERFLOW -i 0 -c 0 \\\n"
+		"\t-t COUNTER_COMPONENT_EXTENSION -s COUNTER_SCOPE_COUNT"
+		" -e COUNTER_EVENT_CAPTURE -i 7 -c 3\n"
+		);
+}

Are you following any particular convention for the usage description? I wonder if there is a particular preferred standard for command-line interface descriptions. A quick search brought up a few, such as the POSIX Utility Conventions[^1] and docopt[^2].

One improvement I would recommend here is to put the short form of the option before the long form and separate them with a command to make it clearer (e.g. "-h, --help").

[^1] https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap12.html
[^2] http://docopt.org

Quoted 52 lines

tools/counter/counter_watch_events.c new file +51 -0

+
+static void print_watch(struct counter_watch *watch, int nwatch)
+{
+	int i;
+
+	/* prints the watch array in C-like structure */
+	printf("watch[%d] = {\n", nwatch);
+	for (i = 0; i < nwatch; i++) {
+		printf(" [%d] =\t{\n"
+		       "\t\t.component.type = %s\n"
+		       "\t\t.component.scope = %s\n"
+		       "\t\t.component.parent = %d\n"
+		       "\t\t.component.id = %d\n"
+		       "\t\t.event = %s\n"
+		       "\t\t.channel = %d\n"
+		       "\t},\n",
+		       i,
+		       counter_component_type_name[watch[i].component.type],
+		       counter_scope_name[watch[i].component.scope],
+		       watch[i].component.parent,
+		       watch[i].component.id,
+		       counter_event_type_name[watch[i].event],
+		       watch[i].channel);
+	}
+	printf("};\n");
+}
+
+static const struct option longopts[] = {
+	{ "channel",		required_argument, 0, 'c' },
+	{ "debug",		no_argument,       0, 'd' },
+	{ "event",		required_argument, 0, 'e' },
+	{ "help",		no_argument,       0, 'h' },
+	{ "device-num",		required_argument, 0, 'n' },
+	{ "id",			required_argument, 0, 'i' },
+	{ "loop",		required_argument, 0, 'l' },
+	{ "parent",		required_argument, 0, 'p' },
+	{ "scope",		required_argument, 0, 's' },
+	{ "type",		required_argument, 0, 't' },
+	{ },
+};
+
+int main(int argc, char **argv)
+{
+	int c, fd, i, ret;
+	struct counter_event event_data;
+	char *device_name = NULL;
+	int debug = 0, loop = -1;
+	char *dummy;
+	int dev_num = 0, nwatch = 0, ncfg[] = {0, 0, 0, 0, 0, 0};
+	int num_chan = 0, num_evt = 0, num_id = 0, num_p = 0, num_s = 0, num_t = 0;
+	struct counter_watch *watches;

tools/counter/counter_watch_events.c line 190

Quoted 6 lines

tools/counter/counter_watch_events.c new file +6 -0

+
+	/*
+	 * 1st pass: count events configurations number to allocate the watch array.
+	 * Each watch argument can be repeated several times: each time it gets repeated,
+	 * a corresponding watch is allocated (and configured) in 2nd pass.
+	 */

It feels a somewhat prone to error (at least cumbersome) to populate

each watch via individual arguments for each field. Since a watch always has these fields, perhaps instead we could pass some format string that represents a watch, and deliminate watches via commas. For example, we could have --watch="cco00,ecc73" to represent the two watches in the usage example.

Of course, we'd need to define a more robust format string convention than in my example to ensure the correct configuration is properly

communicated. What do you think, would this approach would make things simpler, or just more complicated in the end?

Quoted 24 lines

tools/counter/counter_watch_events.c new file +23 -0

+	while ((c = getopt_long(argc, argv, "c:de:hn:i:l:p:s:t:", longopts, NULL)) != -1) {
+		switch (c) {
+		case 'c':
+			ncfg[0]++;
+			break;
+		case 'e':
+			ncfg[1]++;
+			break;
+		case 'i':
+			ncfg[2]++;
+			break;
+		case 'p':
+			ncfg[3]++;
+			break;
+		case 's':
+			ncfg[4]++;
+			break;
+		case 't':
+			ncfg[5]++;
+			break;
+		};
+	};
+

tools/counter/counter_watch_events.c line 219

Quoted 6 lines

tools/counter/counter_watch_events.c new file +6 -0

+	for (i = 0; i < ARRAY_SIZE(ncfg); i++)
+		if (ncfg[i] > nwatch)
+			nwatch = ncfg[i];
+
+	if (nwatch) {
+		watches = calloc(nwatch, sizeof(*watches));

We need to check if calloc fails, right?

William Breathitt Gray

Quoted 16 lines · William Breathitt Gray

Quoted 15 lines · Fabrice Gasnier

This adds a new counter tool to be able to test various watch events. A flexible watch array can be populated from command line, each field may be tuned with a dedicated command line argument.
Each argument can be repeated several times: each time it gets repeated, a corresponding new watch element is allocated.

It also comes with a simple default watch (to monitor overflows), used when no watch parameters are provided.

The print_usage() routine proposes another example, from the command line, which generates a 2 elements watch array, to monitor:
- overflow events
- capture events, on channel 3, that reads read captured data by specifying the component id (capture3_component_id being 7 here).

Quoted 6 lines

Quoted 1 line

Signed-off-by Fabrice Gasnier <[email protected]>

Hi Fabrice,

This is great idea, it'll make it so much easier to test out drivers so I'm excited! :-)

Hi William,

Thanks

Quoted 3 lines

This is a new tool, so would you add a MAINTAINERS entry for the counter_watch_events.c file?

I haven't thought about it.
I can add a MAINTAINERS entry, yes!
Who would you suggest ?

Quoted 34 lines

More comments inline below.

Quoted 31 lines

---
 tools/counter/Build                  |   1 +
 tools/counter/Makefile               |   8 +-
 tools/counter/counter_watch_events.c | 348 +++++++++++++++++++++++++++
 3 files changed, 356 insertions(+), 1 deletion(-)
 create mode 100644 tools/counter/counter_watch_events.c

tools/counter/Build +1 -0

index 33f4a51d715e..4bbadb7ec93a 100644
@@ -1 +1,2 @@
 counter_example-y += counter_example.o
+counter_watch_events-y += counter_watch_events.o

tools/counter/Makefile +1 -1

index b2c2946f44c9..00e211edd768 100644
@@ -14,7 +14,7 @@ MAKEFLAGS += -r
 
 override CFLAGS += -O2 -Wall -g -D_GNU_SOURCE -I$(OUTPUT)include
 
-ALL_TARGETS := counter_example
+ALL_TARGETS := counter_example counter_watch_events
 ALL_PROGRAMS := $(patsubst %,$(OUTPUT)%,$(ALL_TARGETS))
 
 all: $(ALL_PROGRAMS)

@@ -31,6 +31,12 @@ $(OUTPUT)include/linux/counter.h: ../../include/uapi/linux/counter.h
 
 prepare: $(OUTPUT)include/linux/counter.h
 

Quoted 9 lines

tools/counter/Makefile $(OUTPUT)include/linux/counter.h: ../../include/uapi/linux/counter.h · line 39

Quoted 6 lines

tools/counter/Makefile +6 -0

+COUNTER_WATCH_EVENTS := $(OUTPUT)counter_watch_events.o
+$(COUNTER_WATCH_EVENTS): prepare FORCE
+	$(Q)$(MAKE) $(build)=counter_watch_events
+$(OUTPUT)counter_watch_events: $(COUNTER_WATCH_EVENTS)
+	$(QUIET_LINK)$(CC) $(CFLAGS) $(LDFLAGS) $< -o $@
+

Move this below the COUNTER_EXAMPLE block just so we can keep the recipes in alphabetical order.

Ack, will update it.

Quoted 15 lines

tools/counter/counter_watch_events.c line 2

Quoted 12 lines

tools/counter/Makefile +0 -0

 COUNTER_EXAMPLE := $(OUTPUT)counter_example.o
 $(COUNTER_EXAMPLE): prepare FORCE
 	$(Q)$(MAKE) $(build)=counter_example

tools/counter/counter_watch_events.c new file +2 -0

new file mode 100644
index 000000000000..7f73a1519d8e
@@ -0,0 +1,348 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/* Counter - Test various watch events in a userspace application

"Counter" should be "Counter Watch Events" (or "counter_watch_events").

Quoted 5 lines

tools/counter/counter_watch_events.c line 3

Quoted 1 line

tools/counter/counter_watch_events.c new file +1 -0

+ * inspired by counter_example.c

No need to mention counter_example.c, this utility does far more than and bares little resemblance at this point to counter_example.c which is really just a bare minimal example of watching Counter events.

Ack

Quoted 12 lines

Quoted 12 lines

tools/counter/counter_watch_events.c new file +11 -0

+ */
+
+#include <errno.h>
+#include <fcntl.h>
+#include <getopt.h>
+#include <linux/counter.h>
+#include <stdlib.h>
+#include <stdio.h>
+#include <string.h>
+#include <sys/ioctl.h>
+#include <unistd.h>

Quoted 8 lines

tools/counter/counter_watch_events.c line 16

Quoted 2 lines

tools/counter/counter_watch_events.c new file +2 -0

+
+#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))

My initial reaction was that this macro is already exposed in some header for us, but my local /usr/include/linux/kernel.h file doesn't appear to bare it so I guess not. Perhaps it'll be fine for our needs -- I think the only difference between this ARRAY_SIZE and the Linux kernel one is the addition of __must_be_array(x).

I had the same reaction when trying to use it. Then, I figured out several tools redefine this macro.
Digging further, I just found out some tools have in their Makefile CFLAGS:
-I$(srctree)/tools/include
and include from there: <linux/kernel.h>

I'll update the Makefile in v2, and remove this definition from here.

Quoted 15 lines

tools/counter/counter_watch_events.c line 25

Quoted 10 lines

tools/counter/counter_watch_events.c new file +9 -0

+
+static struct counter_watch simple_watch[] = {
+	{
+		/* Component data: Count 0 count */
+		.component.type = COUNTER_COMPONENT_COUNT,
+		.component.scope = COUNTER_SCOPE_COUNT,
+		.component.parent = 0,
+		/* Event type: Index */
+		.event = COUNTER_EVENT_OVERFLOW_UNDERFLOW,

There's a bit of confusion here. The comment says the event type is INDEX, but the structure event type is set for OVERFLOW_UNDERFLOW; also, the commit description states that we're monitoring "overflows" which implies to me the OVERFLOW event type. So which of the three is it?

Ah yes, It's a mix of bad copy paste and updates. I'll fix it.

Quoted 38 lines

Quoted 9 lines

tools/counter/counter_watch_events.c new file +8 -0

+		/* Device event channel 0 */
+		.channel = 0,
+	},
+};
+
+static int find_match_or_number_from_array(char *arg, const char * const str[], int sz, __u8 *val)
+{
+	unsigned int i;

tools/counter/counter_watch_events.c line 43

Quoted 10 lines

tools/counter/counter_watch_events.c new file +10 -0

+	char *dummy;
+	unsigned long idx;
+	int ret;
+
+	for (i = 0; i < sz; i++) {
+		ret = strncmp(arg, str[i], strlen(str[i]));
+		if (!ret && strlen(str[i]) == strlen(arg)) {
+			*val = i;
+			return 0;
+		}

This has several strlen calls so I wonder if it's more wasteful than it needs to me. I suppose the compiler would optimize this away, but I think there is an alternative solution.

We're checking for an exact match, so you don't need the string length. Instead, you can compare each character, break when characters differ, or return 0 when you reached the null byte for both. So something like this:

    for (j = 0; arg[j] == str[i][j]; j++) {
            /* If we reached the end of the strings */
            if (arg[j] == '\0') {
                    *val = i;
                    return 0;
            }
    }
    /* Strings do not match; continue to the next string */

Quoted 3 lines

We end up with the same number of lines, so I'll leave it up to you whether you want to use this solution, or if you consider the existing code clearer read.

I'll look forward in the direction you propose. First, we need to confirm in which form the arguments can be expected. It depends on your proposal to use a --watch string formatted arguments.

Quoted 13 lines

tools/counter/counter_watch_events.c line 46

Quoted 4 lines

tools/counter/counter_watch_events.c new file +3 -0

+	}
+
+	/* fallback to number */

I'm not sure it makes sense to support numbers. Although it's true that the component type, component scope, and event type are passed as __u8 values, users are expected to treat those values are opaque and pass them via the respective enum constants. Since we don't guarantee that the specific enum constant values will remain consistent between kernel versions, I don't think it's a good idea to give to users that sort of implication by allowing them to use raw numbers for configuration selection.

Ack, I can remove this.

I'm a bit surprised by this statement. I may be wrong... I'd expect a userland binary to be compatible when updating to a newer kernel: e.g. user API (ABI?) definitions to be stable (including enum constants) ?

Quoted 94 lines

Quoted 78 lines

tools/counter/counter_watch_events.c new file +77 -0

+	idx = strtoul(optarg, &dummy, 10);
+	if (!errno) {
+		if (idx >= sz)
+			return -EINVAL;
+		*val = idx;
+		return 0;
+	}
+
+	return -errno;
+}
+
+static const char * const counter_event_type_name[] = {
+	"COUNTER_EVENT_OVERFLOW",
+	"COUNTER_EVENT_UNDERFLOW",
+	"COUNTER_EVENT_OVERFLOW_UNDERFLOW",
+	"COUNTER_EVENT_THRESHOLD",
+	"COUNTER_EVENT_INDEX",
+	"COUNTER_EVENT_CHANGE_OF_STATE",
+	"COUNTER_EVENT_CAPTURE",
+};
+
+static int counter_arg_to_event_type(char *arg, __u8 *event)
+{
+	return find_match_or_number_from_array(arg, counter_event_type_name,
+					       ARRAY_SIZE(counter_event_type_name), event);
+}
+
+static const char * const counter_component_type_name[] = {
+	"COUNTER_COMPONENT_NONE",
+	"COUNTER_COMPONENT_SIGNAL",
+	"COUNTER_COMPONENT_COUNT",
+	"COUNTER_COMPONENT_FUNCTION",
+	"COUNTER_COMPONENT_SYNAPSE_ACTION",
+	"COUNTER_COMPONENT_EXTENSION",
+};
+
+static int counter_arg_to_component_type(char *arg, __u8 *type)
+{
+	return find_match_or_number_from_array(arg, counter_component_type_name,
+					       ARRAY_SIZE(counter_component_type_name), type);
+}
+
+static const char * const counter_scope_name[] = {
+	"COUNTER_SCOPE_DEVICE",
+	"COUNTER_SCOPE_SIGNAL",
+	"COUNTER_SCOPE_COUNT",
+};
+
+static int counter_arg_to_scope(char *arg, __u8 *type)
+{
+	return find_match_or_number_from_array(arg, counter_scope_name,
+					       ARRAY_SIZE(counter_scope_name), type);
+}
+
+static void print_usage(void)
+{
+	fprintf(stderr, "Usage: counter_watch_events [options]...\n"
+		"Test various watch events for given counter device\n"
+		"  --channel -c <n>\n"
+		"        Set watch.channel\n"
+		"  --debug -d\n"
+		"        Prints debug information\n"
+		"  --event -e <number or counter_event_type string>\n"
+		"        Sets watch.event\n"
+		"  --help -h\n"
+		"        Prints usage\n"
+		"  --device-num -n <n>\n"
+		"        Set device number (/dev/counter<n>, default to 0)\n"
+		"  --id -i <n>\n"
+		"        Set watch.component.id\n"
+		"  --loop -l <n>\n"
+		"        Loop for a number of events (forever if n < 0)\n"
+		"  --parent -p <n>\n"
+		"        Set watch.component.parent number\n"
+		"  --scope -s <number or counter_scope string>\n"
+		"        Set watch.component.scope\n"
+		"  --type -t <number or counter_component_type string>\n"

tools/counter/counter_watch_events.c line 133

Quoted 10 lines

tools/counter/counter_watch_events.c new file +10 -0

+		"        Set watch.component.type\n"
+		"\n"
+		"Example with two watched events:\n\n"
+		"counter_watch_events -d \\\n"
+		"\t-t COUNTER_COMPONENT_COUNT -s COUNTER_SCOPE_COUNT"
+		" -e COUNTER_EVENT_OVERFLOW_UNDERFLOW -i 0 -c 0 \\\n"
+		"\t-t COUNTER_COMPONENT_EXTENSION -s COUNTER_SCOPE_COUNT"
+		" -e COUNTER_EVENT_CAPTURE -i 7 -c 3\n"
+		);
+}

Are you following any particular convention for the usage description? I wonder if there is a particular preferred standard for command-line interface descriptions. A quick search brought up a few, such as the POSIX Utility Conventions[^1] and docopt[^2].

Quoted 6 lines

One improvement I would recommend here is to put the short form of the option before the long form and separate them with a command to make it clearer (e.g. "-h, --help").

[^1] https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap12.html
[^2] http://docopt.org

Thanks for pointing this! So definitely a good pointer, and suggestion to look at!

I'll try to improve in v2.

Quoted 52 lines

Quoted 52 lines

tools/counter/counter_watch_events.c new file +51 -0

+
+static void print_watch(struct counter_watch *watch, int nwatch)
+{
+	int i;
+
+	/* prints the watch array in C-like structure */
+	printf("watch[%d] = {\n", nwatch);
+	for (i = 0; i < nwatch; i++) {
+		printf(" [%d] =\t{\n"
+		       "\t\t.component.type = %s\n"
+		       "\t\t.component.scope = %s\n"
+		       "\t\t.component.parent = %d\n"
+		       "\t\t.component.id = %d\n"
+		       "\t\t.event = %s\n"
+		       "\t\t.channel = %d\n"
+		       "\t},\n",
+		       i,
+		       counter_component_type_name[watch[i].component.type],
+		       counter_scope_name[watch[i].component.scope],
+		       watch[i].component.parent,
+		       watch[i].component.id,
+		       counter_event_type_name[watch[i].event],
+		       watch[i].channel);
+	}
+	printf("};\n");
+}
+
+static const struct option longopts[] = {
+	{ "channel",		required_argument, 0, 'c' },
+	{ "debug",		no_argument,       0, 'd' },
+	{ "event",		required_argument, 0, 'e' },
+	{ "help",		no_argument,       0, 'h' },
+	{ "device-num",		required_argument, 0, 'n' },
+	{ "id",			required_argument, 0, 'i' },
+	{ "loop",		required_argument, 0, 'l' },
+	{ "parent",		required_argument, 0, 'p' },
+	{ "scope",		required_argument, 0, 's' },
+	{ "type",		required_argument, 0, 't' },
+	{ },
+};
+
+int main(int argc, char **argv)
+{
+	int c, fd, i, ret;
+	struct counter_event event_data;
+	char *device_name = NULL;
+	int debug = 0, loop = -1;
+	char *dummy;
+	int dev_num = 0, nwatch = 0, ncfg[] = {0, 0, 0, 0, 0, 0};
+	int num_chan = 0, num_evt = 0, num_id = 0, num_p = 0, num_s = 0, num_t = 0;
+	struct counter_watch *watches;

Quoted 8 lines

tools/counter/counter_watch_events.c line 190

Quoted 6 lines

tools/counter/counter_watch_events.c new file +6 -0

+
+	/*
+	 * 1st pass: count events configurations number to allocate the watch array.
+	 * Each watch argument can be repeated several times: each time it gets repeated,
+	 * a corresponding watch is allocated (and configured) in 2nd pass.
+	 */

It feels a somewhat prone to error (at least cumbersome) to populate

Yes, this could be error prone. This is also why I added a print of the gathered arguments when using --debug option.
Perhaps this could be better to always print it (e.g. print_watch()) ?

Quoted 5 lines

each watch via individual arguments for each field. Since a watch always has these fields, perhaps instead we could pass some format string that represents a watch, and deliminate watches via commas. For example, we could have --watch="cco00,ecc73" to represent the two watches in the usage example.

I like the idea, to concatenate as a string. With current approach, the command line quickly becomes very long.

It makes it obvious in your example, that two watches are used, and no argument is omitted.
On the opposite, each argument isn't very easy to understand compared to plain text definition.

Quoted 3 lines

Of course, we'd need to define a more robust format string convention than in my example to ensure the correct configuration is properly

Indeed, by using a single letter, we could face limitations (ex:
overflow, underflow, overflow_underflow, which letter for the 3rd here?)

If we go this way, probably need to brainstorm a bit.

Quoted 2 lines

communicated. What do you think, would this approach would make things simpler, or just more complicated in the end?

I'm not 100% sure if some helpers like getopt() will help here? So, I guess this could be more complicated. This may also be against the guideline "options should be preceded by the '-' delimiter character." in [^1] (Ok, this would rather be the --watch option, fed with watch data.)

Would you have suggestions regarding possible helpers ? Or do you have in mind some others tools that already adopted such approach ?

Quoted 25 lines

Quoted 25 lines

tools/counter/counter_watch_events.c new file +23 -0

+	while ((c = getopt_long(argc, argv, "c:de:hn:i:l:p:s:t:", longopts, NULL)) != -1) {
+		switch (c) {
+		case 'c':
+			ncfg[0]++;
+			break;
+		case 'e':
+			ncfg[1]++;
+			break;
+		case 'i':
+			ncfg[2]++;
+			break;
+		case 'p':
+			ncfg[3]++;
+			break;
+		case 's':
+			ncfg[4]++;
+			break;
+		case 't':
+			ncfg[5]++;
+			break;
+		};
+	};
+

Quoted 8 lines

tools/counter/counter_watch_events.c line 219

Quoted 6 lines

tools/counter/counter_watch_events.c new file +6 -0

+	for (i = 0; i < ARRAY_SIZE(ncfg); i++)
+		if (ncfg[i] > nwatch)
+			nwatch = ncfg[i];
+
+	if (nwatch) {
+		watches = calloc(nwatch, sizeof(*watches));

We need to check if calloc fails, right?

Yes, you're right, will fix this too.

Thanks for reviewing!
Best regards,
Fabrice

Quoted 2 lines

William Breathitt Gray

Quoted 10 lines · William Breathitt Gray

Quoted 9 lines · Fabrice Gasnier

This adds a new counter tool to be able to test various watch events. A flexible watch array can be populated from command line, each field may be tuned with a dedicated command line argument.
Each argument can be repeated several times: each time it gets repeated, a corresponding new watch element is allocated.

It also comes with a simple default watch (to monitor overflows), used when no watch parameters are provided.

Quoted 7 lines

Quoted 7 lines

The print_usage() routine proposes another example, from the command line, which generates a 2 elements watch array, to monitor:
- overflow events
- capture events, on channel 3, that reads read captured data by specifying the component id (capture3_component_id being 7 here).

Signed-off-by Fabrice Gasnier <[email protected]>

Quoted 2 lines

This is a new tool, so would you add a MAINTAINERS entry for the counter_watch_events.c file?

I haven't thought about it.
I can add a MAINTAINERS entry, yes!
Who would you suggest ?

Quoted 4 lines

tools/counter/counter_watch_events.c line 46

Quoted 3 lines

tools/counter/counter_watch_events.c new file +3 -0

+	}
+
+	/* fallback to number */

Quoted 8 lines

I'm not sure it makes sense to support numbers. Although it's true that the component type, component scope, and event type are passed as __u8 values, users are expected to treat those values are opaque and pass them via the respective enum constants. Since we don't guarantee that the specific enum constant values will remain consistent between kernel versions, I don't think it's a good idea to give to users that sort of implication by allowing them to use raw numbers for configuration selection.

Ack, I can remove this.

I'm a bit surprised by this statement. I may be wrong... I'd expect a userland binary to be compatible when updating to a newer kernel: e.g. user API (ABI?) definitions to be stable (including enum constants) ?

There are two types of STM32 timers that may have:
- a global interrupt line
- 4 dedicated interrupt lines.
Those interrupts are optional as defined in the dt-bindings. Enforce checks on either one, four or no interrupts are provided with their names. Optionally get them here, to be used by child devices.

[...]

Rename "Channel 1 Quadrature B", as it corresponds to timer input ch2. I suspect it referred to the (unique) counter earlier, but the physical input really is CH2.

Signed-off-by Fabrice Gasnier <[email protected]>

---
 drivers/counter/stm32-timer-cnt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/counter/stm32-timer-cnt.c +1 -1

index 6206d2dc3d47..eae851f6db2c 100644
@@ -283,7 +283,7 @@ static struct counter_signal stm32_signals[] = {
 	},
 	{
 		.id = 1,
-		.name = "Channel 1 Quadrature B"
+		.name = "Channel 2 Quadrature B"
 	}
 };
 

Signature · 1 line

2.25.1

Add support for capture and overflow events. Also add the related validation and configuration. Captured counter value can be retrieved through CCRx register. Register and enable interrupts to push events.

Signed-off-by Fabrice Gasnier <[email protected]>

---
 drivers/counter/stm32-timer-cnt.c | 279 +++++++++++++++++++++++++++++-

 include/linux/mfd/stm32-timers.h  |  15 ++

 2 files changed, 285 insertions(+), 9 deletions(-)

This series combines some fix and improvements to the counter interface, found while stm32 timer counter driver developements.
It also introduces a new tool that can be used for testing.

Then, it improves the stm32 timer counter driver by introducing new signals, e.g. counting frequency, and missing channels.
It also adds support for interrupt based events using the chrdev interface. Two event types are added in this series: overflows and capture.

Up to now, stm32 timer counter driver focused mainly on quadrature encoder feature. With this series, all timer instances can be enabled for simple counting (with overflow and capture events).

Fabrice Gasnier (8):
counter: chrdev: fix getting array extensions
counter: chrdev: remove a typo in header file comment tools/counter: add a flexible watch events tool
mfd: stm32-timers: add support for interrupts
counter: stm32-timer-cnt: rename quadrature signal
counter: stm32-timer-cnt: introduce clock signal
counter: stm32-timer-cnt: populate capture channels and check encoder
counter: stm32-timer-cnt: add support for events

 drivers/counter/counter-chrdev.c     |   4 +-
 drivers/counter/stm32-timer-cnt.c    | 585 ++++++++++++++++++++++++++-
 drivers/mfd/stm32-timers.c           |  46 +++
 include/linux/mfd/stm32-timers.h     |  26 ++
 include/uapi/linux/counter.h         |   2 +-
 tools/counter/Build                  |   1 +
 tools/counter/Makefile               |   8 +-
 tools/counter/counter_watch_events.c | 348 ++++++++++++++++
 8 files changed, 998 insertions(+), 22 deletions(-)
 create mode 100644 tools/counter/counter_watch_events.c

Signature · 1 line

2.25.1