Introduce the Counter character device interface

Hi William,

I love your patch! Perhaps something to improve:

[auto build test WARNING on stm32/stm32-next]
[also build test WARNING on linux/master linus/master v5.10-rc5 next-20201124] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/William-Breathitt-Gray/Introduce-the-Counter-character-device-interface/20201123-043634
base:   https://git.kernel.org/pub/scm/linux/kernel/git/atorgue/stm32.git stm32-next
config: powerpc-randconfig-r016-20201124 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project df9ae5992889560a8f3c6760b54d5051b47c7bf5)

reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# install powerpc cross compiling tool for clang build
# apt-get install binutils-powerpc-linux-gnu
# https://github.com/0day-ci/linux/commit/1bc8c3bb021f3736c23d97f1a965935821f0ac5a
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review William-Breathitt-Gray/Introduce-the-Counter-character-device-interface/20201123-043634 git checkout 1bc8c3bb021f3736c23d97f1a965935821f0ac5a
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=powerpc

If you fix the issue, kindly add following tag as appropriate

Reported-by kernel test robot <[email protected]>

All warnings (new ones prefixed by >>):

In file included from include/linux/iopoll.h:14:
In file included from include/linux/io.h:13:
In file included from arch/powerpc/include/asm/io.h:604:
arch/powerpc/include/asm/io-defs.h:43:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] DEF_PCI_AC_NORET(insb, (unsigned long p, void *b, unsigned long c), ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/include/asm/io.h:601:3: note: expanded from macro 'DEF_PCI_AC_NORET'

                   __do_##name al;                                 \
                   ^~~~~~~~~~~~~~
   <scratch space>:170:1: note: expanded from here
   __do_insb
   ^
   arch/powerpc/include/asm/io.h:541:56: note: expanded from macro '__do_insb'
   #define __do_insb(p, b, n)      readsb((PCI_IO_ADDR)_IO_BASE+(p), (b), (n))
                                          ~~~~~~~~~~~~~~~~~~~~~^
   In file included from drivers/counter/ti-eqep.c:15:
   In file included from include/linux/regmap.h:20:
   In file included from include/linux/iopoll.h:14:
   In file included from include/linux/io.h:13:
   In file included from arch/powerpc/include/asm/io.h:604:
   arch/powerpc/include/asm/io-defs.h:45:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
   DEF_PCI_AC_NORET(insw, (unsigned long p, void *b, unsigned long c),
   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   arch/powerpc/include/asm/io.h:601:3: note: expanded from macro 'DEF_PCI_AC_NORET'
                   __do_##name al;                                 \
                   ^~~~~~~~~~~~~~
   <scratch space>:172:1: note: expanded from here
   __do_insw
   ^
   arch/powerpc/include/asm/io.h:542:56: note: expanded from macro '__do_insw'
   #define __do_insw(p, b, n)      readsw((PCI_IO_ADDR)_IO_BASE+(p), (b), (n))
                                          ~~~~~~~~~~~~~~~~~~~~~^
   In file included from drivers/counter/ti-eqep.c:15:
   In file included from include/linux/regmap.h:20:
   In file included from include/linux/iopoll.h:14:
   In file included from include/linux/io.h:13:
   In file included from arch/powerpc/include/asm/io.h:604:
   arch/powerpc/include/asm/io-defs.h:47:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
   DEF_PCI_AC_NORET(insl, (unsigned long p, void *b, unsigned long c),
   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   arch/powerpc/include/asm/io.h:601:3: note: expanded from macro 'DEF_PCI_AC_NORET'
                   __do_##name al;                                 \
                   ^~~~~~~~~~~~~~
   <scratch space>:174:1: note: expanded from here
   __do_insl
   ^
   arch/powerpc/include/asm/io.h:543:56: note: expanded from macro '__do_insl'
   #define __do_insl(p, b, n)      readsl((PCI_IO_ADDR)_IO_BASE+(p), (b), (n))
                                          ~~~~~~~~~~~~~~~~~~~~~^
   In file included from drivers/counter/ti-eqep.c:15:
   In file included from include/linux/regmap.h:20:
   In file included from include/linux/iopoll.h:14:
   In file included from include/linux/io.h:13:
   In file included from arch/powerpc/include/asm/io.h:604:
   arch/powerpc/include/asm/io-defs.h:49:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
   DEF_PCI_AC_NORET(outsb, (unsigned long p, const void *b, unsigned long c),
   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   arch/powerpc/include/asm/io.h:601:3: note: expanded from macro 'DEF_PCI_AC_NORET'
                   __do_##name al;                                 \
                   ^~~~~~~~~~~~~~
   <scratch space>:176:1: note: expanded from here
   __do_outsb
   ^
   arch/powerpc/include/asm/io.h:544:58: note: expanded from macro '__do_outsb'
   #define __do_outsb(p, b, n)     writesb((PCI_IO_ADDR)_IO_BASE+(p),(b),(n))
                                           ~~~~~~~~~~~~~~~~~~~~~^
   In file included from drivers/counter/ti-eqep.c:15:
   In file included from include/linux/regmap.h:20:
   In file included from include/linux/iopoll.h:14:
   In file included from include/linux/io.h:13:
   In file included from arch/powerpc/include/asm/io.h:604:
   arch/powerpc/include/asm/io-defs.h:51:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
   DEF_PCI_AC_NORET(outsw, (unsigned long p, const void *b, unsigned long c),
   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   arch/powerpc/include/asm/io.h:601:3: note: expanded from macro 'DEF_PCI_AC_NORET'
                   __do_##name al;                                 \
                   ^~~~~~~~~~~~~~
   <scratch space>:178:1: note: expanded from here
   __do_outsw
   ^
   arch/powerpc/include/asm/io.h:545:58: note: expanded from macro '__do_outsw'
   #define __do_outsw(p, b, n)     writesw((PCI_IO_ADDR)_IO_BASE+(p),(b),(n))
                                           ~~~~~~~~~~~~~~~~~~~~~^
   In file included from drivers/counter/ti-eqep.c:15:
   In file included from include/linux/regmap.h:20:
   In file included from include/linux/iopoll.h:14:
   In file included from include/linux/io.h:13:
   In file included from arch/powerpc/include/asm/io.h:604:
   arch/powerpc/include/asm/io-defs.h:53:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
   DEF_PCI_AC_NORET(outsl, (unsigned long p, const void *b, unsigned long c),
   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   arch/powerpc/include/asm/io.h:601:3: note: expanded from macro 'DEF_PCI_AC_NORET'
                   __do_##name al;                                 \
                   ^~~~~~~~~~~~~~
   <scratch space>:180:1: note: expanded from here
   __do_outsl
   ^
   arch/powerpc/include/asm/io.h:546:58: note: expanded from macro '__do_outsl'
   #define __do_outsl(p, b, n)     writesl((PCI_IO_ADDR)_IO_BASE+(p),(b),(n))
                                           ~~~~~~~~~~~~~~~~~~~~~^

Quoted 1 line

Quoted 1 line

drivers/counter/ti-eqep.c:115:17: warning: unused variable 'ti_qep_t2c_functions_map' [-Wunused-const-variable]

   static const u8 ti_qep_t2c_functions_map[] = {
                   ^

13 warnings generated.

vim +/ti_qep_t2c_functions_map +115 drivers/counter/ti-eqep.c

   114	

Quoted 1 line

115 static const u8 ti_qep_t2c_functions_map[] = {

   116	};
   117	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

  14 files changed, 1806 insertions(+), 2546 deletions(-)

drivers/counter/counter-sysfs.c new file +35 -0

+static ssize_t counter_comp_u8_store(struct device *dev,
+				     struct device_attribute *attr,
+				     const char *buf, size_t len)
+{
+	const struct counter_attribute *const a = to_counter_attribute(attr);
+	struct counter_device *const counter = dev_get_drvdata(dev);
+	struct counter_count *const count = a->parent;
+	struct counter_synapse *const synapse = a->comp.priv;
+	const struct counter_available *const avail = a->comp.priv;
+	int err;
+	bool bool_data;
+	int idx;
+	u8 data;
+
+	switch (a->comp.type) {
+	case COUNTER_COMP_BOOL:
+		err = kstrtobool(buf, &bool_data);
+		data = bool_data;
+		break;
+	case COUNTER_COMP_FUNCTION:
+		err = find_in_string_array(&data, count->functions_list,
+					   count->num_functions, buf,
+					   counter_function_str);
+		break;
+	case COUNTER_COMP_SYNAPSE_ACTION:
+		err = find_in_string_array(&data, synapse->actions_list,
+					   synapse->num_actions, buf,
+					   counter_synapse_action_str);
+		break;
+	case COUNTER_COMP_ENUM:
+		idx = __sysfs_match_string(avail->strs, avail->num_items, buf);
+		if (idx < 0)
+			return idx;
+		data = idx;
+		break;

drivers/counter/counter-sysfs.c new file +22 -0

+		return err;
+
+	switch (a->scope) {
+	case COUNTER_SCOPE_DEVICE:
+		err = a->comp.device_u8_write(counter, data);
+		break;
+	case COUNTER_SCOPE_SIGNAL:
+		err = a->comp.signal_u8_write(counter, a->parent, data);
+		break;
+	case COUNTER_SCOPE_COUNT:
+		if (a->comp.type == COUNTER_COMP_SYNAPSE_ACTION)
+			err = a->comp.action_write(counter, count, synapse,
+						   data);
+		else
+			err = a->comp.count_u8_write(counter, count, data);
+		break;
+	}
+	if (err)
+		return err;
+
+	return len;
+}

Quoted 1 line · William Breathitt Gray

  14 files changed, 1806 insertions(+), 2546 deletions(-)

It would be really nice if we could break this down into smaller pieces and start getting it merged. It is really tough to keep reviewing this much code in one patch over and over again.

Here are some initial findings from testing:

drivers/counter/counter-core.c line 25

Quoted 7 lines

drivers/counter/counter-core.c new file +7 -0

+static void counter_device_release(struct device *dev)
+{
+	struct counter_device *const counter = dev_get_drvdata(dev);
+
+	counter_chrdev_remove(counter);
+	ida_simple_remove(&counter_ida, counter->id);
+}

I got the following error after modprobe -r ti-eqep:

[ 1186.045766] ------------[ cut here ]------------
[ 1186.050647] WARNING: CPU: 0 PID: 2625 at lib/refcount.c:28 counter_device_release+0x10/0x24 [counter]
[ 1186.059976] refcount_t: underflow; use-after-free.
[ 1186.064790] Modules linked in: aes_arm_bs(+) crypto_simd cryptd ccm usb_f_mass_storage usb_f_acm u_serial usb_f_ecm rfcomm usb_f_rndis u_ether libcomposite aes_arm aes_generic cmac bnep wl18xx wlcore mac80211 libarc4 sha256_generic libsha256 sha256_arm cfg80211 ti_am335x_adc kfifo_buf omap_aes_driver omap_crypto omap_sham crypto_engine pm33xx ti_emif_sram hci_uart omap_rng btbcm rng_core ti_eqep(-) counter bluetooth c_can_platform c_can ecdh_generic bmp280_spi ecc can_dev libaes bmp280_i2c bmp280 industrialio omap_mailbox musb_dsps wlcore_sdio musb_hdrc udc_core usbcore wkup_m3_ipc at24 omap_wdt phy_am335x watchdog phy_am335x_control ti_am335x_tscadc phy_generic wkup_m3_rproc usb_common cppi41 rtc_omap leds_gpio led_class cpufreq_dt pwm_tiehrpwm autofs4
[ 1186.132376] CPU: 0 PID: 2625 Comm: modprobe Not tainted 5.10.0-rc7bone-counter+ #23
[ 1186.140070] Hardware name: Generic AM33XX (Flattened Device Tree)
[ 1186.146225] [<c0110d70>] (unwind_backtrace) from [<c010b640>] (show_stack+0x10/0x14)
[ 1186.154017] [<c010b640>] (show_stack) from [<c09a0c98>] (dump_stack+0xc4/0xe4)
[ 1186.161285] [<c09a0c98>] (dump_stack) from [<c0137ba0>] (__warn+0xd8/0x100)
[ 1186.168284] [<c0137ba0>] (__warn) from [<c099c8e4>] (warn_slowpath_fmt+0x94/0xbc)
[ 1186.175814] [<c099c8e4>] (warn_slowpath_fmt) from [<bf10b0e8>] (counter_device_release+0x10/0x24 [counter])
[ 1186.185632] [<bf10b0e8>] (counter_device_release [counter]) from [<c0667118>] (device_release+0x30/0xa4)
[ 1186.195163] [<c0667118>] (device_release) from [<c057f73c>] (kobject_put+0x94/0x104)
[ 1186.202944] [<c057f73c>] (kobject_put) from [<c057f73c>] (kobject_put+0x94/0x104)
[ 1186.210472] [<c057f73c>] (kobject_put) from [<bf19004c>] (ti_eqep_remove+0x10/0x30 [ti_eqep])
[ 1186.219047] [<bf19004c>] (ti_eqep_remove [ti_eqep]) from [<c066f390>] (platform_drv_remove+0x24/0x3c)
[ 1186.228313] [<c066f390>] (platform_drv_remove) from [<c066d934>] (device_release_driver_internal+0xfc/0x1d0)
[ 1186.238187] [<c066d934>] (device_release_driver_internal) from [<c066da78>] (driver_detach+0x58/0xa8)

[ 1186.247456] [<c066da78>] (driver_detach) from [<c066c5ec>] (bus_remove_driver+0x4c/0xa0)
[ 1186.255594] [<c066c5ec>] (bus_remove_driver) from [<c01dd150>] (sys_delete_module+0x180/0x264)
[ 1186.264250] [<c01dd150>] (sys_delete_module) from [<c0100080>] (ret_fast_syscall+0x0/0x54)
[ 1186.272551] Exception stack(0xd247ffa8 to 0xd247fff0)
[ 1186.277629] ffa0:                   004fb478 004fb478 004fb4b4 00000800 b3bfcf00 00000000
[ 1186.285847] ffc0: 004fb478 004fb478 004fb478 00000081 00000000 be974900 be974a55 004fb478
[ 1186.294062] ffe0: 004f8f5c be97352c 004ddd97 b6d11d68
[ 1186.299253] ---[ end trace e1c61dea091f1078 ]---

Quoted 41 lines

drivers/counter/counter-sysfs.c new file +41 -0

+static ssize_t counter_comp_u8_store(struct device *dev,
+				     struct device_attribute *attr,
+				     const char *buf, size_t len)
+{
+	const struct counter_attribute *const a = to_counter_attribute(attr);
+	struct counter_device *const counter = dev_get_drvdata(dev);
+	struct counter_count *const count = a->parent;
+	struct counter_synapse *const synapse = a->comp.priv;
+	const struct counter_available *const avail = a->comp.priv;
+	int err;
+	bool bool_data;
+	int idx;
+	u8 data;
+
+	switch (a->comp.type) {
+	case COUNTER_COMP_BOOL:
+		err = kstrtobool(buf, &bool_data);
+		data = bool_data;
+		break;
+	case COUNTER_COMP_FUNCTION:
+		err = find_in_string_array(&data, count->functions_list,
+					   count->num_functions, buf,
+					   counter_function_str);
+		break;
+	case COUNTER_COMP_SYNAPSE_ACTION:
+		err = find_in_string_array(&data, synapse->actions_list,
+					   synapse->num_actions, buf,
+					   counter_synapse_action_str);
+		break;
+	case COUNTER_COMP_ENUM:
+		idx = __sysfs_match_string(avail->strs, avail->num_items, buf);
+		if (idx < 0)
+			return idx;
+		data = idx;
+		break;
+	case COUNTER_COMP_COUNT_MODE:
+		err = find_in_string_array(&data, avail->enums,
+					   avail->num_items, buf,
+					   counter_count_mode_str);
+		break;
+	default:

drivers/counter/counter-sysfs.c line 193

Quoted 4 lines

drivers/counter/counter-sysfs.c new file +4 -0

+		err = kstrtou8(buf, 0, &data);
+		break;
+	}
+	if (err)

This needs to be if (err < 0). There are cases where the functions above return positive values. (And to be overly safe, it probably wouldn't hurt to use err < 0 everywhere - not just in this function.)

Quoted 7 lines · David Lechner

Quoted 1 line · William Breathitt Gray

   14 files changed, 1806 insertions(+), 2546 deletions(-)

It would be really nice if we could break this down into smaller pieces and start getting it merged. It is really tough to keep reviewing this much code in one patch over and over again.

Yes, this is a pretty massive patch. I could break this across the individual files affected to make it simpler to review, but in the end all those patches would need to end up squashed together before merge again (for the sake of git bisect), so the effort feels somewhat moot.

Luckily, I don't think there will be much change in the next revision since it's looking like it'll mainly be a few bug fixes; hopefully this coming version 7 will be the final revision before merge.

Quoted 32 lines

Here are some initial findings from testing:

drivers/counter/counter-core.c line 25

Quoted 7 lines

drivers/counter/counter-core.c new file +7 -0

+static void counter_device_release(struct device *dev)
+{
+	struct counter_device *const counter = dev_get_drvdata(dev);
+
+	counter_chrdev_remove(counter);
+	ida_simple_remove(&counter_ida, counter->id);
+}

I got the following error after modprobe -r ti-eqep:

[ 1186.045766] ------------[ cut here ]------------
[ 1186.050647] WARNING: CPU: 0 PID: 2625 at lib/refcount.c:28 counter_device_release+0x10/0x24 [counter]
[ 1186.059976] refcount_t: underflow; use-after-free.
[ 1186.064790] Modules linked in: aes_arm_bs(+) crypto_simd cryptd ccm usb_f_mass_storage usb_f_acm u_serial usb_f_ecm rfcomm usb_f_rndis u_ether libcomposite aes_arm aes_generic cmac bnep wl18xx wlcore mac80211 libarc4 sha256_generic libsha256 sha256_arm cfg80211 ti_am335x_adc kfifo_buf omap_aes_driver omap_crypto omap_sham crypto_engine pm33xx ti_emif_sram hci_uart omap_rng btbcm rng_core ti_eqep(-) counter bluetooth c_can_platform c_can ecdh_generic bmp280_spi ecc can_dev libaes bmp280_i2c bmp280 industrialio omap_mailbox musb_dsps wlcore_sdio musb_hdrc udc_core usbcore wkup_m3_ipc at24 omap_wdt phy_am335x watchdog phy_am335x_control ti_am335x_tscadc phy_generic wkup_m3_rproc usb_common cppi41 rtc_omap leds_gpio led_class cpufreq_dt pwm_tiehrpwm autofs4
[ 1186.132376] CPU: 0 PID: 2625 Comm: modprobe Not tainted 5.10.0-rc7bone-counter+ #23
[ 1186.140070] Hardware name: Generic AM33XX (Flattened Device Tree)
[ 1186.146225] [<c0110d70>] (unwind_backtrace) from [<c010b640>] (show_stack+0x10/0x14)
[ 1186.154017] [<c010b640>] (show_stack) from [<c09a0c98>] (dump_stack+0xc4/0xe4)
[ 1186.161285] [<c09a0c98>] (dump_stack) from [<c0137ba0>] (__warn+0xd8/0x100)
[ 1186.168284] [<c0137ba0>] (__warn) from [<c099c8e4>] (warn_slowpath_fmt+0x94/0xbc)
[ 1186.175814] [<c099c8e4>] (warn_slowpath_fmt) from [<bf10b0e8>] (counter_device_release+0x10/0x24 [counter])
[ 1186.185632] [<bf10b0e8>] (counter_device_release [counter]) from [<c0667118>] (device_release+0x30/0xa4)
[ 1186.195163] [<c0667118>] (device_release) from [<c057f73c>] (kobject_put+0x94/0x104)
[ 1186.202944] [<c057f73c>] (kobject_put) from [<c057f73c>] (kobject_put+0x94/0x104)
[ 1186.210472] [<c057f73c>] (kobject_put) from [<bf19004c>] (ti_eqep_remove+0x10/0x30 [ti_eqep])
[ 1186.219047] [<bf19004c>] (ti_eqep_remove [ti_eqep]) from [<c066f390>] (platform_drv_remove+0x24/0x3c)
[ 1186.228313] [<c066f390>] (platform_drv_remove) from [<c066d934>] (device_release_driver_internal+0xfc/0x1d0)
[ 1186.238187] [<c066d934>] (device_release_driver_internal) from [<c066da78>] (driver_detach+0x58/0xa8)

Quoted 8 lines

[ 1186.247456] [<c066da78>] (driver_detach) from [<c066c5ec>] (bus_remove_driver+0x4c/0xa0)
[ 1186.255594] [<c066c5ec>] (bus_remove_driver) from [<c01dd150>] (sys_delete_module+0x180/0x264)
[ 1186.264250] [<c01dd150>] (sys_delete_module) from [<c0100080>] (ret_fast_syscall+0x0/0x54)
[ 1186.272551] Exception stack(0xd247ffa8 to 0xd247fff0)
[ 1186.277629] ffa0:                   004fb478 004fb478 004fb4b4 00000800 b3bfcf00 00000000
[ 1186.285847] ffc0: 004fb478 004fb478 004fb478 00000081 00000000 be974900 be974a55 004fb478
[ 1186.294062] ffe0: 004f8f5c be97352c 004ddd97 b6d11d68
[ 1186.299253] ---[ end trace e1c61dea091f1078 ]---

I noticed that I'm calling counter_chrdev_remove() twice: once in counter_unregister(), and again in counter_device_release(). I suspect this is what's causing the refcount to underflow. I'll test and verify that this is the culprit.

In fact, I don't think I need to define a counter_device_release() callback at all, would I? These cleanup function calls could be moved to counter_unregister() instead.

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 <[email protected]>

Cc Gwendal Grignou <[email protected]>

Cc Dan Carpenter <[email protected]>

Signed-off-by William Breathitt Gray <[email protected]>

---

drivers/counter/counter-chrdev.c new file +13 -0

new file mode 100644
index 000000000000..96fa7fbeef92
 
 
+static ssize_t counter_chrdev_read(struct file *filp, char __user *buf,
+				   size_t len, loff_t *f_ps)
+{
+	struct counter_device *const counter = filp->private_data;
+	int err;
+	unsigned long flags;
+	unsigned int copied;
+
+	if (len < sizeof(struct counter_event))
+		return -EINVAL;
+
+	do {
+		if (kfifo_is_empty(&counter->events)) {

drivers/counter/counter-chrdev.c new file +14 -0

+		err = kfifo_to_user(&counter->events, buf, len, &copied);
+		raw_spin_unlock_irqrestore(&counter->events_lock, flags);
+		if (err)
+			return err;
+	} while (!copied);
+
+	return copied;
+}
+
 
 
+static int counter_add_watch(struct counter_device *const counter,
+			     const unsigned long arg)
+{
+	void __user *const uwatch = (void __user *)arg;
+	struct counter_watch watch;

drivers/counter/counter-chrdev.c new file +76 -0

+		goto dummy_component;
+
+	parent = watch.component.parent;
+
+	/* Configure parent component info for comp node */
+	switch (watch.component.scope) {
+	case COUNTER_SCOPE_DEVICE:
+		ext = counter->ext;
+		num_ext = counter->num_ext;
+		break;
+	case COUNTER_SCOPE_SIGNAL:
+		if (parent >= counter->num_signals)
+			return -EINVAL;
+		parent = array_index_nospec(parent, counter->num_signals);
+
+		comp_node.parent = counter->signals + parent;
+
+		ext = counter->signals[parent].ext;
+		num_ext = counter->signals[parent].num_ext;
+		break;
+	case COUNTER_SCOPE_COUNT:
+		if (parent >= counter->num_counts)
+			return -EINVAL;
+		parent = array_index_nospec(parent, counter->num_counts);
+
+		comp_node.parent = counter->counts + parent;
+
+		ext = counter->counts[parent].ext;
+		num_ext = counter->counts[parent].num_ext;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	id = watch.component.id;
+
+	/* Configure component info for comp node */
+	switch (watch.component.type) {
+	case COUNTER_COMPONENT_SIGNAL:
+		if (watch.component.scope != COUNTER_SCOPE_SIGNAL)
+			return -EINVAL;
+
+		comp_node.comp.type = COUNTER_COMP_SIGNAL_LEVEL;
+		comp_node.comp.signal_u8_read = counter->ops->signal_read;
+		break;
+	case COUNTER_COMPONENT_COUNT:
+		if (watch.component.scope != COUNTER_SCOPE_COUNT)
+			return -EINVAL;
+
+		comp_node.comp.type = COUNTER_COMP_U64;
+		comp_node.comp.count_u64_read = counter->ops->count_read;
+		break;
+	case COUNTER_COMPONENT_FUNCTION:
+		if (watch.component.scope != COUNTER_SCOPE_COUNT)
+			return -EINVAL;
+
+		comp_node.comp.type = COUNTER_COMP_FUNCTION;
+		comp_node.comp.count_u8_read = counter->ops->function_read;
+		break;
+	case COUNTER_COMPONENT_SYNAPSE_ACTION:
+		if (watch.component.scope != COUNTER_SCOPE_COUNT)
+			return -EINVAL;
+		if (id >= counter->counts[parent].num_synapses)
+			return -EINVAL;
+		id = array_index_nospec(id, counter->counts[parent].num_synapses);
+
+		comp_node.comp.type = COUNTER_COMP_SYNAPSE_ACTION;
+		comp_node.comp.action_read = counter->ops->action_read;
+		comp_node.comp.priv = counter->counts[parent].synapses + id;
+		break;
+	case COUNTER_COMPONENT_EXTENSION:
+		if (id >= num_ext)
+			return -EINVAL;
+		id = array_index_nospec(id, num_ext);
+
+		comp_node.comp = ext[id];

drivers/counter/counter-chrdev.c new file +13 -0

+
+	return counter_set_event_node(counter, &watch, &comp_node);
+}
+
 
 
+static int counter_chrdev_open(struct inode *inode, struct file *filp)
+{
+	struct counter_device *const counter = container_of(inode->i_cdev,
+							    typeof(*counter),
+							    chrdev);
+
+	get_device(&counter->dev);
+	filp->private_data = counter;
+

drivers/counter/counter-chrdev.c new file +40 -0

+	raw_spin_unlock_irqrestore(&counter->events_lock, flags);
+	counter_events_list_free(&counter->next_events_list);
+
+	put_device(&counter->dev);
+
+	return 0;
+}
 
 
+/**
+ * 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;

drivers/counter/counter-chrdev.c new file +2 -0

+}
+EXPORT_SYMBOL_GPL(counter_push_event);

include/linux/counter.h +4 -1

index 3f3f8ba6c1b4..98cd7c035968 100644
--- a/include/linux/counter.h
 
 
 
+/**
+ * struct counter_event_node - Counter Event node
+ * @l:		list of current watching Counter events
+ * @event:	event that triggers

Quoted 3 lines

drivers/counter/counter-chrdev.c new file +3 -0

+static int counter_add_watch(struct counter_device *const counter,
+			     const unsigned long arg)
+{

drivers/counter/counter-chrdev.c line 250

Quoted 3 lines

drivers/counter/counter-chrdev.c new file +3 -0

+
+dummy_component:
+	comp_node.component = watch.component;

In my experiments, I added a events_validate driver callback here to validate each event as it is added. This way the user can know exactly which event caused the problem rather than waiting for the event_config callback.

Quoted 17 lines

include/linux/counter.h +11 -1

index 3f3f8ba6c1b4..98cd7c035968 100644
--- a/include/linux/counter.h
 
 
 
+/**
+ * struct counter_event_node - Counter Event node
+ * @l:		list of current watching Counter events
+ * @event:	event that triggers
+ * @channel:	event channel
+ * @comp_list:	list of components to watch when event triggers
+ */
+struct counter_event_node {
+	struct list_head l;
+	u8 event;
+	u8 channel;

include/linux/counter.h struct counter_count { · line 187

Quoted 3 lines

include/linux/counter.h +3 -0

+	struct list_head comp_list;
+};
+

Unless this is needed outside of the drivers/counter/ directory, I would suggest putting it in drivers/counter/counter-chrdev.h instead of include/linux/counter.h.

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 <[email protected]>

Cc Gwendal Grignou <[email protected]>

Cc Dan Carpenter <[email protected]>

Signed-off-by William Breathitt Gray <[email protected]>

---

drivers/counter/counter-chrdev.c new file +37 -0

+/**
+ * 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;

Quoted 11 lines · William Breathitt Gray

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 <[email protected]>

Cc Gwendal Grignou <[email protected]>

Cc Dan Carpenter <[email protected]>

Signed-off-by William Breathitt Gray <[email protected]>

---

Hello William,

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

[...]

Quoted 37 lines

drivers/counter/counter-chrdev.c new file +37 -0

+/**
+ * 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;

drivers/counter/counter-chrdev.c line 465

Quoted 10 lines

drivers/counter/counter-chrdev.c new file +10 -0

+
+	/* 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.

[...]

include/linux/counter.h struct counter_device { · line 277

Quoted 11 lines

struct counter_device {

include/linux/counter.h +5 -0

 	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

Quoted 78 lines · Oleksij Rempel

Quoted 11 lines · William Breathitt Gray

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 <[email protected]>

Cc Gwendal Grignou <[email protected]>

Cc Dan Carpenter <[email protected]>

Signed-off-by William Breathitt Gray <[email protected]>

---

Hello William,

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

[...]

Quoted 37 lines

drivers/counter/counter-chrdev.c new file +37 -0

+/**
+ * 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;

drivers/counter/counter-chrdev.c line 465

Quoted 10 lines

drivers/counter/counter-chrdev.c new file +10 -0

+
+	/* 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.

Quoted 2 lines

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.

Quoted 17 lines

[...]

include/linux/counter.h struct counter_device { · line 277

Quoted 11 lines

struct counter_device {

include/linux/counter.h +5 -0

 	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?

Quoted 2 lines

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?

William Breathitt Gray

1. Should standard Counter component data types be defined as u8 or u32?

Many standard Counter component types such COUNTER_COMP_SIGNAL_LEVEL have standard values defined (e.g. COUNTER_SIGNAL_LEVEL_LOW and COUNTER_SIGNAL_LEVEL_HIGH). These values are currently handled by the Counter subsystem code as u8 data types.

If u32 is used for these values instead, C enum structures could be used by driver authors to implicitly cast these values via the driver callback parameters.

This question is primarily addressed to David Lechner. I'm somewhat confused about how this setup would look in device drivers. I've gone ahead and refactored the code to support u32 enums, and pushed it to a separate branch on my repository called counter_chrdev_v6_u32_enum:
https://gitlab.com/vilhelmgray/iio/-/tree/counter_chrdev_v6_u32_enum

Please check it out and let me know what you think. Is this the support you had in mind? I'm curious to see an example of how would your driver callback functions would look in this case. If everything works out fine, then I'll submit this branch as v7 of this patchset.

2. How should we handle "raw" timestamps?

Ahmad Fatoum brought up the possibility of returning "raw" timestamps similar to what the network stack offers (see the network stack SOF_TIMESTAMPING_{RAW,SYS}_HARDWARE support).

I'm not very familiar with the networking stack code, but if I understand correctly the SOF_TIMESTAMPING_RAW_HARDWARE timestamps are values returned from the device. If so, I suspect we would be able to support these "raw" timestamps by defining them as Counter Extensions and returning them in struct counter_event elements similar to the other Extension values.

Quoted 18 lines · William Breathitt Gray

1. Should standard Counter component data types be defined as u8 or u32?

Many standard Counter component types such COUNTER_COMP_SIGNAL_LEVEL have standard values defined (e.g. COUNTER_SIGNAL_LEVEL_LOW and COUNTER_SIGNAL_LEVEL_HIGH). These values are currently handled by the Counter subsystem code as u8 data types.

If u32 is used for these values instead, C enum structures could be used by driver authors to implicitly cast these values via the driver callback parameters.

This question is primarily addressed to David Lechner. I'm somewhat confused about how this setup would look in device drivers. I've gone ahead and refactored the code to support u32 enums, and pushed it to a separate branch on my repository called counter_chrdev_v6_u32_enum:
https://gitlab.com/vilhelmgray/iio/-/tree/counter_chrdev_v6_u32_enum

Quoted 4 lines

Please check it out and let me know what you think. Is this the support you had in mind? I'm curious to see an example of how would your driver callback functions would look in this case. If everything works out fine, then I'll submit this branch as v7 of this patchset.

I haven't had time to look at this in depth, but just superficially looking at it, it is mostly there. The driver callback would just use the enum type in place of u32. For example:

static int ti_eqep_function_write(struct counter_device *counter,
struct counter_count *count, enum counter_function function)

and the COUNTER_FUNCTION_* constants would be defined as:

enum counter_function {
	COUNTER_FUNCTION_INCREASE,
	...
};

instead of using #define macros.

One advantage I see to using u8, at least in the user API data structures, is that it increases the number of events that fit in the kfifo buffer by a significant factor.

And that is not to say that we couldn't do both: have the user API structs use u8 for enum values and still use u32/strong enum types internally in the callback functions.

Quoted 8 lines

2. How should we handle "raw" timestamps?

Ahmad Fatoum brought up the possibility of returning "raw" timestamps similar to what the network stack offers (see the network stack SOF_TIMESTAMPING_{RAW,SYS}_HARDWARE support).

Quoted 6 lines

I'm not very familiar with the networking stack code, but if I understand correctly the SOF_TIMESTAMPING_RAW_HARDWARE timestamps are values returned from the device. If so, I suspect we would be able to support these "raw" timestamps by defining them as Counter Extensions and returning them in struct counter_event elements similar to the other Extension values.

Is nanosecond resolution good enough? In the TI eQEP driver I considered returning the raw timer value, but quickly realized that it would not be very nice to expect the user code to know the clock rate of the timer. It was very easy to get the clock rate in the kernel and just convert the timer value to nanoseconds before returning it to userspace.

So if there is some specialized case where it can be solved no other way besides using raw timestamps, then sure, include it. Otherwise I think we should stick with nanoseconds for time values when possible.

Quoted 41 lines · David Lechner

Quoted 18 lines · William Breathitt Gray

1. Should standard Counter component data types be defined as u8 or u32?

Many standard Counter component types such COUNTER_COMP_SIGNAL_LEVEL have standard values defined (e.g. COUNTER_SIGNAL_LEVEL_LOW and COUNTER_SIGNAL_LEVEL_HIGH). These values are currently handled by the Counter subsystem code as u8 data types.

If u32 is used for these values instead, C enum structures could be used by driver authors to implicitly cast these values via the driver callback parameters.

This question is primarily addressed to David Lechner. I'm somewhat confused about how this setup would look in device drivers. I've gone ahead and refactored the code to support u32 enums, and pushed it to a separate branch on my repository called counter_chrdev_v6_u32_enum:
https://gitlab.com/vilhelmgray/iio/-/tree/counter_chrdev_v6_u32_enum

Quoted 4 lines

Please check it out and let me know what you think. Is this the support you had in mind? I'm curious to see an example of how would your driver callback functions would look in this case. If everything works out fine, then I'll submit this branch as v7 of this patchset.

I haven't had time to look at this in depth, but just superficially looking at it, it is mostly there. The driver callback would just use the enum type in place of u32. For example:

static int ti_eqep_function_write(struct counter_device *counter,
struct counter_count *count, enum counter_function function)

and the COUNTER_FUNCTION_* constants would be defined as:

enum counter_function {
	COUNTER_FUNCTION_INCREASE,
	...
};

instead of using #define macros.

Quoted 7 lines

One advantage I see to using u8, at least in the user API data structures, is that it increases the number of events that fit in the kfifo buffer by a significant factor.

And that is not to say that we couldn't do both: have the user API structs use u8 for enum values and still use u32/strong enum types internally in the callback functions.

I'm including David Laight because he initially opposed enums in favor of fixed size types when we discussed this in an earlier revision:
https://lkml.org/lkml/2020/5/3/159

However, there have been significant changes to this patchset so the context now is different than those earlier discussions (i.e. we're no longer discussing ioctl calls).

I think reimplementing these constants as enums as described could work. If we do so, should the enum constants be given specific values? For example:

enum counter_function {
COUNTER_FUNCTION_INCREASE = 0,
COUNTER_FUNCTION_DECREASE = 1,
...
};