| Message ID | 54190f9875b81b6aa5483a7710b084053a44abb8.1601170670.git.vilhelm.gray@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce the Counter character device interface | expand |
Hi! > + 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); > + ioctl(pfd.fd, COUNTER_LOAD_WATCHES_IOCTL); > + > + for (;;) { > + poll(&pfd, 1, -1); Why do poll, when you are doing blocking read? > + read(pfd.fd, event_data, sizeof(event_data)); Does your new chrdev always guarantee returning complete buffer? If so, should it behave like that? Best regards, Pavel
On Thu, Oct 08, 2020 at 10:09:09AM +0200, Pavel Machek wrote: > Hi! > > > + 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); > > + ioctl(pfd.fd, COUNTER_LOAD_WATCHES_IOCTL); > > + > > + for (;;) { > > + poll(&pfd, 1, -1); > > Why do poll, when you are doing blocking read? > > > + read(pfd.fd, event_data, sizeof(event_data)); > > Does your new chrdev always guarantee returning complete buffer? > > If so, should it behave like that? > > Best regards, > Pavel > -- > (english) http://www.livejournal.com/~pavelmachek > (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html I suppose you're right: a poll() should be redundant now with this version of the character device implementation because buffers will always return complete; so a blocking read() should achieve the same behavior that a poll() with read() would. I'll give some more time for additional feedback to come in for this version of the patchset, and then likely remove support for poll() in the v6 submission. William Breathitt Gray
On 10/8/20 7:28 AM, William Breathitt Gray wrote: > On Thu, Oct 08, 2020 at 10:09:09AM +0200, Pavel Machek wrote: >> Hi! >> >>> + 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); >>> + ioctl(pfd.fd, COUNTER_LOAD_WATCHES_IOCTL); >>> + >>> + for (;;) { >>> + poll(&pfd, 1, -1); >> >> Why do poll, when you are doing blocking read? >> >>> + read(pfd.fd, event_data, sizeof(event_data)); >> >> Does your new chrdev always guarantee returning complete buffer? >> >> If so, should it behave like that? >> >> Best regards, >> Pavel >> -- >> (english) http://www.livejournal.com/~pavelmachek >> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html > > I suppose you're right: a poll() should be redundant now with this > version of the character device implementation because buffers will > always return complete; so a blocking read() should achieve the same > behavior that a poll() with read() would. > > I'll give some more time for additional feedback to come in for this > version of the patchset, and then likely remove support for poll() in > the v6 submission. > > William Breathitt Gray > I hope that you mean that you will just remove it from the example and not from the chardev. Otherwise it won't be possible to integrate this with an event loop.
On Mon, Oct 12, 2020 at 12:04:10PM -0500, David Lechner wrote: > On 10/8/20 7:28 AM, William Breathitt Gray wrote: > > On Thu, Oct 08, 2020 at 10:09:09AM +0200, Pavel Machek wrote: > >> Hi! > >> > >>> + 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); > >>> + ioctl(pfd.fd, COUNTER_LOAD_WATCHES_IOCTL); > >>> + > >>> + for (;;) { > >>> + poll(&pfd, 1, -1); > >> > >> Why do poll, when you are doing blocking read? > >> > >>> + read(pfd.fd, event_data, sizeof(event_data)); > >> > >> Does your new chrdev always guarantee returning complete buffer? > >> > >> If so, should it behave like that? > >> > >> Best regards, > >> Pavel > >> -- > >> (english) http://www.livejournal.com/~pavelmachek > >> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html > > > > I suppose you're right: a poll() should be redundant now with this > > version of the character device implementation because buffers will > > always return complete; so a blocking read() should achieve the same > > behavior that a poll() with read() would. > > > > I'll give some more time for additional feedback to come in for this > > version of the patchset, and then likely remove support for poll() in > > the v6 submission. > > > > William Breathitt Gray > > > > I hope that you mean that you will just remove it from the example > and not from the chardev. Otherwise it won't be possible to > integrate this with an event loop. Would you elaborate a bit further on this? My thought process is that because users must set the Counter Events they want to watch, and only those Counter Events show up in the character device node, a blocking read() would effectively behave the same as poll() with read(); if none of the Counter Events occur, the read() just blocks until one does, thus making the use of a poll() call redundant. William Breathitt Gray
On 10/13/20 1:58 PM, William Breathitt Gray wrote: > On Mon, Oct 12, 2020 at 12:04:10PM -0500, David Lechner wrote: >> On 10/8/20 7:28 AM, William Breathitt Gray wrote: >>> On Thu, Oct 08, 2020 at 10:09:09AM +0200, Pavel Machek wrote: >>>> Hi! >>>> >>>>> + 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); >>>>> + ioctl(pfd.fd, COUNTER_LOAD_WATCHES_IOCTL); >>>>> + >>>>> + for (;;) { >>>>> + poll(&pfd, 1, -1); >>>> >>>> Why do poll, when you are doing blocking read? >>>> >>>>> + read(pfd.fd, event_data, sizeof(event_data)); >>>> >>>> Does your new chrdev always guarantee returning complete buffer? >>>> >>>> If so, should it behave like that? >>>> >>>> Best regards, >>>> Pavel >>>> -- >>>> (english) http://www.livejournal.com/~pavelmachek >>>> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html >>> >>> I suppose you're right: a poll() should be redundant now with this >>> version of the character device implementation because buffers will >>> always return complete; so a blocking read() should achieve the same >>> behavior that a poll() with read() would. >>> >>> I'll give some more time for additional feedback to come in for this >>> version of the patchset, and then likely remove support for poll() in >>> the v6 submission. >>> >>> William Breathitt Gray >>> >> >> I hope that you mean that you will just remove it from the example >> and not from the chardev. Otherwise it won't be possible to >> integrate this with an event loop. > > Would you elaborate a bit further on this? My thought process is that > because users must set the Counter Events they want to watch, and only > those Counter Events show up in the character device node, a blocking > read() would effectively behave the same as poll() with read(); if none > of the Counter Events occur, the read() just blocks until one does, thus > making the use of a poll() call redundant. > > William Breathitt Gray > If the counter device was the only file descriptor being read, then yes it wouldn't matter. But if we are using this in combination with other file descriptors, then it is common to poll all of the file descriptors using a single syscall to see which one is ready to read rather than doing a non-blocking read on all of the file descriptors, which would result in many unnecessary syscalls.
On Tue, Oct 13, 2020 at 02:08:45PM -0500, David Lechner wrote: > On 10/13/20 1:58 PM, William Breathitt Gray wrote: > > On Mon, Oct 12, 2020 at 12:04:10PM -0500, David Lechner wrote: > >> On 10/8/20 7:28 AM, William Breathitt Gray wrote: > >>> On Thu, Oct 08, 2020 at 10:09:09AM +0200, Pavel Machek wrote: > >>>> Hi! > >>>> > >>>>> + 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); > >>>>> + ioctl(pfd.fd, COUNTER_LOAD_WATCHES_IOCTL); > >>>>> + > >>>>> + for (;;) { > >>>>> + poll(&pfd, 1, -1); > >>>> > >>>> Why do poll, when you are doing blocking read? > >>>> > >>>>> + read(pfd.fd, event_data, sizeof(event_data)); > >>>> > >>>> Does your new chrdev always guarantee returning complete buffer? > >>>> > >>>> If so, should it behave like that? > >>>> > >>>> Best regards, > >>>> Pavel > >>>> -- > >>>> (english) http://www.livejournal.com/~pavelmachek > >>>> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html > >>> > >>> I suppose you're right: a poll() should be redundant now with this > >>> version of the character device implementation because buffers will > >>> always return complete; so a blocking read() should achieve the same > >>> behavior that a poll() with read() would. > >>> > >>> I'll give some more time for additional feedback to come in for this > >>> version of the patchset, and then likely remove support for poll() in > >>> the v6 submission. > >>> > >>> William Breathitt Gray > >>> > >> > >> I hope that you mean that you will just remove it from the example > >> and not from the chardev. Otherwise it won't be possible to > >> integrate this with an event loop. > > > > Would you elaborate a bit further on this? My thought process is that > > because users must set the Counter Events they want to watch, and only > > those Counter Events show up in the character device node, a blocking > > read() would effectively behave the same as poll() with read(); if none > > of the Counter Events occur, the read() just blocks until one does, thus > > making the use of a poll() call redundant. > > > > William Breathitt Gray > > > > If the counter device was the only file descriptor being read, then yes > it wouldn't matter. But if we are using this in combination with other > file descriptors, then it is common to poll all of the file descriptors > using a single syscall to see which one is ready to read rather than > doing a non-blocking read on all of the file descriptors, which would > result in many unnecessary syscalls. Ah, that's a fair point, my original view was somewhat myopic there. I'll leave poll support in the Counter chrdev then and just simplify the documentation example code to not use it. William Breathitt Gray
diff --git a/Documentation/ABI/testing/sysfs-bus-counter b/Documentation/ABI/testing/sysfs-bus-counter index 566bd99fe0a5..b7fdb14ae891 100644 --- a/Documentation/ABI/testing/sysfs-bus-counter +++ b/Documentation/ABI/testing/sysfs-bus-counter @@ -99,6 +99,24 @@ Description: Read-only attribute that indicates whether excessive noise is present at the channel Y counter inputs. +What: /sys/bus/counter/devices/counterX/countY/extensionZ_name +What: /sys/bus/counter/devices/counterX/extensionZ_name +What: /sys/bus/counter/devices/counterX/signalY/extensionZ_name +KernelVersion: 5.11 +Contact: linux-iio@vger.kernel.org +Description: + Read-only attribute that indicates the component name of + Extension Z. + +What: /sys/bus/counter/devices/counterX/countY/extensionZ_width +What: /sys/bus/counter/devices/counterX/extensionZ_width +What: /sys/bus/counter/devices/counterX/signalY/extensionZ_width +KernelVersion: 5.11 +Contact: linux-iio@vger.kernel.org +Description: + Read-only attribute that indicates the data width of value of + Extension Z. + What: /sys/bus/counter/devices/counterX/countY/function KernelVersion: 5.2 Contact: linux-iio@vger.kernel.org diff --git a/Documentation/driver-api/generic-counter.rst b/Documentation/driver-api/generic-counter.rst index b842ddbbd8a0..6077bf162ac3 100644 --- a/Documentation/driver-api/generic-counter.rst +++ b/Documentation/driver-api/generic-counter.rst @@ -223,19 +223,6 @@ whether an input line is differential or single-ended) and instead focus on the core idea of what the data and process represent (e.g. position as interpreted from quadrature encoding data). -Userspace Interface -=================== - -Several sysfs attributes are generated by the Generic Counter interface, -and reside under the /sys/bus/counter/devices/counterX directory, where -counterX refers to the respective counter device. Please see -Documentation/ABI/testing/sysfs-bus-counter for detailed -information on each Generic Counter interface sysfs attribute. - -Through these sysfs attributes, programs and scripts may interact with -the Generic Counter paradigm Counts, Signals, and Synapses of respective -counter devices. - Driver API ========== @@ -387,16 +374,16 @@ userspace interface components:: / driver callbacks / ------------------- | - +---------------+ - | - V - +--------------------+ - | Counter sysfs | - +--------------------+ - | Translates to the | - | standard Counter | - | sysfs output | - +--------------------+ + +---------------+---------------+ + | | + V V + +--------------------+ +---------------------+ + | Counter sysfs | | Counter chrdev | + +--------------------+ +---------------------+ + | Translates to the | | Translates to the | + | standard Counter | | standard Counter | + | sysfs output | | character device | + +--------------------+ +---------------------+ Thereafter, data can be transferred directly between the Counter device driver and Counter userspace interface:: @@ -427,23 +414,30 @@ driver and Counter userspace interface:: / u64 / ---------- | - +---------------+ - | - V - +--------------------+ - | Counter sysfs | - +--------------------+ - | Translates to the | - | standard Counter | - | sysfs output | - |--------------------| - | Type: const char * | - | Value: "42" | - +--------------------+ - | - --------------- - / const char * / - --------------- + +---------------+---------------+ + | | + 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 +--------------------------------------------------+ @@ -452,7 +446,7 @@ driver and Counter userspace interface:: \ Count: "42" / -------------------------------------------------- -There are three primary components involved: +There are four primary components involved: Counter device driver --------------------- @@ -472,3 +466,155 @@ and vice versa. Please refer to the `Documentation/ABI/testing/sysfs-bus-counter` file for a detailed breakdown of the available Generic Counter interface sysfs attributes. + +Counter chrdev +-------------- +Translates counter data to the standard Counter character device; data +is transferred via standard character device read calls, while Counter +events are configured via ioctl calls. + +Sysfs Interface +=============== + +Several sysfs attributes are generated by the Generic Counter interface, +and reside under the `/sys/bus/counter/devices/counterX` directory, +where `X` is to the respective counter device id. Please see +`Documentation/ABI/testing/sysfs-bus-counter` for detailed information +on each Generic Counter interface sysfs attribute. + +Through these sysfs attributes, programs and scripts may interact with +the Generic Counter paradigm Counts, Signals, and Synapses of respective +counter devices. + +Counter Character Device +======================== + +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, + const u8 channel); + +The event id is specified by the `event` parameter; the event channel id +is specified by the `channel` 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:: + + +~~~~~~~~~~~~~~~~~~~~~~~~+~~~~~~~~~~~~~~~~~~~~~~~~+ + | COUNTER_EVENT_OVERFLOW | COUNTER_EVENT_INDEX | + +~~~~~~~~~~~~~~~~~~~~~~~~+~~~~~~~~~~~~~~~~~~~~~~~~+ + | Channel 0 | Channel 0 | + +------------------------+------------------------+ + | * Count 0 | * Signal 0 | + | * Count 1 | * Signal 0 Extension 0 | + | * Signal 3 | * Extension 4 | + | * Count 4 Extension 2 +------------------------+ + | * Signal 5 Extension 0 | Channel 1 | + | +------------------------+ + | | * Signal 4 | + | | * Signal 4 Extension 0 | + | | * Count 7 | + +------------------------+------------------------+ + +When `counter_push_event(counter, COUNTER_EVENT_INDEX, 1)` is called for +example, it will go down the list for the `COUNTER_EVENT_INDEX` event +channel 1 and execute the read callbacks for Signal 4, Signal 4 +Extension 0, and Count 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 for the specified event + +* COUNTER_LOAD_WATCHES_IOCTL: + Activates the Counter watches set earlier + +To configure events to gather Counter data, users first populate a +`struct counter_watch` with the relevant event id, event channel 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. + +The `COUNTER_SET_WATCH_IOCTL` command will buffer these Counter watches. +When ready, the `COUNTER_LOAD_WATCHES_IOCTL` ioctl command may be used +to activate these Counter watches. + +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 the `COUNTER_EVENT_INDEX` event channel 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 = COUNTER_EVENT_INDEX, + .channel = 0, + .component.scope = COUNTER_SCOPE_COUNT, + .component.parent = 0, + .component.type = COUNTER_COMPONENT_COUNT, + }, + { + .event = COUNTER_EVENT_INDEX, + .channel = 0, + .component.scope = COUNTER_SCOPE_COUNT, + .component.parent = 1, + .component.type = COUNTER_COMPONENT_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); + ioctl(pfd.fd, COUNTER_LOAD_WATCHES_IOCTL); + + 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; + } diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst b/Documentation/userspace-api/ioctl/ioctl-number.rst index 2a198838fca9..f6e96bb780cd 100644 --- a/Documentation/userspace-api/ioctl/ioctl-number.rst +++ b/Documentation/userspace-api/ioctl/ioctl-number.rst @@ -88,6 +88,7 @@ Code Seq# Include File Comments <http://infiniband.sourceforge.net/> 0x20 all drivers/cdrom/cm206.h 0x22 all scsi/sg.h +0x3E 00-0F linux/counter.h <mailto:linux-iio@vger.kernel.org> '!' 00-1F uapi/linux/seccomp.h '#' 00-3F IEEE 1394 Subsystem Block for the entire subsystem
This patch adds high-level documentation about the Counter subsystem character device interface. Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com> --- Documentation/ABI/testing/sysfs-bus-counter | 18 ++ Documentation/driver-api/generic-counter.rst | 228 ++++++++++++++---- .../userspace-api/ioctl/ioctl-number.rst | 1 + 3 files changed, 206 insertions(+), 41 deletions(-)