| Message ID | 08b3ac7349a59ba7fa5cd438bbe78360842ccd11.1595358237.git.vilhelm.gray@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce the Counter character device interface | expand |
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). > > A high-level view of how a count value is passed down from a counter > driver is exemplified by the following: > > ---------------------- > / Counter device \ > +----------------------+ > | Count register: 0x28 | > +----------------------+ > | > ----------------- > / raw count data / > ----------------- > | > V > +----------------------------+ > | Counter device driver |----------+ > +----------------------------+ | > | Processes data from device | ------------------- > |----------------------------| / driver callbacks / > | Type: u64 | ------------------- > | Value: 42 | | > +----------------------------+ | > | | > ---------- | > / u64 / | > ---------- | > | | > | V > | +----------------------+ > | | Counter core | > | +----------------------+ > | | Routes device driver | > | | callbacks to the | > | | userspace interfaces | > | +----------------------+ > | | > | ------------------- > | / driver callbacks / > | ------------------- > | | > +-------+---------------+ | > | | | > | +-------|-------+ > | | | > V | V > +--------------------+ | +---------------------+ > | Counter sysfs |<-+->| Counter chrdev | > +--------------------+ +---------------------+ > | Translates to the | | Translates to the | > | standard Counter | | standard Counter | > | sysfs output | | character device | > |--------------------| |---------------------+ > | Type: const char * | | Type: u64 | > | Value: "42" | | Value: 42 | > +--------------------+ +---------------------+ > | | > --------------- ----------------------- > / const char * / / struct counter_event / > --------------- ----------------------- > | | > | V > | +-----------+ > | | read | > | +-----------+ > | \ Count: 42 / > | ----------- > | > V > +--------------------------------------------------+ > | `/sys/bus/counter/devices/counterX/countY/count` | > +--------------------------------------------------+ > \ Count: "42" / > -------------------------------------------------- > > Counter character device nodes are created under the `/dev` directory as > `counterX`, where `X` is the respective counter device id. Defines for > the standard Counter data types are exposed via the userspace > `include/uapi/linux/counter.h` file. > > Counter events > -------------- > Counter device drivers can support Counter events by utilizing the > `counter_push_event` function: > > int counter_push_event(struct counter_device *const counter, > const u8 event); > > The event id is specified by the `event` parameter. When this function > is called, the Counter data associated with the respective event is > gathered, and a `struct counter_event` is generated for each datum and > pushed to userspace. > > Counter events can be configured by users to report various Counter > data of interest. This can be conceptualized as a list of Counter > component read calls to perform. For example: > > +------------------------+------------------------+ > | Event 0 | Event 1 | > +------------------------+------------------------+ > | * Count 0 | * Signal 0 | > | * Count 1 | * Signal 0 Extension 0 | > | * Signal 3 | * Extension 4 | > | * Count 4 Extension 2 | | > | * Signal 5 Extension 0 | | > +------------------------+------------------------+ In the current implementation, I can't tell if the event number corresponds to the individual counter or some device-specific interrupt bits. In either case, it seems like it would be better to have a generic enum of possible counter events like overflow, underflow, direction change, etc. > > 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. 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. 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. > > for (;;) { > poll(&pfd, 1, -1); > > read(pfd.fd, event_data, sizeof(event_data)); > > printf("Timestamp 0: %llu\nCount 0: %llu\n" > "Timestamp 1: %llu\nCount 1: %llu\n", > (unsigned long long)event_data[0].timestamp, > (unsigned long long)event_data[0].value_u64, > (unsigned long long)event_data[1].timestamp, > (unsigned long long)event_data[1].value_u64); > } > > return 0; > } > > Cc: David Lechner <david@lechnology.com> > Cc: Gwendal Grignou <gwendal@chromium.org> > Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com> > ---
On 7/28/20 7:20 PM, 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. > By the way, I really appreciate the work you have done here. When reviewing code, it is easy to point out what is wrong or we don't like and to not mention all the parts that are good. And there is a lot of really good work here already. I've been working on this all week to try out some of my suggestions and I'm not getting very far. This is a very difficult problem to solve! I just wanted to mention this since I responded to this patch series already but I am still learning and trying things. So I may have more/ different feedback in the future and I may decide some of my suggestions are not so good. :-) And one more thing, there was a nice talk at the Embedded Linux Conference last month about lessons learned from designing a userspace API for the GPIO subsystem [1]. Unfortunately, there is no video yet, but the slides might have some helpful ideas about mistakes to avoid. [1]: https://elinux.org/ELC_2020_Presentations
On 30/07/2020 17:49:37-0500, David Lechner wrote: > And one more thing, there was a nice talk at the Embedded Linux > Conference last month about lessons learned from designing a userspace > API for the GPIO subsystem [1]. Unfortunately, there is no video yet, > but the slides might have some helpful ideas about mistakes to avoid. > > [1]: https://elinux.org/ELC_2020_Presentations > The video is available on the original conference platform for one year after the event, then it will be made available on youtube.
On Thu, Jul 30, 2020 at 05:49:37PM -0500, David Lechner wrote: > On 7/28/20 7:20 PM, 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. > > > > By the way, I really appreciate the work you have done here. When reviewing > code, it is easy to point out what is wrong or we don't like and to not > mention all the parts that are good. And there is a lot of really good work > here already. > > I've been working on this all week to try out some of my suggestions and > I'm not getting very far. This is a very difficult problem to solve! > > I just wanted to mention this since I responded to this patch series > already but I am still learning and trying things. So I may have more/ > different feedback in the future and I may decide some of my suggestions > are not so good. :-) > > And one more thing, there was a nice talk at the Embedded Linux > Conference last month about lessons learned from designing a userspace > API for the GPIO subsystem [1]. Unfortunately, there is no video yet, > but the slides might have some helpful ideas about mistakes to avoid. > > [1]: https://elinux.org/ELC_2020_Presentations Thanks! I appreciate the words of encouragement. :-) This is a big endeavor so I'm expecting a lot of mistakes and changes along the way. Since we're designing a new userspace interface as well, I want to make sure it's correct before we commit it, because when it's finally introduced we're basically stuck with it. So I'm happy when mistakes are found because it saves me from having to live with those later after the interface is live. I'll respond to your PATCH 3/5 review later this week or coming weekend when I get the chance. Thanks again, William Breathitt Gray
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? > > > > A high-level view of how a count value is passed down from a counter > > driver is exemplified by the following: > > > > ---------------------- > > / Counter device \ > > +----------------------+ > > | Count register: 0x28 | > > +----------------------+ > > | > > ----------------- > > / raw count data / > > ----------------- > > | > > V > > +----------------------------+ > > | Counter device driver |----------+ > > +----------------------------+ | > > | Processes data from device | ------------------- > > |----------------------------| / driver callbacks / > > | Type: u64 | ------------------- > > | Value: 42 | | > > +----------------------------+ | > > | | > > ---------- | > > / u64 / | > > ---------- | > > | | > > | V > > | +----------------------+ > > | | Counter core | > > | +----------------------+ > > | | Routes device driver | > > | | callbacks to the | > > | | userspace interfaces | > > | +----------------------+ > > | | > > | ------------------- > > | / driver callbacks / > > | ------------------- > > | | > > +-------+---------------+ | > > | | | > > | +-------|-------+ > > | | | > > V | V > > +--------------------+ | +---------------------+ > > | Counter sysfs |<-+->| Counter chrdev | > > +--------------------+ +---------------------+ > > | Translates to the | | Translates to the | > > | standard Counter | | standard Counter | > > | sysfs output | | character device | > > |--------------------| |---------------------+ > > | Type: const char * | | Type: u64 | > > | Value: "42" | | Value: 42 | > > +--------------------+ +---------------------+ > > | | > > --------------- ----------------------- > > / const char * / / struct counter_event / > > --------------- ----------------------- > > | | > > | V > > | +-----------+ > > | | read | > > | +-----------+ > > | \ Count: 42 / > > | ----------- > > | > > V > > +--------------------------------------------------+ > > | `/sys/bus/counter/devices/counterX/countY/count` | > > +--------------------------------------------------+ > > \ Count: "42" / > > -------------------------------------------------- > > > > Counter character device nodes are created under the `/dev` directory as > > `counterX`, where `X` is the respective counter device id. Defines for > > the standard Counter data types are exposed via the userspace > > `include/uapi/linux/counter.h` file. > > > > Counter events > > -------------- > > Counter device drivers can support Counter events by utilizing the > > `counter_push_event` function: > > > > int counter_push_event(struct counter_device *const counter, > > const u8 event); > > > > The event id is specified by the `event` parameter. When this function > > is called, the Counter data associated with the respective event is > > gathered, and a `struct counter_event` is generated for each datum and > > pushed to userspace. > > > > Counter events can be configured by users to report various Counter > > data of interest. This can be conceptualized as a list of Counter > > component read calls to perform. For example: > > > > +------------------------+------------------------+ > > | Event 0 | Event 1 | > > +------------------------+------------------------+ > > | * Count 0 | * Signal 0 | > > | * Count 1 | * Signal 0 Extension 0 | > > | * Signal 3 | * Extension 4 | > > | * Count 4 Extension 2 | | > > | * Signal 5 Extension 0 | | > > +------------------------+------------------------+ > > In the current implementation, I can't tell if the event number corresponds > to the individual counter or some device-specific interrupt bits. In either > case, it seems like it would be better to have a generic enum of possible > counter events like overflow, underflow, direction change, etc. In the current implementation, the event number is arbitrarily chosen by the driver author. It would be best to have these well defined, and I think a group of standard Counter events would be the way to go as you point out. We can define a few common ones we expect for this introduction patch, and expand it from there if new types of events are necessary for future drivers. > > > > 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. 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. William Breathitt Gray > > > > for (;;) { > > poll(&pfd, 1, -1); > > > > read(pfd.fd, event_data, sizeof(event_data)); > > > > printf("Timestamp 0: %llu\nCount 0: %llu\n" > > "Timestamp 1: %llu\nCount 1: %llu\n", > > (unsigned long long)event_data[0].timestamp, > > (unsigned long long)event_data[0].value_u64, > > (unsigned long long)event_data[1].timestamp, > > (unsigned long long)event_data[1].value_u64); > > } > > > > return 0; > > } > > > > Cc: David Lechner <david@lechnology.com> > > Cc: Gwendal Grignou <gwendal@chromium.org> > > Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com> > > --- >
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.
On Mon, Aug 10, 2020 at 06:02:16PM -0500, David Lechner wrote: > 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. I agree, after thinking this over a second I realized it's not as apparent as I had hoped to determine what value would be most useful in general. I think the uses of counter devices are too varied, so it's probably best to leave it to the user to choose what value they want to gather for the respective events. The good thing is that the interface is flexible enough for us to defined new COUNTER_COMPONENT_TYPE_XXX types to extend the kind of data that can be gathered on an event push. This provides us with a path we can go down to implement the kind of data read you need without the latency overhead of executing multiple Counter Extension read operations (allowing for a single I2C transaction instead for example). However, standard event types (e.g. COUNTER_EVENT_INDEX) are something I find prudent to define, lest each driver end up with their own differing definitions of what "Event 0" actually means. > >>> > >>> 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. That's a fair use case, and I think have a well-defined swap mechanism in place is good regardless, so I'll go ahead implement this. > > > > 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. Ack. :-) William Breathitt Gray
diff --git a/drivers/counter/Makefile b/drivers/counter/Makefile index cbe1d06af6a9..c4870eb5b1dd 100644 --- a/drivers/counter/Makefile +++ b/drivers/counter/Makefile @@ -4,7 +4,7 @@ # obj-$(CONFIG_COUNTER) += counter.o -counter-y := counter-core.o counter-sysfs.o +counter-y := counter-core.o counter-sysfs.o counter-chrdev.o obj-$(CONFIG_104_QUAD_8) += 104-quad-8.o obj-$(CONFIG_STM32_TIMER_CNT) += stm32-timer-cnt.o diff --git a/drivers/counter/counter-chrdev.c b/drivers/counter/counter-chrdev.c new file mode 100644 index 000000000000..9aa2d32e7bc9 --- /dev/null +++ b/drivers/counter/counter-chrdev.c @@ -0,0 +1,441 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Generic Counter character device interface + * Copyright (C) 2020 William Breathitt Gray + */ + +#include <linux/cdev.h> +#include <linux/counter.h> +#include <linux/device.h> +#include <linux/err.h> +#include <linux/errno.h> +#include <linux/export.h> +#include <linux/fs.h> +#include <linux/list.h> +#include <linux/poll.h> +#include <linux/kdev_t.h> +#include <linux/kfifo.h> +#include <linux/slab.h> +#include <linux/spinlock.h> +#include <linux/timekeeping.h> +#include <linux/types.h> +#include <linux/wait.h> +#include <linux/uaccess.h> + +#include "counter-chrdev.h" + +struct counter_data_item { + struct list_head l; + struct counter_component component; + struct counter_data data; + void *owner; +}; + +struct counter_event_item { + struct list_head l; + u8 event; + struct list_head data_list; +}; + +static ssize_t counter_chrdev_read(struct file *filp, char __user *buf, + size_t len, loff_t *f_ps) +{ + struct counter_device *const counter = filp->private_data; + int err; + unsigned long flags; + unsigned int copied; + + if (len < sizeof(struct counter_event)) + return -EINVAL; + + do { + if (kfifo_is_empty(&counter->events)) { + if (filp->f_flags & O_NONBLOCK) + return -EAGAIN; + + err = wait_event_interruptible(counter->events_wait, + !kfifo_is_empty(&counter->events)); + if (err) + return err; + } + + raw_spin_lock_irqsave(&counter->events_lock, flags); + err = kfifo_to_user(&counter->events, buf, len, &copied); + raw_spin_unlock_irqrestore(&counter->events_lock, flags); + if (err) + return err; + } while (!copied); + + return copied; +} + +static __poll_t counter_chrdev_poll(struct file *filp, + struct poll_table_struct *pollt) +{ + struct counter_device *const counter = filp->private_data; + __poll_t events = 0; + + poll_wait(filp, &counter->events_wait, pollt); + + if (!kfifo_is_empty(&counter->events)) + events = EPOLLIN | EPOLLRDNORM; + + return events; +} + +static void counter_events_list_free(struct counter_device *const counter) +{ + unsigned long flags; + struct counter_event_item *p, *n; + struct counter_data_item *q, *o; + + raw_spin_lock_irqsave(&counter->events_lock, flags); + + list_for_each_entry_safe(p, n, &counter->events_list, l) { + /* Free associated data items */ + list_for_each_entry_safe(q, o, &p->data_list, l) { + list_del(&q->l); + kfree(q); + } + + /* Free event item */ + list_del(&p->l); + kfree(p); + } + + raw_spin_unlock_irqrestore(&counter->events_lock, flags); +} + +static int counter_set_event_item(struct counter_device *const counter, + const u8 event, + const struct counter_data_item *const cfg) +{ + unsigned long flags; + struct counter_event_item *event_item; + int err; + struct counter_data_item *data_item; + + raw_spin_lock_irqsave(&counter->events_lock, flags); + + /* Search for event in the list */ + list_for_each_entry(event_item, &counter->events_list, l) + if (event_item->event == event) + break; + + /* If event is not already in the list */ + if (&event_item->l == &counter->events_list) { + /* Allocate new event item */ + event_item = kmalloc(sizeof(*event_item), GFP_ATOMIC); + if (!event_item) { + err = -ENOMEM; + goto err_event_item; + } + + /* Configure event item and add to the list */ + event_item->event = event; + INIT_LIST_HEAD(&event_item->data_list); + list_add(&event_item->l, &counter->events_list); + } + + /* Search for data item in the list */ + list_for_each_entry(data_item, &event_item->data_list, l) + if (data_item->owner == cfg->owner && + data_item->data.count_u8_read == cfg->data.count_u8_read) { + err = -EINVAL; + goto err_data_item; + } + + /* Allocate data item */ + data_item = kmalloc(sizeof(*data_item), GFP_ATOMIC); + if (!data_item) { + err = -ENOMEM; + goto err_data_item; + } + *data_item = *cfg; + + /* Add data item to event item */ + list_add_tail(&data_item->l, &event_item->data_list); + + raw_spin_unlock_irqrestore(&counter->events_lock, flags); + + return 0; + +err_data_item: + if (list_empty(&event_item->data_list)) { + list_del(&event_item->l); + kfree(event_item); + } +err_event_item: + raw_spin_unlock_irqrestore(&counter->events_lock, flags); + return err; +} + +static int counter_set_watch(struct counter_device *const counter, + const unsigned long arg) +{ + void __user *const uwatch = (void __user *)arg; + struct counter_watch watch; + struct counter_data_item data_item; + size_t owner_id, id; + struct counter_data *ext; + size_t num_ext; + + if (copy_from_user(&watch, uwatch, sizeof(watch))) + return -EFAULT; + owner_id = watch.component.owner_id; + id = watch.component.id; + + /* Configure owner info for data item */ + switch (watch.component.owner_type) { + case COUNTER_OWNER_TYPE_DEVICE: + data_item.owner = NULL; + + ext = counter->ext; + num_ext = counter->num_ext; + break; + case COUNTER_OWNER_TYPE_SIGNAL: + if (counter->num_signals < owner_id + 1) + return -EINVAL; + + data_item.owner = counter->signals + owner_id; + + ext = counter->signals[owner_id].ext; + num_ext = counter->signals[owner_id].num_ext; + break; + case COUNTER_OWNER_TYPE_COUNT: + if (counter->num_counts < owner_id + 1) + return -EINVAL; + + data_item.owner = counter->counts + owner_id; + + ext = counter->counts[owner_id].ext; + num_ext = counter->counts[owner_id].num_ext; + break; + default: + return -EINVAL; + } + + /* Configure component info for data item */ + switch (watch.component.type) { + case COUNTER_COMPONENT_TYPE_SIGNAL: + if (watch.component.owner_type != COUNTER_OWNER_TYPE_SIGNAL) + return -EINVAL; + + data_item.data.type = COUNTER_DATA_TYPE_SIGNAL; + data_item.data.signal_u8_read = counter->signal_read; + break; + case COUNTER_COMPONENT_TYPE_COUNT: + if (watch.component.owner_type != COUNTER_OWNER_TYPE_COUNT) + return -EINVAL; + + data_item.data.type = COUNTER_DATA_TYPE_U64; + data_item.data.count_u64_read = counter->count_read; + break; + case COUNTER_COMPONENT_TYPE_COUNT_FUNCTION: + if (watch.component.owner_type != COUNTER_OWNER_TYPE_COUNT) + return -EINVAL; + + data_item.data.type = COUNTER_DATA_TYPE_COUNT_FUNCTION; + data_item.data.count_u8_read = counter->function_read; + break; + case COUNTER_COMPONENT_TYPE_SYNAPSE_ACTION: + if (watch.component.owner_type != COUNTER_OWNER_TYPE_COUNT) + return -EINVAL; + if (counter->counts[owner_id].num_synapses < id + 1) + return -EINVAL; + + data_item.data.type = COUNTER_DATA_TYPE_SYNAPSE_ACTION; + data_item.data.action_read = counter->action_read; + data_item.data.priv = counter->counts[owner_id].synapses + id; + break; + case COUNTER_COMPONENT_TYPE_EXTENSION: + if (num_ext < id + 1) + return -EINVAL; + + data_item.data = ext[id]; + break; + default: + return -EINVAL; + } + if (!data_item.data.count_u8_read) + return -EFAULT; + data_item.component = watch.component; + + return counter_set_event_item(counter, watch.event, &data_item); +} + +static long counter_chrdev_ioctl(struct file *filp, unsigned int cmd, + unsigned long arg) +{ + struct counter_device *const counter = filp->private_data; + + switch (cmd) { + case COUNTER_CLEAR_WATCHES_IOCTL: + counter_events_list_free(counter); + break; + case COUNTER_SET_WATCH_IOCTL: + return counter_set_watch(counter, arg); + default: + return -ENOIOCTLCMD; + } + + return 0; +} + +static int counter_chrdev_open(struct inode *inode, struct file *filp) +{ + struct counter_device *const counter = container_of(inode->i_cdev, + typeof(*counter), + chrdev); + + get_device(&counter->dev); + filp->private_data = counter; + + return nonseekable_open(inode, filp); +} + +static int counter_chrdev_release(struct inode *inode, struct file *filp) +{ + struct counter_device *const counter = filp->private_data; + + put_device(&counter->dev); + + counter_events_list_free(counter); + + return 0; +} + +static const struct file_operations counter_fops = { + .llseek = no_llseek, + .read = counter_chrdev_read, + .poll = counter_chrdev_poll, + .unlocked_ioctl = counter_chrdev_ioctl, + .open = counter_chrdev_open, + .release = counter_chrdev_release, +}; + +int counter_chrdev_add(struct counter_device *const counter, + const dev_t counter_devt) +{ + struct device *const dev = &counter->dev; + struct cdev *const chrdev = &counter->chrdev; + + /* Initialize Counter events list */ + INIT_LIST_HEAD(&counter->events_list); + raw_spin_lock_init(&counter->events_lock); + + /* Initialize Counter events queue */ + INIT_KFIFO(counter->events); + init_waitqueue_head(&counter->events_wait); + + /* Initialize character device */ + cdev_init(chrdev, &counter_fops); + dev->devt = MKDEV(MAJOR(counter_devt), counter->id); + cdev_set_parent(chrdev, &dev->kobj); + + return cdev_add(chrdev, dev->devt, 1); +} + +void counter_chrdev_free(struct counter_device *const counter) +{ + cdev_del(&counter->chrdev); +} + +static int counter_get_data(struct counter_device *const counter, + const struct counter_data_item *const data_item, + void *const value) +{ + const struct counter_data *const data = &data_item->data; + void *const owner = data_item->owner; + + switch (data->type) { + case COUNTER_DATA_TYPE_U8: + case COUNTER_DATA_TYPE_BOOL: + case COUNTER_DATA_TYPE_SIGNAL: + case COUNTER_DATA_TYPE_COUNT_FUNCTION: + case COUNTER_DATA_TYPE_ENUM: + case COUNTER_DATA_TYPE_COUNT_DIRECTION: + case COUNTER_DATA_TYPE_COUNT_MODE: + switch (data_item->component.owner_type) { + case COUNTER_OWNER_TYPE_DEVICE: + return data->device_u8_read(counter, value); + case COUNTER_OWNER_TYPE_SIGNAL: + return data->signal_u8_read(counter, owner, value); + case COUNTER_OWNER_TYPE_COUNT: + return data->count_u8_read(counter, owner, value); + } + break; + case COUNTER_DATA_TYPE_U64: + switch (data_item->component.owner_type) { + case COUNTER_OWNER_TYPE_DEVICE: + return data->device_u64_read(counter, value); + case COUNTER_OWNER_TYPE_SIGNAL: + return data->signal_u64_read(counter, owner, value); + case COUNTER_OWNER_TYPE_COUNT: + return data->count_u64_read(counter, owner, value); + } + break; + case COUNTER_DATA_TYPE_SYNAPSE_ACTION: + return data->action_read(counter, owner, data->priv, value); + } + + return 0; +} + +/** + * counter_push_event - queue event for userspace reading + * @counter: pointer to Counter structure + * @event: triggered event + * + * 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) +{ + struct counter_event ev = { .watch.event = event }; + unsigned int copied = 0; + unsigned long flags; + struct counter_event_item *event_item; + struct counter_data_item *data_item; + int err; + + ev.timestamp = ktime_get_ns(); + + raw_spin_lock_irqsave(&counter->events_lock, flags); + + /* Search for event in the list */ + list_for_each_entry(event_item, &counter->events_list, l) + if (event_item->event == event) + break; + + /* If event is not in the list */ + if (&event_item->l == &counter->events_list) + goto exit_early; + + /* Read and queue relevant data for userspace */ + list_for_each_entry(data_item, &event_item->data_list, l) { + err = counter_get_data(counter, data_item, &ev.value_u8); + if (err) + goto err_counter_get_data; + + ev.watch.component = data_item->component; + + copied += kfifo_put(&counter->events, ev); + } + + if (copied) + wake_up_poll(&counter->events_wait, EPOLLIN); + +exit_early: + raw_spin_unlock_irqrestore(&counter->events_lock, flags); + + return 0; + +err_counter_get_data: + raw_spin_unlock_irqrestore(&counter->events_lock, flags); + return err; +} +EXPORT_SYMBOL_GPL(counter_push_event); diff --git a/drivers/counter/counter-chrdev.h b/drivers/counter/counter-chrdev.h new file mode 100644 index 000000000000..7ab0797d3857 --- /dev/null +++ b/drivers/counter/counter-chrdev.h @@ -0,0 +1,16 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * Counter character device interface + * Copyright (C) 2020 William Breathitt Gray + */ +#ifndef _COUNTER_CHRDEV_H_ +#define _COUNTER_CHRDEV_H_ + +#include <linux/counter.h> +#include <linux/types.h> + +int counter_chrdev_add(struct counter_device *const counter, + const dev_t counter_devt); +void counter_chrdev_free(struct counter_device *const counter); + +#endif /* _COUNTER_CHRDEV_H_ */ diff --git a/drivers/counter/counter-core.c b/drivers/counter/counter-core.c index 499664809c75..d9ae889a0a8c 100644 --- a/drivers/counter/counter-core.c +++ b/drivers/counter/counter-core.c @@ -5,12 +5,16 @@ */ #include <linux/counter.h> #include <linux/device.h> +#include <linux/device/bus.h> #include <linux/export.h> +#include <linux/fs.h> #include <linux/gfp.h> #include <linux/idr.h> #include <linux/init.h> #include <linux/module.h> +#include <linux/types.h> +#include "counter-chrdev.h" #include "counter-sysfs.h" /* Provides a unique ID for each counter device */ @@ -33,6 +37,8 @@ static struct bus_type counter_bus_type = { .name = "counter" }; +static dev_t counter_devt; + /** * counter_register - register Counter to the system * @counter: pointer to Counter to register @@ -62,10 +68,15 @@ int counter_register(struct counter_device *const counter) device_initialize(dev); dev_set_drvdata(dev, counter); + /* Add Counter character device */ + err = counter_chrdev_add(counter, counter_devt); + if (err) + goto err_free_id; + /* Add Counter sysfs attributes */ err = counter_sysfs_add(counter); if (err) - goto err_free_id; + goto err_free_chrdev; /* Add device to system */ err = device_add(dev); @@ -76,6 +87,8 @@ int counter_register(struct counter_device *const counter) err_free_sysfs: counter_sysfs_free(counter); +err_free_chrdev: + counter_chrdev_free(counter); err_free_id: ida_simple_remove(&counter_ida, counter->id); return err; @@ -93,6 +106,7 @@ void counter_unregister(struct counter_device *const counter) if (counter) { device_del(&counter->dev); counter_sysfs_free(counter); + counter_chrdev_free(counter); } } EXPORT_SYMBOL_GPL(counter_unregister); @@ -139,13 +153,30 @@ int devm_counter_register(struct device *dev, } EXPORT_SYMBOL_GPL(devm_counter_register); +#define COUNTER_DEV_MAX 256 + static int __init counter_init(void) { - return bus_register(&counter_bus_type); + int err; + + err = bus_register(&counter_bus_type); + if (err < 0) + return err; + + err = alloc_chrdev_region(&counter_devt, 0, COUNTER_DEV_MAX, "counter"); + if (err < 0) + goto err_unregister_bus; + + return 0; + +err_unregister_bus: + bus_unregister(&counter_bus_type); + return err; } static void __exit counter_exit(void) { + unregister_chrdev_region(counter_devt, COUNTER_DEV_MAX); bus_unregister(&counter_bus_type); } diff --git a/include/linux/counter.h b/include/linux/counter.h index 76657d203a26..d4f2f2463ea3 100644 --- a/include/linux/counter.h +++ b/include/linux/counter.h @@ -6,10 +6,14 @@ #ifndef _COUNTER_H_ #define _COUNTER_H_ +#include <linux/cdev.h> #include <linux/device.h> #include <linux/kernel.h> +#include <linux/kfifo.h> #include <linux/list.h> +#include <linux/spinlock_types.h> #include <linux/types.h> +#include <linux/wait.h> #include <uapi/linux/counter.h> struct counter_device; @@ -166,6 +170,11 @@ struct counter_attribute_group { * @priv: optional private data supplied by driver * @id: unique ID used to identify the Counter * @dev: internal device structure + * @chrdev: internal character device structure + * @events_list: list for Counter events + * @events_lock: synchronization lock for Counter events + * @events_wait: wait queue to allow blocking reads of Counter events + * @events: queue of detected Counter events * @dynamic_names_list: list for dynamically allocated names * @groups_list: attribute groups list (for Signals, Counts, and ext) * @num_groups: number of attribute groups containers @@ -204,6 +213,11 @@ struct counter_device { int id; struct device dev; + struct cdev chrdev; + struct list_head events_list; + raw_spinlock_t events_lock; + wait_queue_head_t events_wait; + DECLARE_KFIFO(events, struct counter_event, 64); struct list_head dynamic_names_list; struct counter_attribute_group *groups_list; size_t num_groups; @@ -216,6 +230,7 @@ int devm_counter_register(struct device *dev, struct counter_device *const counter); void devm_counter_unregister(struct device *dev, struct counter_device *const counter); +int counter_push_event(struct counter_device *const counter, const u8 event); #define COUNTER_DATA_DEVICE_U8(_name, _read, _write) \ { \ diff --git a/include/uapi/linux/counter.h b/include/uapi/linux/counter.h index 2ddee9fc93e0..b903d2ad9a94 100644 --- a/include/uapi/linux/counter.h +++ b/include/uapi/linux/counter.h @@ -6,10 +6,62 @@ #ifndef _UAPI_COUNTER_H_ #define _UAPI_COUNTER_H_ +#include <linux/ioctl.h> +#include <linux/types.h> + #define COUNTER_OWNER_TYPE_DEVICE 0 #define COUNTER_OWNER_TYPE_SIGNAL 1 #define COUNTER_OWNER_TYPE_COUNT 2 +#define COUNTER_COMPONENT_TYPE_SIGNAL 0 +#define COUNTER_COMPONENT_TYPE_COUNT 1 +#define COUNTER_COMPONENT_TYPE_COUNT_FUNCTION 2 +#define COUNTER_COMPONENT_TYPE_SYNAPSE_ACTION 3 +#define COUNTER_COMPONENT_TYPE_EXTENSION 4 + +/** + * struct counter_component - Counter component identification + * @owner_type: owner type (Device, Count, or Signal) + * @owner_id: owner identification number + * @type: component type (Count, extension, etc.) + * @id: component identification number + */ +struct counter_component { + __u8 owner_type; + __u64 owner_id; + __u8 type; + __u64 id; +}; + +/** + * struct counter_watch - Counter component watch configuration + * @event: event that triggers + * @component: component to watch when event triggers + */ +struct counter_watch { + __u8 event; + struct counter_component component; +}; + +#define COUNTER_CLEAR_WATCHES_IOCTL _IO(0x3E, 0x00) +#define COUNTER_SET_WATCH_IOCTL _IOW(0x3E, 0x01, struct counter_watch) + +/** + * struct counter_event - Counter event data + * @timestamp: best estimate of time of event occurrence, in nanoseconds + * @watch: component watch configuration + * @value_u8: component value as __u8 data type + * @value_u64: component value as __u64 data type + */ +struct counter_event { + __u64 timestamp; + struct counter_watch watch; + union { + __u8 value_u8; + __u64 value_u64; + }; +}; + #define COUNTER_COUNT_DIRECTION_FORWARD 0 #define COUNTER_COUNT_DIRECTION_BACKWARD 1
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. A high-level view of how a count value is passed down from a counter driver is exemplified by the following: ---------------------- / Counter device \ +----------------------+ | Count register: 0x28 | +----------------------+ | ----------------- / raw count data / ----------------- | V +----------------------------+ | Counter device driver |----------+ +----------------------------+ | | Processes data from device | ------------------- |----------------------------| / driver callbacks / | Type: u64 | ------------------- | Value: 42 | | +----------------------------+ | | | ---------- | / u64 / | ---------- | | | | V | +----------------------+ | | Counter core | | +----------------------+ | | Routes device driver | | | callbacks to the | | | userspace interfaces | | +----------------------+ | | | ------------------- | / driver callbacks / | ------------------- | | +-------+---------------+ | | | | | +-------|-------+ | | | V | V +--------------------+ | +---------------------+ | Counter sysfs |<-+->| Counter chrdev | +--------------------+ +---------------------+ | Translates to the | | Translates to the | | standard Counter | | standard Counter | | sysfs output | | character device | |--------------------| |---------------------+ | Type: const char * | | Type: u64 | | Value: "42" | | Value: 42 | +--------------------+ +---------------------+ | | --------------- ----------------------- / const char * / / struct counter_event / --------------- ----------------------- | | | V | +-----------+ | | read | | +-----------+ | \ Count: 42 / | ----------- | V +--------------------------------------------------+ | `/sys/bus/counter/devices/counterX/countY/count` | +--------------------------------------------------+ \ Count: "42" / -------------------------------------------------- Counter character device nodes are created under the `/dev` directory as `counterX`, where `X` is the respective counter device id. Defines for the standard Counter data types are exposed via the userspace `include/uapi/linux/counter.h` file. Counter events -------------- Counter device drivers can support Counter events by utilizing the `counter_push_event` function: int counter_push_event(struct counter_device *const counter, const u8 event); The event id is specified by the `event` parameter. When this function is called, the Counter data associated with the respective event is gathered, and a `struct counter_event` is generated for each datum and pushed to userspace. Counter events can be configured by users to report various Counter data of interest. This can be conceptualized as a list of Counter component read calls to perform. For example: +------------------------+------------------------+ | Event 0 | Event 1 | +------------------------+------------------------+ | * Count 0 | * Signal 0 | | * Count 1 | * Signal 0 Extension 0 | | * Signal 3 | * Extension 4 | | * Count 4 Extension 2 | | | * Signal 5 Extension 0 | | +------------------------+------------------------+ 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); for (;;) { poll(&pfd, 1, -1); read(pfd.fd, event_data, sizeof(event_data)); printf("Timestamp 0: %llu\nCount 0: %llu\n" "Timestamp 1: %llu\nCount 1: %llu\n", (unsigned long long)event_data[0].timestamp, (unsigned long long)event_data[0].value_u64, (unsigned long long)event_data[1].timestamp, (unsigned long long)event_data[1].value_u64); } return 0; } Cc: David Lechner <david@lechnology.com> Cc: Gwendal Grignou <gwendal@chromium.org> Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com> --- drivers/counter/Makefile | 2 +- drivers/counter/counter-chrdev.c | 441 +++++++++++++++++++++++++++++++ drivers/counter/counter-chrdev.h | 16 ++ drivers/counter/counter-core.c | 35 ++- include/linux/counter.h | 15 ++ include/uapi/linux/counter.h | 52 ++++ 6 files changed, 558 insertions(+), 3 deletions(-) create mode 100644 drivers/counter/counter-chrdev.c create mode 100644 drivers/counter/counter-chrdev.h