diff mbox series

[v12,12/17] tools/counter: Create Counter tools

Message ID e97aa3e529f54d5651df7edcc1b43a8157d9e9c3.1625471640.git.vilhelm.gray@gmail.com (mailing list archive)
State Superseded
Delegated to: Jonathan Cameron
Headers show
Series Introduce the Counter character device interface | expand

Commit Message

William Breathitt Gray July 5, 2021, 8:19 a.m. UTC
This creates an example Counter program under tools/counter/*
to exemplify the Counter character device interface.

Cc: Pavel Machek <pavel@ucw.cz>
Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
---
 MAINTAINERS                     |  1 +
 tools/Makefile                  | 13 ++---
 tools/counter/Build             |  1 +
 tools/counter/Makefile          | 53 ++++++++++++++++++
 tools/counter/counter_example.c | 95 +++++++++++++++++++++++++++++++++
 5 files changed, 157 insertions(+), 6 deletions(-)
 create mode 100644 tools/counter/Build
 create mode 100644 tools/counter/Makefile
 create mode 100644 tools/counter/counter_example.c

Comments

David Lechner July 10, 2021, 4:53 p.m. UTC | #1
On 7/5/21 3:19 AM, William Breathitt Gray wrote:
> This creates an example Counter program under tools/counter/*
> to exemplify the Counter character device interface.
> 
> Cc: Pavel Machek <pavel@ucw.cz>
> Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
> ---


> --- a/tools/Makefile
> +++ b/tools/Makefile
> @@ -12,6 +12,7 @@ help:
>   	@echo '  acpi                   - ACPI tools'
>   	@echo '  bpf                    - misc BPF tools'
>   	@echo '  cgroup                 - cgroup tools'
> +	@echo '  counter                - Counter tools'

nit: other descriptions start with lower case letter, so to be
consistent, this should too


> --- /dev/null
> +++ b/tools/counter/counter_example.c
> @@ -0,0 +1,95 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Counter - example userspace application
> + *
> + * The userspace application opens /dev/counter0, configures the
> + * COUNTER_EVENT_INDEX event channel 0 to gather Count 0 count and Count
> + * 1 count, and prints out the data as it becomes available on the
> + * character device node.
> + *
> + * Copyright (C) 2021 William Breathitt Gray
> + */
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <linux/counter.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <sys/ioctl.h>
> +#include <unistd.h>
> +
> +struct counter_watch watches[2] = {

nit: this can be static

> +	{
> +		/* Component data: Count 0 count */
> +		.component.type = COUNTER_COMPONENT_COUNT,
> +		.component.scope = COUNTER_SCOPE_COUNT,
> +		.component.parent = 0,
> +		/* Event type: Index */
> +		.event = COUNTER_EVENT_INDEX,
> +		/* Device event channel 0 */
> +		.channel = 0,
> +	},
> +	{
> +		/* Component data: Count 1 count */
> +		.component.type = COUNTER_COMPONENT_COUNT,
> +		.component.scope = COUNTER_SCOPE_COUNT,
> +		.component.parent = 1,
> +		/* Event type: Index */
> +		.event = COUNTER_EVENT_INDEX,
> +		/* Device event channel 0 */
> +		.channel = 0,
> +	},
> +};
> +
> +int main(void)
> +{
> +	int fd;
> +	int ret;
> +	struct counter_event event_data[2];
> +
> +	fd = open("/dev/counter0", O_RDWR);
> +	if (fd == -1) {
> +		perror("Unable to open /dev/counter0");
> +		return -errno;

errno is no longer valid after calling perror(). Since this
is example code, we can just return 1 instead (exit codes
positive number between 0 and 255 so -1 would be 255).

> +	}
> +
> +	ret = ioctl(fd, COUNTER_ADD_WATCH_IOCTL, watches);
> +	if (ret == -1) {
> +		perror("Error adding watches[0]");
> +		return -errno;
> +	}
> +	ret = ioctl(fd, COUNTER_ADD_WATCH_IOCTL, watches + 1);
> +	if (ret == -1) {
> +		perror("Error adding watches[1]");
> +		return -errno;
> +	}
> +	ret = ioctl(fd, COUNTER_ENABLE_EVENTS_IOCTL);
> +	if (ret == -1) {
> +		perror("Error enabling events");
> +		return -errno;
> +	}
> +
> +	for (;;) {
> +		ret = read(fd, event_data, sizeof(event_data));
> +		if (ret == -1) {
> +			perror("Failed to read event data");
> +			return -errno;
> +		}
> +
> +		if (ret != sizeof(event_data)) {
> +			fprintf(stderr, "Failed to read event data\n");
> +			return -EIO;
> +		}
> +
> +		printf("Timestamp 0: %llu\tCount 0: %llu\n"
> +		       "Error Message 0: %s\n"
> +		       "Timestamp 1: %llu\tCount 1: %llu\n"
> +		       "Error Message 1: %s\n",
> +		       (unsigned long long)event_data[0].timestamp,
> +		       (unsigned long long)event_data[0].value,
> +		       strerror(event_data[0].status),
> +		       (unsigned long long)event_data[1].timestamp,
> +		       (unsigned long long)event_data[1].value,
> +		       strerror(event_data[1].status));
> +	}

Aren't the Count 0 and Count 1 events independent? Why should we expect to
always get both events at the same time in the same order?

> +
> +	return 0;
> +}
>
William Breathitt Gray July 11, 2021, 11:28 a.m. UTC | #2
On Sat, Jul 10, 2021 at 11:53:35AM -0500, David Lechner wrote:
> On 7/5/21 3:19 AM, William Breathitt Gray wrote:
> > This creates an example Counter program under tools/counter/*
> > to exemplify the Counter character device interface.
> > 
> > Cc: Pavel Machek <pavel@ucw.cz>
> > Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
> > ---
> 
> 
> > --- a/tools/Makefile
> > +++ b/tools/Makefile
> > @@ -12,6 +12,7 @@ help:
> >   	@echo '  acpi                   - ACPI tools'
> >   	@echo '  bpf                    - misc BPF tools'
> >   	@echo '  cgroup                 - cgroup tools'
> > +	@echo '  counter                - Counter tools'
> 
> nit: other descriptions start with lower case letter, so to be
> consistent, this should too

Ack.

> > --- /dev/null
> > +++ b/tools/counter/counter_example.c
> > @@ -0,0 +1,95 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/* Counter - example userspace application
> > + *
> > + * The userspace application opens /dev/counter0, configures the
> > + * COUNTER_EVENT_INDEX event channel 0 to gather Count 0 count and Count
> > + * 1 count, and prints out the data as it becomes available on the
> > + * character device node.
> > + *
> > + * Copyright (C) 2021 William Breathitt Gray
> > + */
> > +#include <errno.h>
> > +#include <fcntl.h>
> > +#include <linux/counter.h>
> > +#include <stdio.h>
> > +#include <string.h>
> > +#include <sys/ioctl.h>
> > +#include <unistd.h>
> > +
> > +struct counter_watch watches[2] = {
> 
> nit: this can be static

Ack.

> > +	{
> > +		/* Component data: Count 0 count */
> > +		.component.type = COUNTER_COMPONENT_COUNT,
> > +		.component.scope = COUNTER_SCOPE_COUNT,
> > +		.component.parent = 0,
> > +		/* Event type: Index */
> > +		.event = COUNTER_EVENT_INDEX,
> > +		/* Device event channel 0 */
> > +		.channel = 0,
> > +	},
> > +	{
> > +		/* Component data: Count 1 count */
> > +		.component.type = COUNTER_COMPONENT_COUNT,
> > +		.component.scope = COUNTER_SCOPE_COUNT,
> > +		.component.parent = 1,
> > +		/* Event type: Index */
> > +		.event = COUNTER_EVENT_INDEX,
> > +		/* Device event channel 0 */
> > +		.channel = 0,
> > +	},
> > +};
> > +
> > +int main(void)
> > +{
> > +	int fd;
> > +	int ret;
> > +	struct counter_event event_data[2];
> > +
> > +	fd = open("/dev/counter0", O_RDWR);
> > +	if (fd == -1) {
> > +		perror("Unable to open /dev/counter0");
> > +		return -errno;
> 
> errno is no longer valid after calling perror(). Since this
> is example code, we can just return 1 instead (exit codes
> positive number between 0 and 255 so -1 would be 255).

Ack.

> > +	}
> > +
> > +	ret = ioctl(fd, COUNTER_ADD_WATCH_IOCTL, watches);
> > +	if (ret == -1) {
> > +		perror("Error adding watches[0]");
> > +		return -errno;
> > +	}
> > +	ret = ioctl(fd, COUNTER_ADD_WATCH_IOCTL, watches + 1);
> > +	if (ret == -1) {
> > +		perror("Error adding watches[1]");
> > +		return -errno;
> > +	}
> > +	ret = ioctl(fd, COUNTER_ENABLE_EVENTS_IOCTL);
> > +	if (ret == -1) {
> > +		perror("Error enabling events");
> > +		return -errno;
> > +	}
> > +
> > +	for (;;) {
> > +		ret = read(fd, event_data, sizeof(event_data));
> > +		if (ret == -1) {
> > +			perror("Failed to read event data");
> > +			return -errno;
> > +		}
> > +
> > +		if (ret != sizeof(event_data)) {
> > +			fprintf(stderr, "Failed to read event data\n");
> > +			return -EIO;
> > +		}
> > +
> > +		printf("Timestamp 0: %llu\tCount 0: %llu\n"
> > +		       "Error Message 0: %s\n"
> > +		       "Timestamp 1: %llu\tCount 1: %llu\n"
> > +		       "Error Message 1: %s\n",
> > +		       (unsigned long long)event_data[0].timestamp,
> > +		       (unsigned long long)event_data[0].value,
> > +		       strerror(event_data[0].status),
> > +		       (unsigned long long)event_data[1].timestamp,
> > +		       (unsigned long long)event_data[1].value,
> > +		       strerror(event_data[1].status));
> > +	}
> 
> Aren't the Count 0 and Count 1 events independent? Why should we expect to
> always get both events at the same time in the same order?

Watch 0 and Watch 1 are both triggered by the same event: a
COUNTER_EVENT_INDEX event on device event channel 0. If we had set
channel to 1 for Watch 1, then we would have two independent events, but
in this case both Watches have their respective channel set to 0.

To make the sequence of events clearer, here's a timeline:

* The user configures the watch list via COUNTER_ADD_WATCH_IOCTL.

* The watch list consists of Watch 0 and Watch 1. Watch 0 is configured
  to report the Count 0 count, while Watch 1 is configured to report the
  Count 1 count. Both watches are configured to trigger on the same
  event (COUNTER_EVENT_INDEX on device event channel 0).

* The user enables Counter events via COUNTER_ENABLE_EVENTS_IOCTL.

* The user calls read() from userspace and blocks until data is
  available in the Counter events list kfifo; this corresponds to
  wait_event_interruptible() in counter_chrdev_read().

* A COUNTER_EVENT_INDEX event occurs on device event channel 0.

* All Watches in the watch list that are waiting for COUNTER_EVENT_INDEX
  on device event channel 0 will now trigger; both Watch 0 and Watch 1
  will trigger, one after the other.

* A read operation is performed for the Count 0 count component; the
  data is pushed to the Counter event list.

* A read operation is performed for the Count 1 count component; the
  data is pushed to the Counter event list.

* Counter subsystem notifies that data is available in the Counter
  events list kfifo; this corresponds to the wake_up_poll() in
  counter_push_event().

* The userspace read() call returns the Counter event list data.

So in the counter_example.c reference code, we will always get both
event data elements returned to the user at the same time (with the
exception of errors which break early).

William Breathitt Gray
Jonathan Cameron July 11, 2021, 1:22 p.m. UTC | #3
On Mon,  5 Jul 2021 17:19:00 +0900
William Breathitt Gray <vilhelm.gray@gmail.com> wrote:

> This creates an example Counter program under tools/counter/*
> to exemplify the Counter character device interface.
> 
> Cc: Pavel Machek <pavel@ucw.cz>
> Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
> ---
>  MAINTAINERS                     |  1 +
>  tools/Makefile                  | 13 ++---
>  tools/counter/Build             |  1 +
>  tools/counter/Makefile          | 53 ++++++++++++++++++
>  tools/counter/counter_example.c | 95 +++++++++++++++++++++++++++++++++
>  5 files changed, 157 insertions(+), 6 deletions(-)
>  create mode 100644 tools/counter/Build
>  create mode 100644 tools/counter/Makefile
>  create mode 100644 tools/counter/counter_example.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 5de4d2164844..e96797f57f04 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -4688,6 +4688,7 @@ F:	Documentation/driver-api/generic-counter.rst
>  F:	drivers/counter/
>  F:	include/linux/counter.h
>  F:	include/uapi/linux/counter.h
> +F:	tools/counter/
>  
>  CP2615 I2C DRIVER
>  M:	Bence Csókás <bence98@sch.bme.hu>
> diff --git a/tools/Makefile b/tools/Makefile
> index 7e9d34ddd74c..4c26400ffc03 100644
> --- a/tools/Makefile
> +++ b/tools/Makefile
> @@ -12,6 +12,7 @@ help:
>  	@echo '  acpi                   - ACPI tools'
>  	@echo '  bpf                    - misc BPF tools'
>  	@echo '  cgroup                 - cgroup tools'
> +	@echo '  counter                - Counter tools'
>  	@echo '  cpupower               - a tool for all things x86 CPU power'
>  	@echo '  debugging              - tools for debugging'
>  	@echo '  firewire               - the userspace part of nosy, an IEEE-1394 traffic sniffer'
> @@ -65,7 +66,7 @@ acpi: FORCE
>  cpupower: FORCE
>  	$(call descend,power/$@)
>  
> -cgroup firewire hv guest bootconfig spi usb virtio vm bpf iio gpio objtool leds wmi pci firmware debugging tracing: FORCE
> +cgroup counter firewire hv guest bootconfig spi usb virtio vm bpf iio gpio objtool leds wmi pci firmware debugging tracing: FORCE
>  	$(call descend,$@)
>  
>  bpf/%: FORCE
> @@ -100,7 +101,7 @@ freefall: FORCE
>  kvm_stat: FORCE
>  	$(call descend,kvm/$@)
>  
> -all: acpi cgroup cpupower gpio hv firewire liblockdep \
> +all: acpi cgroup counter cpupower gpio hv firewire liblockdep \
>  		perf selftests bootconfig spi turbostat usb \
>  		virtio vm bpf x86_energy_perf_policy \
>  		tmon freefall iio objtool kvm_stat wmi \
> @@ -112,7 +113,7 @@ acpi_install:
>  cpupower_install:
>  	$(call descend,power/$(@:_install=),install)
>  
> -cgroup_install firewire_install gpio_install hv_install iio_install perf_install bootconfig_install spi_install usb_install virtio_install vm_install bpf_install objtool_install wmi_install pci_install debugging_install tracing_install:
> +cgroup_install counter_install firewire_install gpio_install hv_install iio_install perf_install bootconfig_install spi_install usb_install virtio_install vm_install bpf_install objtool_install wmi_install pci_install debugging_install tracing_install:
>  	$(call descend,$(@:_install=),install)
>  
>  liblockdep_install:
> @@ -133,7 +134,7 @@ freefall_install:
>  kvm_stat_install:
>  	$(call descend,kvm/$(@:_install=),install)
>  
> -install: acpi_install cgroup_install cpupower_install gpio_install \
> +install: acpi_install cgroup_install counter_install cpupower_install gpio_install \
>  		hv_install firewire_install iio_install liblockdep_install \
>  		perf_install selftests_install turbostat_install usb_install \
>  		virtio_install vm_install bpf_install x86_energy_perf_policy_install \
> @@ -147,7 +148,7 @@ acpi_clean:
>  cpupower_clean:
>  	$(call descend,power/cpupower,clean)
>  
> -cgroup_clean hv_clean firewire_clean bootconfig_clean spi_clean usb_clean virtio_clean vm_clean wmi_clean bpf_clean iio_clean gpio_clean objtool_clean leds_clean pci_clean firmware_clean debugging_clean tracing_clean:
> +cgroup_clean counter_clean hv_clean firewire_clean bootconfig_clean spi_clean usb_clean virtio_clean vm_clean wmi_clean bpf_clean iio_clean gpio_clean objtool_clean leds_clean pci_clean firmware_clean debugging_clean tracing_clean:
>  	$(call descend,$(@:_clean=),clean)
>  
>  liblockdep_clean:
> @@ -181,7 +182,7 @@ freefall_clean:
>  build_clean:
>  	$(call descend,build,clean)
>  
> -clean: acpi_clean cgroup_clean cpupower_clean hv_clean firewire_clean \
> +clean: acpi_clean cgroup_clean counter_clean cpupower_clean hv_clean firewire_clean \
>  		perf_clean selftests_clean turbostat_clean bootconfig_clean spi_clean usb_clean virtio_clean \
>  		vm_clean bpf_clean iio_clean x86_energy_perf_policy_clean tmon_clean \
>  		freefall_clean build_clean libbpf_clean libsubcmd_clean liblockdep_clean \
> diff --git a/tools/counter/Build b/tools/counter/Build
> new file mode 100644
> index 000000000000..33f4a51d715e
> --- /dev/null
> +++ b/tools/counter/Build
> @@ -0,0 +1 @@
> +counter_example-y += counter_example.o
> diff --git a/tools/counter/Makefile b/tools/counter/Makefile
> new file mode 100644
> index 000000000000..5ebc195fd9c0
> --- /dev/null
> +++ b/tools/counter/Makefile
> @@ -0,0 +1,53 @@
> +# SPDX-License-Identifier: GPL-2.0
> +include ../scripts/Makefile.include
> +
> +bindir ?= /usr/bin
> +
> +ifeq ($(srctree),)
> +srctree := $(patsubst %/,%,$(dir $(CURDIR)))
> +srctree := $(patsubst %/,%,$(dir $(srctree)))
> +endif
> +
> +# Do not use make's built-in rules
> +# (this improves performance and avoids hard-to-debug behaviour);
> +MAKEFLAGS += -r
> +
> +override CFLAGS += -O2 -Wall -g -D_GNU_SOURCE -I$(OUTPUT)include
> +
> +ALL_TARGETS := counter_example
> +ALL_PROGRAMS := $(patsubst %,$(OUTPUT)%,$(ALL_TARGETS))
> +
> +all: $(ALL_PROGRAMS)
> +
> +export srctree OUTPUT CC LD CFLAGS
> +include $(srctree)/tools/build/Makefile.include
> +
> +#
> +# We need the following to be outside of kernel tree
> +#
> +$(OUTPUT)include/linux/counter.h: ../../include/uapi/linux/counter.h
> +	mkdir -p $(OUTPUT)include/linux 2>&1 || true
> +	ln -sf $(CURDIR)/../../include/uapi/linux/counter.h $@
> +
> +prepare: $(OUTPUT)include/linux/counter.h
> +
> +COUNTER_EXAMPLE := $(OUTPUT)counter_example.o
> +$(COUNTER_EXAMPLE): prepare FORCE
> +	$(Q)$(MAKE) $(build)=counter_example
> +$(OUTPUT)counter_example: $(COUNTER_EXAMPLE)
> +	$(QUIET_LINK)$(CC) $(CFLAGS) $(LDFLAGS) $< -o $@
> +
> +clean:
> +	rm -f $(ALL_PROGRAMS)
> +	rm -rf $(OUTPUT)include/linux/counter.h
> +	find $(if $(OUTPUT),$(OUTPUT),.) -name '*.o' -delete -o -name '\.*.d' -delete
> +
> +install: $(ALL_PROGRAMS)
> +	install -d -m 755 $(DESTDIR)$(bindir);		\
> +	for program in $(ALL_PROGRAMS); do		\
> +		install $$program $(DESTDIR)$(bindir);	\
> +	done
> +
> +FORCE:
> +
> +.PHONY: all install clean FORCE prepare
> diff --git a/tools/counter/counter_example.c b/tools/counter/counter_example.c
> new file mode 100644
> index 000000000000..71dfec673c11
> --- /dev/null
> +++ b/tools/counter/counter_example.c
> @@ -0,0 +1,95 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Counter - example userspace application
> + *
> + * The userspace application opens /dev/counter0, configures the
> + * COUNTER_EVENT_INDEX event channel 0 to gather Count 0 count and Count
> + * 1 count, and prints out the data as it becomes available on the
> + * character device node.
> + *
> + * Copyright (C) 2021 William Breathitt Gray
> + */
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <linux/counter.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <sys/ioctl.h>
> +#include <unistd.h>
> +
> +struct counter_watch watches[2] = {
> +	{
> +		/* Component data: Count 0 count */
> +		.component.type = COUNTER_COMPONENT_COUNT,
> +		.component.scope = COUNTER_SCOPE_COUNT,
> +		.component.parent = 0,
> +		/* Event type: Index */
> +		.event = COUNTER_EVENT_INDEX,
> +		/* Device event channel 0 */
> +		.channel = 0,
> +	},
> +	{
> +		/* Component data: Count 1 count */
> +		.component.type = COUNTER_COMPONENT_COUNT,
> +		.component.scope = COUNTER_SCOPE_COUNT,
> +		.component.parent = 1,
> +		/* Event type: Index */
> +		.event = COUNTER_EVENT_INDEX,
> +		/* Device event channel 0 */
> +		.channel = 0,
> +	},
> +};
> +
> +int main(void)
> +{
> +	int fd;
> +	int ret;
> +	struct counter_event event_data[2];
> +
> +	fd = open("/dev/counter0", O_RDWR);
> +	if (fd == -1) {
> +		perror("Unable to open /dev/counter0");
> +		return -errno;
> +	}
> +
> +	ret = ioctl(fd, COUNTER_ADD_WATCH_IOCTL, watches);
> +	if (ret == -1) {
> +		perror("Error adding watches[0]");
> +		return -errno;
> +	}
> +	ret = ioctl(fd, COUNTER_ADD_WATCH_IOCTL, watches + 1);
> +	if (ret == -1) {
> +		perror("Error adding watches[1]");
> +		return -errno;
> +	}
> +	ret = ioctl(fd, COUNTER_ENABLE_EVENTS_IOCTL);
> +	if (ret == -1) {
> +		perror("Error enabling events");
> +		return -errno;
> +	}
> +
> +	for (;;) {
> +		ret = read(fd, event_data, sizeof(event_data));
> +		if (ret == -1) {
> +			perror("Failed to read event data");
> +			return -errno;
> +		}
> +
> +		if (ret != sizeof(event_data)) {
> +			fprintf(stderr, "Failed to read event data\n");
> +			return -EIO;
> +		}
> +
> +		printf("Timestamp 0: %llu\tCount 0: %llu\n"
> +		       "Error Message 0: %s\n"
> +		       "Timestamp 1: %llu\tCount 1: %llu\n"
> +		       "Error Message 1: %s\n",
> +		       (unsigned long long)event_data[0].timestamp,

I think there should be no need to cast this one.  __aligned_u64 is always
going to be of type llu anyway.


> +		       (unsigned long long)event_data[0].value,
> +		       strerror(event_data[0].status),
> +		       (unsigned long long)event_data[1].timestamp,
> +		       (unsigned long long)event_data[1].value,
> +		       strerror(event_data[1].status));
> +	}
> +
> +	return 0;
> +}
David Lechner July 11, 2021, 2:05 p.m. UTC | #4
On 7/11/21 6:28 AM, William Breathitt Gray wrote:
> On Sat, Jul 10, 2021 at 11:53:35AM -0500, David Lechner wrote:
>> On 7/5/21 3:19 AM, William Breathitt Gray wrote:

>>> +	{
>>> +		/* Component data: Count 0 count */
>>> +		.component.type = COUNTER_COMPONENT_COUNT,
>>> +		.component.scope = COUNTER_SCOPE_COUNT,
>>> +		.component.parent = 0,
>>> +		/* Event type: Index */
>>> +		.event = COUNTER_EVENT_INDEX,
>>> +		/* Device event channel 0 */
>>> +		.channel = 0,
>>> +	},
>>> +	{
>>> +		/* Component data: Count 1 count */
>>> +		.component.type = COUNTER_COMPONENT_COUNT,
>>> +		.component.scope = COUNTER_SCOPE_COUNT,
>>> +		.component.parent = 1,
>>> +		/* Event type: Index */
>>> +		.event = COUNTER_EVENT_INDEX,
>>> +		/* Device event channel 0 */
>>> +		.channel = 0,
>>> +	},
>>> +};
>>> +
>>> +int main(void)
>>> +{
>>> +	int fd;
>>> +	int ret;
>>> +	struct counter_event event_data[2];
>>> +
>>> +	fd = open("/dev/counter0", O_RDWR);
>>> +	if (fd == -1) {
>>> +		perror("Unable to open /dev/counter0");
>>> +		return -errno;
>>
>> errno is no longer valid after calling perror(). Since this
>> is example code, we can just return 1 instead (exit codes
>> positive number between 0 and 255 so -1 would be 255).
> 
> Ack.
> 
>>> +	}
>>> +
>>> +	ret = ioctl(fd, COUNTER_ADD_WATCH_IOCTL, watches);
>>> +	if (ret == -1) {
>>> +		perror("Error adding watches[0]");
>>> +		return -errno;
>>> +	}
>>> +	ret = ioctl(fd, COUNTER_ADD_WATCH_IOCTL, watches + 1);
>>> +	if (ret == -1) {
>>> +		perror("Error adding watches[1]");
>>> +		return -errno;
>>> +	}
>>> +	ret = ioctl(fd, COUNTER_ENABLE_EVENTS_IOCTL);
>>> +	if (ret == -1) {
>>> +		perror("Error enabling events");
>>> +		return -errno;
>>> +	}
>>> +
>>> +	for (;;) {
>>> +		ret = read(fd, event_data, sizeof(event_data));
>>> +		if (ret == -1) {
>>> +			perror("Failed to read event data");
>>> +			return -errno;
>>> +		}
>>> +
>>> +		if (ret != sizeof(event_data)) {
>>> +			fprintf(stderr, "Failed to read event data\n");
>>> +			return -EIO;
>>> +		}
>>> +
>>> +		printf("Timestamp 0: %llu\tCount 0: %llu\n"
>>> +		       "Error Message 0: %s\n"
>>> +		       "Timestamp 1: %llu\tCount 1: %llu\n"
>>> +		       "Error Message 1: %s\n",
>>> +		       (unsigned long long)event_data[0].timestamp,
>>> +		       (unsigned long long)event_data[0].value,
>>> +		       strerror(event_data[0].status),
>>> +		       (unsigned long long)event_data[1].timestamp,
>>> +		       (unsigned long long)event_data[1].value,
>>> +		       strerror(event_data[1].status));
>>> +	}
>>
>> Aren't the Count 0 and Count 1 events independent? Why should we expect to
>> always get both events at the same time in the same order?
> 
> Watch 0 and Watch 1 are both triggered by the same event: a
> COUNTER_EVENT_INDEX event on device event channel 0. If we had set
> channel to 1 for Watch 1, then we would have two independent events, but
> in this case both Watches have their respective channel set to 0.

The thing that jumped out to me is that they have different parents.
But I guess I forgot that the event itself always has a scope of
device and that the component just says what value to record and
is otherwise independent of the event.
diff mbox series

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index 5de4d2164844..e96797f57f04 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4688,6 +4688,7 @@  F:	Documentation/driver-api/generic-counter.rst
 F:	drivers/counter/
 F:	include/linux/counter.h
 F:	include/uapi/linux/counter.h
+F:	tools/counter/
 
 CP2615 I2C DRIVER
 M:	Bence Csókás <bence98@sch.bme.hu>
diff --git a/tools/Makefile b/tools/Makefile
index 7e9d34ddd74c..4c26400ffc03 100644
--- a/tools/Makefile
+++ b/tools/Makefile
@@ -12,6 +12,7 @@  help:
 	@echo '  acpi                   - ACPI tools'
 	@echo '  bpf                    - misc BPF tools'
 	@echo '  cgroup                 - cgroup tools'
+	@echo '  counter                - Counter tools'
 	@echo '  cpupower               - a tool for all things x86 CPU power'
 	@echo '  debugging              - tools for debugging'
 	@echo '  firewire               - the userspace part of nosy, an IEEE-1394 traffic sniffer'
@@ -65,7 +66,7 @@  acpi: FORCE
 cpupower: FORCE
 	$(call descend,power/$@)
 
-cgroup firewire hv guest bootconfig spi usb virtio vm bpf iio gpio objtool leds wmi pci firmware debugging tracing: FORCE
+cgroup counter firewire hv guest bootconfig spi usb virtio vm bpf iio gpio objtool leds wmi pci firmware debugging tracing: FORCE
 	$(call descend,$@)
 
 bpf/%: FORCE
@@ -100,7 +101,7 @@  freefall: FORCE
 kvm_stat: FORCE
 	$(call descend,kvm/$@)
 
-all: acpi cgroup cpupower gpio hv firewire liblockdep \
+all: acpi cgroup counter cpupower gpio hv firewire liblockdep \
 		perf selftests bootconfig spi turbostat usb \
 		virtio vm bpf x86_energy_perf_policy \
 		tmon freefall iio objtool kvm_stat wmi \
@@ -112,7 +113,7 @@  acpi_install:
 cpupower_install:
 	$(call descend,power/$(@:_install=),install)
 
-cgroup_install firewire_install gpio_install hv_install iio_install perf_install bootconfig_install spi_install usb_install virtio_install vm_install bpf_install objtool_install wmi_install pci_install debugging_install tracing_install:
+cgroup_install counter_install firewire_install gpio_install hv_install iio_install perf_install bootconfig_install spi_install usb_install virtio_install vm_install bpf_install objtool_install wmi_install pci_install debugging_install tracing_install:
 	$(call descend,$(@:_install=),install)
 
 liblockdep_install:
@@ -133,7 +134,7 @@  freefall_install:
 kvm_stat_install:
 	$(call descend,kvm/$(@:_install=),install)
 
-install: acpi_install cgroup_install cpupower_install gpio_install \
+install: acpi_install cgroup_install counter_install cpupower_install gpio_install \
 		hv_install firewire_install iio_install liblockdep_install \
 		perf_install selftests_install turbostat_install usb_install \
 		virtio_install vm_install bpf_install x86_energy_perf_policy_install \
@@ -147,7 +148,7 @@  acpi_clean:
 cpupower_clean:
 	$(call descend,power/cpupower,clean)
 
-cgroup_clean hv_clean firewire_clean bootconfig_clean spi_clean usb_clean virtio_clean vm_clean wmi_clean bpf_clean iio_clean gpio_clean objtool_clean leds_clean pci_clean firmware_clean debugging_clean tracing_clean:
+cgroup_clean counter_clean hv_clean firewire_clean bootconfig_clean spi_clean usb_clean virtio_clean vm_clean wmi_clean bpf_clean iio_clean gpio_clean objtool_clean leds_clean pci_clean firmware_clean debugging_clean tracing_clean:
 	$(call descend,$(@:_clean=),clean)
 
 liblockdep_clean:
@@ -181,7 +182,7 @@  freefall_clean:
 build_clean:
 	$(call descend,build,clean)
 
-clean: acpi_clean cgroup_clean cpupower_clean hv_clean firewire_clean \
+clean: acpi_clean cgroup_clean counter_clean cpupower_clean hv_clean firewire_clean \
 		perf_clean selftests_clean turbostat_clean bootconfig_clean spi_clean usb_clean virtio_clean \
 		vm_clean bpf_clean iio_clean x86_energy_perf_policy_clean tmon_clean \
 		freefall_clean build_clean libbpf_clean libsubcmd_clean liblockdep_clean \
diff --git a/tools/counter/Build b/tools/counter/Build
new file mode 100644
index 000000000000..33f4a51d715e
--- /dev/null
+++ b/tools/counter/Build
@@ -0,0 +1 @@ 
+counter_example-y += counter_example.o
diff --git a/tools/counter/Makefile b/tools/counter/Makefile
new file mode 100644
index 000000000000..5ebc195fd9c0
--- /dev/null
+++ b/tools/counter/Makefile
@@ -0,0 +1,53 @@ 
+# SPDX-License-Identifier: GPL-2.0
+include ../scripts/Makefile.include
+
+bindir ?= /usr/bin
+
+ifeq ($(srctree),)
+srctree := $(patsubst %/,%,$(dir $(CURDIR)))
+srctree := $(patsubst %/,%,$(dir $(srctree)))
+endif
+
+# Do not use make's built-in rules
+# (this improves performance and avoids hard-to-debug behaviour);
+MAKEFLAGS += -r
+
+override CFLAGS += -O2 -Wall -g -D_GNU_SOURCE -I$(OUTPUT)include
+
+ALL_TARGETS := counter_example
+ALL_PROGRAMS := $(patsubst %,$(OUTPUT)%,$(ALL_TARGETS))
+
+all: $(ALL_PROGRAMS)
+
+export srctree OUTPUT CC LD CFLAGS
+include $(srctree)/tools/build/Makefile.include
+
+#
+# We need the following to be outside of kernel tree
+#
+$(OUTPUT)include/linux/counter.h: ../../include/uapi/linux/counter.h
+	mkdir -p $(OUTPUT)include/linux 2>&1 || true
+	ln -sf $(CURDIR)/../../include/uapi/linux/counter.h $@
+
+prepare: $(OUTPUT)include/linux/counter.h
+
+COUNTER_EXAMPLE := $(OUTPUT)counter_example.o
+$(COUNTER_EXAMPLE): prepare FORCE
+	$(Q)$(MAKE) $(build)=counter_example
+$(OUTPUT)counter_example: $(COUNTER_EXAMPLE)
+	$(QUIET_LINK)$(CC) $(CFLAGS) $(LDFLAGS) $< -o $@
+
+clean:
+	rm -f $(ALL_PROGRAMS)
+	rm -rf $(OUTPUT)include/linux/counter.h
+	find $(if $(OUTPUT),$(OUTPUT),.) -name '*.o' -delete -o -name '\.*.d' -delete
+
+install: $(ALL_PROGRAMS)
+	install -d -m 755 $(DESTDIR)$(bindir);		\
+	for program in $(ALL_PROGRAMS); do		\
+		install $$program $(DESTDIR)$(bindir);	\
+	done
+
+FORCE:
+
+.PHONY: all install clean FORCE prepare
diff --git a/tools/counter/counter_example.c b/tools/counter/counter_example.c
new file mode 100644
index 000000000000..71dfec673c11
--- /dev/null
+++ b/tools/counter/counter_example.c
@@ -0,0 +1,95 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+/* Counter - example userspace application
+ *
+ * The userspace application opens /dev/counter0, configures the
+ * COUNTER_EVENT_INDEX event channel 0 to gather Count 0 count and Count
+ * 1 count, and prints out the data as it becomes available on the
+ * character device node.
+ *
+ * Copyright (C) 2021 William Breathitt Gray
+ */
+#include <errno.h>
+#include <fcntl.h>
+#include <linux/counter.h>
+#include <stdio.h>
+#include <string.h>
+#include <sys/ioctl.h>
+#include <unistd.h>
+
+struct counter_watch watches[2] = {
+	{
+		/* Component data: Count 0 count */
+		.component.type = COUNTER_COMPONENT_COUNT,
+		.component.scope = COUNTER_SCOPE_COUNT,
+		.component.parent = 0,
+		/* Event type: Index */
+		.event = COUNTER_EVENT_INDEX,
+		/* Device event channel 0 */
+		.channel = 0,
+	},
+	{
+		/* Component data: Count 1 count */
+		.component.type = COUNTER_COMPONENT_COUNT,
+		.component.scope = COUNTER_SCOPE_COUNT,
+		.component.parent = 1,
+		/* Event type: Index */
+		.event = COUNTER_EVENT_INDEX,
+		/* Device event channel 0 */
+		.channel = 0,
+	},
+};
+
+int main(void)
+{
+	int fd;
+	int ret;
+	struct counter_event event_data[2];
+
+	fd = open("/dev/counter0", O_RDWR);
+	if (fd == -1) {
+		perror("Unable to open /dev/counter0");
+		return -errno;
+	}
+
+	ret = ioctl(fd, COUNTER_ADD_WATCH_IOCTL, watches);
+	if (ret == -1) {
+		perror("Error adding watches[0]");
+		return -errno;
+	}
+	ret = ioctl(fd, COUNTER_ADD_WATCH_IOCTL, watches + 1);
+	if (ret == -1) {
+		perror("Error adding watches[1]");
+		return -errno;
+	}
+	ret = ioctl(fd, COUNTER_ENABLE_EVENTS_IOCTL);
+	if (ret == -1) {
+		perror("Error enabling events");
+		return -errno;
+	}
+
+	for (;;) {
+		ret = read(fd, event_data, sizeof(event_data));
+		if (ret == -1) {
+			perror("Failed to read event data");
+			return -errno;
+		}
+
+		if (ret != sizeof(event_data)) {
+			fprintf(stderr, "Failed to read event data\n");
+			return -EIO;
+		}
+
+		printf("Timestamp 0: %llu\tCount 0: %llu\n"
+		       "Error Message 0: %s\n"
+		       "Timestamp 1: %llu\tCount 1: %llu\n"
+		       "Error Message 1: %s\n",
+		       (unsigned long long)event_data[0].timestamp,
+		       (unsigned long long)event_data[0].value,
+		       strerror(event_data[0].status),
+		       (unsigned long long)event_data[1].timestamp,
+		       (unsigned long long)event_data[1].value,
+		       strerror(event_data[1].status));
+	}
+
+	return 0;
+}