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.
>
> [...]
>
> > 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?
William Breathitt Gray
Attachment:
signature.asc
Description: PGP signature
