On Thu, 21 Jan 2021 17:03:11 +0900
William Breathitt Gray <vilhelm.gray@xxxxxxxxx> 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@xxxxxxxxxxxxxx>
> > > Cc: Gwendal Grignou <gwendal@xxxxxxxxxxxx>
> > > Cc: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> > > Signed-off-by: William Breathitt Gray <vilhelm.gray@xxxxxxxxx>
> > > ---
> >
> > 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
>
