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
