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

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

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 10914 bytes --]

CC: kbuild-all(a)lists.01.org
In-Reply-To: <57bc509273bf288d74835e6ebdaebf27b4991888.1608935587.git.vilhelm.gray@gmail.com>
References: <57bc509273bf288d74835e6ebdaebf27b4991888.1608935587.git.vilhelm.gray@gmail.com>
TO: William Breathitt Gray <vilhelm.gray@gmail.com>

Hi William,

I love your patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on next-20201223]
[cannot apply to stm32/stm32-next linux/master v5.10]
[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/20201226-082226
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 61d791365b72a89062fbbea69aa61479476da946
:::::: branch date: 20 hours ago
:::::: commit date: 20 hours ago
config: i386-randconfig-m021-20201226 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

smatch warnings:
drivers/counter/counter-chrdev.c:254 counter_add_watch() error: uninitialized symbol 'num_ext'.
drivers/counter/counter-chrdev.c:258 counter_add_watch() error: uninitialized symbol 'ext'.

vim +/num_ext +254 drivers/counter/counter-chrdev.c

0eae595bb599eaa William Breathitt Gray 2020-12-25  169  
0eae595bb599eaa William Breathitt Gray 2020-12-25  170  static int counter_add_watch(struct counter_device *const counter,
0eae595bb599eaa William Breathitt Gray 2020-12-25  171  			     const unsigned long arg)
0eae595bb599eaa William Breathitt Gray 2020-12-25  172  {
0eae595bb599eaa William Breathitt Gray 2020-12-25  173  	void __user *const uwatch = (void __user *)arg;
0eae595bb599eaa William Breathitt Gray 2020-12-25  174  	struct counter_watch watch;
0eae595bb599eaa William Breathitt Gray 2020-12-25  175  	struct counter_comp_node comp_node = {0};
0eae595bb599eaa William Breathitt Gray 2020-12-25  176  	size_t parent, id;
0eae595bb599eaa William Breathitt Gray 2020-12-25  177  	struct counter_comp *ext;
0eae595bb599eaa William Breathitt Gray 2020-12-25  178  	size_t num_ext;
0eae595bb599eaa William Breathitt Gray 2020-12-25  179  	int err;
0eae595bb599eaa William Breathitt Gray 2020-12-25  180  
0eae595bb599eaa William Breathitt Gray 2020-12-25  181  	if (copy_from_user(&watch, uwatch, sizeof(watch)))
0eae595bb599eaa William Breathitt Gray 2020-12-25  182  		return -EFAULT;
0eae595bb599eaa William Breathitt Gray 2020-12-25  183  
0eae595bb599eaa William Breathitt Gray 2020-12-25  184  	if (watch.component.type == COUNTER_COMPONENT_NONE)
0eae595bb599eaa William Breathitt Gray 2020-12-25  185  		goto no_component;
0eae595bb599eaa William Breathitt Gray 2020-12-25  186  
0eae595bb599eaa William Breathitt Gray 2020-12-25  187  	parent = watch.component.parent;
0eae595bb599eaa William Breathitt Gray 2020-12-25  188  
0eae595bb599eaa William Breathitt Gray 2020-12-25  189  	/* Configure parent component info for comp node */
0eae595bb599eaa William Breathitt Gray 2020-12-25  190  	switch (watch.component.scope) {
0eae595bb599eaa William Breathitt Gray 2020-12-25  191  	case COUNTER_SCOPE_DEVICE:
0eae595bb599eaa William Breathitt Gray 2020-12-25  192  		ext = counter->ext;
0eae595bb599eaa William Breathitt Gray 2020-12-25  193  		num_ext = counter->num_ext;
0eae595bb599eaa William Breathitt Gray 2020-12-25  194  		break;
0eae595bb599eaa William Breathitt Gray 2020-12-25  195  	case COUNTER_SCOPE_SIGNAL:
0eae595bb599eaa William Breathitt Gray 2020-12-25  196  		if (parent >= counter->num_signals)
0eae595bb599eaa William Breathitt Gray 2020-12-25  197  			return -EINVAL;
0eae595bb599eaa William Breathitt Gray 2020-12-25  198  		parent = array_index_nospec(parent, counter->num_signals);
0eae595bb599eaa William Breathitt Gray 2020-12-25  199  
0eae595bb599eaa William Breathitt Gray 2020-12-25  200  		comp_node.parent = counter->signals + parent;
0eae595bb599eaa William Breathitt Gray 2020-12-25  201  
0eae595bb599eaa William Breathitt Gray 2020-12-25  202  		ext = counter->signals[parent].ext;
0eae595bb599eaa William Breathitt Gray 2020-12-25  203  		num_ext = counter->signals[parent].num_ext;
0eae595bb599eaa William Breathitt Gray 2020-12-25  204  		break;
0eae595bb599eaa William Breathitt Gray 2020-12-25  205  	case COUNTER_SCOPE_COUNT:
0eae595bb599eaa William Breathitt Gray 2020-12-25  206  		if (parent >= counter->num_counts)
0eae595bb599eaa William Breathitt Gray 2020-12-25  207  			return -EINVAL;
0eae595bb599eaa William Breathitt Gray 2020-12-25  208  		parent = array_index_nospec(parent, counter->num_counts);
0eae595bb599eaa William Breathitt Gray 2020-12-25  209  
0eae595bb599eaa William Breathitt Gray 2020-12-25  210  		comp_node.parent = counter->counts + parent;
0eae595bb599eaa William Breathitt Gray 2020-12-25  211  
0eae595bb599eaa William Breathitt Gray 2020-12-25  212  		ext = counter->counts[parent].ext;
0eae595bb599eaa William Breathitt Gray 2020-12-25  213  		num_ext = counter->counts[parent].num_ext;
0eae595bb599eaa William Breathitt Gray 2020-12-25  214  		break;
0eae595bb599eaa William Breathitt Gray 2020-12-25  215  	}
0eae595bb599eaa William Breathitt Gray 2020-12-25  216  
0eae595bb599eaa William Breathitt Gray 2020-12-25  217  	id = watch.component.id;
0eae595bb599eaa William Breathitt Gray 2020-12-25  218  
0eae595bb599eaa William Breathitt Gray 2020-12-25  219  	/* Configure component info for comp node */
0eae595bb599eaa William Breathitt Gray 2020-12-25  220  	switch (watch.component.type) {
0eae595bb599eaa William Breathitt Gray 2020-12-25  221  	case COUNTER_COMPONENT_SIGNAL:
0eae595bb599eaa William Breathitt Gray 2020-12-25  222  		if (watch.component.scope != COUNTER_SCOPE_SIGNAL)
0eae595bb599eaa William Breathitt Gray 2020-12-25  223  			return -EINVAL;
0eae595bb599eaa William Breathitt Gray 2020-12-25  224  
0eae595bb599eaa William Breathitt Gray 2020-12-25  225  		comp_node.comp.type = COUNTER_COMP_SIGNAL_LEVEL;
0eae595bb599eaa William Breathitt Gray 2020-12-25  226  		comp_node.comp.signal_u32_read = counter->ops->signal_read;
0eae595bb599eaa William Breathitt Gray 2020-12-25  227  		break;
0eae595bb599eaa William Breathitt Gray 2020-12-25  228  	case COUNTER_COMPONENT_COUNT:
0eae595bb599eaa William Breathitt Gray 2020-12-25  229  		if (watch.component.scope != COUNTER_SCOPE_COUNT)
0eae595bb599eaa William Breathitt Gray 2020-12-25  230  			return -EINVAL;
0eae595bb599eaa William Breathitt Gray 2020-12-25  231  
0eae595bb599eaa William Breathitt Gray 2020-12-25  232  		comp_node.comp.type = COUNTER_COMP_U64;
0eae595bb599eaa William Breathitt Gray 2020-12-25  233  		comp_node.comp.count_u64_read = counter->ops->count_read;
0eae595bb599eaa William Breathitt Gray 2020-12-25  234  		break;
0eae595bb599eaa William Breathitt Gray 2020-12-25  235  	case COUNTER_COMPONENT_FUNCTION:
0eae595bb599eaa William Breathitt Gray 2020-12-25  236  		if (watch.component.scope != COUNTER_SCOPE_COUNT)
0eae595bb599eaa William Breathitt Gray 2020-12-25  237  			return -EINVAL;
0eae595bb599eaa William Breathitt Gray 2020-12-25  238  
0eae595bb599eaa William Breathitt Gray 2020-12-25  239  		comp_node.comp.type = COUNTER_COMP_FUNCTION;
0eae595bb599eaa William Breathitt Gray 2020-12-25  240  		comp_node.comp.count_u32_read = counter->ops->function_read;
0eae595bb599eaa William Breathitt Gray 2020-12-25  241  		break;
0eae595bb599eaa William Breathitt Gray 2020-12-25  242  	case COUNTER_COMPONENT_SYNAPSE_ACTION:
0eae595bb599eaa William Breathitt Gray 2020-12-25  243  		if (watch.component.scope != COUNTER_SCOPE_COUNT)
0eae595bb599eaa William Breathitt Gray 2020-12-25  244  			return -EINVAL;
0eae595bb599eaa William Breathitt Gray 2020-12-25  245  		if (id >= counter->counts[parent].num_synapses)
0eae595bb599eaa William Breathitt Gray 2020-12-25  246  			return -EINVAL;
0eae595bb599eaa William Breathitt Gray 2020-12-25  247  		id = array_index_nospec(id, counter->counts[parent].num_synapses);
0eae595bb599eaa William Breathitt Gray 2020-12-25  248  
0eae595bb599eaa William Breathitt Gray 2020-12-25  249  		comp_node.comp.type = COUNTER_COMP_SYNAPSE_ACTION;
0eae595bb599eaa William Breathitt Gray 2020-12-25  250  		comp_node.comp.action_read = counter->ops->action_read;
0eae595bb599eaa William Breathitt Gray 2020-12-25  251  		comp_node.comp.priv = counter->counts[parent].synapses + id;
0eae595bb599eaa William Breathitt Gray 2020-12-25  252  		break;
0eae595bb599eaa William Breathitt Gray 2020-12-25  253  	case COUNTER_COMPONENT_EXTENSION:
0eae595bb599eaa William Breathitt Gray 2020-12-25 @254  		if (id >= num_ext)
0eae595bb599eaa William Breathitt Gray 2020-12-25  255  			return -EINVAL;
0eae595bb599eaa William Breathitt Gray 2020-12-25  256  		id = array_index_nospec(id, num_ext);
0eae595bb599eaa William Breathitt Gray 2020-12-25  257  
0eae595bb599eaa William Breathitt Gray 2020-12-25 @258  		comp_node.comp = ext[id];
0eae595bb599eaa William Breathitt Gray 2020-12-25  259  		break;
0eae595bb599eaa William Breathitt Gray 2020-12-25  260  	default:
0eae595bb599eaa William Breathitt Gray 2020-12-25  261  		return -EINVAL;
0eae595bb599eaa William Breathitt Gray 2020-12-25  262  	}
0eae595bb599eaa William Breathitt Gray 2020-12-25  263  	/* Check if any read callback is set; this is part of a union */
0eae595bb599eaa William Breathitt Gray 2020-12-25  264  	if (!comp_node.comp.count_u8_read)
0eae595bb599eaa William Breathitt Gray 2020-12-25  265  		return -EOPNOTSUPP;
0eae595bb599eaa William Breathitt Gray 2020-12-25  266  
0eae595bb599eaa William Breathitt Gray 2020-12-25  267  no_component:
0eae595bb599eaa William Breathitt Gray 2020-12-25  268  	if (counter->ops->watch_validate) {
0eae595bb599eaa William Breathitt Gray 2020-12-25  269  		err = counter->ops->watch_validate(counter, &watch);
0eae595bb599eaa William Breathitt Gray 2020-12-25  270  		if (err < 0)
0eae595bb599eaa William Breathitt Gray 2020-12-25  271  			return err;
0eae595bb599eaa William Breathitt Gray 2020-12-25  272  	}
0eae595bb599eaa William Breathitt Gray 2020-12-25  273  
0eae595bb599eaa William Breathitt Gray 2020-12-25  274  	comp_node.component = watch.component;
0eae595bb599eaa William Breathitt Gray 2020-12-25  275  
0eae595bb599eaa William Breathitt Gray 2020-12-25  276  	return counter_set_event_node(counter, &watch, &comp_node);
0eae595bb599eaa William Breathitt Gray 2020-12-25  277  }
0eae595bb599eaa William Breathitt Gray 2020-12-25  278  

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

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 37701 bytes --]

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

[William Breathitt Gray]

[-- Attachment #1: Type: text/plain, Size: 7511 bytes --]

On Wed, Dec 30, 2020 at 03:04:40PM +0000, Jonathan Cameron wrote:
> On Fri, 25 Dec 2020 19:15:36 -0500
> William Breathitt Gray <vilhelm.gray@gmail.com> wrote:
> 
> > This patch introduces a character device interface for the Counter
> > subsystem. Device data is exposed through standard character device read
> > operations. Device data is gathered when a Counter event is pushed by
> > the respective Counter device driver. Configuration is handled via ioctl
> > operations on the respective Counter character device node.
> > 
> > Cc: David Lechner <david@lechnology.com>
> > Cc: Gwendal Grignou <gwendal@chromium.org>
> > Cc: Dan Carpenter <dan.carpenter@oracle.com>
> > Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
> 
> There are a few things in here that could profitably be pulled out as precursor
> patches.  I don't really understand the connection of extension_name to the
> addition of a chardev for example.  Might be needed to provide enough
> info to actually use the chardev, but does it have meaning without that?
> Either way, definitely feels like it can be done in a separate patch.

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

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

Ack.

> > +static int counter_get_data(struct counter_device *const counter,
> > +			    const struct counter_comp_node *const comp_node,
> > +			    u64 *const value)
> > +{
> > +	const struct counter_comp *const comp = &comp_node->comp;
> > +	void *const parent = comp_node->parent;
> > +	int err = 0;
> > +	u8 value_u8 = 0;
> > +	u32 value_u32 = 0;
> > +
> > +	if (comp_node->component.type == COUNTER_COMPONENT_NONE)
> > +		return 0;
> > +
> > +	switch (comp->type) {
> > +	case COUNTER_COMP_U8:
> > +	case COUNTER_COMP_BOOL:
> > +		switch (comp_node->component.scope) {
> > +		case COUNTER_SCOPE_DEVICE:
> > +			err = comp->device_u8_read(counter, &value_u8);
> > +			break;
> > +		case COUNTER_SCOPE_SIGNAL:
> > +			err = comp->signal_u8_read(counter, parent, &value_u8);
> > +			break;
> > +		case COUNTER_SCOPE_COUNT:
> > +			err = comp->count_u8_read(counter, parent, &value_u8);
> > +			break;
> > +		}
> > +		*value = value_u8;
> > +		break;
> > +	case COUNTER_COMP_SIGNAL_LEVEL:
> > +	case COUNTER_COMP_FUNCTION:
> > +	case COUNTER_COMP_ENUM:
> > +	case COUNTER_COMP_COUNT_DIRECTION:
> > +	case COUNTER_COMP_COUNT_MODE:
> > +		switch (comp_node->component.scope) {
> > +		case COUNTER_SCOPE_DEVICE:
> > +			err = comp->device_u32_read(counter, &value_u32);
> > +			break;
> > +		case COUNTER_SCOPE_SIGNAL:
> > +			err = comp->signal_u32_read(counter, parent,
> > +						    &value_u32);
> > +			break;
> > +		case COUNTER_SCOPE_COUNT:
> > +			err = comp->count_u32_read(counter, parent, &value_u32);
> > +			break;
> > +		}
> > +		*value = value_u32;
> 
> Seems like a return here would make more sense as no shared stuff to do at
> end of the switch. Same in other similar cases.

Ack.

> > +		break;
> > +	case COUNTER_COMP_U64:
> > +		switch (comp_node->component.scope) {
> > +		case COUNTER_SCOPE_DEVICE:
> > +			return comp->device_u64_read(counter, value);
> > +		case COUNTER_SCOPE_SIGNAL:
> > +			return comp->signal_u64_read(counter, parent, value);
> > +		case COUNTER_SCOPE_COUNT:
> > +			return comp->count_u64_read(counter, parent, value);
> > +		}
> > +		break;
> > +	case COUNTER_COMP_SYNAPSE_ACTION:
> > +		err = comp->action_read(counter, parent, comp->priv,
> > +					&value_u32);
> > +		*value = value_u32;
> > +		break;
> > +	}
> > +
> > +	return err;
> > +}
> > +
> > +/**
> > + * counter_push_event - queue event for userspace reading
> > + * @counter:	pointer to Counter structure
> > + * @event:	triggered event
> > + * @channel:	event channel
> > + *
> > + * Note: If no one is watching for the respective event, it is silently
> > + * discarded.
> > + */
> > +void counter_push_event(struct counter_device *const counter, const u8 event,
> > +			const u8 channel)
> > +{
> > +	struct counter_event ev = {0};
> > +	unsigned int copied = 0;
> > +	unsigned long flags;
> > +	struct counter_event_node *event_node;
> > +	struct counter_comp_node *comp_node;
> > +
> > +	ev.timestamp = ktime_get_ns();
> > +	ev.watch.event = event;
> > +	ev.watch.channel = channel;
> > +
> > +	raw_spin_lock_irqsave(&counter->events_list_lock, flags);
> 
> For a raw spin lock, we definitely want to see comments on why it
> is necessary.

Ack.

> > @@ -650,7 +670,7 @@ static int counter_count_attrs_create(struct counter_device *const counter,
> >  		return err;
> >  
> >  	/* Create Count name attribute */
> > -	err = counter_name_attr_create(dev, group, count->name);
> > +	err = counter_name_attr_create(dev, group, "name", count->name);
> 
> This refactoring could also be pulled out to a precusor patch.

Ack. This will be part of the extension_name patch.

> > @@ -319,12 +315,21 @@ struct counter_device {
> >  
> >  	int id;
> >  	struct device dev;
> > +	struct cdev chrdev;
> > +	struct list_head events_list;
> > +	raw_spinlock_t events_list_lock;
> > +	struct list_head next_events_list;
> > +	DECLARE_KFIFO(events, struct counter_event, 64);
> 
> Why 64?  Probably want that to be somewhat dynamic, even if only at build time.

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

> > +	wait_queue_head_t events_wait;
> > +	struct mutex events_lock;
> >  };
> >  
> >  int counter_register(struct counter_device *const counter);
> >  void counter_unregister(struct counter_device *const counter);
> >  int devm_counter_register(struct device *dev,
> >  			  struct counter_device *const counter);
> > +void counter_push_event(struct counter_device *const counter, const u8 event,
> > +			const u8 channel);
> >  
> >  #define COUNTER_COMP_DEVICE_U8(_name, _read, _write) \
> >  { \
> > diff --git a/include/uapi/linux/counter.h b/include/uapi/linux/counter.h
> > new file mode 100644
> > index 000000000000..7585dc9db19d
> > --- /dev/null
> > +++ b/include/uapi/linux/counter.h
> Small thing but I would have been tempted to do a precursor patch to the
> main change simply putting in place the userspace header.
> 
> Classic Nop patch that makes it easier to focus on the real stuff in this
> patch by getting that noise out of the way!
> 
> Jonathan

Ack.

William Breathitt Gray

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[William Breathitt Gray]

[-- Attachment #1.1: Type: text/plain, Size: 7511 bytes --]

On Wed, Dec 30, 2020 at 03:04:40PM +0000, Jonathan Cameron wrote:
> On Fri, 25 Dec 2020 19:15:36 -0500
> William Breathitt Gray <vilhelm.gray@gmail.com> wrote:
> 
> > This patch introduces a character device interface for the Counter
> > subsystem. Device data is exposed through standard character device read
> > operations. Device data is gathered when a Counter event is pushed by
> > the respective Counter device driver. Configuration is handled via ioctl
> > operations on the respective Counter character device node.
> > 
> > Cc: David Lechner <david@lechnology.com>
> > Cc: Gwendal Grignou <gwendal@chromium.org>
> > Cc: Dan Carpenter <dan.carpenter@oracle.com>
> > Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
> 
> There are a few things in here that could profitably be pulled out as precursor
> patches.  I don't really understand the connection of extension_name to the
> addition of a chardev for example.  Might be needed to provide enough
> info to actually use the chardev, but does it have meaning without that?
> Either way, definitely feels like it can be done in a separate patch.

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

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

Ack.

> > +static int counter_get_data(struct counter_device *const counter,
> > +			    const struct counter_comp_node *const comp_node,
> > +			    u64 *const value)
> > +{
> > +	const struct counter_comp *const comp = &comp_node->comp;
> > +	void *const parent = comp_node->parent;
> > +	int err = 0;
> > +	u8 value_u8 = 0;
> > +	u32 value_u32 = 0;
> > +
> > +	if (comp_node->component.type == COUNTER_COMPONENT_NONE)
> > +		return 0;
> > +
> > +	switch (comp->type) {
> > +	case COUNTER_COMP_U8:
> > +	case COUNTER_COMP_BOOL:
> > +		switch (comp_node->component.scope) {
> > +		case COUNTER_SCOPE_DEVICE:
> > +			err = comp->device_u8_read(counter, &value_u8);
> > +			break;
> > +		case COUNTER_SCOPE_SIGNAL:
> > +			err = comp->signal_u8_read(counter, parent, &value_u8);
> > +			break;
> > +		case COUNTER_SCOPE_COUNT:
> > +			err = comp->count_u8_read(counter, parent, &value_u8);
> > +			break;
> > +		}
> > +		*value = value_u8;
> > +		break;
> > +	case COUNTER_COMP_SIGNAL_LEVEL:
> > +	case COUNTER_COMP_FUNCTION:
> > +	case COUNTER_COMP_ENUM:
> > +	case COUNTER_COMP_COUNT_DIRECTION:
> > +	case COUNTER_COMP_COUNT_MODE:
> > +		switch (comp_node->component.scope) {
> > +		case COUNTER_SCOPE_DEVICE:
> > +			err = comp->device_u32_read(counter, &value_u32);
> > +			break;
> > +		case COUNTER_SCOPE_SIGNAL:
> > +			err = comp->signal_u32_read(counter, parent,
> > +						    &value_u32);
> > +			break;
> > +		case COUNTER_SCOPE_COUNT:
> > +			err = comp->count_u32_read(counter, parent, &value_u32);
> > +			break;
> > +		}
> > +		*value = value_u32;
> 
> Seems like a return here would make more sense as no shared stuff to do at
> end of the switch. Same in other similar cases.

Ack.

> > +		break;
> > +	case COUNTER_COMP_U64:
> > +		switch (comp_node->component.scope) {
> > +		case COUNTER_SCOPE_DEVICE:
> > +			return comp->device_u64_read(counter, value);
> > +		case COUNTER_SCOPE_SIGNAL:
> > +			return comp->signal_u64_read(counter, parent, value);
> > +		case COUNTER_SCOPE_COUNT:
> > +			return comp->count_u64_read(counter, parent, value);
> > +		}
> > +		break;
> > +	case COUNTER_COMP_SYNAPSE_ACTION:
> > +		err = comp->action_read(counter, parent, comp->priv,
> > +					&value_u32);
> > +		*value = value_u32;
> > +		break;
> > +	}
> > +
> > +	return err;
> > +}
> > +
> > +/**
> > + * counter_push_event - queue event for userspace reading
> > + * @counter:	pointer to Counter structure
> > + * @event:	triggered event
> > + * @channel:	event channel
> > + *
> > + * Note: If no one is watching for the respective event, it is silently
> > + * discarded.
> > + */
> > +void counter_push_event(struct counter_device *const counter, const u8 event,
> > +			const u8 channel)
> > +{
> > +	struct counter_event ev = {0};
> > +	unsigned int copied = 0;
> > +	unsigned long flags;
> > +	struct counter_event_node *event_node;
> > +	struct counter_comp_node *comp_node;
> > +
> > +	ev.timestamp = ktime_get_ns();
> > +	ev.watch.event = event;
> > +	ev.watch.channel = channel;
> > +
> > +	raw_spin_lock_irqsave(&counter->events_list_lock, flags);
> 
> For a raw spin lock, we definitely want to see comments on why it
> is necessary.

Ack.

> > @@ -650,7 +670,7 @@ static int counter_count_attrs_create(struct counter_device *const counter,
> >  		return err;
> >  
> >  	/* Create Count name attribute */
> > -	err = counter_name_attr_create(dev, group, count->name);
> > +	err = counter_name_attr_create(dev, group, "name", count->name);
> 
> This refactoring could also be pulled out to a precusor patch.

Ack. This will be part of the extension_name patch.

> > @@ -319,12 +315,21 @@ struct counter_device {
> >  
> >  	int id;
> >  	struct device dev;
> > +	struct cdev chrdev;
> > +	struct list_head events_list;
> > +	raw_spinlock_t events_list_lock;
> > +	struct list_head next_events_list;
> > +	DECLARE_KFIFO(events, struct counter_event, 64);
> 
> Why 64?  Probably want that to be somewhat dynamic, even if only at build time.

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

> > +	wait_queue_head_t events_wait;
> > +	struct mutex events_lock;
> >  };
> >  
> >  int counter_register(struct counter_device *const counter);
> >  void counter_unregister(struct counter_device *const counter);
> >  int devm_counter_register(struct device *dev,
> >  			  struct counter_device *const counter);
> > +void counter_push_event(struct counter_device *const counter, const u8 event,
> > +			const u8 channel);
> >  
> >  #define COUNTER_COMP_DEVICE_U8(_name, _read, _write) \
> >  { \
> > diff --git a/include/uapi/linux/counter.h b/include/uapi/linux/counter.h
> > new file mode 100644
> > index 000000000000..7585dc9db19d
> > --- /dev/null
> > +++ b/include/uapi/linux/counter.h
> Small thing but I would have been tempted to do a precursor patch to the
> main change simply putting in place the userspace header.
> 
> Classic Nop patch that makes it easier to focus on the real stuff in this
> patch by getting that noise out of the way!
> 
> Jonathan

Ack.

William Breathitt Gray

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[William Breathitt Gray]

[-- Attachment #1: Type: text/plain, Size: 1936 bytes --]

On Thu, Jan 28, 2021 at 10:01:13AM +0100, Oleksij Rempel wrote:
> Hello William,
> 
> 
> On Fri, Dec 25, 2020 at 07:15:36PM -0500, William Breathitt Gray wrote:
> > This patch introduces a character device interface for the Counter
> > subsystem. Device data is exposed through standard character device read
> > operations. Device data is gathered when a Counter event is pushed by
> > the respective Counter device driver. Configuration is handled via ioctl
> > operations on the respective Counter character device node.
> > 
> > Cc: David Lechner <david@lechnology.com>
> > Cc: Gwendal Grignou <gwendal@chromium.org>
> > Cc: Dan Carpenter <dan.carpenter@oracle.com>
> > Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
> > ---
> ...
> > +struct counter_event {
> > +	__aligned_u64 timestamp;
> > +	__aligned_u64 value;
> > +	struct counter_watch watch;
> > +	__u8 errno;
> 
> This variable clashed in user space, as soon as you include errno.h,
> with the libc's "magic" definition of errno. What about "err" instead.
> I'm not sure it an __u8 is the proper type, IIRC usually it's an int.
> 
> Regards,
> Oleksij
> -- 
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Sure, I can rename this to avoid a possible clash with libc's errno.
Maybe "status" would be more apt to indicate that this is an exit status
for the event -- the code returned may simply be a warning and not
necessarily a critical error.

Regarding the datatype for this value, I've opened up the discussion in
my reply to David Lechner [1], so perhaps we can continue it there.

[1] https://lkml.org/lkml/2021/1/30/5

William Breathitt Gray

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[William Breathitt Gray]

[-- Attachment #1.1: Type: text/plain, Size: 1936 bytes --]

On Thu, Jan 28, 2021 at 10:01:13AM +0100, Oleksij Rempel wrote:
> Hello William,
> 
> 
> On Fri, Dec 25, 2020 at 07:15:36PM -0500, William Breathitt Gray wrote:
> > This patch introduces a character device interface for the Counter
> > subsystem. Device data is exposed through standard character device read
> > operations. Device data is gathered when a Counter event is pushed by
> > the respective Counter device driver. Configuration is handled via ioctl
> > operations on the respective Counter character device node.
> > 
> > Cc: David Lechner <david@lechnology.com>
> > Cc: Gwendal Grignou <gwendal@chromium.org>
> > Cc: Dan Carpenter <dan.carpenter@oracle.com>
> > Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
> > ---
> ...
> > +struct counter_event {
> > +	__aligned_u64 timestamp;
> > +	__aligned_u64 value;
> > +	struct counter_watch watch;
> > +	__u8 errno;
> 
> This variable clashed in user space, as soon as you include errno.h,
> with the libc's "magic" definition of errno. What about "err" instead.
> I'm not sure it an __u8 is the proper type, IIRC usually it's an int.
> 
> Regards,
> Oleksij
> -- 
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Sure, I can rename this to avoid a possible clash with libc's errno.
Maybe "status" would be more apt to indicate that this is an exit status
for the event -- the code returned may simply be a warning and not
necessarily a critical error.

Regarding the datatype for this value, I've opened up the discussion in
my reply to David Lechner [1], so perhaps we can continue it there.

[1] https://lkml.org/lkml/2021/1/30/5

William Breathitt Gray

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[William Breathitt Gray]

[-- Attachment #1: Type: text/plain, Size: 4024 bytes --]

On Wed, Dec 30, 2020 at 03:36:35PM -0600, David Lechner wrote:
> On 12/25/20 6:15 PM, William Breathitt Gray wrote:
> 
> > diff --git a/include/uapi/linux/counter.h b/include/uapi/linux/counter.h
> > new file mode 100644
> > index 000000000000..7585dc9db19d
> > --- /dev/null
> > +++ b/include/uapi/linux/counter.h
> > @@ -0,0 +1,123 @@
> > +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> > +/*
> > + * Userspace ABI for Counter character devices
> > + * Copyright (C) 2020 William Breathitt Gray
> > + */
> > +#ifndef _UAPI_COUNTER_H_
> > +#define _UAPI_COUNTER_H_
> > +
> > +#include <linux/ioctl.h>
> > +#include <linux/types.h>
> > +
> > +/* Component type definitions */
> > +enum counter_component_type {
> > +	COUNTER_COMPONENT_NONE,
> > +	COUNTER_COMPONENT_SIGNAL,
> > +	COUNTER_COMPONENT_COUNT,
> > +	COUNTER_COMPONENT_FUNCTION,
> > +	COUNTER_COMPONENT_SYNAPSE_ACTION,
> > +	COUNTER_COMPONENT_EXTENSION,
> > +};
> > +
> > +/* Component scope definitions */
> > +enum counter_scope {
> 
> Do we need COUNTER_SCOPE_NONE to go with COUNTER_COMPONENT_NONE?

COUNTER_COMPONENT_NONE alone should be fine because it already indicates
that the 'component' member of the struct counter_watch is to be ignored
(i.e. type, scope, etc. will not be evaluated and that section of code
is bypassed).

> > +	COUNTER_SCOPE_DEVICE,
> > +	COUNTER_SCOPE_SIGNAL,
> > +	COUNTER_SCOPE_COUNT,
> > +};
> > +
> > +/**
> > + * struct counter_component - Counter component identification
> > + * @type: component type (Count, extension, etc.)
> 
> Instead of "Count, extension, etc.", it could be more helpful
> to say one of enum counter_component_type.

Ack.

> > + * @scope: component scope (Device, Count, or Signal)
> 
> Same here. @scope must be one of enum counter_scope.

Ack.

> > + * @parent: parent component identification number
> > + * @id: component identification number
> 
> It could be helpful to say that these id numbers match
> the X/Y/Z in the sysfs paths as described in the sysfs
> ABI.

Ack.

> > + */
> > +struct counter_component {
> > +	__u8 type;
> > +	__u8 scope;
> > +	__u8 parent;
> > +	__u8 id;
> > +};
> > +
> > +/* Event type definitions */
> > +enum counter_event_type {
> > +	COUNTER_EVENT_OVERFLOW,
> > +	COUNTER_EVENT_UNDERFLOW,
> > +	COUNTER_EVENT_OVERFLOW_UNDERFLOW,
> > +	COUNTER_EVENT_THRESHOLD,
> > +	COUNTER_EVENT_INDEX,
> > +};
> > +
> > +/**
> > + * struct counter_watch - Counter component watch configuration
> > + * @component: component to watch when event triggers
> > + * @event: event that triggers
> 
> It would be helpful to say that @event must be one of
> enum counter_event_type.

Ack.

> > + * @channel: event channel
> 
> It would be useful to say that @channel should be 0 unless
> a component has more than one event of the same type.

I'll make this clearer.

> > + */
> > +struct counter_watch {
> > +	struct counter_component component;
> > +	__u8 event;
> > +	__u8 channel;
> > +};
> > +
> > +/* ioctl commands */
> > +#define COUNTER_CLEAR_WATCHES_IOCTL _IO(0x3E, 0x00)
> > +#define COUNTER_ADD_WATCH_IOCTL _IOW(0x3E, 0x01, struct counter_watch)
> > +#define COUNTER_LOAD_WATCHES_IOCTL _IO(0x3E, 0x02)
> > +
> > +/**
> > + * struct counter_event - Counter event data
> > + * @timestamp: best estimate of time of event occurrence, in nanoseconds
> > + * @value: component value
> > + * @watch: component watch configuration
> > + * @errno: system error number
> > + */
> > +struct counter_event {
> > +	__aligned_u64 timestamp;
> > +	__aligned_u64 value;
> > +	struct counter_watch watch;
> > +	__u8 errno;
> 
> There are error codes larger than 255. Probably better
> make this __u32.

Are error codes larger than 255 actually useful in this case? I noticed
the exit() function will only return the least significant byte of
status to the parent: https://man7.org/linux/man-pages/man3/exit.3.html

William Breathitt Gray

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[William Breathitt Gray]

[-- Attachment #1.1: Type: text/plain, Size: 4024 bytes --]

On Wed, Dec 30, 2020 at 03:36:35PM -0600, David Lechner wrote:
> On 12/25/20 6:15 PM, William Breathitt Gray wrote:
> 
> > diff --git a/include/uapi/linux/counter.h b/include/uapi/linux/counter.h
> > new file mode 100644
> > index 000000000000..7585dc9db19d
> > --- /dev/null
> > +++ b/include/uapi/linux/counter.h
> > @@ -0,0 +1,123 @@
> > +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> > +/*
> > + * Userspace ABI for Counter character devices
> > + * Copyright (C) 2020 William Breathitt Gray
> > + */
> > +#ifndef _UAPI_COUNTER_H_
> > +#define _UAPI_COUNTER_H_
> > +
> > +#include <linux/ioctl.h>
> > +#include <linux/types.h>
> > +
> > +/* Component type definitions */
> > +enum counter_component_type {
> > +	COUNTER_COMPONENT_NONE,
> > +	COUNTER_COMPONENT_SIGNAL,
> > +	COUNTER_COMPONENT_COUNT,
> > +	COUNTER_COMPONENT_FUNCTION,
> > +	COUNTER_COMPONENT_SYNAPSE_ACTION,
> > +	COUNTER_COMPONENT_EXTENSION,
> > +};
> > +
> > +/* Component scope definitions */
> > +enum counter_scope {
> 
> Do we need COUNTER_SCOPE_NONE to go with COUNTER_COMPONENT_NONE?

COUNTER_COMPONENT_NONE alone should be fine because it already indicates
that the 'component' member of the struct counter_watch is to be ignored
(i.e. type, scope, etc. will not be evaluated and that section of code
is bypassed).

> > +	COUNTER_SCOPE_DEVICE,
> > +	COUNTER_SCOPE_SIGNAL,
> > +	COUNTER_SCOPE_COUNT,
> > +};
> > +
> > +/**
> > + * struct counter_component - Counter component identification
> > + * @type: component type (Count, extension, etc.)
> 
> Instead of "Count, extension, etc.", it could be more helpful
> to say one of enum counter_component_type.

Ack.

> > + * @scope: component scope (Device, Count, or Signal)
> 
> Same here. @scope must be one of enum counter_scope.

Ack.

> > + * @parent: parent component identification number
> > + * @id: component identification number
> 
> It could be helpful to say that these id numbers match
> the X/Y/Z in the sysfs paths as described in the sysfs
> ABI.

Ack.

> > + */
> > +struct counter_component {
> > +	__u8 type;
> > +	__u8 scope;
> > +	__u8 parent;
> > +	__u8 id;
> > +};
> > +
> > +/* Event type definitions */
> > +enum counter_event_type {
> > +	COUNTER_EVENT_OVERFLOW,
> > +	COUNTER_EVENT_UNDERFLOW,
> > +	COUNTER_EVENT_OVERFLOW_UNDERFLOW,
> > +	COUNTER_EVENT_THRESHOLD,
> > +	COUNTER_EVENT_INDEX,
> > +};
> > +
> > +/**
> > + * struct counter_watch - Counter component watch configuration
> > + * @component: component to watch when event triggers
> > + * @event: event that triggers
> 
> It would be helpful to say that @event must be one of
> enum counter_event_type.

Ack.

> > + * @channel: event channel
> 
> It would be useful to say that @channel should be 0 unless
> a component has more than one event of the same type.

I'll make this clearer.

> > + */
> > +struct counter_watch {
> > +	struct counter_component component;
> > +	__u8 event;
> > +	__u8 channel;
> > +};
> > +
> > +/* ioctl commands */
> > +#define COUNTER_CLEAR_WATCHES_IOCTL _IO(0x3E, 0x00)
> > +#define COUNTER_ADD_WATCH_IOCTL _IOW(0x3E, 0x01, struct counter_watch)
> > +#define COUNTER_LOAD_WATCHES_IOCTL _IO(0x3E, 0x02)
> > +
> > +/**
> > + * struct counter_event - Counter event data
> > + * @timestamp: best estimate of time of event occurrence, in nanoseconds
> > + * @value: component value
> > + * @watch: component watch configuration
> > + * @errno: system error number
> > + */
> > +struct counter_event {
> > +	__aligned_u64 timestamp;
> > +	__aligned_u64 value;
> > +	struct counter_watch watch;
> > +	__u8 errno;
> 
> There are error codes larger than 255. Probably better
> make this __u32.

Are error codes larger than 255 actually useful in this case? I noticed
the exit() function will only return the least significant byte of
status to the parent: https://man7.org/linux/man-pages/man3/exit.3.html

William Breathitt Gray

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Oleksij Rempel]

Hello William,


On Fri, Dec 25, 2020 at 07:15:36PM -0500, William Breathitt Gray wrote:
> This patch introduces a character device interface for the Counter
> subsystem. Device data is exposed through standard character device read
> operations. Device data is gathered when a Counter event is pushed by
> the respective Counter device driver. Configuration is handled via ioctl
> operations on the respective Counter character device node.
> 
> Cc: David Lechner <david@lechnology.com>
> Cc: Gwendal Grignou <gwendal@chromium.org>
> Cc: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
> ---
...
> +struct counter_event {
> +	__aligned_u64 timestamp;
> +	__aligned_u64 value;
> +	struct counter_watch watch;
> +	__u8 errno;

This variable clashed in user space, as soon as you include errno.h,
with the libc's "magic" definition of errno. What about "err" instead.
I'm not sure it an __u8 is the proper type, IIRC usually it's an int.

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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

[Oleksij Rempel]

Hello William,


On Fri, Dec 25, 2020 at 07:15:36PM -0500, William Breathitt Gray wrote:
> This patch introduces a character device interface for the Counter
> subsystem. Device data is exposed through standard character device read
> operations. Device data is gathered when a Counter event is pushed by
> the respective Counter device driver. Configuration is handled via ioctl
> operations on the respective Counter character device node.
> 
> Cc: David Lechner <david@lechnology.com>
> Cc: Gwendal Grignou <gwendal@chromium.org>
> Cc: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
> ---
...
> +struct counter_event {
> +	__aligned_u64 timestamp;
> +	__aligned_u64 value;
> +	struct counter_watch watch;
> +	__u8 errno;

This variable clashed in user space, as soon as you include errno.h,
with the libc's "magic" definition of errno. What about "err" instead.
I'm not sure it an __u8 is the proper type, IIRC usually it's an int.

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 10222 bytes --]

Hi William,

url:    https://github.com/0day-ci/linux/commits/William-Breathitt-Gray/Introduce-the-Counter-character-device-interface/20201226-082226
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 61d791365b72a89062fbbea69aa61479476da946
config: i386-randconfig-m021-20201226 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

smatch warnings:
drivers/counter/counter-chrdev.c:254 counter_add_watch() error: uninitialized symbol 'num_ext'.
drivers/counter/counter-chrdev.c:258 counter_add_watch() error: uninitialized symbol 'ext'.

vim +/num_ext +254 drivers/counter/counter-chrdev.c

0eae595bb599eaa William Breathitt Gray 2020-12-25  170  static int counter_add_watch(struct counter_device *const counter,
0eae595bb599eaa William Breathitt Gray 2020-12-25  171  			     const unsigned long arg)
0eae595bb599eaa William Breathitt Gray 2020-12-25  172  {
0eae595bb599eaa William Breathitt Gray 2020-12-25  173  	void __user *const uwatch = (void __user *)arg;
0eae595bb599eaa William Breathitt Gray 2020-12-25  174  	struct counter_watch watch;
0eae595bb599eaa William Breathitt Gray 2020-12-25  175  	struct counter_comp_node comp_node = {0};
0eae595bb599eaa William Breathitt Gray 2020-12-25  176  	size_t parent, id;
0eae595bb599eaa William Breathitt Gray 2020-12-25  177  	struct counter_comp *ext;
0eae595bb599eaa William Breathitt Gray 2020-12-25  178  	size_t num_ext;
0eae595bb599eaa William Breathitt Gray 2020-12-25  179  	int err;
0eae595bb599eaa William Breathitt Gray 2020-12-25  180  
0eae595bb599eaa William Breathitt Gray 2020-12-25  181  	if (copy_from_user(&watch, uwatch, sizeof(watch)))
0eae595bb599eaa William Breathitt Gray 2020-12-25  182  		return -EFAULT;
0eae595bb599eaa William Breathitt Gray 2020-12-25  183  
0eae595bb599eaa William Breathitt Gray 2020-12-25  184  	if (watch.component.type == COUNTER_COMPONENT_NONE)
0eae595bb599eaa William Breathitt Gray 2020-12-25  185  		goto no_component;
0eae595bb599eaa William Breathitt Gray 2020-12-25  186  
0eae595bb599eaa William Breathitt Gray 2020-12-25  187  	parent = watch.component.parent;
0eae595bb599eaa William Breathitt Gray 2020-12-25  188  
0eae595bb599eaa William Breathitt Gray 2020-12-25  189  	/* Configure parent component info for comp node */
0eae595bb599eaa William Breathitt Gray 2020-12-25  190  	switch (watch.component.scope) {
0eae595bb599eaa William Breathitt Gray 2020-12-25  191  	case COUNTER_SCOPE_DEVICE:
0eae595bb599eaa William Breathitt Gray 2020-12-25  192  		ext = counter->ext;
0eae595bb599eaa William Breathitt Gray 2020-12-25  193  		num_ext = counter->num_ext;
0eae595bb599eaa William Breathitt Gray 2020-12-25  194  		break;
0eae595bb599eaa William Breathitt Gray 2020-12-25  195  	case COUNTER_SCOPE_SIGNAL:
0eae595bb599eaa William Breathitt Gray 2020-12-25  196  		if (parent >= counter->num_signals)
0eae595bb599eaa William Breathitt Gray 2020-12-25  197  			return -EINVAL;
0eae595bb599eaa William Breathitt Gray 2020-12-25  198  		parent = array_index_nospec(parent, counter->num_signals);
0eae595bb599eaa William Breathitt Gray 2020-12-25  199  
0eae595bb599eaa William Breathitt Gray 2020-12-25  200  		comp_node.parent = counter->signals + parent;
0eae595bb599eaa William Breathitt Gray 2020-12-25  201  
0eae595bb599eaa William Breathitt Gray 2020-12-25  202  		ext = counter->signals[parent].ext;
0eae595bb599eaa William Breathitt Gray 2020-12-25  203  		num_ext = counter->signals[parent].num_ext;
0eae595bb599eaa William Breathitt Gray 2020-12-25  204  		break;
0eae595bb599eaa William Breathitt Gray 2020-12-25  205  	case COUNTER_SCOPE_COUNT:
0eae595bb599eaa William Breathitt Gray 2020-12-25  206  		if (parent >= counter->num_counts)
0eae595bb599eaa William Breathitt Gray 2020-12-25  207  			return -EINVAL;
0eae595bb599eaa William Breathitt Gray 2020-12-25  208  		parent = array_index_nospec(parent, counter->num_counts);
0eae595bb599eaa William Breathitt Gray 2020-12-25  209  
0eae595bb599eaa William Breathitt Gray 2020-12-25  210  		comp_node.parent = counter->counts + parent;
0eae595bb599eaa William Breathitt Gray 2020-12-25  211  
0eae595bb599eaa William Breathitt Gray 2020-12-25  212  		ext = counter->counts[parent].ext;
0eae595bb599eaa William Breathitt Gray 2020-12-25  213  		num_ext = counter->counts[parent].num_ext;
0eae595bb599eaa William Breathitt Gray 2020-12-25  214  		break;

Uninitialized on the defualt: path.  I can't actually find how the
counter_watch struct is defined, but Smatch normally gets this stuff
correct.

0eae595bb599eaa William Breathitt Gray 2020-12-25  215  	}
0eae595bb599eaa William Breathitt Gray 2020-12-25  216  
0eae595bb599eaa William Breathitt Gray 2020-12-25  217  	id = watch.component.id;
0eae595bb599eaa William Breathitt Gray 2020-12-25  218  
0eae595bb599eaa William Breathitt Gray 2020-12-25  219  	/* Configure component info for comp node */
0eae595bb599eaa William Breathitt Gray 2020-12-25  220  	switch (watch.component.type) {
0eae595bb599eaa William Breathitt Gray 2020-12-25  221  	case COUNTER_COMPONENT_SIGNAL:
0eae595bb599eaa William Breathitt Gray 2020-12-25  222  		if (watch.component.scope != COUNTER_SCOPE_SIGNAL)
0eae595bb599eaa William Breathitt Gray 2020-12-25  223  			return -EINVAL;
0eae595bb599eaa William Breathitt Gray 2020-12-25  224  
0eae595bb599eaa William Breathitt Gray 2020-12-25  225  		comp_node.comp.type = COUNTER_COMP_SIGNAL_LEVEL;
0eae595bb599eaa William Breathitt Gray 2020-12-25  226  		comp_node.comp.signal_u32_read = counter->ops->signal_read;
0eae595bb599eaa William Breathitt Gray 2020-12-25  227  		break;
0eae595bb599eaa William Breathitt Gray 2020-12-25  228  	case COUNTER_COMPONENT_COUNT:
0eae595bb599eaa William Breathitt Gray 2020-12-25  229  		if (watch.component.scope != COUNTER_SCOPE_COUNT)
0eae595bb599eaa William Breathitt Gray 2020-12-25  230  			return -EINVAL;
0eae595bb599eaa William Breathitt Gray 2020-12-25  231  
0eae595bb599eaa William Breathitt Gray 2020-12-25  232  		comp_node.comp.type = COUNTER_COMP_U64;
0eae595bb599eaa William Breathitt Gray 2020-12-25  233  		comp_node.comp.count_u64_read = counter->ops->count_read;
0eae595bb599eaa William Breathitt Gray 2020-12-25  234  		break;
0eae595bb599eaa William Breathitt Gray 2020-12-25  235  	case COUNTER_COMPONENT_FUNCTION:
0eae595bb599eaa William Breathitt Gray 2020-12-25  236  		if (watch.component.scope != COUNTER_SCOPE_COUNT)
0eae595bb599eaa William Breathitt Gray 2020-12-25  237  			return -EINVAL;
0eae595bb599eaa William Breathitt Gray 2020-12-25  238  
0eae595bb599eaa William Breathitt Gray 2020-12-25  239  		comp_node.comp.type = COUNTER_COMP_FUNCTION;
0eae595bb599eaa William Breathitt Gray 2020-12-25  240  		comp_node.comp.count_u32_read = counter->ops->function_read;
0eae595bb599eaa William Breathitt Gray 2020-12-25  241  		break;
0eae595bb599eaa William Breathitt Gray 2020-12-25  242  	case COUNTER_COMPONENT_SYNAPSE_ACTION:
0eae595bb599eaa William Breathitt Gray 2020-12-25  243  		if (watch.component.scope != COUNTER_SCOPE_COUNT)
0eae595bb599eaa William Breathitt Gray 2020-12-25  244  			return -EINVAL;
0eae595bb599eaa William Breathitt Gray 2020-12-25  245  		if (id >= counter->counts[parent].num_synapses)
0eae595bb599eaa William Breathitt Gray 2020-12-25  246  			return -EINVAL;
0eae595bb599eaa William Breathitt Gray 2020-12-25  247  		id = array_index_nospec(id, counter->counts[parent].num_synapses);
0eae595bb599eaa William Breathitt Gray 2020-12-25  248  
0eae595bb599eaa William Breathitt Gray 2020-12-25  249  		comp_node.comp.type = COUNTER_COMP_SYNAPSE_ACTION;
0eae595bb599eaa William Breathitt Gray 2020-12-25  250  		comp_node.comp.action_read = counter->ops->action_read;
0eae595bb599eaa William Breathitt Gray 2020-12-25  251  		comp_node.comp.priv = counter->counts[parent].synapses + id;
0eae595bb599eaa William Breathitt Gray 2020-12-25  252  		break;
0eae595bb599eaa William Breathitt Gray 2020-12-25  253  	case COUNTER_COMPONENT_EXTENSION:
0eae595bb599eaa William Breathitt Gray 2020-12-25 @254  		if (id >= num_ext)
0eae595bb599eaa William Breathitt Gray 2020-12-25  255  			return -EINVAL;
0eae595bb599eaa William Breathitt Gray 2020-12-25  256  		id = array_index_nospec(id, num_ext);
0eae595bb599eaa William Breathitt Gray 2020-12-25  257  
0eae595bb599eaa William Breathitt Gray 2020-12-25 @258  		comp_node.comp = ext[id];
0eae595bb599eaa William Breathitt Gray 2020-12-25  259  		break;
0eae595bb599eaa William Breathitt Gray 2020-12-25  260  	default:
0eae595bb599eaa William Breathitt Gray 2020-12-25  261  		return -EINVAL;
0eae595bb599eaa William Breathitt Gray 2020-12-25  262  	}
0eae595bb599eaa William Breathitt Gray 2020-12-25  263  	/* Check if any read callback is set; this is part of a union */
0eae595bb599eaa William Breathitt Gray 2020-12-25  264  	if (!comp_node.comp.count_u8_read)
0eae595bb599eaa William Breathitt Gray 2020-12-25  265  		return -EOPNOTSUPP;
0eae595bb599eaa William Breathitt Gray 2020-12-25  266  
0eae595bb599eaa William Breathitt Gray 2020-12-25  267  no_component:
0eae595bb599eaa William Breathitt Gray 2020-12-25  268  	if (counter->ops->watch_validate) {
0eae595bb599eaa William Breathitt Gray 2020-12-25  269  		err = counter->ops->watch_validate(counter, &watch);
0eae595bb599eaa William Breathitt Gray 2020-12-25  270  		if (err < 0)
0eae595bb599eaa William Breathitt Gray 2020-12-25  271  			return err;
0eae595bb599eaa William Breathitt Gray 2020-12-25  272  	}
0eae595bb599eaa William Breathitt Gray 2020-12-25  273  
0eae595bb599eaa William Breathitt Gray 2020-12-25  274  	comp_node.component = watch.component;
0eae595bb599eaa William Breathitt Gray 2020-12-25  275  
0eae595bb599eaa William Breathitt Gray 2020-12-25  276  	return counter_set_event_node(counter, &watch, &comp_node);
0eae595bb599eaa William Breathitt Gray 2020-12-25  277  }

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

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 37701 bytes --]

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

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 10222 bytes --]

Hi William,

url:    https://github.com/0day-ci/linux/commits/William-Breathitt-Gray/Introduce-the-Counter-character-device-interface/20201226-082226
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 61d791365b72a89062fbbea69aa61479476da946
config: i386-randconfig-m021-20201226 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

smatch warnings:
drivers/counter/counter-chrdev.c:254 counter_add_watch() error: uninitialized symbol 'num_ext'.
drivers/counter/counter-chrdev.c:258 counter_add_watch() error: uninitialized symbol 'ext'.

vim +/num_ext +254 drivers/counter/counter-chrdev.c

0eae595bb599eaa William Breathitt Gray 2020-12-25  170  static int counter_add_watch(struct counter_device *const counter,
0eae595bb599eaa William Breathitt Gray 2020-12-25  171  			     const unsigned long arg)
0eae595bb599eaa William Breathitt Gray 2020-12-25  172  {
0eae595bb599eaa William Breathitt Gray 2020-12-25  173  	void __user *const uwatch = (void __user *)arg;
0eae595bb599eaa William Breathitt Gray 2020-12-25  174  	struct counter_watch watch;
0eae595bb599eaa William Breathitt Gray 2020-12-25  175  	struct counter_comp_node comp_node = {0};
0eae595bb599eaa William Breathitt Gray 2020-12-25  176  	size_t parent, id;
0eae595bb599eaa William Breathitt Gray 2020-12-25  177  	struct counter_comp *ext;
0eae595bb599eaa William Breathitt Gray 2020-12-25  178  	size_t num_ext;
0eae595bb599eaa William Breathitt Gray 2020-12-25  179  	int err;
0eae595bb599eaa William Breathitt Gray 2020-12-25  180  
0eae595bb599eaa William Breathitt Gray 2020-12-25  181  	if (copy_from_user(&watch, uwatch, sizeof(watch)))
0eae595bb599eaa William Breathitt Gray 2020-12-25  182  		return -EFAULT;
0eae595bb599eaa William Breathitt Gray 2020-12-25  183  
0eae595bb599eaa William Breathitt Gray 2020-12-25  184  	if (watch.component.type == COUNTER_COMPONENT_NONE)
0eae595bb599eaa William Breathitt Gray 2020-12-25  185  		goto no_component;
0eae595bb599eaa William Breathitt Gray 2020-12-25  186  
0eae595bb599eaa William Breathitt Gray 2020-12-25  187  	parent = watch.component.parent;
0eae595bb599eaa William Breathitt Gray 2020-12-25  188  
0eae595bb599eaa William Breathitt Gray 2020-12-25  189  	/* Configure parent component info for comp node */
0eae595bb599eaa William Breathitt Gray 2020-12-25  190  	switch (watch.component.scope) {
0eae595bb599eaa William Breathitt Gray 2020-12-25  191  	case COUNTER_SCOPE_DEVICE:
0eae595bb599eaa William Breathitt Gray 2020-12-25  192  		ext = counter->ext;
0eae595bb599eaa William Breathitt Gray 2020-12-25  193  		num_ext = counter->num_ext;
0eae595bb599eaa William Breathitt Gray 2020-12-25  194  		break;
0eae595bb599eaa William Breathitt Gray 2020-12-25  195  	case COUNTER_SCOPE_SIGNAL:
0eae595bb599eaa William Breathitt Gray 2020-12-25  196  		if (parent >= counter->num_signals)
0eae595bb599eaa William Breathitt Gray 2020-12-25  197  			return -EINVAL;
0eae595bb599eaa William Breathitt Gray 2020-12-25  198  		parent = array_index_nospec(parent, counter->num_signals);
0eae595bb599eaa William Breathitt Gray 2020-12-25  199  
0eae595bb599eaa William Breathitt Gray 2020-12-25  200  		comp_node.parent = counter->signals + parent;
0eae595bb599eaa William Breathitt Gray 2020-12-25  201  
0eae595bb599eaa William Breathitt Gray 2020-12-25  202  		ext = counter->signals[parent].ext;
0eae595bb599eaa William Breathitt Gray 2020-12-25  203  		num_ext = counter->signals[parent].num_ext;
0eae595bb599eaa William Breathitt Gray 2020-12-25  204  		break;
0eae595bb599eaa William Breathitt Gray 2020-12-25  205  	case COUNTER_SCOPE_COUNT:
0eae595bb599eaa William Breathitt Gray 2020-12-25  206  		if (parent >= counter->num_counts)
0eae595bb599eaa William Breathitt Gray 2020-12-25  207  			return -EINVAL;
0eae595bb599eaa William Breathitt Gray 2020-12-25  208  		parent = array_index_nospec(parent, counter->num_counts);
0eae595bb599eaa William Breathitt Gray 2020-12-25  209  
0eae595bb599eaa William Breathitt Gray 2020-12-25  210  		comp_node.parent = counter->counts + parent;
0eae595bb599eaa William Breathitt Gray 2020-12-25  211  
0eae595bb599eaa William Breathitt Gray 2020-12-25  212  		ext = counter->counts[parent].ext;
0eae595bb599eaa William Breathitt Gray 2020-12-25  213  		num_ext = counter->counts[parent].num_ext;
0eae595bb599eaa William Breathitt Gray 2020-12-25  214  		break;

Uninitialized on the defualt: path.  I can't actually find how the
counter_watch struct is defined, but Smatch normally gets this stuff
correct.

0eae595bb599eaa William Breathitt Gray 2020-12-25  215  	}
0eae595bb599eaa William Breathitt Gray 2020-12-25  216  
0eae595bb599eaa William Breathitt Gray 2020-12-25  217  	id = watch.component.id;
0eae595bb599eaa William Breathitt Gray 2020-12-25  218  
0eae595bb599eaa William Breathitt Gray 2020-12-25  219  	/* Configure component info for comp node */
0eae595bb599eaa William Breathitt Gray 2020-12-25  220  	switch (watch.component.type) {
0eae595bb599eaa William Breathitt Gray 2020-12-25  221  	case COUNTER_COMPONENT_SIGNAL:
0eae595bb599eaa William Breathitt Gray 2020-12-25  222  		if (watch.component.scope != COUNTER_SCOPE_SIGNAL)
0eae595bb599eaa William Breathitt Gray 2020-12-25  223  			return -EINVAL;
0eae595bb599eaa William Breathitt Gray 2020-12-25  224  
0eae595bb599eaa William Breathitt Gray 2020-12-25  225  		comp_node.comp.type = COUNTER_COMP_SIGNAL_LEVEL;
0eae595bb599eaa William Breathitt Gray 2020-12-25  226  		comp_node.comp.signal_u32_read = counter->ops->signal_read;
0eae595bb599eaa William Breathitt Gray 2020-12-25  227  		break;
0eae595bb599eaa William Breathitt Gray 2020-12-25  228  	case COUNTER_COMPONENT_COUNT:
0eae595bb599eaa William Breathitt Gray 2020-12-25  229  		if (watch.component.scope != COUNTER_SCOPE_COUNT)
0eae595bb599eaa William Breathitt Gray 2020-12-25  230  			return -EINVAL;
0eae595bb599eaa William Breathitt Gray 2020-12-25  231  
0eae595bb599eaa William Breathitt Gray 2020-12-25  232  		comp_node.comp.type = COUNTER_COMP_U64;
0eae595bb599eaa William Breathitt Gray 2020-12-25  233  		comp_node.comp.count_u64_read = counter->ops->count_read;
0eae595bb599eaa William Breathitt Gray 2020-12-25  234  		break;
0eae595bb599eaa William Breathitt Gray 2020-12-25  235  	case COUNTER_COMPONENT_FUNCTION:
0eae595bb599eaa William Breathitt Gray 2020-12-25  236  		if (watch.component.scope != COUNTER_SCOPE_COUNT)
0eae595bb599eaa William Breathitt Gray 2020-12-25  237  			return -EINVAL;
0eae595bb599eaa William Breathitt Gray 2020-12-25  238  
0eae595bb599eaa William Breathitt Gray 2020-12-25  239  		comp_node.comp.type = COUNTER_COMP_FUNCTION;
0eae595bb599eaa William Breathitt Gray 2020-12-25  240  		comp_node.comp.count_u32_read = counter->ops->function_read;
0eae595bb599eaa William Breathitt Gray 2020-12-25  241  		break;
0eae595bb599eaa William Breathitt Gray 2020-12-25  242  	case COUNTER_COMPONENT_SYNAPSE_ACTION:
0eae595bb599eaa William Breathitt Gray 2020-12-25  243  		if (watch.component.scope != COUNTER_SCOPE_COUNT)
0eae595bb599eaa William Breathitt Gray 2020-12-25  244  			return -EINVAL;
0eae595bb599eaa William Breathitt Gray 2020-12-25  245  		if (id >= counter->counts[parent].num_synapses)
0eae595bb599eaa William Breathitt Gray 2020-12-25  246  			return -EINVAL;
0eae595bb599eaa William Breathitt Gray 2020-12-25  247  		id = array_index_nospec(id, counter->counts[parent].num_synapses);
0eae595bb599eaa William Breathitt Gray 2020-12-25  248  
0eae595bb599eaa William Breathitt Gray 2020-12-25  249  		comp_node.comp.type = COUNTER_COMP_SYNAPSE_ACTION;
0eae595bb599eaa William Breathitt Gray 2020-12-25  250  		comp_node.comp.action_read = counter->ops->action_read;
0eae595bb599eaa William Breathitt Gray 2020-12-25  251  		comp_node.comp.priv = counter->counts[parent].synapses + id;
0eae595bb599eaa William Breathitt Gray 2020-12-25  252  		break;
0eae595bb599eaa William Breathitt Gray 2020-12-25  253  	case COUNTER_COMPONENT_EXTENSION:
0eae595bb599eaa William Breathitt Gray 2020-12-25 @254  		if (id >= num_ext)
0eae595bb599eaa William Breathitt Gray 2020-12-25  255  			return -EINVAL;
0eae595bb599eaa William Breathitt Gray 2020-12-25  256  		id = array_index_nospec(id, num_ext);
0eae595bb599eaa William Breathitt Gray 2020-12-25  257  
0eae595bb599eaa William Breathitt Gray 2020-12-25 @258  		comp_node.comp = ext[id];
0eae595bb599eaa William Breathitt Gray 2020-12-25  259  		break;
0eae595bb599eaa William Breathitt Gray 2020-12-25  260  	default:
0eae595bb599eaa William Breathitt Gray 2020-12-25  261  		return -EINVAL;
0eae595bb599eaa William Breathitt Gray 2020-12-25  262  	}
0eae595bb599eaa William Breathitt Gray 2020-12-25  263  	/* Check if any read callback is set; this is part of a union */
0eae595bb599eaa William Breathitt Gray 2020-12-25  264  	if (!comp_node.comp.count_u8_read)
0eae595bb599eaa William Breathitt Gray 2020-12-25  265  		return -EOPNOTSUPP;
0eae595bb599eaa William Breathitt Gray 2020-12-25  266  
0eae595bb599eaa William Breathitt Gray 2020-12-25  267  no_component:
0eae595bb599eaa William Breathitt Gray 2020-12-25  268  	if (counter->ops->watch_validate) {
0eae595bb599eaa William Breathitt Gray 2020-12-25  269  		err = counter->ops->watch_validate(counter, &watch);
0eae595bb599eaa William Breathitt Gray 2020-12-25  270  		if (err < 0)
0eae595bb599eaa William Breathitt Gray 2020-12-25  271  			return err;
0eae595bb599eaa William Breathitt Gray 2020-12-25  272  	}
0eae595bb599eaa William Breathitt Gray 2020-12-25  273  
0eae595bb599eaa William Breathitt Gray 2020-12-25  274  	comp_node.component = watch.component;
0eae595bb599eaa William Breathitt Gray 2020-12-25  275  
0eae595bb599eaa William Breathitt Gray 2020-12-25  276  	return counter_set_event_node(counter, &watch, &comp_node);
0eae595bb599eaa William Breathitt Gray 2020-12-25  277  }

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

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 37701 bytes --]

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

[David Lechner]

On 12/25/20 6:15 PM, William Breathitt Gray wrote:

> diff --git a/include/uapi/linux/counter.h b/include/uapi/linux/counter.h
> new file mode 100644
> index 000000000000..7585dc9db19d
> --- /dev/null
> +++ b/include/uapi/linux/counter.h
> @@ -0,0 +1,123 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +/*
> + * Userspace ABI for Counter character devices
> + * Copyright (C) 2020 William Breathitt Gray
> + */
> +#ifndef _UAPI_COUNTER_H_
> +#define _UAPI_COUNTER_H_
> +
> +#include <linux/ioctl.h>
> +#include <linux/types.h>
> +
> +/* Component type definitions */
> +enum counter_component_type {
> +	COUNTER_COMPONENT_NONE,
> +	COUNTER_COMPONENT_SIGNAL,
> +	COUNTER_COMPONENT_COUNT,
> +	COUNTER_COMPONENT_FUNCTION,
> +	COUNTER_COMPONENT_SYNAPSE_ACTION,
> +	COUNTER_COMPONENT_EXTENSION,
> +};
> +
> +/* Component scope definitions */
> +enum counter_scope {

Do we need COUNTER_SCOPE_NONE to go with COUNTER_COMPONENT_NONE?

> +	COUNTER_SCOPE_DEVICE,
> +	COUNTER_SCOPE_SIGNAL,
> +	COUNTER_SCOPE_COUNT,
> +};
> +
> +/**
> + * struct counter_component - Counter component identification
> + * @type: component type (Count, extension, etc.)

Instead of "Count, extension, etc.", it could be more helpful
to say one of enum counter_component_type.

> + * @scope: component scope (Device, Count, or Signal)

Same here. @scope must be one of enum counter_scope.

> + * @parent: parent component identification number
> + * @id: component identification number

It could be helpful to say that these id numbers match
the X/Y/Z in the sysfs paths as described in the sysfs
ABI.

> + */
> +struct counter_component {
> +	__u8 type;
> +	__u8 scope;
> +	__u8 parent;
> +	__u8 id;
> +};
> +
> +/* Event type definitions */
> +enum counter_event_type {
> +	COUNTER_EVENT_OVERFLOW,
> +	COUNTER_EVENT_UNDERFLOW,
> +	COUNTER_EVENT_OVERFLOW_UNDERFLOW,
> +	COUNTER_EVENT_THRESHOLD,
> +	COUNTER_EVENT_INDEX,
> +};
> +
> +/**
> + * struct counter_watch - Counter component watch configuration
> + * @component: component to watch when event triggers
> + * @event: event that triggers

It would be helpful to say that @event must be one of
enum counter_event_type.

> + * @channel: event channel

It would be useful to say that @channel should be 0 unless
a component has more than one event of the same type.

> + */
> +struct counter_watch {
> +	struct counter_component component;
> +	__u8 event;
> +	__u8 channel;
> +};
> +
> +/* ioctl commands */
> +#define COUNTER_CLEAR_WATCHES_IOCTL _IO(0x3E, 0x00)
> +#define COUNTER_ADD_WATCH_IOCTL _IOW(0x3E, 0x01, struct counter_watch)
> +#define COUNTER_LOAD_WATCHES_IOCTL _IO(0x3E, 0x02)
> +
> +/**
> + * struct counter_event - Counter event data
> + * @timestamp: best estimate of time of event occurrence, in nanoseconds
> + * @value: component value
> + * @watch: component watch configuration
> + * @errno: system error number
> + */
> +struct counter_event {
> +	__aligned_u64 timestamp;
> +	__aligned_u64 value;
> +	struct counter_watch watch;
> +	__u8 errno;

There are error codes larger than 255. Probably better
make this __u32.

> +};
> +
> +/* Count direction values */
> +enum counter_count_direction {
> +	COUNTER_COUNT_DIRECTION_FORWARD,
> +	COUNTER_COUNT_DIRECTION_BACKWARD,
> +};
> +
> +/* Count mode values */
> +enum counter_count_mode {
> +	COUNTER_COUNT_MODE_NORMAL,
> +	COUNTER_COUNT_MODE_RANGE_LIMIT,
> +	COUNTER_COUNT_MODE_NON_RECYCLE,
> +	COUNTER_COUNT_MODE_MODULO_N,
> +};
> +
> +/* Count function values */
> +enum counter_function {
> +	COUNTER_FUNCTION_INCREASE,
> +	COUNTER_FUNCTION_DECREASE,
> +	COUNTER_FUNCTION_PULSE_DIRECTION,
> +	COUNTER_FUNCTION_QUADRATURE_X1_A,
> +	COUNTER_FUNCTION_QUADRATURE_X1_B,
> +	COUNTER_FUNCTION_QUADRATURE_X2_A,
> +	COUNTER_FUNCTION_QUADRATURE_X2_B,
> +	COUNTER_FUNCTION_QUADRATURE_X4,
> +};
> +
> +/* Signal values */
> +enum counter_signal_level {
> +	COUNTER_SIGNAL_LEVEL_LOW,
> +	COUNTER_SIGNAL_LEVEL_HIGH,
> +};
> +
> +/* Action mode values */
> +enum counter_synapse_action {
> +	COUNTER_SYNAPSE_ACTION_NONE,
> +	COUNTER_SYNAPSE_ACTION_RISING_EDGE,
> +	COUNTER_SYNAPSE_ACTION_FALLING_EDGE,
> +	COUNTER_SYNAPSE_ACTION_BOTH_EDGES,
> +};
> +
> +#endif /* _UAPI_COUNTER_H_ */
> 

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

[David Lechner]

On 12/25/20 6:15 PM, William Breathitt Gray wrote:

> diff --git a/include/uapi/linux/counter.h b/include/uapi/linux/counter.h
> new file mode 100644
> index 000000000000..7585dc9db19d
> --- /dev/null
> +++ b/include/uapi/linux/counter.h
> @@ -0,0 +1,123 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +/*
> + * Userspace ABI for Counter character devices
> + * Copyright (C) 2020 William Breathitt Gray
> + */
> +#ifndef _UAPI_COUNTER_H_
> +#define _UAPI_COUNTER_H_
> +
> +#include <linux/ioctl.h>
> +#include <linux/types.h>
> +
> +/* Component type definitions */
> +enum counter_component_type {
> +	COUNTER_COMPONENT_NONE,
> +	COUNTER_COMPONENT_SIGNAL,
> +	COUNTER_COMPONENT_COUNT,
> +	COUNTER_COMPONENT_FUNCTION,
> +	COUNTER_COMPONENT_SYNAPSE_ACTION,
> +	COUNTER_COMPONENT_EXTENSION,
> +};
> +
> +/* Component scope definitions */
> +enum counter_scope {

Do we need COUNTER_SCOPE_NONE to go with COUNTER_COMPONENT_NONE?

> +	COUNTER_SCOPE_DEVICE,
> +	COUNTER_SCOPE_SIGNAL,
> +	COUNTER_SCOPE_COUNT,
> +};
> +
> +/**
> + * struct counter_component - Counter component identification
> + * @type: component type (Count, extension, etc.)

Instead of "Count, extension, etc.", it could be more helpful
to say one of enum counter_component_type.

> + * @scope: component scope (Device, Count, or Signal)

Same here. @scope must be one of enum counter_scope.

> + * @parent: parent component identification number
> + * @id: component identification number

It could be helpful to say that these id numbers match
the X/Y/Z in the sysfs paths as described in the sysfs
ABI.

> + */
> +struct counter_component {
> +	__u8 type;
> +	__u8 scope;
> +	__u8 parent;
> +	__u8 id;
> +};
> +
> +/* Event type definitions */
> +enum counter_event_type {
> +	COUNTER_EVENT_OVERFLOW,
> +	COUNTER_EVENT_UNDERFLOW,
> +	COUNTER_EVENT_OVERFLOW_UNDERFLOW,
> +	COUNTER_EVENT_THRESHOLD,
> +	COUNTER_EVENT_INDEX,
> +};
> +
> +/**
> + * struct counter_watch - Counter component watch configuration
> + * @component: component to watch when event triggers
> + * @event: event that triggers

It would be helpful to say that @event must be one of
enum counter_event_type.

> + * @channel: event channel

It would be useful to say that @channel should be 0 unless
a component has more than one event of the same type.

> + */
> +struct counter_watch {
> +	struct counter_component component;
> +	__u8 event;
> +	__u8 channel;
> +};
> +
> +/* ioctl commands */
> +#define COUNTER_CLEAR_WATCHES_IOCTL _IO(0x3E, 0x00)
> +#define COUNTER_ADD_WATCH_IOCTL _IOW(0x3E, 0x01, struct counter_watch)
> +#define COUNTER_LOAD_WATCHES_IOCTL _IO(0x3E, 0x02)
> +
> +/**
> + * struct counter_event - Counter event data
> + * @timestamp: best estimate of time of event occurrence, in nanoseconds
> + * @value: component value
> + * @watch: component watch configuration
> + * @errno: system error number
> + */
> +struct counter_event {
> +	__aligned_u64 timestamp;
> +	__aligned_u64 value;
> +	struct counter_watch watch;
> +	__u8 errno;

There are error codes larger than 255. Probably better
make this __u32.

> +};
> +
> +/* Count direction values */
> +enum counter_count_direction {
> +	COUNTER_COUNT_DIRECTION_FORWARD,
> +	COUNTER_COUNT_DIRECTION_BACKWARD,
> +};
> +
> +/* Count mode values */
> +enum counter_count_mode {
> +	COUNTER_COUNT_MODE_NORMAL,
> +	COUNTER_COUNT_MODE_RANGE_LIMIT,
> +	COUNTER_COUNT_MODE_NON_RECYCLE,
> +	COUNTER_COUNT_MODE_MODULO_N,
> +};
> +
> +/* Count function values */
> +enum counter_function {
> +	COUNTER_FUNCTION_INCREASE,
> +	COUNTER_FUNCTION_DECREASE,
> +	COUNTER_FUNCTION_PULSE_DIRECTION,
> +	COUNTER_FUNCTION_QUADRATURE_X1_A,
> +	COUNTER_FUNCTION_QUADRATURE_X1_B,
> +	COUNTER_FUNCTION_QUADRATURE_X2_A,
> +	COUNTER_FUNCTION_QUADRATURE_X2_B,
> +	COUNTER_FUNCTION_QUADRATURE_X4,
> +};
> +
> +/* Signal values */
> +enum counter_signal_level {
> +	COUNTER_SIGNAL_LEVEL_LOW,
> +	COUNTER_SIGNAL_LEVEL_HIGH,
> +};
> +
> +/* Action mode values */
> +enum counter_synapse_action {
> +	COUNTER_SYNAPSE_ACTION_NONE,
> +	COUNTER_SYNAPSE_ACTION_RISING_EDGE,
> +	COUNTER_SYNAPSE_ACTION_FALLING_EDGE,
> +	COUNTER_SYNAPSE_ACTION_BOTH_EDGES,
> +};
> +
> +#endif /* _UAPI_COUNTER_H_ */
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Jonathan Cameron]

On Fri, 25 Dec 2020 19:15:36 -0500
William Breathitt Gray <vilhelm.gray@gmail.com> wrote:

> This patch introduces a character device interface for the Counter
> subsystem. Device data is exposed through standard character device read
> operations. Device data is gathered when a Counter event is pushed by
> the respective Counter device driver. Configuration is handled via ioctl
> operations on the respective Counter character device node.
> 
> Cc: David Lechner <david@lechnology.com>
> Cc: Gwendal Grignou <gwendal@chromium.org>
> Cc: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>

There are a few things in here that could profitably be pulled out as precursor
patches.  I don't really understand the connection of extension_name to the
addition of a chardev for example.  Might be needed to provide enough
info to actually use the chardev, but does it have meaning without that?
Either way, definitely feels like it can be done in a separate patch.
 
> ---
>  MAINTAINERS                      |   1 +
>  drivers/counter/Makefile         |   2 +-
>  drivers/counter/counter-chrdev.c | 490 +++++++++++++++++++++++++++++++
>  drivers/counter/counter-chrdev.h |  16 +
>  drivers/counter/counter-core.c   |  37 ++-
>  drivers/counter/counter-sysfs.c  |  51 +++-
>  include/linux/counter.h          |  87 +++---
>  include/uapi/linux/counter.h     | 123 ++++++++
>  8 files changed, 751 insertions(+), 56 deletions(-)
>  create mode 100644 drivers/counter/counter-chrdev.c
>  create mode 100644 drivers/counter/counter-chrdev.h
>  create mode 100644 include/uapi/linux/counter.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index b64fa49d5796..3a240f70bdd4 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -4494,6 +4494,7 @@ F:	Documentation/ABI/testing/sysfs-bus-counter*
>  F:	Documentation/driver-api/generic-counter.rst
>  F:	drivers/counter/
>  F:	include/linux/counter.h
> +F:	include/uapi/linux/counter.h
>  
>  CPMAC ETHERNET DRIVER
>  M:	Florian Fainelli <f.fainelli@gmail.com>
> diff --git a/drivers/counter/Makefile b/drivers/counter/Makefile
> index cbe1d06af6a9..c4870eb5b1dd 100644
> --- a/drivers/counter/Makefile
> +++ b/drivers/counter/Makefile
> @@ -4,7 +4,7 @@
>  #
>  
>  obj-$(CONFIG_COUNTER) += counter.o
> -counter-y := counter-core.o counter-sysfs.o
> +counter-y := counter-core.o counter-sysfs.o counter-chrdev.o
>  
>  obj-$(CONFIG_104_QUAD_8)	+= 104-quad-8.o
>  obj-$(CONFIG_STM32_TIMER_CNT)	+= stm32-timer-cnt.o
> diff --git a/drivers/counter/counter-chrdev.c b/drivers/counter/counter-chrdev.c
> new file mode 100644
> index 000000000000..61b11989546a
> --- /dev/null
> +++ b/drivers/counter/counter-chrdev.c
> @@ -0,0 +1,490 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Generic Counter character device interface
> + * Copyright (C) 2020 William Breathitt Gray
> + */
> +
> +#include <linux/cdev.h>
> +#include <linux/counter.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/errno.h>
> +#include <linux/export.h>
> +#include <linux/fs.h>
> +#include <linux/kdev_t.h>
> +#include <linux/kfifo.h>
> +#include <linux/list.h>
> +#include <linux/mutex.h>
> +#include <linux/nospec.h>
> +#include <linux/poll.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +#include <linux/timekeeping.h>
> +#include <linux/types.h>
> +#include <linux/wait.h>
> +#include <linux/uaccess.h>
> +
> +#include "counter-chrdev.h"
> +
> +struct counter_comp_node {
> +	struct list_head l;
> +	struct counter_component component;
> +	struct counter_comp comp;
> +	void *parent;
> +};
> +
> +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 int copied;
> +
> +	if (len < sizeof(struct counter_event))
> +		return -EINVAL;
> +
> +	do {
> +		if (kfifo_is_empty(&counter->events)) {
> +			if (filp->f_flags & O_NONBLOCK)
> +				return -EAGAIN;
> +
> +			err = wait_event_interruptible(counter->events_wait,
> +					!kfifo_is_empty(&counter->events));
> +			if (err < 0)
> +				return err;
> +		}
> +
> +		if (mutex_lock_interruptible(&counter->events_lock))
> +			return -ERESTARTSYS;
> +		err = kfifo_to_user(&counter->events, buf, len, &copied);
> +		mutex_unlock(&counter->events_lock);
> +		if (err < 0)
> +			return err;
> +	} while (!copied);
> +
> +	return copied;
> +}
> +
> +static __poll_t counter_chrdev_poll(struct file *filp,
> +				    struct poll_table_struct *pollt)
> +{
> +	struct counter_device *const counter = filp->private_data;
> +	__poll_t events = 0;
> +
> +	poll_wait(filp, &counter->events_wait, pollt);
> +
> +	if (!kfifo_is_empty(&counter->events))
> +		events = EPOLLIN | EPOLLRDNORM;
> +
> +	return events;
> +}
> +
> +static void counter_events_list_free(struct list_head *const events_list)
> +{
> +	struct counter_event_node *p, *n;
> +	struct counter_comp_node *q, *o;
> +
> +	list_for_each_entry_safe(p, n, events_list, l) {
> +		/* Free associated component nodes */
> +		list_for_each_entry_safe(q, o, &p->comp_list, l) {
> +			list_del(&q->l);
> +			kfree(q);
> +		}
> +
> +		/* Free event node */
> +		list_del(&p->l);
> +		kfree(p);
> +	}
> +}
> +
> +static int counter_set_event_node(struct counter_device *const counter,
> +				  struct counter_watch *const watch,
> +				  const struct counter_comp_node *const cfg)
> +{
> +	struct counter_event_node *event_node;
> +	struct counter_comp_node *comp_node;
> +
> +	/* Search for event in the list */
> +	list_for_each_entry(event_node, &counter->next_events_list, l)
> +		if (event_node->event == watch->event &&
> +		    event_node->channel == watch->channel)
> +			break;
> +
> +	/* If event is not already in the list */
> +	if (&event_node->l == &counter->next_events_list) {
> +		/* Allocate new event node */
> +		event_node = kmalloc(sizeof(*event_node), GFP_ATOMIC);
> +		if (!event_node)
> +			return -ENOMEM;
> +
> +		/* Configure event node and add to the list */
> +		event_node->event = watch->event;
> +		event_node->channel = watch->channel;
> +		INIT_LIST_HEAD(&event_node->comp_list);
> +		list_add(&event_node->l, &counter->next_events_list);
> +	}
> +
> +	/* Check if component watch has already been set before */
> +	list_for_each_entry(comp_node, &event_node->comp_list, l)
> +		if (comp_node->parent == cfg->parent &&
> +		    comp_node->comp.count_u8_read == cfg->comp.count_u8_read)
> +			return -EINVAL;
> +
> +	/* Allocate component node */
> +	comp_node = kmalloc(sizeof(*comp_node), GFP_ATOMIC);
> +	if (!comp_node) {
> +		/* Free event node if no one else is watching */
> +		if (list_empty(&event_node->comp_list)) {
> +			list_del(&event_node->l);
> +			kfree(event_node);
> +		}
> +		return -ENOMEM;
> +	}
> +	*comp_node = *cfg;
> +
> +	/* Add component node to event node */
> +	list_add_tail(&comp_node->l, &event_node->comp_list);
> +
> +	return 0;
> +}
> +
> +static int counter_clear_watches(struct counter_device *const counter)
> +{
> +	unsigned long flags;
> +	int err = 0;
> +
> +	raw_spin_lock_irqsave(&counter->events_list_lock, flags);
> +
> +	counter_events_list_free(&counter->events_list);
> +
> +	if (counter->ops->events_configure)
> +		err = counter->ops->events_configure(counter);
> +
> +	raw_spin_unlock_irqrestore(&counter->events_list_lock, flags);
> +
> +	counter_events_list_free(&counter->next_events_list);
> +
> +	return err;
> +}
> +
> +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;
> +	struct counter_comp_node comp_node = {0};
> +	size_t parent, id;
> +	struct counter_comp *ext;
> +	size_t num_ext;
> +	int err;
> +
> +	if (copy_from_user(&watch, uwatch, sizeof(watch)))
> +		return -EFAULT;
> +
> +	if (watch.component.type == COUNTER_COMPONENT_NONE)
> +		goto no_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;
> +	}
> +
> +	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_u32_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_u32_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];
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +	/* Check if any read callback is set; this is part of a union */
> +	if (!comp_node.comp.count_u8_read)
> +		return -EOPNOTSUPP;
> +
> +no_component:
> +	if (counter->ops->watch_validate) {
> +		err = counter->ops->watch_validate(counter, &watch);
> +		if (err < 0)
> +			return err;
> +	}
> +
> +	comp_node.component = watch.component;
> +
> +	return counter_set_event_node(counter, &watch, &comp_node);
> +}
> +
> +static long counter_chrdev_ioctl(struct file *filp, unsigned int cmd,
> +				 unsigned long arg)
> +{
> +	struct counter_device *const counter = filp->private_data;
> +	unsigned long flags;
> +	int err = 0;
> +
> +	switch (cmd) {
> +	case COUNTER_CLEAR_WATCHES_IOCTL:
> +		return counter_clear_watches(counter);
> +	case COUNTER_ADD_WATCH_IOCTL:
> +		return counter_add_watch(counter, arg);
> +	case COUNTER_LOAD_WATCHES_IOCTL:
> +		raw_spin_lock_irqsave(&counter->events_list_lock, flags);
> +
> +		counter_events_list_free(&counter->events_list);
> +		list_replace_init(&counter->next_events_list,
> +				  &counter->events_list);
> +
> +		if (counter->ops->events_configure)
> +			err = counter->ops->events_configure(counter);
> +
> +		raw_spin_unlock_irqrestore(&counter->events_list_lock, flags);
> +		break;

return here. 

> +	default:
> +		return -ENOIOCTLCMD;
> +	}
> +
> +	return err;
> +}
> +
> +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;
> +
> +	return nonseekable_open(inode, filp);
> +}
> +
> +static int counter_chrdev_release(struct inode *inode, struct file *filp)
> +{
> +	struct counter_device *const counter = filp->private_data;
> +	int err;
> +
> +	err = counter_clear_watches(counter);
> +	if (err < 0)
> +		return err;
> +
> +	put_device(&counter->dev);
> +
> +	return 0;
> +}
> +
> +static const struct file_operations counter_fops = {
> +	.llseek = no_llseek,
> +	.read = counter_chrdev_read,
> +	.poll = counter_chrdev_poll,
> +	.unlocked_ioctl = counter_chrdev_ioctl,
> +	.open = counter_chrdev_open,
> +	.release = counter_chrdev_release,
> +};
> +
> +int counter_chrdev_add(struct counter_device *const counter,
> +		       const dev_t counter_devt)
> +{
> +	struct device *const dev = &counter->dev;
> +	struct cdev *const chrdev = &counter->chrdev;
> +
> +	/* Initialize Counter events lists */
> +	INIT_LIST_HEAD(&counter->events_list);
> +	INIT_LIST_HEAD(&counter->next_events_list);
> +	raw_spin_lock_init(&counter->events_list_lock);
> +
> +	/* Initialize Counter events queue */
> +	INIT_KFIFO(counter->events);
> +	init_waitqueue_head(&counter->events_wait);
> +	mutex_init(&counter->events_lock);
> +
> +	/* Initialize character device */
> +	cdev_init(chrdev, &counter_fops);
> +	dev->devt = MKDEV(MAJOR(counter_devt), counter->id);
> +	cdev_set_parent(chrdev, &dev->kobj);
> +
> +	return cdev_add(chrdev, dev->devt, 1);
> +}
> +
> +void counter_chrdev_remove(struct counter_device *const counter)
> +{
> +	cdev_del(&counter->chrdev);
> +}
> +
> +static int counter_get_data(struct counter_device *const counter,
> +			    const struct counter_comp_node *const comp_node,
> +			    u64 *const value)
> +{
> +	const struct counter_comp *const comp = &comp_node->comp;
> +	void *const parent = comp_node->parent;
> +	int err = 0;
> +	u8 value_u8 = 0;
> +	u32 value_u32 = 0;
> +
> +	if (comp_node->component.type == COUNTER_COMPONENT_NONE)
> +		return 0;
> +
> +	switch (comp->type) {
> +	case COUNTER_COMP_U8:
> +	case COUNTER_COMP_BOOL:
> +		switch (comp_node->component.scope) {
> +		case COUNTER_SCOPE_DEVICE:
> +			err = comp->device_u8_read(counter, &value_u8);
> +			break;
> +		case COUNTER_SCOPE_SIGNAL:
> +			err = comp->signal_u8_read(counter, parent, &value_u8);
> +			break;
> +		case COUNTER_SCOPE_COUNT:
> +			err = comp->count_u8_read(counter, parent, &value_u8);
> +			break;
> +		}
> +		*value = value_u8;
> +		break;
> +	case COUNTER_COMP_SIGNAL_LEVEL:
> +	case COUNTER_COMP_FUNCTION:
> +	case COUNTER_COMP_ENUM:
> +	case COUNTER_COMP_COUNT_DIRECTION:
> +	case COUNTER_COMP_COUNT_MODE:
> +		switch (comp_node->component.scope) {
> +		case COUNTER_SCOPE_DEVICE:
> +			err = comp->device_u32_read(counter, &value_u32);
> +			break;
> +		case COUNTER_SCOPE_SIGNAL:
> +			err = comp->signal_u32_read(counter, parent,
> +						    &value_u32);
> +			break;
> +		case COUNTER_SCOPE_COUNT:
> +			err = comp->count_u32_read(counter, parent, &value_u32);
> +			break;
> +		}
> +		*value = value_u32;

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

> +		break;
> +	case COUNTER_COMP_U64:
> +		switch (comp_node->component.scope) {
> +		case COUNTER_SCOPE_DEVICE:
> +			return comp->device_u64_read(counter, value);
> +		case COUNTER_SCOPE_SIGNAL:
> +			return comp->signal_u64_read(counter, parent, value);
> +		case COUNTER_SCOPE_COUNT:
> +			return comp->count_u64_read(counter, parent, value);
> +		}
> +		break;
> +	case COUNTER_COMP_SYNAPSE_ACTION:
> +		err = comp->action_read(counter, parent, comp->priv,
> +					&value_u32);
> +		*value = value_u32;
> +		break;
> +	}
> +
> +	return err;
> +}
> +
> +/**
> + * counter_push_event - queue event for userspace reading
> + * @counter:	pointer to Counter structure
> + * @event:	triggered event
> + * @channel:	event channel
> + *
> + * Note: If no one is watching for the respective event, it is silently
> + * discarded.
> + */
> +void counter_push_event(struct counter_device *const counter, const u8 event,
> +			const u8 channel)
> +{
> +	struct counter_event ev = {0};
> +	unsigned int copied = 0;
> +	unsigned long flags;
> +	struct counter_event_node *event_node;
> +	struct counter_comp_node *comp_node;
> +
> +	ev.timestamp = ktime_get_ns();
> +	ev.watch.event = event;
> +	ev.watch.channel = channel;
> +
> +	raw_spin_lock_irqsave(&counter->events_list_lock, flags);

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

> +
> +	/* Search for event in the list */
> +	list_for_each_entry(event_node, &counter->events_list, l)
> +		if (event_node->event == event &&
> +		    event_node->channel == channel)
> +			break;
> +
> +	/* If event is not in the list */
> +	if (&event_node->l == &counter->events_list)
> +		goto exit_early;
> +
> +	/* Read and queue relevant comp for userspace */
> +	list_for_each_entry(comp_node, &event_node->comp_list, l) {
> +		ev.watch.component = comp_node->component;
> +		ev.errno = -counter_get_data(counter, comp_node, &ev.value);
> +
> +		copied += kfifo_put(&counter->events, ev);
> +	}
> +
> +	if (copied)
> +		wake_up_poll(&counter->events_wait, EPOLLIN);
> +
> +exit_early:
> +	raw_spin_unlock_irqrestore(&counter->events_list_lock, flags);
> +}
> +EXPORT_SYMBOL_GPL(counter_push_event);
> diff --git a/drivers/counter/counter-chrdev.h b/drivers/counter/counter-chrdev.h
> new file mode 100644
> index 000000000000..cf5a318fe540
> --- /dev/null
> +++ b/drivers/counter/counter-chrdev.h
> @@ -0,0 +1,16 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Counter character device interface
> + * Copyright (C) 2020 William Breathitt Gray
> + */
> +#ifndef _COUNTER_CHRDEV_H_
> +#define _COUNTER_CHRDEV_H_
> +
> +#include <linux/counter.h>
> +#include <linux/types.h>
> +
> +int counter_chrdev_add(struct counter_device *const counter,
> +		       const dev_t counter_devt);
> +void counter_chrdev_remove(struct counter_device *const counter);
> +
> +#endif /* _COUNTER_CHRDEV_H_ */
> diff --git a/drivers/counter/counter-core.c b/drivers/counter/counter-core.c
> index 78e07588717b..1334e2f4ce4e 100644
> --- a/drivers/counter/counter-core.c
> +++ b/drivers/counter/counter-core.c
> @@ -5,12 +5,16 @@
>   */
>  #include <linux/counter.h>
>  #include <linux/device.h>
> +#include <linux/device/bus.h>
>  #include <linux/export.h>
> +#include <linux/fs.h>
>  #include <linux/gfp.h>
>  #include <linux/idr.h>
>  #include <linux/init.h>
>  #include <linux/module.h>
> +#include <linux/types.h>
>  
> +#include "counter-chrdev.h"
>  #include "counter-sysfs.h"
>  
>  /* Provides a unique ID for each counter device */
> @@ -33,6 +37,8 @@ static struct bus_type counter_bus_type = {
>  	.name = "counter"
>  };
>  
> +static dev_t counter_devt;
> +
>  /**
>   * counter_register - register Counter to the system
>   * @counter:	pointer to Counter to register
> @@ -51,7 +57,6 @@ int counter_register(struct counter_device *const counter)
>  	if (counter->id < 0)
>  		return counter->id;
>  
> -	/* Configure device structure for Counter */
>  	dev->type = &counter_device_type;
>  	dev->bus = &counter_bus_type;
>  	if (counter->parent) {
> @@ -62,18 +67,25 @@ int counter_register(struct counter_device *const counter)
>  	device_initialize(dev);
>  	dev_set_drvdata(dev, counter);
>  
> +	/* Add Counter character device */
> +	err = counter_chrdev_add(counter, counter_devt);
> +	if (err)
> +		goto err_free_id;
> +
>  	/* Add Counter sysfs attributes */
>  	err = counter_sysfs_add(counter);
>  	if (err < 0)
> -		goto err_free_id;
> +		goto err_remove_chrdev;
>  
>  	/* Add device to system */
>  	err = device_add(dev);
>  	if (err < 0)
> -		goto err_free_id;
> +		goto err_remove_chrdev;
>  
>  	return 0;
>  
> +err_remove_chrdev:
> +	counter_chrdev_remove(counter);
>  err_free_id:
>  	put_device(dev);
>  	return err;
> @@ -135,13 +147,30 @@ int devm_counter_register(struct device *dev,
>  }
>  EXPORT_SYMBOL_GPL(devm_counter_register);
>  
> +#define COUNTER_DEV_MAX 256
> +
>  static int __init counter_init(void)
>  {
> -	return bus_register(&counter_bus_type);
> +	int err;
> +
> +	err = bus_register(&counter_bus_type);
> +	if (err < 0)
> +		return err;
> +
> +	err = alloc_chrdev_region(&counter_devt, 0, COUNTER_DEV_MAX, "counter");
> +	if (err < 0)
> +		goto err_unregister_bus;
> +
> +	return 0;
> +
> +err_unregister_bus:
> +	bus_unregister(&counter_bus_type);
> +	return err;
>  }
>  
>  static void __exit counter_exit(void)
>  {
> +	unregister_chrdev_region(counter_devt, COUNTER_DEV_MAX);
>  	bus_unregister(&counter_bus_type);
>  }
>  
> diff --git a/drivers/counter/counter-sysfs.c b/drivers/counter/counter-sysfs.c
> index 654afa91ae9f..4d97d6e50d48 100644
> --- a/drivers/counter/counter-sysfs.c
> +++ b/drivers/counter/counter-sysfs.c
> @@ -499,6 +499,7 @@ static ssize_t counter_comp_name_show(struct device *dev,
>  
>  static int counter_name_attr_create(struct device *const dev,
>  				    struct counter_attribute_group *const group,
> +				    const char *const attr_name,
>  				    const char *const name)
>  {
>  	struct counter_attribute *counter_attr;
> @@ -513,7 +514,7 @@ static int counter_name_attr_create(struct device *const dev,
>  
>  	/* Configure device attribute */
>  	sysfs_attr_init(&counter_attr->dev_attr.attr);
> -	counter_attr->dev_attr.attr.name = "name";
> +	counter_attr->dev_attr.attr.name = attr_name;
>  	counter_attr->dev_attr.attr.mode = 0444;
>  	counter_attr->dev_attr.show = counter_comp_name_show;
>  
> @@ -524,6 +525,18 @@ static int counter_name_attr_create(struct device *const dev,
>  	return 0;
>  }
>  
> +static int counter_ext_name_attr_create(struct device *const dev,
> +	struct counter_attribute_group *const group, const size_t i,
> +	const char *const name)
> +{
> +	const char *attr_name;
> +
> +	attr_name = devm_kasprintf(dev, GFP_KERNEL, "extension%zu_name", i);
> +	if (!attr_name)
> +		return -ENOMEM;
> +
> +	return counter_name_attr_create(dev, group, attr_name, name);
> +}
>  
>  static struct counter_comp counter_signal_comp = {
>  	.type = COUNTER_COMP_SIGNAL_LEVEL,
> @@ -539,6 +552,7 @@ static int counter_signal_attrs_create(struct counter_device *const counter,
>  	int err;
>  	struct counter_comp comp;
>  	size_t i;
> +	struct counter_comp *ext;
>  
>  	/* Create main Signal attribute */
>  	comp = counter_signal_comp;
> @@ -548,14 +562,19 @@ static int counter_signal_attrs_create(struct counter_device *const counter,
>  		return err;
>  
>  	/* Create Signal name attribute */
> -	err = counter_name_attr_create(dev, group, signal->name);
> +	err = counter_name_attr_create(dev, group, "name", signal->name);
>  	if (err < 0)
>  		return err;
>  
>  	/* Create an attribute for each extension */
>  	for (i = 0; i < signal->num_ext; i++) {
> -		err = counter_attr_create(dev, group, signal->ext + i, scope,
> -					  signal);
> +		ext = signal->ext + i;
> +
> +		err = counter_attr_create(dev, group, ext, scope, signal);
> +		if (err < 0)
> +			return err;
> +
> +		err = counter_ext_name_attr_create(dev, group, i, ext->name);
>  		if (err < 0)
>  			return err;
>  	}
> @@ -640,6 +659,7 @@ static int counter_count_attrs_create(struct counter_device *const counter,
>  	int err;
>  	struct counter_comp comp;
>  	size_t i;
> +	struct counter_comp *ext;
>  
>  	/* Create main Count attribute */
>  	comp = counter_count_comp;
> @@ -650,7 +670,7 @@ static int counter_count_attrs_create(struct counter_device *const counter,
>  		return err;
>  
>  	/* Create Count name attribute */
> -	err = counter_name_attr_create(dev, group, count->name);
> +	err = counter_name_attr_create(dev, group, "name", count->name);

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

>  	if (err < 0)
>  		return err;
>  
> @@ -664,8 +684,13 @@ static int counter_count_attrs_create(struct counter_device *const counter,
>  
>  	/* Create an attribute for each extension */
>  	for (i = 0; i < count->num_ext; i++) {
> -		err = counter_attr_create(dev, group, count->ext + i, scope,
> -					  count);
> +		ext = count->ext + i;
> +
> +		err = counter_attr_create(dev, group, ext, scope, count);
> +		if (err < 0)
> +			return err;
> +
> +		err = counter_ext_name_attr_create(dev, group, i, ext->name);
>  		if (err < 0)
>  			return err;
>  	}
> @@ -729,6 +754,7 @@ static int counter_sysfs_attr_add(struct counter_device *const counter,
>  	struct device *const dev = &counter->dev;
>  	int err;
>  	size_t i;
> +	struct counter_comp *ext;
>  
>  	/* Add Signals sysfs attributes */
>  	err = counter_sysfs_signals_add(counter, group);
> @@ -743,7 +769,7 @@ static int counter_sysfs_attr_add(struct counter_device *const counter,
>  	group += counter->num_counts;
>  
>  	/* Create name attribute */
> -	err = counter_name_attr_create(dev, group, counter->name);
> +	err = counter_name_attr_create(dev, group, "name", counter->name);
>  	if (err < 0)
>  		return err;
>  
> @@ -761,8 +787,13 @@ static int counter_sysfs_attr_add(struct counter_device *const counter,
>  
>  	/* Create an attribute for each extension */
>  	for (i = 0; i < counter->num_ext; i++) {
> -		err = counter_attr_create(dev, group, counter->ext + i, scope,
> -					  NULL);
> +		ext = counter->ext + i;
> +
> +		err = counter_attr_create(dev, group, ext, scope, NULL);
> +		if (err < 0)
> +			return err;
> +
> +		err = counter_ext_name_attr_create(dev, group, i, ext->name);
>  		if (err < 0)
>  			return err;
>  	}
> diff --git a/include/linux/counter.h b/include/linux/counter.h
> index 2f01e1fec857..ce79e62a2e58 100644
> --- a/include/linux/counter.h
> +++ b/include/linux/counter.h
> @@ -6,9 +6,15 @@
>  #ifndef _COUNTER_H_
>  #define _COUNTER_H_
>  
> +#include <linux/cdev.h>
>  #include <linux/device.h>
>  #include <linux/kernel.h>
> +#include <linux/kfifo.h>
> +#include <linux/mutex.h>
> +#include <linux/spinlock_types.h>
>  #include <linux/types.h>
> +#include <linux/wait.h>
> +#include <uapi/linux/counter.h>
>  
>  struct counter_device;
>  struct counter_count;
> @@ -27,47 +33,6 @@ enum counter_comp_type {
>  	COUNTER_COMP_COUNT_MODE,
>  };
>  
> -enum counter_scope {
> -	COUNTER_SCOPE_DEVICE,
> -	COUNTER_SCOPE_SIGNAL,
> -	COUNTER_SCOPE_COUNT,
> -};
> -
> -enum counter_count_direction {
> -	COUNTER_COUNT_DIRECTION_FORWARD,
> -	COUNTER_COUNT_DIRECTION_BACKWARD,
> -};
> -
> -enum counter_count_mode {
> -	COUNTER_COUNT_MODE_NORMAL,
> -	COUNTER_COUNT_MODE_RANGE_LIMIT,
> -	COUNTER_COUNT_MODE_NON_RECYCLE,
> -	COUNTER_COUNT_MODE_MODULO_N,
> -};
> -
> -enum counter_function {
> -	COUNTER_FUNCTION_INCREASE,
> -	COUNTER_FUNCTION_DECREASE,
> -	COUNTER_FUNCTION_PULSE_DIRECTION,
> -	COUNTER_FUNCTION_QUADRATURE_X1_A,
> -	COUNTER_FUNCTION_QUADRATURE_X1_B,
> -	COUNTER_FUNCTION_QUADRATURE_X2_A,
> -	COUNTER_FUNCTION_QUADRATURE_X2_B,
> -	COUNTER_FUNCTION_QUADRATURE_X4,
> -};
> -
> -enum counter_signal_level {
> -	COUNTER_SIGNAL_LEVEL_LOW,
> -	COUNTER_SIGNAL_LEVEL_HIGH,
> -};
> -
> -enum counter_synapse_action {
> -	COUNTER_SYNAPSE_ACTION_NONE,
> -	COUNTER_SYNAPSE_ACTION_RISING_EDGE,
> -	COUNTER_SYNAPSE_ACTION_FALLING_EDGE,
> -	COUNTER_SYNAPSE_ACTION_BOTH_EDGES,
> -};
> -
>  /**
>   * struct counter_comp - Counter component node
>   * @type:		Counter component data type
> @@ -239,6 +204,20 @@ struct counter_count {
>  	size_t num_ext;
>  };
>  
> +/**
> + * 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;
> +	struct list_head comp_list;
> +};
> +
>  /**
>   * struct counter_ops - Callbacks from driver
>   * @signal_read:	read callback for Signals. The read level of the
> @@ -261,6 +240,13 @@ struct counter_count {
>   * @action_write:	write callback for Synapse action modes. The action mode
>   *			to write for the respective Synapse is passed in via the
>   *			action parameter.
> + * @events_configure:	write callback to configure events. The list of struct
> + *			counter_event_node may be accessed via the events_list
> + *			member of the counter parameter.
> + * @watch_validate:	callback to validate a watch. The Counter component
> + *			watch configuration is passed in via the watch
> + *			parameter. A return value of 0 indicates a valid Counter
> + *			component watch configuration.
>   */
>  struct counter_ops {
>  	int (*signal_read)(struct counter_device *counter,
> @@ -284,6 +270,9 @@ struct counter_ops {
>  			    struct counter_count *count,
>  			    struct counter_synapse *synapse,
>  			    enum counter_synapse_action action);
> +	int (*events_configure)(struct counter_device *counter);
> +	int (*watch_validate)(struct counter_device *counter,
> +			      const struct counter_watch *watch);
>  };
>  
>  /**
> @@ -300,6 +289,13 @@ struct counter_ops {
>   * @priv:		optional private data supplied by driver
>   * @id:			unique ID used to identify the Counter
>   * @dev:		internal device structure
> + * @chrdev:		internal character device structure
> + * @events_list:	list of current watching Counter events
> + * @events_list_lock:	lock to protect Counter events list operations
> + * @next_events_list:	list of next watching Counter events
> + * @events:		queue of detected Counter events
> + * @events_wait:	wait queue to allow blocking reads of Counter events
> + * @events_lock:	lock to protect Counter events queue read operations
>   */
>  struct counter_device {
>  	const char *name;
> @@ -319,12 +315,21 @@ struct counter_device {
>  
>  	int id;
>  	struct device dev;
> +	struct cdev chrdev;
> +	struct list_head events_list;
> +	raw_spinlock_t events_list_lock;
> +	struct list_head next_events_list;
> +	DECLARE_KFIFO(events, struct counter_event, 64);

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

> +	wait_queue_head_t events_wait;
> +	struct mutex events_lock;
>  };
>  
>  int counter_register(struct counter_device *const counter);
>  void counter_unregister(struct counter_device *const counter);
>  int devm_counter_register(struct device *dev,
>  			  struct counter_device *const counter);
> +void counter_push_event(struct counter_device *const counter, const u8 event,
> +			const u8 channel);
>  
>  #define COUNTER_COMP_DEVICE_U8(_name, _read, _write) \
>  { \
> diff --git a/include/uapi/linux/counter.h b/include/uapi/linux/counter.h
> new file mode 100644
> index 000000000000..7585dc9db19d
> --- /dev/null
> +++ b/include/uapi/linux/counter.h
Small thing but I would have been tempted to do a precursor patch to the
main change simply putting in place the userspace header.

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

Jonathan

> @@ -0,0 +1,123 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +/*
> + * Userspace ABI for Counter character devices
> + * Copyright (C) 2020 William Breathitt Gray
> + */
> +#ifndef _UAPI_COUNTER_H_
> +#define _UAPI_COUNTER_H_
> +
> +#include <linux/ioctl.h>
> +#include <linux/types.h>
> +
> +/* Component type definitions */
> +enum counter_component_type {
> +	COUNTER_COMPONENT_NONE,
> +	COUNTER_COMPONENT_SIGNAL,
> +	COUNTER_COMPONENT_COUNT,
> +	COUNTER_COMPONENT_FUNCTION,
> +	COUNTER_COMPONENT_SYNAPSE_ACTION,
> +	COUNTER_COMPONENT_EXTENSION,
> +};
> +
> +/* Component scope definitions */
> +enum counter_scope {
> +	COUNTER_SCOPE_DEVICE,
> +	COUNTER_SCOPE_SIGNAL,
> +	COUNTER_SCOPE_COUNT,
> +};
> +
> +/**
> + * struct counter_component - Counter component identification
> + * @type: component type (Count, extension, etc.)
> + * @scope: component scope (Device, Count, or Signal)
> + * @parent: parent component identification number
> + * @id: component identification number
> + */
> +struct counter_component {
> +	__u8 type;
> +	__u8 scope;
> +	__u8 parent;
> +	__u8 id;
> +};
> +
> +/* Event type definitions */
> +enum counter_event_type {
> +	COUNTER_EVENT_OVERFLOW,
> +	COUNTER_EVENT_UNDERFLOW,
> +	COUNTER_EVENT_OVERFLOW_UNDERFLOW,
> +	COUNTER_EVENT_THRESHOLD,
> +	COUNTER_EVENT_INDEX,
> +};
> +
> +/**
> + * struct counter_watch - Counter component watch configuration
> + * @component: component to watch when event triggers
> + * @event: event that triggers
> + * @channel: event channel
> + */
> +struct counter_watch {
> +	struct counter_component component;
> +	__u8 event;
> +	__u8 channel;
> +};
> +
> +/* ioctl commands */
> +#define COUNTER_CLEAR_WATCHES_IOCTL _IO(0x3E, 0x00)
> +#define COUNTER_ADD_WATCH_IOCTL _IOW(0x3E, 0x01, struct counter_watch)
> +#define COUNTER_LOAD_WATCHES_IOCTL _IO(0x3E, 0x02)
> +
> +/**
> + * struct counter_event - Counter event data
> + * @timestamp: best estimate of time of event occurrence, in nanoseconds
> + * @value: component value
> + * @watch: component watch configuration
> + * @errno: system error number
> + */
> +struct counter_event {
> +	__aligned_u64 timestamp;
> +	__aligned_u64 value;
> +	struct counter_watch watch;
> +	__u8 errno;
> +};
> +
> +/* Count direction values */
> +enum counter_count_direction {
> +	COUNTER_COUNT_DIRECTION_FORWARD,
> +	COUNTER_COUNT_DIRECTION_BACKWARD,
> +};
> +
> +/* Count mode values */
> +enum counter_count_mode {
> +	COUNTER_COUNT_MODE_NORMAL,
> +	COUNTER_COUNT_MODE_RANGE_LIMIT,
> +	COUNTER_COUNT_MODE_NON_RECYCLE,
> +	COUNTER_COUNT_MODE_MODULO_N,
> +};
> +
> +/* Count function values */
> +enum counter_function {
> +	COUNTER_FUNCTION_INCREASE,
> +	COUNTER_FUNCTION_DECREASE,
> +	COUNTER_FUNCTION_PULSE_DIRECTION,
> +	COUNTER_FUNCTION_QUADRATURE_X1_A,
> +	COUNTER_FUNCTION_QUADRATURE_X1_B,
> +	COUNTER_FUNCTION_QUADRATURE_X2_A,
> +	COUNTER_FUNCTION_QUADRATURE_X2_B,
> +	COUNTER_FUNCTION_QUADRATURE_X4,
> +};
> +
> +/* Signal values */
> +enum counter_signal_level {
> +	COUNTER_SIGNAL_LEVEL_LOW,
> +	COUNTER_SIGNAL_LEVEL_HIGH,
> +};
> +
> +/* Action mode values */
> +enum counter_synapse_action {
> +	COUNTER_SYNAPSE_ACTION_NONE,
> +	COUNTER_SYNAPSE_ACTION_RISING_EDGE,
> +	COUNTER_SYNAPSE_ACTION_FALLING_EDGE,
> +	COUNTER_SYNAPSE_ACTION_BOTH_EDGES,
> +};
> +
> +#endif /* _UAPI_COUNTER_H_ */

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

[Jonathan Cameron]

On Fri, 25 Dec 2020 19:15:36 -0500
William Breathitt Gray <vilhelm.gray@gmail.com> wrote:

> This patch introduces a character device interface for the Counter
> subsystem. Device data is exposed through standard character device read
> operations. Device data is gathered when a Counter event is pushed by
> the respective Counter device driver. Configuration is handled via ioctl
> operations on the respective Counter character device node.
> 
> Cc: David Lechner <david@lechnology.com>
> Cc: Gwendal Grignou <gwendal@chromium.org>
> Cc: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>

There are a few things in here that could profitably be pulled out as precursor
patches.  I don't really understand the connection of extension_name to the
addition of a chardev for example.  Might be needed to provide enough
info to actually use the chardev, but does it have meaning without that?
Either way, definitely feels like it can be done in a separate patch.
 
> ---
>  MAINTAINERS                      |   1 +
>  drivers/counter/Makefile         |   2 +-
>  drivers/counter/counter-chrdev.c | 490 +++++++++++++++++++++++++++++++
>  drivers/counter/counter-chrdev.h |  16 +
>  drivers/counter/counter-core.c   |  37 ++-
>  drivers/counter/counter-sysfs.c  |  51 +++-
>  include/linux/counter.h          |  87 +++---
>  include/uapi/linux/counter.h     | 123 ++++++++
>  8 files changed, 751 insertions(+), 56 deletions(-)
>  create mode 100644 drivers/counter/counter-chrdev.c
>  create mode 100644 drivers/counter/counter-chrdev.h
>  create mode 100644 include/uapi/linux/counter.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index b64fa49d5796..3a240f70bdd4 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -4494,6 +4494,7 @@ F:	Documentation/ABI/testing/sysfs-bus-counter*
>  F:	Documentation/driver-api/generic-counter.rst
>  F:	drivers/counter/
>  F:	include/linux/counter.h
> +F:	include/uapi/linux/counter.h
>  
>  CPMAC ETHERNET DRIVER
>  M:	Florian Fainelli <f.fainelli@gmail.com>
> diff --git a/drivers/counter/Makefile b/drivers/counter/Makefile
> index cbe1d06af6a9..c4870eb5b1dd 100644
> --- a/drivers/counter/Makefile
> +++ b/drivers/counter/Makefile
> @@ -4,7 +4,7 @@
>  #
>  
>  obj-$(CONFIG_COUNTER) += counter.o
> -counter-y := counter-core.o counter-sysfs.o
> +counter-y := counter-core.o counter-sysfs.o counter-chrdev.o
>  
>  obj-$(CONFIG_104_QUAD_8)	+= 104-quad-8.o
>  obj-$(CONFIG_STM32_TIMER_CNT)	+= stm32-timer-cnt.o
> diff --git a/drivers/counter/counter-chrdev.c b/drivers/counter/counter-chrdev.c
> new file mode 100644
> index 000000000000..61b11989546a
> --- /dev/null
> +++ b/drivers/counter/counter-chrdev.c
> @@ -0,0 +1,490 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Generic Counter character device interface
> + * Copyright (C) 2020 William Breathitt Gray
> + */
> +
> +#include <linux/cdev.h>
> +#include <linux/counter.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/errno.h>
> +#include <linux/export.h>
> +#include <linux/fs.h>
> +#include <linux/kdev_t.h>
> +#include <linux/kfifo.h>
> +#include <linux/list.h>
> +#include <linux/mutex.h>
> +#include <linux/nospec.h>
> +#include <linux/poll.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +#include <linux/timekeeping.h>
> +#include <linux/types.h>
> +#include <linux/wait.h>
> +#include <linux/uaccess.h>
> +
> +#include "counter-chrdev.h"
> +
> +struct counter_comp_node {
> +	struct list_head l;
> +	struct counter_component component;
> +	struct counter_comp comp;
> +	void *parent;
> +};
> +
> +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 int copied;
> +
> +	if (len < sizeof(struct counter_event))
> +		return -EINVAL;
> +
> +	do {
> +		if (kfifo_is_empty(&counter->events)) {
> +			if (filp->f_flags & O_NONBLOCK)
> +				return -EAGAIN;
> +
> +			err = wait_event_interruptible(counter->events_wait,
> +					!kfifo_is_empty(&counter->events));
> +			if (err < 0)
> +				return err;
> +		}
> +
> +		if (mutex_lock_interruptible(&counter->events_lock))
> +			return -ERESTARTSYS;
> +		err = kfifo_to_user(&counter->events, buf, len, &copied);
> +		mutex_unlock(&counter->events_lock);
> +		if (err < 0)
> +			return err;
> +	} while (!copied);
> +
> +	return copied;
> +}
> +
> +static __poll_t counter_chrdev_poll(struct file *filp,
> +				    struct poll_table_struct *pollt)
> +{
> +	struct counter_device *const counter = filp->private_data;
> +	__poll_t events = 0;
> +
> +	poll_wait(filp, &counter->events_wait, pollt);
> +
> +	if (!kfifo_is_empty(&counter->events))
> +		events = EPOLLIN | EPOLLRDNORM;
> +
> +	return events;
> +}
> +
> +static void counter_events_list_free(struct list_head *const events_list)
> +{
> +	struct counter_event_node *p, *n;
> +	struct counter_comp_node *q, *o;
> +
> +	list_for_each_entry_safe(p, n, events_list, l) {
> +		/* Free associated component nodes */
> +		list_for_each_entry_safe(q, o, &p->comp_list, l) {
> +			list_del(&q->l);
> +			kfree(q);
> +		}
> +
> +		/* Free event node */
> +		list_del(&p->l);
> +		kfree(p);
> +	}
> +}
> +
> +static int counter_set_event_node(struct counter_device *const counter,
> +				  struct counter_watch *const watch,
> +				  const struct counter_comp_node *const cfg)
> +{
> +	struct counter_event_node *event_node;
> +	struct counter_comp_node *comp_node;
> +
> +	/* Search for event in the list */
> +	list_for_each_entry(event_node, &counter->next_events_list, l)
> +		if (event_node->event == watch->event &&
> +		    event_node->channel == watch->channel)
> +			break;
> +
> +	/* If event is not already in the list */
> +	if (&event_node->l == &counter->next_events_list) {
> +		/* Allocate new event node */
> +		event_node = kmalloc(sizeof(*event_node), GFP_ATOMIC);
> +		if (!event_node)
> +			return -ENOMEM;
> +
> +		/* Configure event node and add to the list */
> +		event_node->event = watch->event;
> +		event_node->channel = watch->channel;
> +		INIT_LIST_HEAD(&event_node->comp_list);
> +		list_add(&event_node->l, &counter->next_events_list);
> +	}
> +
> +	/* Check if component watch has already been set before */
> +	list_for_each_entry(comp_node, &event_node->comp_list, l)
> +		if (comp_node->parent == cfg->parent &&
> +		    comp_node->comp.count_u8_read == cfg->comp.count_u8_read)
> +			return -EINVAL;
> +
> +	/* Allocate component node */
> +	comp_node = kmalloc(sizeof(*comp_node), GFP_ATOMIC);
> +	if (!comp_node) {
> +		/* Free event node if no one else is watching */
> +		if (list_empty(&event_node->comp_list)) {
> +			list_del(&event_node->l);
> +			kfree(event_node);
> +		}
> +		return -ENOMEM;
> +	}
> +	*comp_node = *cfg;
> +
> +	/* Add component node to event node */
> +	list_add_tail(&comp_node->l, &event_node->comp_list);
> +
> +	return 0;
> +}
> +
> +static int counter_clear_watches(struct counter_device *const counter)
> +{
> +	unsigned long flags;
> +	int err = 0;
> +
> +	raw_spin_lock_irqsave(&counter->events_list_lock, flags);
> +
> +	counter_events_list_free(&counter->events_list);
> +
> +	if (counter->ops->events_configure)
> +		err = counter->ops->events_configure(counter);
> +
> +	raw_spin_unlock_irqrestore(&counter->events_list_lock, flags);
> +
> +	counter_events_list_free(&counter->next_events_list);
> +
> +	return err;
> +}
> +
> +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;
> +	struct counter_comp_node comp_node = {0};
> +	size_t parent, id;
> +	struct counter_comp *ext;
> +	size_t num_ext;
> +	int err;
> +
> +	if (copy_from_user(&watch, uwatch, sizeof(watch)))
> +		return -EFAULT;
> +
> +	if (watch.component.type == COUNTER_COMPONENT_NONE)
> +		goto no_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;
> +	}
> +
> +	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_u32_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_u32_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];
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +	/* Check if any read callback is set; this is part of a union */
> +	if (!comp_node.comp.count_u8_read)
> +		return -EOPNOTSUPP;
> +
> +no_component:
> +	if (counter->ops->watch_validate) {
> +		err = counter->ops->watch_validate(counter, &watch);
> +		if (err < 0)
> +			return err;
> +	}
> +
> +	comp_node.component = watch.component;
> +
> +	return counter_set_event_node(counter, &watch, &comp_node);
> +}
> +
> +static long counter_chrdev_ioctl(struct file *filp, unsigned int cmd,
> +				 unsigned long arg)
> +{
> +	struct counter_device *const counter = filp->private_data;
> +	unsigned long flags;
> +	int err = 0;
> +
> +	switch (cmd) {
> +	case COUNTER_CLEAR_WATCHES_IOCTL:
> +		return counter_clear_watches(counter);
> +	case COUNTER_ADD_WATCH_IOCTL:
> +		return counter_add_watch(counter, arg);
> +	case COUNTER_LOAD_WATCHES_IOCTL:
> +		raw_spin_lock_irqsave(&counter->events_list_lock, flags);
> +
> +		counter_events_list_free(&counter->events_list);
> +		list_replace_init(&counter->next_events_list,
> +				  &counter->events_list);
> +
> +		if (counter->ops->events_configure)
> +			err = counter->ops->events_configure(counter);
> +
> +		raw_spin_unlock_irqrestore(&counter->events_list_lock, flags);
> +		break;

return here. 

> +	default:
> +		return -ENOIOCTLCMD;
> +	}
> +
> +	return err;
> +}
> +
> +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;
> +
> +	return nonseekable_open(inode, filp);
> +}
> +
> +static int counter_chrdev_release(struct inode *inode, struct file *filp)
> +{
> +	struct counter_device *const counter = filp->private_data;
> +	int err;
> +
> +	err = counter_clear_watches(counter);
> +	if (err < 0)
> +		return err;
> +
> +	put_device(&counter->dev);
> +
> +	return 0;
> +}
> +
> +static const struct file_operations counter_fops = {
> +	.llseek = no_llseek,
> +	.read = counter_chrdev_read,
> +	.poll = counter_chrdev_poll,
> +	.unlocked_ioctl = counter_chrdev_ioctl,
> +	.open = counter_chrdev_open,
> +	.release = counter_chrdev_release,
> +};
> +
> +int counter_chrdev_add(struct counter_device *const counter,
> +		       const dev_t counter_devt)
> +{
> +	struct device *const dev = &counter->dev;
> +	struct cdev *const chrdev = &counter->chrdev;
> +
> +	/* Initialize Counter events lists */
> +	INIT_LIST_HEAD(&counter->events_list);
> +	INIT_LIST_HEAD(&counter->next_events_list);
> +	raw_spin_lock_init(&counter->events_list_lock);
> +
> +	/* Initialize Counter events queue */
> +	INIT_KFIFO(counter->events);
> +	init_waitqueue_head(&counter->events_wait);
> +	mutex_init(&counter->events_lock);
> +
> +	/* Initialize character device */
> +	cdev_init(chrdev, &counter_fops);
> +	dev->devt = MKDEV(MAJOR(counter_devt), counter->id);
> +	cdev_set_parent(chrdev, &dev->kobj);
> +
> +	return cdev_add(chrdev, dev->devt, 1);
> +}
> +
> +void counter_chrdev_remove(struct counter_device *const counter)
> +{
> +	cdev_del(&counter->chrdev);
> +}
> +
> +static int counter_get_data(struct counter_device *const counter,
> +			    const struct counter_comp_node *const comp_node,
> +			    u64 *const value)
> +{
> +	const struct counter_comp *const comp = &comp_node->comp;
> +	void *const parent = comp_node->parent;
> +	int err = 0;
> +	u8 value_u8 = 0;
> +	u32 value_u32 = 0;
> +
> +	if (comp_node->component.type == COUNTER_COMPONENT_NONE)
> +		return 0;
> +
> +	switch (comp->type) {
> +	case COUNTER_COMP_U8:
> +	case COUNTER_COMP_BOOL:
> +		switch (comp_node->component.scope) {
> +		case COUNTER_SCOPE_DEVICE:
> +			err = comp->device_u8_read(counter, &value_u8);
> +			break;
> +		case COUNTER_SCOPE_SIGNAL:
> +			err = comp->signal_u8_read(counter, parent, &value_u8);
> +			break;
> +		case COUNTER_SCOPE_COUNT:
> +			err = comp->count_u8_read(counter, parent, &value_u8);
> +			break;
> +		}
> +		*value = value_u8;
> +		break;
> +	case COUNTER_COMP_SIGNAL_LEVEL:
> +	case COUNTER_COMP_FUNCTION:
> +	case COUNTER_COMP_ENUM:
> +	case COUNTER_COMP_COUNT_DIRECTION:
> +	case COUNTER_COMP_COUNT_MODE:
> +		switch (comp_node->component.scope) {
> +		case COUNTER_SCOPE_DEVICE:
> +			err = comp->device_u32_read(counter, &value_u32);
> +			break;
> +		case COUNTER_SCOPE_SIGNAL:
> +			err = comp->signal_u32_read(counter, parent,
> +						    &value_u32);
> +			break;
> +		case COUNTER_SCOPE_COUNT:
> +			err = comp->count_u32_read(counter, parent, &value_u32);
> +			break;
> +		}
> +		*value = value_u32;

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

> +		break;
> +	case COUNTER_COMP_U64:
> +		switch (comp_node->component.scope) {
> +		case COUNTER_SCOPE_DEVICE:
> +			return comp->device_u64_read(counter, value);
> +		case COUNTER_SCOPE_SIGNAL:
> +			return comp->signal_u64_read(counter, parent, value);
> +		case COUNTER_SCOPE_COUNT:
> +			return comp->count_u64_read(counter, parent, value);
> +		}
> +		break;
> +	case COUNTER_COMP_SYNAPSE_ACTION:
> +		err = comp->action_read(counter, parent, comp->priv,
> +					&value_u32);
> +		*value = value_u32;
> +		break;
> +	}
> +
> +	return err;
> +}
> +
> +/**
> + * counter_push_event - queue event for userspace reading
> + * @counter:	pointer to Counter structure
> + * @event:	triggered event
> + * @channel:	event channel
> + *
> + * Note: If no one is watching for the respective event, it is silently
> + * discarded.
> + */
> +void counter_push_event(struct counter_device *const counter, const u8 event,
> +			const u8 channel)
> +{
> +	struct counter_event ev = {0};
> +	unsigned int copied = 0;
> +	unsigned long flags;
> +	struct counter_event_node *event_node;
> +	struct counter_comp_node *comp_node;
> +
> +	ev.timestamp = ktime_get_ns();
> +	ev.watch.event = event;
> +	ev.watch.channel = channel;
> +
> +	raw_spin_lock_irqsave(&counter->events_list_lock, flags);

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

> +
> +	/* Search for event in the list */
> +	list_for_each_entry(event_node, &counter->events_list, l)
> +		if (event_node->event == event &&
> +		    event_node->channel == channel)
> +			break;
> +
> +	/* If event is not in the list */
> +	if (&event_node->l == &counter->events_list)
> +		goto exit_early;
> +
> +	/* Read and queue relevant comp for userspace */
> +	list_for_each_entry(comp_node, &event_node->comp_list, l) {
> +		ev.watch.component = comp_node->component;
> +		ev.errno = -counter_get_data(counter, comp_node, &ev.value);
> +
> +		copied += kfifo_put(&counter->events, ev);
> +	}
> +
> +	if (copied)
> +		wake_up_poll(&counter->events_wait, EPOLLIN);
> +
> +exit_early:
> +	raw_spin_unlock_irqrestore(&counter->events_list_lock, flags);
> +}
> +EXPORT_SYMBOL_GPL(counter_push_event);
> diff --git a/drivers/counter/counter-chrdev.h b/drivers/counter/counter-chrdev.h
> new file mode 100644
> index 000000000000..cf5a318fe540
> --- /dev/null
> +++ b/drivers/counter/counter-chrdev.h
> @@ -0,0 +1,16 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Counter character device interface
> + * Copyright (C) 2020 William Breathitt Gray
> + */
> +#ifndef _COUNTER_CHRDEV_H_
> +#define _COUNTER_CHRDEV_H_
> +
> +#include <linux/counter.h>
> +#include <linux/types.h>
> +
> +int counter_chrdev_add(struct counter_device *const counter,
> +		       const dev_t counter_devt);
> +void counter_chrdev_remove(struct counter_device *const counter);
> +
> +#endif /* _COUNTER_CHRDEV_H_ */
> diff --git a/drivers/counter/counter-core.c b/drivers/counter/counter-core.c
> index 78e07588717b..1334e2f4ce4e 100644
> --- a/drivers/counter/counter-core.c
> +++ b/drivers/counter/counter-core.c
> @@ -5,12 +5,16 @@
>   */
>  #include <linux/counter.h>
>  #include <linux/device.h>
> +#include <linux/device/bus.h>
>  #include <linux/export.h>
> +#include <linux/fs.h>
>  #include <linux/gfp.h>
>  #include <linux/idr.h>
>  #include <linux/init.h>
>  #include <linux/module.h>
> +#include <linux/types.h>
>  
> +#include "counter-chrdev.h"
>  #include "counter-sysfs.h"
>  
>  /* Provides a unique ID for each counter device */
> @@ -33,6 +37,8 @@ static struct bus_type counter_bus_type = {
>  	.name = "counter"
>  };
>  
> +static dev_t counter_devt;
> +
>  /**
>   * counter_register - register Counter to the system
>   * @counter:	pointer to Counter to register
> @@ -51,7 +57,6 @@ int counter_register(struct counter_device *const counter)
>  	if (counter->id < 0)
>  		return counter->id;
>  
> -	/* Configure device structure for Counter */
>  	dev->type = &counter_device_type;
>  	dev->bus = &counter_bus_type;
>  	if (counter->parent) {
> @@ -62,18 +67,25 @@ int counter_register(struct counter_device *const counter)
>  	device_initialize(dev);
>  	dev_set_drvdata(dev, counter);
>  
> +	/* Add Counter character device */
> +	err = counter_chrdev_add(counter, counter_devt);
> +	if (err)
> +		goto err_free_id;
> +
>  	/* Add Counter sysfs attributes */
>  	err = counter_sysfs_add(counter);
>  	if (err < 0)
> -		goto err_free_id;
> +		goto err_remove_chrdev;
>  
>  	/* Add device to system */
>  	err = device_add(dev);
>  	if (err < 0)
> -		goto err_free_id;
> +		goto err_remove_chrdev;
>  
>  	return 0;
>  
> +err_remove_chrdev:
> +	counter_chrdev_remove(counter);
>  err_free_id:
>  	put_device(dev);
>  	return err;
> @@ -135,13 +147,30 @@ int devm_counter_register(struct device *dev,
>  }
>  EXPORT_SYMBOL_GPL(devm_counter_register);
>  
> +#define COUNTER_DEV_MAX 256
> +
>  static int __init counter_init(void)
>  {
> -	return bus_register(&counter_bus_type);
> +	int err;
> +
> +	err = bus_register(&counter_bus_type);
> +	if (err < 0)
> +		return err;
> +
> +	err = alloc_chrdev_region(&counter_devt, 0, COUNTER_DEV_MAX, "counter");
> +	if (err < 0)
> +		goto err_unregister_bus;
> +
> +	return 0;
> +
> +err_unregister_bus:
> +	bus_unregister(&counter_bus_type);
> +	return err;
>  }
>  
>  static void __exit counter_exit(void)
>  {
> +	unregister_chrdev_region(counter_devt, COUNTER_DEV_MAX);
>  	bus_unregister(&counter_bus_type);
>  }
>  
> diff --git a/drivers/counter/counter-sysfs.c b/drivers/counter/counter-sysfs.c
> index 654afa91ae9f..4d97d6e50d48 100644
> --- a/drivers/counter/counter-sysfs.c
> +++ b/drivers/counter/counter-sysfs.c
> @@ -499,6 +499,7 @@ static ssize_t counter_comp_name_show(struct device *dev,
>  
>  static int counter_name_attr_create(struct device *const dev,
>  				    struct counter_attribute_group *const group,
> +				    const char *const attr_name,
>  				    const char *const name)
>  {
>  	struct counter_attribute *counter_attr;
> @@ -513,7 +514,7 @@ static int counter_name_attr_create(struct device *const dev,
>  
>  	/* Configure device attribute */
>  	sysfs_attr_init(&counter_attr->dev_attr.attr);
> -	counter_attr->dev_attr.attr.name = "name";
> +	counter_attr->dev_attr.attr.name = attr_name;
>  	counter_attr->dev_attr.attr.mode = 0444;
>  	counter_attr->dev_attr.show = counter_comp_name_show;
>  
> @@ -524,6 +525,18 @@ static int counter_name_attr_create(struct device *const dev,
>  	return 0;
>  }
>  
> +static int counter_ext_name_attr_create(struct device *const dev,
> +	struct counter_attribute_group *const group, const size_t i,
> +	const char *const name)
> +{
> +	const char *attr_name;
> +
> +	attr_name = devm_kasprintf(dev, GFP_KERNEL, "extension%zu_name", i);
> +	if (!attr_name)
> +		return -ENOMEM;
> +
> +	return counter_name_attr_create(dev, group, attr_name, name);
> +}
>  
>  static struct counter_comp counter_signal_comp = {
>  	.type = COUNTER_COMP_SIGNAL_LEVEL,
> @@ -539,6 +552,7 @@ static int counter_signal_attrs_create(struct counter_device *const counter,
>  	int err;
>  	struct counter_comp comp;
>  	size_t i;
> +	struct counter_comp *ext;
>  
>  	/* Create main Signal attribute */
>  	comp = counter_signal_comp;
> @@ -548,14 +562,19 @@ static int counter_signal_attrs_create(struct counter_device *const counter,
>  		return err;
>  
>  	/* Create Signal name attribute */
> -	err = counter_name_attr_create(dev, group, signal->name);
> +	err = counter_name_attr_create(dev, group, "name", signal->name);
>  	if (err < 0)
>  		return err;
>  
>  	/* Create an attribute for each extension */
>  	for (i = 0; i < signal->num_ext; i++) {
> -		err = counter_attr_create(dev, group, signal->ext + i, scope,
> -					  signal);
> +		ext = signal->ext + i;
> +
> +		err = counter_attr_create(dev, group, ext, scope, signal);
> +		if (err < 0)
> +			return err;
> +
> +		err = counter_ext_name_attr_create(dev, group, i, ext->name);
>  		if (err < 0)
>  			return err;
>  	}
> @@ -640,6 +659,7 @@ static int counter_count_attrs_create(struct counter_device *const counter,
>  	int err;
>  	struct counter_comp comp;
>  	size_t i;
> +	struct counter_comp *ext;
>  
>  	/* Create main Count attribute */
>  	comp = counter_count_comp;
> @@ -650,7 +670,7 @@ static int counter_count_attrs_create(struct counter_device *const counter,
>  		return err;
>  
>  	/* Create Count name attribute */
> -	err = counter_name_attr_create(dev, group, count->name);
> +	err = counter_name_attr_create(dev, group, "name", count->name);

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

>  	if (err < 0)
>  		return err;
>  
> @@ -664,8 +684,13 @@ static int counter_count_attrs_create(struct counter_device *const counter,
>  
>  	/* Create an attribute for each extension */
>  	for (i = 0; i < count->num_ext; i++) {
> -		err = counter_attr_create(dev, group, count->ext + i, scope,
> -					  count);
> +		ext = count->ext + i;
> +
> +		err = counter_attr_create(dev, group, ext, scope, count);
> +		if (err < 0)
> +			return err;
> +
> +		err = counter_ext_name_attr_create(dev, group, i, ext->name);
>  		if (err < 0)
>  			return err;
>  	}
> @@ -729,6 +754,7 @@ static int counter_sysfs_attr_add(struct counter_device *const counter,
>  	struct device *const dev = &counter->dev;
>  	int err;
>  	size_t i;
> +	struct counter_comp *ext;
>  
>  	/* Add Signals sysfs attributes */
>  	err = counter_sysfs_signals_add(counter, group);
> @@ -743,7 +769,7 @@ static int counter_sysfs_attr_add(struct counter_device *const counter,
>  	group += counter->num_counts;
>  
>  	/* Create name attribute */
> -	err = counter_name_attr_create(dev, group, counter->name);
> +	err = counter_name_attr_create(dev, group, "name", counter->name);
>  	if (err < 0)
>  		return err;
>  
> @@ -761,8 +787,13 @@ static int counter_sysfs_attr_add(struct counter_device *const counter,
>  
>  	/* Create an attribute for each extension */
>  	for (i = 0; i < counter->num_ext; i++) {
> -		err = counter_attr_create(dev, group, counter->ext + i, scope,
> -					  NULL);
> +		ext = counter->ext + i;
> +
> +		err = counter_attr_create(dev, group, ext, scope, NULL);
> +		if (err < 0)
> +			return err;
> +
> +		err = counter_ext_name_attr_create(dev, group, i, ext->name);
>  		if (err < 0)
>  			return err;
>  	}
> diff --git a/include/linux/counter.h b/include/linux/counter.h
> index 2f01e1fec857..ce79e62a2e58 100644
> --- a/include/linux/counter.h
> +++ b/include/linux/counter.h
> @@ -6,9 +6,15 @@
>  #ifndef _COUNTER_H_
>  #define _COUNTER_H_
>  
> +#include <linux/cdev.h>
>  #include <linux/device.h>
>  #include <linux/kernel.h>
> +#include <linux/kfifo.h>
> +#include <linux/mutex.h>
> +#include <linux/spinlock_types.h>
>  #include <linux/types.h>
> +#include <linux/wait.h>
> +#include <uapi/linux/counter.h>
>  
>  struct counter_device;
>  struct counter_count;
> @@ -27,47 +33,6 @@ enum counter_comp_type {
>  	COUNTER_COMP_COUNT_MODE,
>  };
>  
> -enum counter_scope {
> -	COUNTER_SCOPE_DEVICE,
> -	COUNTER_SCOPE_SIGNAL,
> -	COUNTER_SCOPE_COUNT,
> -};
> -
> -enum counter_count_direction {
> -	COUNTER_COUNT_DIRECTION_FORWARD,
> -	COUNTER_COUNT_DIRECTION_BACKWARD,
> -};
> -
> -enum counter_count_mode {
> -	COUNTER_COUNT_MODE_NORMAL,
> -	COUNTER_COUNT_MODE_RANGE_LIMIT,
> -	COUNTER_COUNT_MODE_NON_RECYCLE,
> -	COUNTER_COUNT_MODE_MODULO_N,
> -};
> -
> -enum counter_function {
> -	COUNTER_FUNCTION_INCREASE,
> -	COUNTER_FUNCTION_DECREASE,
> -	COUNTER_FUNCTION_PULSE_DIRECTION,
> -	COUNTER_FUNCTION_QUADRATURE_X1_A,
> -	COUNTER_FUNCTION_QUADRATURE_X1_B,
> -	COUNTER_FUNCTION_QUADRATURE_X2_A,
> -	COUNTER_FUNCTION_QUADRATURE_X2_B,
> -	COUNTER_FUNCTION_QUADRATURE_X4,
> -};
> -
> -enum counter_signal_level {
> -	COUNTER_SIGNAL_LEVEL_LOW,
> -	COUNTER_SIGNAL_LEVEL_HIGH,
> -};
> -
> -enum counter_synapse_action {
> -	COUNTER_SYNAPSE_ACTION_NONE,
> -	COUNTER_SYNAPSE_ACTION_RISING_EDGE,
> -	COUNTER_SYNAPSE_ACTION_FALLING_EDGE,
> -	COUNTER_SYNAPSE_ACTION_BOTH_EDGES,
> -};
> -
>  /**
>   * struct counter_comp - Counter component node
>   * @type:		Counter component data type
> @@ -239,6 +204,20 @@ struct counter_count {
>  	size_t num_ext;
>  };
>  
> +/**
> + * 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;
> +	struct list_head comp_list;
> +};
> +
>  /**
>   * struct counter_ops - Callbacks from driver
>   * @signal_read:	read callback for Signals. The read level of the
> @@ -261,6 +240,13 @@ struct counter_count {
>   * @action_write:	write callback for Synapse action modes. The action mode
>   *			to write for the respective Synapse is passed in via the
>   *			action parameter.
> + * @events_configure:	write callback to configure events. The list of struct
> + *			counter_event_node may be accessed via the events_list
> + *			member of the counter parameter.
> + * @watch_validate:	callback to validate a watch. The Counter component
> + *			watch configuration is passed in via the watch
> + *			parameter. A return value of 0 indicates a valid Counter
> + *			component watch configuration.
>   */
>  struct counter_ops {
>  	int (*signal_read)(struct counter_device *counter,
> @@ -284,6 +270,9 @@ struct counter_ops {
>  			    struct counter_count *count,
>  			    struct counter_synapse *synapse,
>  			    enum counter_synapse_action action);
> +	int (*events_configure)(struct counter_device *counter);
> +	int (*watch_validate)(struct counter_device *counter,
> +			      const struct counter_watch *watch);
>  };
>  
>  /**
> @@ -300,6 +289,13 @@ struct counter_ops {
>   * @priv:		optional private data supplied by driver
>   * @id:			unique ID used to identify the Counter
>   * @dev:		internal device structure
> + * @chrdev:		internal character device structure
> + * @events_list:	list of current watching Counter events
> + * @events_list_lock:	lock to protect Counter events list operations
> + * @next_events_list:	list of next watching Counter events
> + * @events:		queue of detected Counter events
> + * @events_wait:	wait queue to allow blocking reads of Counter events
> + * @events_lock:	lock to protect Counter events queue read operations
>   */
>  struct counter_device {
>  	const char *name;
> @@ -319,12 +315,21 @@ struct counter_device {
>  
>  	int id;
>  	struct device dev;
> +	struct cdev chrdev;
> +	struct list_head events_list;
> +	raw_spinlock_t events_list_lock;
> +	struct list_head next_events_list;
> +	DECLARE_KFIFO(events, struct counter_event, 64);

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

> +	wait_queue_head_t events_wait;
> +	struct mutex events_lock;
>  };
>  
>  int counter_register(struct counter_device *const counter);
>  void counter_unregister(struct counter_device *const counter);
>  int devm_counter_register(struct device *dev,
>  			  struct counter_device *const counter);
> +void counter_push_event(struct counter_device *const counter, const u8 event,
> +			const u8 channel);
>  
>  #define COUNTER_COMP_DEVICE_U8(_name, _read, _write) \
>  { \
> diff --git a/include/uapi/linux/counter.h b/include/uapi/linux/counter.h
> new file mode 100644
> index 000000000000..7585dc9db19d
> --- /dev/null
> +++ b/include/uapi/linux/counter.h
Small thing but I would have been tempted to do a precursor patch to the
main change simply putting in place the userspace header.

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

Jonathan

> @@ -0,0 +1,123 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +/*
> + * Userspace ABI for Counter character devices
> + * Copyright (C) 2020 William Breathitt Gray
> + */
> +#ifndef _UAPI_COUNTER_H_
> +#define _UAPI_COUNTER_H_
> +
> +#include <linux/ioctl.h>
> +#include <linux/types.h>
> +
> +/* Component type definitions */
> +enum counter_component_type {
> +	COUNTER_COMPONENT_NONE,
> +	COUNTER_COMPONENT_SIGNAL,
> +	COUNTER_COMPONENT_COUNT,
> +	COUNTER_COMPONENT_FUNCTION,
> +	COUNTER_COMPONENT_SYNAPSE_ACTION,
> +	COUNTER_COMPONENT_EXTENSION,
> +};
> +
> +/* Component scope definitions */
> +enum counter_scope {
> +	COUNTER_SCOPE_DEVICE,
> +	COUNTER_SCOPE_SIGNAL,
> +	COUNTER_SCOPE_COUNT,
> +};
> +
> +/**
> + * struct counter_component - Counter component identification
> + * @type: component type (Count, extension, etc.)
> + * @scope: component scope (Device, Count, or Signal)
> + * @parent: parent component identification number
> + * @id: component identification number
> + */
> +struct counter_component {
> +	__u8 type;
> +	__u8 scope;
> +	__u8 parent;
> +	__u8 id;
> +};
> +
> +/* Event type definitions */
> +enum counter_event_type {
> +	COUNTER_EVENT_OVERFLOW,
> +	COUNTER_EVENT_UNDERFLOW,
> +	COUNTER_EVENT_OVERFLOW_UNDERFLOW,
> +	COUNTER_EVENT_THRESHOLD,
> +	COUNTER_EVENT_INDEX,
> +};
> +
> +/**
> + * struct counter_watch - Counter component watch configuration
> + * @component: component to watch when event triggers
> + * @event: event that triggers
> + * @channel: event channel
> + */
> +struct counter_watch {
> +	struct counter_component component;
> +	__u8 event;
> +	__u8 channel;
> +};
> +
> +/* ioctl commands */
> +#define COUNTER_CLEAR_WATCHES_IOCTL _IO(0x3E, 0x00)
> +#define COUNTER_ADD_WATCH_IOCTL _IOW(0x3E, 0x01, struct counter_watch)
> +#define COUNTER_LOAD_WATCHES_IOCTL _IO(0x3E, 0x02)
> +
> +/**
> + * struct counter_event - Counter event data
> + * @timestamp: best estimate of time of event occurrence, in nanoseconds
> + * @value: component value
> + * @watch: component watch configuration
> + * @errno: system error number
> + */
> +struct counter_event {
> +	__aligned_u64 timestamp;
> +	__aligned_u64 value;
> +	struct counter_watch watch;
> +	__u8 errno;
> +};
> +
> +/* Count direction values */
> +enum counter_count_direction {
> +	COUNTER_COUNT_DIRECTION_FORWARD,
> +	COUNTER_COUNT_DIRECTION_BACKWARD,
> +};
> +
> +/* Count mode values */
> +enum counter_count_mode {
> +	COUNTER_COUNT_MODE_NORMAL,
> +	COUNTER_COUNT_MODE_RANGE_LIMIT,
> +	COUNTER_COUNT_MODE_NON_RECYCLE,
> +	COUNTER_COUNT_MODE_MODULO_N,
> +};
> +
> +/* Count function values */
> +enum counter_function {
> +	COUNTER_FUNCTION_INCREASE,
> +	COUNTER_FUNCTION_DECREASE,
> +	COUNTER_FUNCTION_PULSE_DIRECTION,
> +	COUNTER_FUNCTION_QUADRATURE_X1_A,
> +	COUNTER_FUNCTION_QUADRATURE_X1_B,
> +	COUNTER_FUNCTION_QUADRATURE_X2_A,
> +	COUNTER_FUNCTION_QUADRATURE_X2_B,
> +	COUNTER_FUNCTION_QUADRATURE_X4,
> +};
> +
> +/* Signal values */
> +enum counter_signal_level {
> +	COUNTER_SIGNAL_LEVEL_LOW,
> +	COUNTER_SIGNAL_LEVEL_HIGH,
> +};
> +
> +/* Action mode values */
> +enum counter_synapse_action {
> +	COUNTER_SYNAPSE_ACTION_NONE,
> +	COUNTER_SYNAPSE_ACTION_RISING_EDGE,
> +	COUNTER_SYNAPSE_ACTION_FALLING_EDGE,
> +	COUNTER_SYNAPSE_ACTION_BOTH_EDGES,
> +};
> +
> +#endif /* _UAPI_COUNTER_H_ */


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[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 <david@lechnology.com>
Cc: Gwendal Grignou <gwendal@chromium.org>
Cc: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
---
 MAINTAINERS                      |   1 +
 drivers/counter/Makefile         |   2 +-
 drivers/counter/counter-chrdev.c | 490 +++++++++++++++++++++++++++++++
 drivers/counter/counter-chrdev.h |  16 +
 drivers/counter/counter-core.c   |  37 ++-
 drivers/counter/counter-sysfs.c  |  51 +++-
 include/linux/counter.h          |  87 +++---
 include/uapi/linux/counter.h     | 123 ++++++++
 8 files changed, 751 insertions(+), 56 deletions(-)
 create mode 100644 drivers/counter/counter-chrdev.c
 create mode 100644 drivers/counter/counter-chrdev.h
 create mode 100644 include/uapi/linux/counter.h

diff --git a/MAINTAINERS b/MAINTAINERS
index b64fa49d5796..3a240f70bdd4 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4494,6 +4494,7 @@ F:	Documentation/ABI/testing/sysfs-bus-counter*
 F:	Documentation/driver-api/generic-counter.rst
 F:	drivers/counter/
 F:	include/linux/counter.h
+F:	include/uapi/linux/counter.h
 
 CPMAC ETHERNET DRIVER
 M:	Florian Fainelli <f.fainelli@gmail.com>
diff --git a/drivers/counter/Makefile b/drivers/counter/Makefile
index cbe1d06af6a9..c4870eb5b1dd 100644
--- a/drivers/counter/Makefile
+++ b/drivers/counter/Makefile
@@ -4,7 +4,7 @@
 #
 
 obj-$(CONFIG_COUNTER) += counter.o
-counter-y := counter-core.o counter-sysfs.o
+counter-y := counter-core.o counter-sysfs.o counter-chrdev.o
 
 obj-$(CONFIG_104_QUAD_8)	+= 104-quad-8.o
 obj-$(CONFIG_STM32_TIMER_CNT)	+= stm32-timer-cnt.o
diff --git a/drivers/counter/counter-chrdev.c b/drivers/counter/counter-chrdev.c
new file mode 100644
index 000000000000..61b11989546a
--- /dev/null
+++ b/drivers/counter/counter-chrdev.c
@@ -0,0 +1,490 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Generic Counter character device interface
+ * Copyright (C) 2020 William Breathitt Gray
+ */
+
+#include <linux/cdev.h>
+#include <linux/counter.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/errno.h>
+#include <linux/export.h>
+#include <linux/fs.h>
+#include <linux/kdev_t.h>
+#include <linux/kfifo.h>
+#include <linux/list.h>
+#include <linux/mutex.h>
+#include <linux/nospec.h>
+#include <linux/poll.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+#include <linux/timekeeping.h>
+#include <linux/types.h>
+#include <linux/wait.h>
+#include <linux/uaccess.h>
+
+#include "counter-chrdev.h"
+
+struct counter_comp_node {
+	struct list_head l;
+	struct counter_component component;
+	struct counter_comp comp;
+	void *parent;
+};
+
+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 int copied;
+
+	if (len < sizeof(struct counter_event))
+		return -EINVAL;
+
+	do {
+		if (kfifo_is_empty(&counter->events)) {
+			if (filp->f_flags & O_NONBLOCK)
+				return -EAGAIN;
+
+			err = wait_event_interruptible(counter->events_wait,
+					!kfifo_is_empty(&counter->events));
+			if (err < 0)
+				return err;
+		}
+
+		if (mutex_lock_interruptible(&counter->events_lock))
+			return -ERESTARTSYS;
+		err = kfifo_to_user(&counter->events, buf, len, &copied);
+		mutex_unlock(&counter->events_lock);
+		if (err < 0)
+			return err;
+	} while (!copied);
+
+	return copied;
+}
+
+static __poll_t counter_chrdev_poll(struct file *filp,
+				    struct poll_table_struct *pollt)
+{
+	struct counter_device *const counter = filp->private_data;
+	__poll_t events = 0;
+
+	poll_wait(filp, &counter->events_wait, pollt);
+
+	if (!kfifo_is_empty(&counter->events))
+		events = EPOLLIN | EPOLLRDNORM;
+
+	return events;
+}
+
+static void counter_events_list_free(struct list_head *const events_list)
+{
+	struct counter_event_node *p, *n;
+	struct counter_comp_node *q, *o;
+
+	list_for_each_entry_safe(p, n, events_list, l) {
+		/* Free associated component nodes */
+		list_for_each_entry_safe(q, o, &p->comp_list, l) {
+			list_del(&q->l);
+			kfree(q);
+		}
+
+		/* Free event node */
+		list_del(&p->l);
+		kfree(p);
+	}
+}
+
+static int counter_set_event_node(struct counter_device *const counter,
+				  struct counter_watch *const watch,
+				  const struct counter_comp_node *const cfg)
+{
+	struct counter_event_node *event_node;
+	struct counter_comp_node *comp_node;
+
+	/* Search for event in the list */
+	list_for_each_entry(event_node, &counter->next_events_list, l)
+		if (event_node->event == watch->event &&
+		    event_node->channel == watch->channel)
+			break;
+
+	/* If event is not already in the list */
+	if (&event_node->l == &counter->next_events_list) {
+		/* Allocate new event node */
+		event_node = kmalloc(sizeof(*event_node), GFP_ATOMIC);
+		if (!event_node)
+			return -ENOMEM;
+
+		/* Configure event node and add to the list */
+		event_node->event = watch->event;
+		event_node->channel = watch->channel;
+		INIT_LIST_HEAD(&event_node->comp_list);
+		list_add(&event_node->l, &counter->next_events_list);
+	}
+
+	/* Check if component watch has already been set before */
+	list_for_each_entry(comp_node, &event_node->comp_list, l)
+		if (comp_node->parent == cfg->parent &&
+		    comp_node->comp.count_u8_read == cfg->comp.count_u8_read)
+			return -EINVAL;
+
+	/* Allocate component node */
+	comp_node = kmalloc(sizeof(*comp_node), GFP_ATOMIC);
+	if (!comp_node) {
+		/* Free event node if no one else is watching */
+		if (list_empty(&event_node->comp_list)) {
+			list_del(&event_node->l);
+			kfree(event_node);
+		}
+		return -ENOMEM;
+	}
+	*comp_node = *cfg;
+
+	/* Add component node to event node */
+	list_add_tail(&comp_node->l, &event_node->comp_list);
+
+	return 0;
+}
+
+static int counter_clear_watches(struct counter_device *const counter)
+{
+	unsigned long flags;
+	int err = 0;
+
+	raw_spin_lock_irqsave(&counter->events_list_lock, flags);
+
+	counter_events_list_free(&counter->events_list);
+
+	if (counter->ops->events_configure)
+		err = counter->ops->events_configure(counter);
+
+	raw_spin_unlock_irqrestore(&counter->events_list_lock, flags);
+
+	counter_events_list_free(&counter->next_events_list);
+
+	return err;
+}
+
+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;
+	struct counter_comp_node comp_node = {0};
+	size_t parent, id;
+	struct counter_comp *ext;
+	size_t num_ext;
+	int err;
+
+	if (copy_from_user(&watch, uwatch, sizeof(watch)))
+		return -EFAULT;
+
+	if (watch.component.type == COUNTER_COMPONENT_NONE)
+		goto no_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;
+	}
+
+	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_u32_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_u32_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];
+		break;
+	default:
+		return -EINVAL;
+	}
+	/* Check if any read callback is set; this is part of a union */
+	if (!comp_node.comp.count_u8_read)
+		return -EOPNOTSUPP;
+
+no_component:
+	if (counter->ops->watch_validate) {
+		err = counter->ops->watch_validate(counter, &watch);
+		if (err < 0)
+			return err;
+	}
+
+	comp_node.component = watch.component;
+
+	return counter_set_event_node(counter, &watch, &comp_node);
+}
+
+static long counter_chrdev_ioctl(struct file *filp, unsigned int cmd,
+				 unsigned long arg)
+{
+	struct counter_device *const counter = filp->private_data;
+	unsigned long flags;
+	int err = 0;
+
+	switch (cmd) {
+	case COUNTER_CLEAR_WATCHES_IOCTL:
+		return counter_clear_watches(counter);
+	case COUNTER_ADD_WATCH_IOCTL:
+		return counter_add_watch(counter, arg);
+	case COUNTER_LOAD_WATCHES_IOCTL:
+		raw_spin_lock_irqsave(&counter->events_list_lock, flags);
+
+		counter_events_list_free(&counter->events_list);
+		list_replace_init(&counter->next_events_list,
+				  &counter->events_list);
+
+		if (counter->ops->events_configure)
+			err = counter->ops->events_configure(counter);
+
+		raw_spin_unlock_irqrestore(&counter->events_list_lock, flags);
+		break;
+	default:
+		return -ENOIOCTLCMD;
+	}
+
+	return err;
+}
+
+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;
+
+	return nonseekable_open(inode, filp);
+}
+
+static int counter_chrdev_release(struct inode *inode, struct file *filp)
+{
+	struct counter_device *const counter = filp->private_data;
+	int err;
+
+	err = counter_clear_watches(counter);
+	if (err < 0)
+		return err;
+
+	put_device(&counter->dev);
+
+	return 0;
+}
+
+static const struct file_operations counter_fops = {
+	.llseek = no_llseek,
+	.read = counter_chrdev_read,
+	.poll = counter_chrdev_poll,
+	.unlocked_ioctl = counter_chrdev_ioctl,
+	.open = counter_chrdev_open,
+	.release = counter_chrdev_release,
+};
+
+int counter_chrdev_add(struct counter_device *const counter,
+		       const dev_t counter_devt)
+{
+	struct device *const dev = &counter->dev;
+	struct cdev *const chrdev = &counter->chrdev;
+
+	/* Initialize Counter events lists */
+	INIT_LIST_HEAD(&counter->events_list);
+	INIT_LIST_HEAD(&counter->next_events_list);
+	raw_spin_lock_init(&counter->events_list_lock);
+
+	/* Initialize Counter events queue */
+	INIT_KFIFO(counter->events);
+	init_waitqueue_head(&counter->events_wait);
+	mutex_init(&counter->events_lock);
+
+	/* Initialize character device */
+	cdev_init(chrdev, &counter_fops);
+	dev->devt = MKDEV(MAJOR(counter_devt), counter->id);
+	cdev_set_parent(chrdev, &dev->kobj);
+
+	return cdev_add(chrdev, dev->devt, 1);
+}
+
+void counter_chrdev_remove(struct counter_device *const counter)
+{
+	cdev_del(&counter->chrdev);
+}
+
+static int counter_get_data(struct counter_device *const counter,
+			    const struct counter_comp_node *const comp_node,
+			    u64 *const value)
+{
+	const struct counter_comp *const comp = &comp_node->comp;
+	void *const parent = comp_node->parent;
+	int err = 0;
+	u8 value_u8 = 0;
+	u32 value_u32 = 0;
+
+	if (comp_node->component.type == COUNTER_COMPONENT_NONE)
+		return 0;
+
+	switch (comp->type) {
+	case COUNTER_COMP_U8:
+	case COUNTER_COMP_BOOL:
+		switch (comp_node->component.scope) {
+		case COUNTER_SCOPE_DEVICE:
+			err = comp->device_u8_read(counter, &value_u8);
+			break;
+		case COUNTER_SCOPE_SIGNAL:
+			err = comp->signal_u8_read(counter, parent, &value_u8);
+			break;
+		case COUNTER_SCOPE_COUNT:
+			err = comp->count_u8_read(counter, parent, &value_u8);
+			break;
+		}
+		*value = value_u8;
+		break;
+	case COUNTER_COMP_SIGNAL_LEVEL:
+	case COUNTER_COMP_FUNCTION:
+	case COUNTER_COMP_ENUM:
+	case COUNTER_COMP_COUNT_DIRECTION:
+	case COUNTER_COMP_COUNT_MODE:
+		switch (comp_node->component.scope) {
+		case COUNTER_SCOPE_DEVICE:
+			err = comp->device_u32_read(counter, &value_u32);
+			break;
+		case COUNTER_SCOPE_SIGNAL:
+			err = comp->signal_u32_read(counter, parent,
+						    &value_u32);
+			break;
+		case COUNTER_SCOPE_COUNT:
+			err = comp->count_u32_read(counter, parent, &value_u32);
+			break;
+		}
+		*value = value_u32;
+		break;
+	case COUNTER_COMP_U64:
+		switch (comp_node->component.scope) {
+		case COUNTER_SCOPE_DEVICE:
+			return comp->device_u64_read(counter, value);
+		case COUNTER_SCOPE_SIGNAL:
+			return comp->signal_u64_read(counter, parent, value);
+		case COUNTER_SCOPE_COUNT:
+			return comp->count_u64_read(counter, parent, value);
+		}
+		break;
+	case COUNTER_COMP_SYNAPSE_ACTION:
+		err = comp->action_read(counter, parent, comp->priv,
+					&value_u32);
+		*value = value_u32;
+		break;
+	}
+
+	return err;
+}
+
+/**
+ * counter_push_event - queue event for userspace reading
+ * @counter:	pointer to Counter structure
+ * @event:	triggered event
+ * @channel:	event channel
+ *
+ * Note: If no one is watching for the respective event, it is silently
+ * discarded.
+ */
+void counter_push_event(struct counter_device *const counter, const u8 event,
+			const u8 channel)
+{
+	struct counter_event ev = {0};
+	unsigned int copied = 0;
+	unsigned long flags;
+	struct counter_event_node *event_node;
+	struct counter_comp_node *comp_node;
+
+	ev.timestamp = ktime_get_ns();
+	ev.watch.event = event;
+	ev.watch.channel = channel;
+
+	raw_spin_lock_irqsave(&counter->events_list_lock, flags);
+
+	/* Search for event in the list */
+	list_for_each_entry(event_node, &counter->events_list, l)
+		if (event_node->event == event &&
+		    event_node->channel == channel)
+			break;
+
+	/* If event is not in the list */
+	if (&event_node->l == &counter->events_list)
+		goto exit_early;
+
+	/* Read and queue relevant comp for userspace */
+	list_for_each_entry(comp_node, &event_node->comp_list, l) {
+		ev.watch.component = comp_node->component;
+		ev.errno = -counter_get_data(counter, comp_node, &ev.value);
+
+		copied += kfifo_put(&counter->events, ev);
+	}
+
+	if (copied)
+		wake_up_poll(&counter->events_wait, EPOLLIN);
+
+exit_early:
+	raw_spin_unlock_irqrestore(&counter->events_list_lock, flags);
+}
+EXPORT_SYMBOL_GPL(counter_push_event);
diff --git a/drivers/counter/counter-chrdev.h b/drivers/counter/counter-chrdev.h
new file mode 100644
index 000000000000..cf5a318fe540
--- /dev/null
+++ b/drivers/counter/counter-chrdev.h
@@ -0,0 +1,16 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Counter character device interface
+ * Copyright (C) 2020 William Breathitt Gray
+ */
+#ifndef _COUNTER_CHRDEV_H_
+#define _COUNTER_CHRDEV_H_
+
+#include <linux/counter.h>
+#include <linux/types.h>
+
+int counter_chrdev_add(struct counter_device *const counter,
+		       const dev_t counter_devt);
+void counter_chrdev_remove(struct counter_device *const counter);
+
+#endif /* _COUNTER_CHRDEV_H_ */
diff --git a/drivers/counter/counter-core.c b/drivers/counter/counter-core.c
index 78e07588717b..1334e2f4ce4e 100644
--- a/drivers/counter/counter-core.c
+++ b/drivers/counter/counter-core.c
@@ -5,12 +5,16 @@
  */
 #include <linux/counter.h>
 #include <linux/device.h>
+#include <linux/device/bus.h>
 #include <linux/export.h>
+#include <linux/fs.h>
 #include <linux/gfp.h>
 #include <linux/idr.h>
 #include <linux/init.h>
 #include <linux/module.h>
+#include <linux/types.h>
 
+#include "counter-chrdev.h"
 #include "counter-sysfs.h"
 
 /* Provides a unique ID for each counter device */
@@ -33,6 +37,8 @@ static struct bus_type counter_bus_type = {
 	.name = "counter"
 };
 
+static dev_t counter_devt;
+
 /**
  * counter_register - register Counter to the system
  * @counter:	pointer to Counter to register
@@ -51,7 +57,6 @@ int counter_register(struct counter_device *const counter)
 	if (counter->id < 0)
 		return counter->id;
 
-	/* Configure device structure for Counter */
 	dev->type = &counter_device_type;
 	dev->bus = &counter_bus_type;
 	if (counter->parent) {
@@ -62,18 +67,25 @@ int counter_register(struct counter_device *const counter)
 	device_initialize(dev);
 	dev_set_drvdata(dev, counter);
 
+	/* Add Counter character device */
+	err = counter_chrdev_add(counter, counter_devt);
+	if (err)
+		goto err_free_id;
+
 	/* Add Counter sysfs attributes */
 	err = counter_sysfs_add(counter);
 	if (err < 0)
-		goto err_free_id;
+		goto err_remove_chrdev;
 
 	/* Add device to system */
 	err = device_add(dev);
 	if (err < 0)
-		goto err_free_id;
+		goto err_remove_chrdev;
 
 	return 0;
 
+err_remove_chrdev:
+	counter_chrdev_remove(counter);
 err_free_id:
 	put_device(dev);
 	return err;
@@ -135,13 +147,30 @@ int devm_counter_register(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(devm_counter_register);
 
+#define COUNTER_DEV_MAX 256
+
 static int __init counter_init(void)
 {
-	return bus_register(&counter_bus_type);
+	int err;
+
+	err = bus_register(&counter_bus_type);
+	if (err < 0)
+		return err;
+
+	err = alloc_chrdev_region(&counter_devt, 0, COUNTER_DEV_MAX, "counter");
+	if (err < 0)
+		goto err_unregister_bus;
+
+	return 0;
+
+err_unregister_bus:
+	bus_unregister(&counter_bus_type);
+	return err;
 }
 
 static void __exit counter_exit(void)
 {
+	unregister_chrdev_region(counter_devt, COUNTER_DEV_MAX);
 	bus_unregister(&counter_bus_type);
 }
 
diff --git a/drivers/counter/counter-sysfs.c b/drivers/counter/counter-sysfs.c
index 654afa91ae9f..4d97d6e50d48 100644
--- a/drivers/counter/counter-sysfs.c
+++ b/drivers/counter/counter-sysfs.c
@@ -499,6 +499,7 @@ static ssize_t counter_comp_name_show(struct device *dev,
 
 static int counter_name_attr_create(struct device *const dev,
 				    struct counter_attribute_group *const group,
+				    const char *const attr_name,
 				    const char *const name)
 {
 	struct counter_attribute *counter_attr;
@@ -513,7 +514,7 @@ static int counter_name_attr_create(struct device *const dev,
 
 	/* Configure device attribute */
 	sysfs_attr_init(&counter_attr->dev_attr.attr);
-	counter_attr->dev_attr.attr.name = "name";
+	counter_attr->dev_attr.attr.name = attr_name;
 	counter_attr->dev_attr.attr.mode = 0444;
 	counter_attr->dev_attr.show = counter_comp_name_show;
 
@@ -524,6 +525,18 @@ static int counter_name_attr_create(struct device *const dev,
 	return 0;
 }
 
+static int counter_ext_name_attr_create(struct device *const dev,
+	struct counter_attribute_group *const group, const size_t i,
+	const char *const name)
+{
+	const char *attr_name;
+
+	attr_name = devm_kasprintf(dev, GFP_KERNEL, "extension%zu_name", i);
+	if (!attr_name)
+		return -ENOMEM;
+
+	return counter_name_attr_create(dev, group, attr_name, name);
+}
 
 static struct counter_comp counter_signal_comp = {
 	.type = COUNTER_COMP_SIGNAL_LEVEL,
@@ -539,6 +552,7 @@ static int counter_signal_attrs_create(struct counter_device *const counter,
 	int err;
 	struct counter_comp comp;
 	size_t i;
+	struct counter_comp *ext;
 
 	/* Create main Signal attribute */
 	comp = counter_signal_comp;
@@ -548,14 +562,19 @@ static int counter_signal_attrs_create(struct counter_device *const counter,
 		return err;
 
 	/* Create Signal name attribute */
-	err = counter_name_attr_create(dev, group, signal->name);
+	err = counter_name_attr_create(dev, group, "name", signal->name);
 	if (err < 0)
 		return err;
 
 	/* Create an attribute for each extension */
 	for (i = 0; i < signal->num_ext; i++) {
-		err = counter_attr_create(dev, group, signal->ext + i, scope,
-					  signal);
+		ext = signal->ext + i;
+
+		err = counter_attr_create(dev, group, ext, scope, signal);
+		if (err < 0)
+			return err;
+
+		err = counter_ext_name_attr_create(dev, group, i, ext->name);
 		if (err < 0)
 			return err;
 	}
@@ -640,6 +659,7 @@ static int counter_count_attrs_create(struct counter_device *const counter,
 	int err;
 	struct counter_comp comp;
 	size_t i;
+	struct counter_comp *ext;
 
 	/* Create main Count attribute */
 	comp = counter_count_comp;
@@ -650,7 +670,7 @@ static int counter_count_attrs_create(struct counter_device *const counter,
 		return err;
 
 	/* Create Count name attribute */
-	err = counter_name_attr_create(dev, group, count->name);
+	err = counter_name_attr_create(dev, group, "name", count->name);
 	if (err < 0)
 		return err;
 
@@ -664,8 +684,13 @@ static int counter_count_attrs_create(struct counter_device *const counter,
 
 	/* Create an attribute for each extension */
 	for (i = 0; i < count->num_ext; i++) {
-		err = counter_attr_create(dev, group, count->ext + i, scope,
-					  count);
+		ext = count->ext + i;
+
+		err = counter_attr_create(dev, group, ext, scope, count);
+		if (err < 0)
+			return err;
+
+		err = counter_ext_name_attr_create(dev, group, i, ext->name);
 		if (err < 0)
 			return err;
 	}
@@ -729,6 +754,7 @@ static int counter_sysfs_attr_add(struct counter_device *const counter,
 	struct device *const dev = &counter->dev;
 	int err;
 	size_t i;
+	struct counter_comp *ext;
 
 	/* Add Signals sysfs attributes */
 	err = counter_sysfs_signals_add(counter, group);
@@ -743,7 +769,7 @@ static int counter_sysfs_attr_add(struct counter_device *const counter,
 	group += counter->num_counts;
 
 	/* Create name attribute */
-	err = counter_name_attr_create(dev, group, counter->name);
+	err = counter_name_attr_create(dev, group, "name", counter->name);
 	if (err < 0)
 		return err;
 
@@ -761,8 +787,13 @@ static int counter_sysfs_attr_add(struct counter_device *const counter,
 
 	/* Create an attribute for each extension */
 	for (i = 0; i < counter->num_ext; i++) {
-		err = counter_attr_create(dev, group, counter->ext + i, scope,
-					  NULL);
+		ext = counter->ext + i;
+
+		err = counter_attr_create(dev, group, ext, scope, NULL);
+		if (err < 0)
+			return err;
+
+		err = counter_ext_name_attr_create(dev, group, i, ext->name);
 		if (err < 0)
 			return err;
 	}
diff --git a/include/linux/counter.h b/include/linux/counter.h
index 2f01e1fec857..ce79e62a2e58 100644
--- a/include/linux/counter.h
+++ b/include/linux/counter.h
@@ -6,9 +6,15 @@
 #ifndef _COUNTER_H_
 #define _COUNTER_H_
 
+#include <linux/cdev.h>
 #include <linux/device.h>
 #include <linux/kernel.h>
+#include <linux/kfifo.h>
+#include <linux/mutex.h>
+#include <linux/spinlock_types.h>
 #include <linux/types.h>
+#include <linux/wait.h>
+#include <uapi/linux/counter.h>
 
 struct counter_device;
 struct counter_count;
@@ -27,47 +33,6 @@ enum counter_comp_type {
 	COUNTER_COMP_COUNT_MODE,
 };
 
-enum counter_scope {
-	COUNTER_SCOPE_DEVICE,
-	COUNTER_SCOPE_SIGNAL,
-	COUNTER_SCOPE_COUNT,
-};
-
-enum counter_count_direction {
-	COUNTER_COUNT_DIRECTION_FORWARD,
-	COUNTER_COUNT_DIRECTION_BACKWARD,
-};
-
-enum counter_count_mode {
-	COUNTER_COUNT_MODE_NORMAL,
-	COUNTER_COUNT_MODE_RANGE_LIMIT,
-	COUNTER_COUNT_MODE_NON_RECYCLE,
-	COUNTER_COUNT_MODE_MODULO_N,
-};
-
-enum counter_function {
-	COUNTER_FUNCTION_INCREASE,
-	COUNTER_FUNCTION_DECREASE,
-	COUNTER_FUNCTION_PULSE_DIRECTION,
-	COUNTER_FUNCTION_QUADRATURE_X1_A,
-	COUNTER_FUNCTION_QUADRATURE_X1_B,
-	COUNTER_FUNCTION_QUADRATURE_X2_A,
-	COUNTER_FUNCTION_QUADRATURE_X2_B,
-	COUNTER_FUNCTION_QUADRATURE_X4,
-};
-
-enum counter_signal_level {
-	COUNTER_SIGNAL_LEVEL_LOW,
-	COUNTER_SIGNAL_LEVEL_HIGH,
-};
-
-enum counter_synapse_action {
-	COUNTER_SYNAPSE_ACTION_NONE,
-	COUNTER_SYNAPSE_ACTION_RISING_EDGE,
-	COUNTER_SYNAPSE_ACTION_FALLING_EDGE,
-	COUNTER_SYNAPSE_ACTION_BOTH_EDGES,
-};
-
 /**
  * struct counter_comp - Counter component node
  * @type:		Counter component data type
@@ -239,6 +204,20 @@ struct counter_count {
 	size_t num_ext;
 };
 
+/**
+ * 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;
+	struct list_head comp_list;
+};
+
 /**
  * struct counter_ops - Callbacks from driver
  * @signal_read:	read callback for Signals. The read level of the
@@ -261,6 +240,13 @@ struct counter_count {
  * @action_write:	write callback for Synapse action modes. The action mode
  *			to write for the respective Synapse is passed in via the
  *			action parameter.
+ * @events_configure:	write callback to configure events. The list of struct
+ *			counter_event_node may be accessed via the events_list
+ *			member of the counter parameter.
+ * @watch_validate:	callback to validate a watch. The Counter component
+ *			watch configuration is passed in via the watch
+ *			parameter. A return value of 0 indicates a valid Counter
+ *			component watch configuration.
  */
 struct counter_ops {
 	int (*signal_read)(struct counter_device *counter,
@@ -284,6 +270,9 @@ struct counter_ops {
 			    struct counter_count *count,
 			    struct counter_synapse *synapse,
 			    enum counter_synapse_action action);
+	int (*events_configure)(struct counter_device *counter);
+	int (*watch_validate)(struct counter_device *counter,
+			      const struct counter_watch *watch);
 };
 
 /**
@@ -300,6 +289,13 @@ struct counter_ops {
  * @priv:		optional private data supplied by driver
  * @id:			unique ID used to identify the Counter
  * @dev:		internal device structure
+ * @chrdev:		internal character device structure
+ * @events_list:	list of current watching Counter events
+ * @events_list_lock:	lock to protect Counter events list operations
+ * @next_events_list:	list of next watching Counter events
+ * @events:		queue of detected Counter events
+ * @events_wait:	wait queue to allow blocking reads of Counter events
+ * @events_lock:	lock to protect Counter events queue read operations
  */
 struct counter_device {
 	const char *name;
@@ -319,12 +315,21 @@ struct counter_device {
 
 	int id;
 	struct device dev;
+	struct cdev chrdev;
+	struct list_head events_list;
+	raw_spinlock_t events_list_lock;
+	struct list_head next_events_list;
+	DECLARE_KFIFO(events, struct counter_event, 64);
+	wait_queue_head_t events_wait;
+	struct mutex events_lock;
 };
 
 int counter_register(struct counter_device *const counter);
 void counter_unregister(struct counter_device *const counter);
 int devm_counter_register(struct device *dev,
 			  struct counter_device *const counter);
+void counter_push_event(struct counter_device *const counter, const u8 event,
+			const u8 channel);
 
 #define COUNTER_COMP_DEVICE_U8(_name, _read, _write) \
 { \
diff --git a/include/uapi/linux/counter.h b/include/uapi/linux/counter.h
new file mode 100644
index 000000000000..7585dc9db19d
--- /dev/null
+++ b/include/uapi/linux/counter.h
@@ -0,0 +1,123 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+/*
+ * Userspace ABI for Counter character devices
+ * Copyright (C) 2020 William Breathitt Gray
+ */
+#ifndef _UAPI_COUNTER_H_
+#define _UAPI_COUNTER_H_
+
+#include <linux/ioctl.h>
+#include <linux/types.h>
+
+/* Component type definitions */
+enum counter_component_type {
+	COUNTER_COMPONENT_NONE,
+	COUNTER_COMPONENT_SIGNAL,
+	COUNTER_COMPONENT_COUNT,
+	COUNTER_COMPONENT_FUNCTION,
+	COUNTER_COMPONENT_SYNAPSE_ACTION,
+	COUNTER_COMPONENT_EXTENSION,
+};
+
+/* Component scope definitions */
+enum counter_scope {
+	COUNTER_SCOPE_DEVICE,
+	COUNTER_SCOPE_SIGNAL,
+	COUNTER_SCOPE_COUNT,
+};
+
+/**
+ * struct counter_component - Counter component identification
+ * @type: component type (Count, extension, etc.)
+ * @scope: component scope (Device, Count, or Signal)
+ * @parent: parent component identification number
+ * @id: component identification number
+ */
+struct counter_component {
+	__u8 type;
+	__u8 scope;
+	__u8 parent;
+	__u8 id;
+};
+
+/* Event type definitions */
+enum counter_event_type {
+	COUNTER_EVENT_OVERFLOW,
+	COUNTER_EVENT_UNDERFLOW,
+	COUNTER_EVENT_OVERFLOW_UNDERFLOW,
+	COUNTER_EVENT_THRESHOLD,
+	COUNTER_EVENT_INDEX,
+};
+
+/**
+ * struct counter_watch - Counter component watch configuration
+ * @component: component to watch when event triggers
+ * @event: event that triggers
+ * @channel: event channel
+ */
+struct counter_watch {
+	struct counter_component component;
+	__u8 event;
+	__u8 channel;
+};
+
+/* ioctl commands */
+#define COUNTER_CLEAR_WATCHES_IOCTL _IO(0x3E, 0x00)
+#define COUNTER_ADD_WATCH_IOCTL _IOW(0x3E, 0x01, struct counter_watch)
+#define COUNTER_LOAD_WATCHES_IOCTL _IO(0x3E, 0x02)
+
+/**
+ * struct counter_event - Counter event data
+ * @timestamp: best estimate of time of event occurrence, in nanoseconds
+ * @value: component value
+ * @watch: component watch configuration
+ * @errno: system error number
+ */
+struct counter_event {
+	__aligned_u64 timestamp;
+	__aligned_u64 value;
+	struct counter_watch watch;
+	__u8 errno;
+};
+
+/* Count direction values */
+enum counter_count_direction {
+	COUNTER_COUNT_DIRECTION_FORWARD,
+	COUNTER_COUNT_DIRECTION_BACKWARD,
+};
+
+/* Count mode values */
+enum counter_count_mode {
+	COUNTER_COUNT_MODE_NORMAL,
+	COUNTER_COUNT_MODE_RANGE_LIMIT,
+	COUNTER_COUNT_MODE_NON_RECYCLE,
+	COUNTER_COUNT_MODE_MODULO_N,
+};
+
+/* Count function values */
+enum counter_function {
+	COUNTER_FUNCTION_INCREASE,
+	COUNTER_FUNCTION_DECREASE,
+	COUNTER_FUNCTION_PULSE_DIRECTION,
+	COUNTER_FUNCTION_QUADRATURE_X1_A,
+	COUNTER_FUNCTION_QUADRATURE_X1_B,
+	COUNTER_FUNCTION_QUADRATURE_X2_A,
+	COUNTER_FUNCTION_QUADRATURE_X2_B,
+	COUNTER_FUNCTION_QUADRATURE_X4,
+};
+
+/* Signal values */
+enum counter_signal_level {
+	COUNTER_SIGNAL_LEVEL_LOW,
+	COUNTER_SIGNAL_LEVEL_HIGH,
+};
+
+/* Action mode values */
+enum counter_synapse_action {
+	COUNTER_SYNAPSE_ACTION_NONE,
+	COUNTER_SYNAPSE_ACTION_RISING_EDGE,
+	COUNTER_SYNAPSE_ACTION_FALLING_EDGE,
+	COUNTER_SYNAPSE_ACTION_BOTH_EDGES,
+};
+
+#endif /* _UAPI_COUNTER_H_ */
-- 
2.29.2

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

[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 <david@lechnology.com>
Cc: Gwendal Grignou <gwendal@chromium.org>
Cc: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
---
 MAINTAINERS                      |   1 +
 drivers/counter/Makefile         |   2 +-
 drivers/counter/counter-chrdev.c | 490 +++++++++++++++++++++++++++++++
 drivers/counter/counter-chrdev.h |  16 +
 drivers/counter/counter-core.c   |  37 ++-
 drivers/counter/counter-sysfs.c  |  51 +++-
 include/linux/counter.h          |  87 +++---
 include/uapi/linux/counter.h     | 123 ++++++++
 8 files changed, 751 insertions(+), 56 deletions(-)
 create mode 100644 drivers/counter/counter-chrdev.c
 create mode 100644 drivers/counter/counter-chrdev.h
 create mode 100644 include/uapi/linux/counter.h

diff --git a/MAINTAINERS b/MAINTAINERS
index b64fa49d5796..3a240f70bdd4 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4494,6 +4494,7 @@ F:	Documentation/ABI/testing/sysfs-bus-counter*
 F:	Documentation/driver-api/generic-counter.rst
 F:	drivers/counter/
 F:	include/linux/counter.h
+F:	include/uapi/linux/counter.h
 
 CPMAC ETHERNET DRIVER
 M:	Florian Fainelli <f.fainelli@gmail.com>
diff --git a/drivers/counter/Makefile b/drivers/counter/Makefile
index cbe1d06af6a9..c4870eb5b1dd 100644
--- a/drivers/counter/Makefile
+++ b/drivers/counter/Makefile
@@ -4,7 +4,7 @@
 #
 
 obj-$(CONFIG_COUNTER) += counter.o
-counter-y := counter-core.o counter-sysfs.o
+counter-y := counter-core.o counter-sysfs.o counter-chrdev.o
 
 obj-$(CONFIG_104_QUAD_8)	+= 104-quad-8.o
 obj-$(CONFIG_STM32_TIMER_CNT)	+= stm32-timer-cnt.o
diff --git a/drivers/counter/counter-chrdev.c b/drivers/counter/counter-chrdev.c
new file mode 100644
index 000000000000..61b11989546a
--- /dev/null
+++ b/drivers/counter/counter-chrdev.c
@@ -0,0 +1,490 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Generic Counter character device interface
+ * Copyright (C) 2020 William Breathitt Gray
+ */
+
+#include <linux/cdev.h>
+#include <linux/counter.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/errno.h>
+#include <linux/export.h>
+#include <linux/fs.h>
+#include <linux/kdev_t.h>
+#include <linux/kfifo.h>
+#include <linux/list.h>
+#include <linux/mutex.h>
+#include <linux/nospec.h>
+#include <linux/poll.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+#include <linux/timekeeping.h>
+#include <linux/types.h>
+#include <linux/wait.h>
+#include <linux/uaccess.h>
+
+#include "counter-chrdev.h"
+
+struct counter_comp_node {
+	struct list_head l;
+	struct counter_component component;
+	struct counter_comp comp;
+	void *parent;
+};
+
+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 int copied;
+
+	if (len < sizeof(struct counter_event))
+		return -EINVAL;
+
+	do {
+		if (kfifo_is_empty(&counter->events)) {
+			if (filp->f_flags & O_NONBLOCK)
+				return -EAGAIN;
+
+			err = wait_event_interruptible(counter->events_wait,
+					!kfifo_is_empty(&counter->events));
+			if (err < 0)
+				return err;
+		}
+
+		if (mutex_lock_interruptible(&counter->events_lock))
+			return -ERESTARTSYS;
+		err = kfifo_to_user(&counter->events, buf, len, &copied);
+		mutex_unlock(&counter->events_lock);
+		if (err < 0)
+			return err;
+	} while (!copied);
+
+	return copied;
+}
+
+static __poll_t counter_chrdev_poll(struct file *filp,
+				    struct poll_table_struct *pollt)
+{
+	struct counter_device *const counter = filp->private_data;
+	__poll_t events = 0;
+
+	poll_wait(filp, &counter->events_wait, pollt);
+
+	if (!kfifo_is_empty(&counter->events))
+		events = EPOLLIN | EPOLLRDNORM;
+
+	return events;
+}
+
+static void counter_events_list_free(struct list_head *const events_list)
+{
+	struct counter_event_node *p, *n;
+	struct counter_comp_node *q, *o;
+
+	list_for_each_entry_safe(p, n, events_list, l) {
+		/* Free associated component nodes */
+		list_for_each_entry_safe(q, o, &p->comp_list, l) {
+			list_del(&q->l);
+			kfree(q);
+		}
+
+		/* Free event node */
+		list_del(&p->l);
+		kfree(p);
+	}
+}
+
+static int counter_set_event_node(struct counter_device *const counter,
+				  struct counter_watch *const watch,
+				  const struct counter_comp_node *const cfg)
+{
+	struct counter_event_node *event_node;
+	struct counter_comp_node *comp_node;
+
+	/* Search for event in the list */
+	list_for_each_entry(event_node, &counter->next_events_list, l)
+		if (event_node->event == watch->event &&
+		    event_node->channel == watch->channel)
+			break;
+
+	/* If event is not already in the list */
+	if (&event_node->l == &counter->next_events_list) {
+		/* Allocate new event node */
+		event_node = kmalloc(sizeof(*event_node), GFP_ATOMIC);
+		if (!event_node)
+			return -ENOMEM;
+
+		/* Configure event node and add to the list */
+		event_node->event = watch->event;
+		event_node->channel = watch->channel;
+		INIT_LIST_HEAD(&event_node->comp_list);
+		list_add(&event_node->l, &counter->next_events_list);
+	}
+
+	/* Check if component watch has already been set before */
+	list_for_each_entry(comp_node, &event_node->comp_list, l)
+		if (comp_node->parent == cfg->parent &&
+		    comp_node->comp.count_u8_read == cfg->comp.count_u8_read)
+			return -EINVAL;
+
+	/* Allocate component node */
+	comp_node = kmalloc(sizeof(*comp_node), GFP_ATOMIC);
+	if (!comp_node) {
+		/* Free event node if no one else is watching */
+		if (list_empty(&event_node->comp_list)) {
+			list_del(&event_node->l);
+			kfree(event_node);
+		}
+		return -ENOMEM;
+	}
+	*comp_node = *cfg;
+
+	/* Add component node to event node */
+	list_add_tail(&comp_node->l, &event_node->comp_list);
+
+	return 0;
+}
+
+static int counter_clear_watches(struct counter_device *const counter)
+{
+	unsigned long flags;
+	int err = 0;
+
+	raw_spin_lock_irqsave(&counter->events_list_lock, flags);
+
+	counter_events_list_free(&counter->events_list);
+
+	if (counter->ops->events_configure)
+		err = counter->ops->events_configure(counter);
+
+	raw_spin_unlock_irqrestore(&counter->events_list_lock, flags);
+
+	counter_events_list_free(&counter->next_events_list);
+
+	return err;
+}
+
+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;
+	struct counter_comp_node comp_node = {0};
+	size_t parent, id;
+	struct counter_comp *ext;
+	size_t num_ext;
+	int err;
+
+	if (copy_from_user(&watch, uwatch, sizeof(watch)))
+		return -EFAULT;
+
+	if (watch.component.type == COUNTER_COMPONENT_NONE)
+		goto no_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;
+	}
+
+	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_u32_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_u32_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];
+		break;
+	default:
+		return -EINVAL;
+	}
+	/* Check if any read callback is set; this is part of a union */
+	if (!comp_node.comp.count_u8_read)
+		return -EOPNOTSUPP;
+
+no_component:
+	if (counter->ops->watch_validate) {
+		err = counter->ops->watch_validate(counter, &watch);
+		if (err < 0)
+			return err;
+	}
+
+	comp_node.component = watch.component;
+
+	return counter_set_event_node(counter, &watch, &comp_node);
+}
+
+static long counter_chrdev_ioctl(struct file *filp, unsigned int cmd,
+				 unsigned long arg)
+{
+	struct counter_device *const counter = filp->private_data;
+	unsigned long flags;
+	int err = 0;
+
+	switch (cmd) {
+	case COUNTER_CLEAR_WATCHES_IOCTL:
+		return counter_clear_watches(counter);
+	case COUNTER_ADD_WATCH_IOCTL:
+		return counter_add_watch(counter, arg);
+	case COUNTER_LOAD_WATCHES_IOCTL:
+		raw_spin_lock_irqsave(&counter->events_list_lock, flags);
+
+		counter_events_list_free(&counter->events_list);
+		list_replace_init(&counter->next_events_list,
+				  &counter->events_list);
+
+		if (counter->ops->events_configure)
+			err = counter->ops->events_configure(counter);
+
+		raw_spin_unlock_irqrestore(&counter->events_list_lock, flags);
+		break;
+	default:
+		return -ENOIOCTLCMD;
+	}
+
+	return err;
+}
+
+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;
+
+	return nonseekable_open(inode, filp);
+}
+
+static int counter_chrdev_release(struct inode *inode, struct file *filp)
+{
+	struct counter_device *const counter = filp->private_data;
+	int err;
+
+	err = counter_clear_watches(counter);
+	if (err < 0)
+		return err;
+
+	put_device(&counter->dev);
+
+	return 0;
+}
+
+static const struct file_operations counter_fops = {
+	.llseek = no_llseek,
+	.read = counter_chrdev_read,
+	.poll = counter_chrdev_poll,
+	.unlocked_ioctl = counter_chrdev_ioctl,
+	.open = counter_chrdev_open,
+	.release = counter_chrdev_release,
+};
+
+int counter_chrdev_add(struct counter_device *const counter,
+		       const dev_t counter_devt)
+{
+	struct device *const dev = &counter->dev;
+	struct cdev *const chrdev = &counter->chrdev;
+
+	/* Initialize Counter events lists */
+	INIT_LIST_HEAD(&counter->events_list);
+	INIT_LIST_HEAD(&counter->next_events_list);
+	raw_spin_lock_init(&counter->events_list_lock);
+
+	/* Initialize Counter events queue */
+	INIT_KFIFO(counter->events);
+	init_waitqueue_head(&counter->events_wait);
+	mutex_init(&counter->events_lock);
+
+	/* Initialize character device */
+	cdev_init(chrdev, &counter_fops);
+	dev->devt = MKDEV(MAJOR(counter_devt), counter->id);
+	cdev_set_parent(chrdev, &dev->kobj);
+
+	return cdev_add(chrdev, dev->devt, 1);
+}
+
+void counter_chrdev_remove(struct counter_device *const counter)
+{
+	cdev_del(&counter->chrdev);
+}
+
+static int counter_get_data(struct counter_device *const counter,
+			    const struct counter_comp_node *const comp_node,
+			    u64 *const value)
+{
+	const struct counter_comp *const comp = &comp_node->comp;
+	void *const parent = comp_node->parent;
+	int err = 0;
+	u8 value_u8 = 0;
+	u32 value_u32 = 0;
+
+	if (comp_node->component.type == COUNTER_COMPONENT_NONE)
+		return 0;
+
+	switch (comp->type) {
+	case COUNTER_COMP_U8:
+	case COUNTER_COMP_BOOL:
+		switch (comp_node->component.scope) {
+		case COUNTER_SCOPE_DEVICE:
+			err = comp->device_u8_read(counter, &value_u8);
+			break;
+		case COUNTER_SCOPE_SIGNAL:
+			err = comp->signal_u8_read(counter, parent, &value_u8);
+			break;
+		case COUNTER_SCOPE_COUNT:
+			err = comp->count_u8_read(counter, parent, &value_u8);
+			break;
+		}
+		*value = value_u8;
+		break;
+	case COUNTER_COMP_SIGNAL_LEVEL:
+	case COUNTER_COMP_FUNCTION:
+	case COUNTER_COMP_ENUM:
+	case COUNTER_COMP_COUNT_DIRECTION:
+	case COUNTER_COMP_COUNT_MODE:
+		switch (comp_node->component.scope) {
+		case COUNTER_SCOPE_DEVICE:
+			err = comp->device_u32_read(counter, &value_u32);
+			break;
+		case COUNTER_SCOPE_SIGNAL:
+			err = comp->signal_u32_read(counter, parent,
+						    &value_u32);
+			break;
+		case COUNTER_SCOPE_COUNT:
+			err = comp->count_u32_read(counter, parent, &value_u32);
+			break;
+		}
+		*value = value_u32;
+		break;
+	case COUNTER_COMP_U64:
+		switch (comp_node->component.scope) {
+		case COUNTER_SCOPE_DEVICE:
+			return comp->device_u64_read(counter, value);
+		case COUNTER_SCOPE_SIGNAL:
+			return comp->signal_u64_read(counter, parent, value);
+		case COUNTER_SCOPE_COUNT:
+			return comp->count_u64_read(counter, parent, value);
+		}
+		break;
+	case COUNTER_COMP_SYNAPSE_ACTION:
+		err = comp->action_read(counter, parent, comp->priv,
+					&value_u32);
+		*value = value_u32;
+		break;
+	}
+
+	return err;
+}
+
+/**
+ * counter_push_event - queue event for userspace reading
+ * @counter:	pointer to Counter structure
+ * @event:	triggered event
+ * @channel:	event channel
+ *
+ * Note: If no one is watching for the respective event, it is silently
+ * discarded.
+ */
+void counter_push_event(struct counter_device *const counter, const u8 event,
+			const u8 channel)
+{
+	struct counter_event ev = {0};
+	unsigned int copied = 0;
+	unsigned long flags;
+	struct counter_event_node *event_node;
+	struct counter_comp_node *comp_node;
+
+	ev.timestamp = ktime_get_ns();
+	ev.watch.event = event;
+	ev.watch.channel = channel;
+
+	raw_spin_lock_irqsave(&counter->events_list_lock, flags);
+
+	/* Search for event in the list */
+	list_for_each_entry(event_node, &counter->events_list, l)
+		if (event_node->event == event &&
+		    event_node->channel == channel)
+			break;
+
+	/* If event is not in the list */
+	if (&event_node->l == &counter->events_list)
+		goto exit_early;
+
+	/* Read and queue relevant comp for userspace */
+	list_for_each_entry(comp_node, &event_node->comp_list, l) {
+		ev.watch.component = comp_node->component;
+		ev.errno = -counter_get_data(counter, comp_node, &ev.value);
+
+		copied += kfifo_put(&counter->events, ev);
+	}
+
+	if (copied)
+		wake_up_poll(&counter->events_wait, EPOLLIN);
+
+exit_early:
+	raw_spin_unlock_irqrestore(&counter->events_list_lock, flags);
+}
+EXPORT_SYMBOL_GPL(counter_push_event);
diff --git a/drivers/counter/counter-chrdev.h b/drivers/counter/counter-chrdev.h
new file mode 100644
index 000000000000..cf5a318fe540
--- /dev/null
+++ b/drivers/counter/counter-chrdev.h
@@ -0,0 +1,16 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Counter character device interface
+ * Copyright (C) 2020 William Breathitt Gray
+ */
+#ifndef _COUNTER_CHRDEV_H_
+#define _COUNTER_CHRDEV_H_
+
+#include <linux/counter.h>
+#include <linux/types.h>
+
+int counter_chrdev_add(struct counter_device *const counter,
+		       const dev_t counter_devt);
+void counter_chrdev_remove(struct counter_device *const counter);
+
+#endif /* _COUNTER_CHRDEV_H_ */
diff --git a/drivers/counter/counter-core.c b/drivers/counter/counter-core.c
index 78e07588717b..1334e2f4ce4e 100644
--- a/drivers/counter/counter-core.c
+++ b/drivers/counter/counter-core.c
@@ -5,12 +5,16 @@
  */
 #include <linux/counter.h>
 #include <linux/device.h>
+#include <linux/device/bus.h>
 #include <linux/export.h>
+#include <linux/fs.h>
 #include <linux/gfp.h>
 #include <linux/idr.h>
 #include <linux/init.h>
 #include <linux/module.h>
+#include <linux/types.h>
 
+#include "counter-chrdev.h"
 #include "counter-sysfs.h"
 
 /* Provides a unique ID for each counter device */
@@ -33,6 +37,8 @@ static struct bus_type counter_bus_type = {
 	.name = "counter"
 };
 
+static dev_t counter_devt;
+
 /**
  * counter_register - register Counter to the system
  * @counter:	pointer to Counter to register
@@ -51,7 +57,6 @@ int counter_register(struct counter_device *const counter)
 	if (counter->id < 0)
 		return counter->id;
 
-	/* Configure device structure for Counter */
 	dev->type = &counter_device_type;
 	dev->bus = &counter_bus_type;
 	if (counter->parent) {
@@ -62,18 +67,25 @@ int counter_register(struct counter_device *const counter)
 	device_initialize(dev);
 	dev_set_drvdata(dev, counter);
 
+	/* Add Counter character device */
+	err = counter_chrdev_add(counter, counter_devt);
+	if (err)
+		goto err_free_id;
+
 	/* Add Counter sysfs attributes */
 	err = counter_sysfs_add(counter);
 	if (err < 0)
-		goto err_free_id;
+		goto err_remove_chrdev;
 
 	/* Add device to system */
 	err = device_add(dev);
 	if (err < 0)
-		goto err_free_id;
+		goto err_remove_chrdev;
 
 	return 0;
 
+err_remove_chrdev:
+	counter_chrdev_remove(counter);
 err_free_id:
 	put_device(dev);
 	return err;
@@ -135,13 +147,30 @@ int devm_counter_register(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(devm_counter_register);
 
+#define COUNTER_DEV_MAX 256
+
 static int __init counter_init(void)
 {
-	return bus_register(&counter_bus_type);
+	int err;
+
+	err = bus_register(&counter_bus_type);
+	if (err < 0)
+		return err;
+
+	err = alloc_chrdev_region(&counter_devt, 0, COUNTER_DEV_MAX, "counter");
+	if (err < 0)
+		goto err_unregister_bus;
+
+	return 0;
+
+err_unregister_bus:
+	bus_unregister(&counter_bus_type);
+	return err;
 }
 
 static void __exit counter_exit(void)
 {
+	unregister_chrdev_region(counter_devt, COUNTER_DEV_MAX);
 	bus_unregister(&counter_bus_type);
 }
 
diff --git a/drivers/counter/counter-sysfs.c b/drivers/counter/counter-sysfs.c
index 654afa91ae9f..4d97d6e50d48 100644
--- a/drivers/counter/counter-sysfs.c
+++ b/drivers/counter/counter-sysfs.c
@@ -499,6 +499,7 @@ static ssize_t counter_comp_name_show(struct device *dev,
 
 static int counter_name_attr_create(struct device *const dev,
 				    struct counter_attribute_group *const group,
+				    const char *const attr_name,
 				    const char *const name)
 {
 	struct counter_attribute *counter_attr;
@@ -513,7 +514,7 @@ static int counter_name_attr_create(struct device *const dev,
 
 	/* Configure device attribute */
 	sysfs_attr_init(&counter_attr->dev_attr.attr);
-	counter_attr->dev_attr.attr.name = "name";
+	counter_attr->dev_attr.attr.name = attr_name;
 	counter_attr->dev_attr.attr.mode = 0444;
 	counter_attr->dev_attr.show = counter_comp_name_show;
 
@@ -524,6 +525,18 @@ static int counter_name_attr_create(struct device *const dev,
 	return 0;
 }
 
+static int counter_ext_name_attr_create(struct device *const dev,
+	struct counter_attribute_group *const group, const size_t i,
+	const char *const name)
+{
+	const char *attr_name;
+
+	attr_name = devm_kasprintf(dev, GFP_KERNEL, "extension%zu_name", i);
+	if (!attr_name)
+		return -ENOMEM;
+
+	return counter_name_attr_create(dev, group, attr_name, name);
+}
 
 static struct counter_comp counter_signal_comp = {
 	.type = COUNTER_COMP_SIGNAL_LEVEL,
@@ -539,6 +552,7 @@ static int counter_signal_attrs_create(struct counter_device *const counter,
 	int err;
 	struct counter_comp comp;
 	size_t i;
+	struct counter_comp *ext;
 
 	/* Create main Signal attribute */
 	comp = counter_signal_comp;
@@ -548,14 +562,19 @@ static int counter_signal_attrs_create(struct counter_device *const counter,
 		return err;
 
 	/* Create Signal name attribute */
-	err = counter_name_attr_create(dev, group, signal->name);
+	err = counter_name_attr_create(dev, group, "name", signal->name);
 	if (err < 0)
 		return err;
 
 	/* Create an attribute for each extension */
 	for (i = 0; i < signal->num_ext; i++) {
-		err = counter_attr_create(dev, group, signal->ext + i, scope,
-					  signal);
+		ext = signal->ext + i;
+
+		err = counter_attr_create(dev, group, ext, scope, signal);
+		if (err < 0)
+			return err;
+
+		err = counter_ext_name_attr_create(dev, group, i, ext->name);
 		if (err < 0)
 			return err;
 	}
@@ -640,6 +659,7 @@ static int counter_count_attrs_create(struct counter_device *const counter,
 	int err;
 	struct counter_comp comp;
 	size_t i;
+	struct counter_comp *ext;
 
 	/* Create main Count attribute */
 	comp = counter_count_comp;
@@ -650,7 +670,7 @@ static int counter_count_attrs_create(struct counter_device *const counter,
 		return err;
 
 	/* Create Count name attribute */
-	err = counter_name_attr_create(dev, group, count->name);
+	err = counter_name_attr_create(dev, group, "name", count->name);
 	if (err < 0)
 		return err;
 
@@ -664,8 +684,13 @@ static int counter_count_attrs_create(struct counter_device *const counter,
 
 	/* Create an attribute for each extension */
 	for (i = 0; i < count->num_ext; i++) {
-		err = counter_attr_create(dev, group, count->ext + i, scope,
-					  count);
+		ext = count->ext + i;
+
+		err = counter_attr_create(dev, group, ext, scope, count);
+		if (err < 0)
+			return err;
+
+		err = counter_ext_name_attr_create(dev, group, i, ext->name);
 		if (err < 0)
 			return err;
 	}
@@ -729,6 +754,7 @@ static int counter_sysfs_attr_add(struct counter_device *const counter,
 	struct device *const dev = &counter->dev;
 	int err;
 	size_t i;
+	struct counter_comp *ext;
 
 	/* Add Signals sysfs attributes */
 	err = counter_sysfs_signals_add(counter, group);
@@ -743,7 +769,7 @@ static int counter_sysfs_attr_add(struct counter_device *const counter,
 	group += counter->num_counts;
 
 	/* Create name attribute */
-	err = counter_name_attr_create(dev, group, counter->name);
+	err = counter_name_attr_create(dev, group, "name", counter->name);
 	if (err < 0)
 		return err;
 
@@ -761,8 +787,13 @@ static int counter_sysfs_attr_add(struct counter_device *const counter,
 
 	/* Create an attribute for each extension */
 	for (i = 0; i < counter->num_ext; i++) {
-		err = counter_attr_create(dev, group, counter->ext + i, scope,
-					  NULL);
+		ext = counter->ext + i;
+
+		err = counter_attr_create(dev, group, ext, scope, NULL);
+		if (err < 0)
+			return err;
+
+		err = counter_ext_name_attr_create(dev, group, i, ext->name);
 		if (err < 0)
 			return err;
 	}
diff --git a/include/linux/counter.h b/include/linux/counter.h
index 2f01e1fec857..ce79e62a2e58 100644
--- a/include/linux/counter.h
+++ b/include/linux/counter.h
@@ -6,9 +6,15 @@
 #ifndef _COUNTER_H_
 #define _COUNTER_H_
 
+#include <linux/cdev.h>
 #include <linux/device.h>
 #include <linux/kernel.h>
+#include <linux/kfifo.h>
+#include <linux/mutex.h>
+#include <linux/spinlock_types.h>
 #include <linux/types.h>
+#include <linux/wait.h>
+#include <uapi/linux/counter.h>
 
 struct counter_device;
 struct counter_count;
@@ -27,47 +33,6 @@ enum counter_comp_type {
 	COUNTER_COMP_COUNT_MODE,
 };
 
-enum counter_scope {
-	COUNTER_SCOPE_DEVICE,
-	COUNTER_SCOPE_SIGNAL,
-	COUNTER_SCOPE_COUNT,
-};
-
-enum counter_count_direction {
-	COUNTER_COUNT_DIRECTION_FORWARD,
-	COUNTER_COUNT_DIRECTION_BACKWARD,
-};
-
-enum counter_count_mode {
-	COUNTER_COUNT_MODE_NORMAL,
-	COUNTER_COUNT_MODE_RANGE_LIMIT,
-	COUNTER_COUNT_MODE_NON_RECYCLE,
-	COUNTER_COUNT_MODE_MODULO_N,
-};
-
-enum counter_function {
-	COUNTER_FUNCTION_INCREASE,
-	COUNTER_FUNCTION_DECREASE,
-	COUNTER_FUNCTION_PULSE_DIRECTION,
-	COUNTER_FUNCTION_QUADRATURE_X1_A,
-	COUNTER_FUNCTION_QUADRATURE_X1_B,
-	COUNTER_FUNCTION_QUADRATURE_X2_A,
-	COUNTER_FUNCTION_QUADRATURE_X2_B,
-	COUNTER_FUNCTION_QUADRATURE_X4,
-};
-
-enum counter_signal_level {
-	COUNTER_SIGNAL_LEVEL_LOW,
-	COUNTER_SIGNAL_LEVEL_HIGH,
-};
-
-enum counter_synapse_action {
-	COUNTER_SYNAPSE_ACTION_NONE,
-	COUNTER_SYNAPSE_ACTION_RISING_EDGE,
-	COUNTER_SYNAPSE_ACTION_FALLING_EDGE,
-	COUNTER_SYNAPSE_ACTION_BOTH_EDGES,
-};
-
 /**
  * struct counter_comp - Counter component node
  * @type:		Counter component data type
@@ -239,6 +204,20 @@ struct counter_count {
 	size_t num_ext;
 };
 
+/**
+ * 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;
+	struct list_head comp_list;
+};
+
 /**
  * struct counter_ops - Callbacks from driver
  * @signal_read:	read callback for Signals. The read level of the
@@ -261,6 +240,13 @@ struct counter_count {
  * @action_write:	write callback for Synapse action modes. The action mode
  *			to write for the respective Synapse is passed in via the
  *			action parameter.
+ * @events_configure:	write callback to configure events. The list of struct
+ *			counter_event_node may be accessed via the events_list
+ *			member of the counter parameter.
+ * @watch_validate:	callback to validate a watch. The Counter component
+ *			watch configuration is passed in via the watch
+ *			parameter. A return value of 0 indicates a valid Counter
+ *			component watch configuration.
  */
 struct counter_ops {
 	int (*signal_read)(struct counter_device *counter,
@@ -284,6 +270,9 @@ struct counter_ops {
 			    struct counter_count *count,
 			    struct counter_synapse *synapse,
 			    enum counter_synapse_action action);
+	int (*events_configure)(struct counter_device *counter);
+	int (*watch_validate)(struct counter_device *counter,
+			      const struct counter_watch *watch);
 };
 
 /**
@@ -300,6 +289,13 @@ struct counter_ops {
  * @priv:		optional private data supplied by driver
  * @id:			unique ID used to identify the Counter
  * @dev:		internal device structure
+ * @chrdev:		internal character device structure
+ * @events_list:	list of current watching Counter events
+ * @events_list_lock:	lock to protect Counter events list operations
+ * @next_events_list:	list of next watching Counter events
+ * @events:		queue of detected Counter events
+ * @events_wait:	wait queue to allow blocking reads of Counter events
+ * @events_lock:	lock to protect Counter events queue read operations
  */
 struct counter_device {
 	const char *name;
@@ -319,12 +315,21 @@ struct counter_device {
 
 	int id;
 	struct device dev;
+	struct cdev chrdev;
+	struct list_head events_list;
+	raw_spinlock_t events_list_lock;
+	struct list_head next_events_list;
+	DECLARE_KFIFO(events, struct counter_event, 64);
+	wait_queue_head_t events_wait;
+	struct mutex events_lock;
 };
 
 int counter_register(struct counter_device *const counter);
 void counter_unregister(struct counter_device *const counter);
 int devm_counter_register(struct device *dev,
 			  struct counter_device *const counter);
+void counter_push_event(struct counter_device *const counter, const u8 event,
+			const u8 channel);
 
 #define COUNTER_COMP_DEVICE_U8(_name, _read, _write) \
 { \
diff --git a/include/uapi/linux/counter.h b/include/uapi/linux/counter.h
new file mode 100644
index 000000000000..7585dc9db19d
--- /dev/null
+++ b/include/uapi/linux/counter.h
@@ -0,0 +1,123 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+/*
+ * Userspace ABI for Counter character devices
+ * Copyright (C) 2020 William Breathitt Gray
+ */
+#ifndef _UAPI_COUNTER_H_
+#define _UAPI_COUNTER_H_
+
+#include <linux/ioctl.h>
+#include <linux/types.h>
+
+/* Component type definitions */
+enum counter_component_type {
+	COUNTER_COMPONENT_NONE,
+	COUNTER_COMPONENT_SIGNAL,
+	COUNTER_COMPONENT_COUNT,
+	COUNTER_COMPONENT_FUNCTION,
+	COUNTER_COMPONENT_SYNAPSE_ACTION,
+	COUNTER_COMPONENT_EXTENSION,
+};
+
+/* Component scope definitions */
+enum counter_scope {
+	COUNTER_SCOPE_DEVICE,
+	COUNTER_SCOPE_SIGNAL,
+	COUNTER_SCOPE_COUNT,
+};
+
+/**
+ * struct counter_component - Counter component identification
+ * @type: component type (Count, extension, etc.)
+ * @scope: component scope (Device, Count, or Signal)
+ * @parent: parent component identification number
+ * @id: component identification number
+ */
+struct counter_component {
+	__u8 type;
+	__u8 scope;
+	__u8 parent;
+	__u8 id;
+};
+
+/* Event type definitions */
+enum counter_event_type {
+	COUNTER_EVENT_OVERFLOW,
+	COUNTER_EVENT_UNDERFLOW,
+	COUNTER_EVENT_OVERFLOW_UNDERFLOW,
+	COUNTER_EVENT_THRESHOLD,
+	COUNTER_EVENT_INDEX,
+};
+
+/**
+ * struct counter_watch - Counter component watch configuration
+ * @component: component to watch when event triggers
+ * @event: event that triggers
+ * @channel: event channel
+ */
+struct counter_watch {
+	struct counter_component component;
+	__u8 event;
+	__u8 channel;
+};
+
+/* ioctl commands */
+#define COUNTER_CLEAR_WATCHES_IOCTL _IO(0x3E, 0x00)
+#define COUNTER_ADD_WATCH_IOCTL _IOW(0x3E, 0x01, struct counter_watch)
+#define COUNTER_LOAD_WATCHES_IOCTL _IO(0x3E, 0x02)
+
+/**
+ * struct counter_event - Counter event data
+ * @timestamp: best estimate of time of event occurrence, in nanoseconds
+ * @value: component value
+ * @watch: component watch configuration
+ * @errno: system error number
+ */
+struct counter_event {
+	__aligned_u64 timestamp;
+	__aligned_u64 value;
+	struct counter_watch watch;
+	__u8 errno;
+};
+
+/* Count direction values */
+enum counter_count_direction {
+	COUNTER_COUNT_DIRECTION_FORWARD,
+	COUNTER_COUNT_DIRECTION_BACKWARD,
+};
+
+/* Count mode values */
+enum counter_count_mode {
+	COUNTER_COUNT_MODE_NORMAL,
+	COUNTER_COUNT_MODE_RANGE_LIMIT,
+	COUNTER_COUNT_MODE_NON_RECYCLE,
+	COUNTER_COUNT_MODE_MODULO_N,
+};
+
+/* Count function values */
+enum counter_function {
+	COUNTER_FUNCTION_INCREASE,
+	COUNTER_FUNCTION_DECREASE,
+	COUNTER_FUNCTION_PULSE_DIRECTION,
+	COUNTER_FUNCTION_QUADRATURE_X1_A,
+	COUNTER_FUNCTION_QUADRATURE_X1_B,
+	COUNTER_FUNCTION_QUADRATURE_X2_A,
+	COUNTER_FUNCTION_QUADRATURE_X2_B,
+	COUNTER_FUNCTION_QUADRATURE_X4,
+};
+
+/* Signal values */
+enum counter_signal_level {
+	COUNTER_SIGNAL_LEVEL_LOW,
+	COUNTER_SIGNAL_LEVEL_HIGH,
+};
+
+/* Action mode values */
+enum counter_synapse_action {
+	COUNTER_SYNAPSE_ACTION_NONE,
+	COUNTER_SYNAPSE_ACTION_RISING_EDGE,
+	COUNTER_SYNAPSE_ACTION_FALLING_EDGE,
+	COUNTER_SYNAPSE_ACTION_BOTH_EDGES,
+};
+
+#endif /* _UAPI_COUNTER_H_ */
-- 
2.29.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel