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

[William Breathitt Gray]

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@xxxxxxxxx> 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@xxxxxxxxxxxxxx>
> > Cc: Gwendal Grignou <gwendal@xxxxxxxxxxxx>
> > Cc: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> > Signed-off-by: William Breathitt Gray <vilhelm.gray@xxxxxxxxx>
> 
> 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: signature.asc
Description: PGP signature