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

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Thu, 21 Jan 2021 17:03:11 +0900
William Breathitt Gray <vilhelm.gray@gmail.com> wrote:

> On Tue, Jan 19, 2021 at 10:20:22AM +0100, Oleksij Rempel wrote:
> > On Sun, Nov 22, 2020 at 03:29:54PM -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>
> > > ---  
> > 
> > Hello William,
> > 
> > the series looks quite interesting, we have some thoughts... see below:
> > 
> > [...]  
> > > +/**
> > > + * counter_push_event - queue event for userspace reading
> > > + * @counter:	pointer to Counter structure
> > > + * @event:	triggered event
> > > + * @channel:	event channel
> > > + *
> > > + * Note: If no one is watching for the respective event, it is silently
> > > + * discarded.
> > > + *
> > > + * RETURNS:
> > > + * 0 on success, negative error number on failure.
> > > + */
> > > +int counter_push_event(struct counter_device *const counter, const u8 event,
> > > +		       const u8 channel)
> > > +{
> > > +	struct counter_event ev = {0};
> > > +	unsigned int copied = 0;
> > > +	unsigned long flags;
> > > +	struct counter_event_node *event_node;
> > > +	struct counter_comp_node *comp_node;
> > > +	int err = 0;
> > > +
> > > +	ev.timestamp = ktime_get_ns();
> > > +	ev.watch.event = event;
> > > +	ev.watch.channel = channel;
> > > +
> > > +	raw_spin_lock_irqsave(&counter->events_lock, flags);
> > > +
> > > +	/* Search for event in the list */
> > > +	list_for_each_entry(event_node, &counter->events_list, l)
> > > +		if (event_node->event == event &&
> > > +		    event_node->channel == channel)
> > > +			break;
> > > +
> > > +	/* If event is not in the list */
> > > +	if (&event_node->l == &counter->events_list)
> > > +		goto exit_early;
> > > +
> > > +	/* Read and queue relevant comp for userspace */
> > > +	list_for_each_entry(comp_node, &event_node->comp_list, l) {
> > > +		err = counter_get_data(counter, comp_node, &ev.value);
> > > +		if (err)
> > > +			goto exit_early;
> > > +
> > > +		ev.watch.component = comp_node->component;
> > > +
> > > +		copied += kfifo_put(&counter->events, ev);  
> > 
> > We want to calculate the frequency of some IRQ pulses in user space and
> > counter values with time stamps really fits well here. As the pulses are
> > from a physical system (rotating wheel), they will only change at a
> > certain rate. We want to have the possibility to read from the counter
> > device less often, we intentionally want to skip (meaning miss)
> > events.
> > 
> > When reading we're interested in the newest events. The kfifo implements
> > a "tail" drop FIFO, which means new values are added at the end, and if
> > the FIFO is full, they are dropped. We need a "head" drop FIFO which
> > discards the oldest events, keeping only the recent ones.
> > 
> > As far as we know, kfifo doesn't offer a head drop mode, but I think
> > this can be added.  
> 
> I'm not sure if kfifo has this kind of mode, but it seems like something
> that should be there if it is not already -- I imagine this kind of
> operation mode would be pretty common. Perhaps someone knows how to
> achieve this and will share here.
> 

I don't think it does.  In IIO we started with a ring (effectively this)
- hopefully no one else remembers the horrible mess I made of it ;),
but eventually decided that there were very few usecases that
actually demanded it so just switched over to kfifo.

It's actually pretty rare that you aren't better off draining to
a backing store of some type.

Not had any demand to bring a ring back to IIO since we dropped it.


> > 
> > [...]
> >   
> > >  struct counter_device {
> > >  	const char *name;
> > > @@ -270,12 +270,20 @@ struct counter_device {
> > >  
> > >  	int id;
> > >  	struct device dev;
> > > +	struct cdev chrdev;
> > > +	raw_spinlock_t events_lock;
> > > +	struct list_head events_list;
> > > +	struct list_head next_events_list;
> > > +	DECLARE_KFIFO(events, struct counter_event, 64);  
> > 
> > Do you plan to make the size of the FIFO configurable?
> > 
> > regards,
> > Oleksij & Marc  
> 
> I suppose it wouldn't be a problem to make this a configurable setting;
> I think I can implement this via kfifo_alloc() and kfifo_free(). How
> would users control this -- maybe using a sysfs attribute?

Would make sense to do it from sysfs.

Jonathan

> 
> William Breathitt Gray
>