Re: [PATCH v4 3/5] counter: Add character device interface
From: David Lechner
Date: Mon Aug 10 2020 - 19:02:26 EST
On 8/9/20 9:51 AM, William Breathitt Gray wrote:
On Tue, Jul 28, 2020 at 07:20:03PM -0500, David Lechner wrote:
On 7/21/20 2:35 PM, 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.
This sounds similar to triggers and buffers in the iio subsystem. And
I can see how it might be useful in some cases. But I think it would not
give the desired results when performance is important.
Thinking through a few cases here...
Suppose there was a new counter device that used the I2C bus. This would
either have to be periodically polled for events or it might have a
separate GPIO line to notify the MCU. In any case, with the proposed
implementation, there would be a separate I2C transaction for each data
point for that event. So none of the data for that event would actually
be from the same point in time. And with I2C, this time difference could
be significant.
With the TI eQEP I have been working with, there are special latched
registers for some events. To make use of these with events, we would have
add extensions for each one we want to use (and expose it in sysfs). But
really, the fact that we are using a latched register should be an
implementation detail in the driver and not something userspace should have
to know about.
So, I'm wondering if it would make sense to keep things simpler and have
events like the input subsystem where the event value is directly tied
to the event. It would probably be rare for an event to have more than
one or two values. And error events probably would not have a value at
all.
For example, with the TI eQEP, there is a unit timer time out event.
This latches the position count, the timer count and the timer period.
To translate this to an event data structure, the latched time would
be the event timestamp and the position count would be the event value.
The timer period should already be known since we would have configured
the timer ourselves. There is also a count event that works similarly.
In this case, the latched time would be the event timestamp and the
latched timer period would be the event value. We would know the count
already since we get an event for each count (and a separate direction
change event if the direction changes).
There are use-cases where it would be useful to have the extension reads
occur as close to the event trigger as possible (e.g. multiple-axes
positioning with boundary sensor flags) so I don't think this
functionality should be completely abadoned, but I think your argument
for standard event types makes sense.
We could treat those extensions reads as an optional feature that can be
enabled and configured by ioctls. However, the use-case you are
concerned with, we can redesign Counter events to return specific data
based on the specific event type.
For example, we could have a COUNTER_EVENT_INDEX which occurs when an
Index signal edge is detected, and the return data is the Count value
for that channel; we can also have a COUNTER_EVENT_TIMEOUT which occurs
when a unit timer times out, and returns the data you mentioned you are
interested in seeing.
These Counter event types would be standard, so user applications
wouldn't need to know driver/device implementation details, but instead
just follow the API to get the data they expect for that particular
event type. Would this kind of design work for your needs?
Yes. After trying (and failing) to implement my suggestions here, I
came to the conclusion that it was not sufficient to only have one
value per event. And it doesn't seem as obvious as I initially thought
which should be the "standard" value for an event in some cases.
When `counter_push_event(counter, 1)` is called for example, it will go
down the list for Event 1 and execute the read callbacks for Signal 0,
Signal 0 Extension 0, and Extension 4 -- the data returned for each is
pushed to a kfifo as a `struct counter_event`, which userspace can
retrieve via a standard read operation on the respective character
device node.
Userspace
---------
Userspace applications can configure Counter events via ioctl operations
on the Counter character device node. There following ioctl codes are
supported and provided by the `linux/counter.h` userspace header file:
* COUNTER_CLEAR_WATCHES_IOCTL:
Clear all Counter watches from all events
* COUNTER_SET_WATCH_IOCTL:
Set a Counter watch on the specified event
To configure events to gather Counter data, users first populate a
`struct counter_watch` with the relevant event id and the information
for the desired Counter component from which to read, and then pass it
via the `COUNTER_SET_WATCH_IOCTL` ioctl command.
Userspace applications can then execute a `read` operation (optionally
calling `poll` first) on the Counter character device node to retrieve
`struct counter_event` elements with the desired data.
For example, the following userspace code opens `/dev/counter0`,
configures Event 0 to gather Count 0 and Count 1, and prints out the
data as it becomes available on the character device node:
#include <fcntl.h>
#include <linux/counter.h>
#include <poll.h>
#include <stdio.h>
#include <sys/ioctl.h>
#include <unistd.h>
struct counter_watch watches[2] = {
{
.event = 0,
.component.owner_type = COUNTER_OWNER_TYPE_COUNT,
.component.owner_id = 0,
.component.type = COUNTER_COMPONENT_TYPE_COUNT,
},
{
.event = 0,
.component.owner_type = COUNTER_OWNER_TYPE_COUNT,
.component.owner_id = 1,
.component.type = COUNTER_COMPONENT_TYPE_COUNT,
},
};
int main(void)
{
struct pollfd pfd = { .events = POLLIN };
struct counter_event event_data[2];
pfd.fd = open("/dev/counter0", O_RDWR);
ioctl(pfd.fd, COUNTER_SET_WATCH_IOCTL, watches);
ioctl(pfd.fd, COUNTER_SET_WATCH_IOCTL, watches + 1);
What enables events? If an event is enabled for each of these ioctls,
then we have a race condition where events events from the first watch
can start to be queued before the second watch is added. So we would
have to flush the chardev first before polling, otherwise the assumption
that event_data[0] is owner_id=0 and event_data[1] is owner_id=1 is
not true.
That's a good point, we could theoretically have a situation where an
event is pushed before the configuration of watches is complete. I'm not
sure if the solution is to implement an enable/disable ioctl to control
when events are recorded, or a flush ioctl to remove the invalid events
in the queue.
This is also racy if we want to clear watches and set up new watches
at runtime. There would be a period of time where there were no watches
and we could miss events.
I'm not sure how typical this use-case is -- would an operator ever want
to change watch configuration on-the-fly? I assumed watches configured
once at the start of a production run, and then stay essentially static
until the production stops.
The use case I am thinking of is measuring motor speed in robotics. At
low speed, we need an event for each count increase. But at high speed,
this would be too many events and we instead need a periodic event based
on the timer timeout. A maneuver may require operating at both high and
low speeds without stopping and so we would want to be able to switch
back and forth without interruption.
Well regardless, if we want to support this kind of functionality we
will need to implement a kind of atomic replacement for all watches with
new ones. This shouldn't be too difficult to achieve: buffer the desired
watches instead, and then activate them together atomically via a new
ioctl command.
With my suggested changes of having fixed values per event and generic
events, we could just have a single ioctl to enable and disable events.
This would probably need to take an array of event descriptors as an
argument where event descriptors contain the component type/id and the
event to enable.
I agree with having specified data for particular event types, but I
think we should still be able to support general extension watches as an
optional functionality. In fact, I don't think we'll need to implement
enable/disable event ioctl commands.
The current implementation only records events if the user is watching
for them (i.e. a watch has been set); if no one is watching for these
events, they are just silently dropped by the counter_event_push
function. If we implement an ioctl to atomically set the watches, there
is no need to explicitly enable/disable events: events will always
report the specified data for those their respective type -- the watch
data is extra optional data and will start flowing automatically when
atomically activated.
This sounds reasonable to me.