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

[Oleksij Rempel]

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.

[...]

>  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

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |