| Message ID | cover.1588176662.git.vilhelm.gray@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Introduce the Counter character device interface | expand |
On 4/29/20 1:11 PM, William Breathitt Gray wrote: > Over the past couple years we have noticed some shortcomings with the > Counter sysfs interface. Although useful in the majority of situations, > there are certain use-cases where interacting through sysfs attributes > can become cumbersome and inefficient. A desire to support more advanced > functionality such as timestamps, multi-axis positioning tables, and > other such latency-sensitive applications, has motivated a reevaluation > of the Counter subsystem. I believe a character device interface will be > helpful for this more niche area of counter device use. Nice to see some progress being made. :-) > > Interaction with Counter character devices occurs via ioctl commands. > This allows userspace applications to access and set counter data using > native C datatypes rather than working through string translations. For most aspects of the counter subsystem, this is not an issue since configuring a counter is not a time-sensitive operation. Instead of ioctls, I was expecting to just be able to read the character device and receive counter events or poll to wait for events similar to how the input subsystem works or how buffers work in the iio subsystem. I'm afraid I don't really see much use in having ioctls that do exactly what sysfs already does. And my intuition tells me that the extra work needed to maintain it will probably cost more than any benefit gained. (Maybe other have a different experience that leads to a different conclusion?)
Hi, On 29/04/2020 14:11:34-0400, William Breathitt Gray wrote: > Over the past couple years we have noticed some shortcomings with the > Counter sysfs interface. Although useful in the majority of situations, > there are certain use-cases where interacting through sysfs attributes > can become cumbersome and inefficient. A desire to support more advanced > functionality such as timestamps, multi-axis positioning tables, and > other such latency-sensitive applications, has motivated a reevaluation > of the Counter subsystem. I believe a character device interface will be > helpful for this more niche area of counter device use. > > To quell any concerns from the offset: this patchset makes no changes to > the existing Counter sysfs userspace interface -- existing userspace > applications will continue to work with no modifications necessary. I > request that driver maintainers please test their applications to verify > that this is true, and report any discrepancies if they arise. > On that topic, I'm wondering why the counter subsystem uses /sys/bus instead of /sys/class that would be more natural for a class of devices. I can't see how counters would be considered busses. I think you should consider moving it over to /sys/class (even if deprecating /sys/bus/counter will be long). > Interaction with Counter character devices occurs via ioctl commands. > This allows userspace applications to access and set counter data using > native C datatypes rather than working through string translations. > I agree with David that you should consider using read to retrieve the counter data as this will simplify interrupt handling/polling and blocking/non-blocking reads can be used by an application. ABI wise, this can also be a good move as you could always consider having an ioctl requesting a specific format when reading the device so you are not stuck with the initial format you are going to choose. > 2. Should device driver callbacks return int or long? I sometimes see > error values returned as long (e.g. PTR_ERR(), the file_operations > structure's ioctl callbacks, etc.); when is it necessary to return > long as opposed to int? > You should use a long if you ever have to return a point as it is guaranteed to have the correct size. Else, just stick to an int if you are not going to overflow it. > 3. I only implemented the unlocked_ioctl callback. Should I implement a > compat_ioctl callback as well? > The compat_ioctl is to handle 32bit userspace running on a 64bit kernel. If your structures have the same size in both cases, then you don't have to implement compat_ioctl. Have a look at Documentation/driver-api/ioctl.rst
On Thu, Apr 30, 2020 at 10:13:45PM +0200, Alexandre Belloni wrote: > Hi, > > On 29/04/2020 14:11:34-0400, William Breathitt Gray wrote: > > Over the past couple years we have noticed some shortcomings with the > > Counter sysfs interface. Although useful in the majority of situations, > > there are certain use-cases where interacting through sysfs attributes > > can become cumbersome and inefficient. A desire to support more advanced > > functionality such as timestamps, multi-axis positioning tables, and > > other such latency-sensitive applications, has motivated a reevaluation > > of the Counter subsystem. I believe a character device interface will be > > helpful for this more niche area of counter device use. > > > > To quell any concerns from the offset: this patchset makes no changes to > > the existing Counter sysfs userspace interface -- existing userspace > > applications will continue to work with no modifications necessary. I > > request that driver maintainers please test their applications to verify > > that this is true, and report any discrepancies if they arise. > > > > On that topic, I'm wondering why the counter subsystem uses /sys/bus > instead of /sys/class that would be more natural for a class of devices. > I can't see how counters would be considered busses. I think you should > consider moving it over to /sys/class (even if deprecating > /sys/bus/counter will be long). At the time I wasn't quite familiar with sysfs development so I was following the iio sysfs code rather closely. However, I see now that you're probably right: this isn't really a bus but rather a collection of various types of counters -- i.e. a class of devices. Perhaps I should migrate this then to /sys/class/counter. Of course, the /sys/bus/counter location will have to remain for compatibility with existing applications, but I think a simple symlink to the new /sys/class/counter location should suffice for that. If anyone sees an issue with this give me a heads up. > > Interaction with Counter character devices occurs via ioctl commands. > > This allows userspace applications to access and set counter data using > > native C datatypes rather than working through string translations. > > > > I agree with David that you should consider using read to retrieve the > counter data as this will simplify interrupt handling/polling and > blocking/non-blocking reads can be used by an application. ABI wise, > this can also be a good move as you could always consider having an > ioctl requesting a specific format when reading the device so you are > not stuck with the initial format you are going to choose. My hesitation to implement support for read/write calls is due to a concern that we will end up with various incompatible formats between counter drivers (thus requiring users to have intimate knowledge of the drivers and therefore defeating the purpose of a subsystem). However, if we can standardize on a format that is flexible enough to work for all counter drivers, then read/write calls should not be a problem. I think a general format could be possible. For example, the counter character device can return a standard header data at the start which provides general information about the counter device: number of counters, number or signals, number of extensions, etc. From this information, offsets can be computed (or perhaps provided by the device) to where the binary data for the count, extension, etc., can be read or written. Interrupts can then be handled as blocking reads, as could other types of events we implement. Would something like this work well? William Breathitt Gray > > 2. Should device driver callbacks return int or long? I sometimes see > > error values returned as long (e.g. PTR_ERR(), the file_operations > > structure's ioctl callbacks, etc.); when is it necessary to return > > long as opposed to int? > > > > You should use a long if you ever have to return a point as it is > guaranteed to have the correct size. Else, just stick to an int if you > are not going to overflow it. > > > 3. I only implemented the unlocked_ioctl callback. Should I implement a > > compat_ioctl callback as well? > > > > The compat_ioctl is to handle 32bit userspace running on a 64bit kernel. > If your structures have the same size in both cases, then you don't have > to implement compat_ioctl. > > Have a look at Documentation/driver-api/ioctl.rst > > > -- > Alexandre Belloni, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com
On Fri, 1 May 2020 11:46:10 -0400 William Breathitt Gray <vilhelm.gray@gmail.com> wrote: > On Thu, Apr 30, 2020 at 10:13:45PM +0200, Alexandre Belloni wrote: > > Hi, > > > > On 29/04/2020 14:11:34-0400, William Breathitt Gray wrote: > > > Over the past couple years we have noticed some shortcomings with the > > > Counter sysfs interface. Although useful in the majority of situations, > > > there are certain use-cases where interacting through sysfs attributes > > > can become cumbersome and inefficient. A desire to support more advanced > > > functionality such as timestamps, multi-axis positioning tables, and > > > other such latency-sensitive applications, has motivated a reevaluation > > > of the Counter subsystem. I believe a character device interface will be > > > helpful for this more niche area of counter device use. > > > > > > To quell any concerns from the offset: this patchset makes no changes to > > > the existing Counter sysfs userspace interface -- existing userspace > > > applications will continue to work with no modifications necessary. I > > > request that driver maintainers please test their applications to verify > > > that this is true, and report any discrepancies if they arise. > > > > > > > On that topic, I'm wondering why the counter subsystem uses /sys/bus > > instead of /sys/class that would be more natural for a class of devices. > > I can't see how counters would be considered busses. I think you should > > consider moving it over to /sys/class (even if deprecating > > /sys/bus/counter will be long). > > At the time I wasn't quite familiar with sysfs development so I was > following the iio sysfs code rather closely. However, I see now that > you're probably right: this isn't really a bus but rather a collection > of various types of counters -- i.e. a class of devices. > > Perhaps I should migrate this then to /sys/class/counter. Of course, the > /sys/bus/counter location will have to remain for compatibility with > existing applications, but I think a simple symlink to the new > /sys/class/counter location should suffice for that. > > If anyone sees an issue with this give me a heads up. To just address this point as I've not read the rest of the thread yet... I would resist moving it. This one is an old argument. Some info in https://lwn.net/Articles/645810/ As that puts it a "bus" is better known as a "subsystem". When we originally considered class vs bus for IIO, the view expressed at the times was that the whole separation of the two didn't mean anything and for non trivial cases bus was always preferred. It's nothing to do with with whether the thing is a bus or not. Now I suppose it's possible opinion has moved on this topic... However, I'd say there is really 0 advantage in moving an existing subsystem even if opinion has changed. +CC Greg in case he wants to add anything. > > > > Interaction with Counter character devices occurs via ioctl commands. > > > This allows userspace applications to access and set counter data using > > > native C datatypes rather than working through string translations. > > > > > > > I agree with David that you should consider using read to retrieve the > > counter data as this will simplify interrupt handling/polling and > > blocking/non-blocking reads can be used by an application. ABI wise, > > this can also be a good move as you could always consider having an > > ioctl requesting a specific format when reading the device so you are > > not stuck with the initial format you are going to choose. > > My hesitation to implement support for read/write calls is due to a > concern that we will end up with various incompatible formats between > counter drivers (thus requiring users to have intimate knowledge of the > drivers and therefore defeating the purpose of a subsystem). However, if > we can standardize on a format that is flexible enough to work for all > counter drivers, then read/write calls should not be a problem. > > I think a general format could be possible. For example, the counter > character device can return a standard header data at the start which > provides general information about the counter device: number of > counters, number or signals, number of extensions, etc. From this > information, offsets can be computed (or perhaps provided by the device) > to where the binary data for the count, extension, etc., can be read or > written. Interrupts can then be handled as blocking reads, as could > other types of events we implement. > > Would something like this work well? > > William Breathitt Gray > > > > 2. Should device driver callbacks return int or long? I sometimes see > > > error values returned as long (e.g. PTR_ERR(), the file_operations > > > structure's ioctl callbacks, etc.); when is it necessary to return > > > long as opposed to int? > > > > > > > You should use a long if you ever have to return a point as it is > > guaranteed to have the correct size. Else, just stick to an int if you > > are not going to overflow it. > > > > > 3. I only implemented the unlocked_ioctl callback. Should I implement a > > > compat_ioctl callback as well? > > > > > > > The compat_ioctl is to handle 32bit userspace running on a 64bit kernel. > > If your structures have the same size in both cases, then you don't have > > to implement compat_ioctl. > > > > Have a look at Documentation/driver-api/ioctl.rst > > > > > > -- > > Alexandre Belloni, Bootlin > > Embedded Linux and Kernel engineering > > https://bootlin.com
On Sat, May 02, 2020 at 05:55:36PM +0100, Jonathan Cameron wrote: > On Fri, 1 May 2020 11:46:10 -0400 > William Breathitt Gray <vilhelm.gray@gmail.com> wrote: > > > On Thu, Apr 30, 2020 at 10:13:45PM +0200, Alexandre Belloni wrote: > > > Hi, > > > > > > On 29/04/2020 14:11:34-0400, William Breathitt Gray wrote: > > > > Over the past couple years we have noticed some shortcomings with the > > > > Counter sysfs interface. Although useful in the majority of situations, > > > > there are certain use-cases where interacting through sysfs attributes > > > > can become cumbersome and inefficient. A desire to support more advanced > > > > functionality such as timestamps, multi-axis positioning tables, and > > > > other such latency-sensitive applications, has motivated a reevaluation > > > > of the Counter subsystem. I believe a character device interface will be > > > > helpful for this more niche area of counter device use. > > > > > > > > To quell any concerns from the offset: this patchset makes no changes to > > > > the existing Counter sysfs userspace interface -- existing userspace > > > > applications will continue to work with no modifications necessary. I > > > > request that driver maintainers please test their applications to verify > > > > that this is true, and report any discrepancies if they arise. > > > > > > > > > > On that topic, I'm wondering why the counter subsystem uses /sys/bus > > > instead of /sys/class that would be more natural for a class of devices. > > > I can't see how counters would be considered busses. I think you should > > > consider moving it over to /sys/class (even if deprecating > > > /sys/bus/counter will be long). > > > > At the time I wasn't quite familiar with sysfs development so I was > > following the iio sysfs code rather closely. However, I see now that > > you're probably right: this isn't really a bus but rather a collection > > of various types of counters -- i.e. a class of devices. > > > > Perhaps I should migrate this then to /sys/class/counter. Of course, the > > /sys/bus/counter location will have to remain for compatibility with > > existing applications, but I think a simple symlink to the new > > /sys/class/counter location should suffice for that. > > > > If anyone sees an issue with this give me a heads up. > To just address this point as I've not read the rest of the thread yet... > > I would resist moving it. This one is an old argument. > > Some info in https://lwn.net/Articles/645810/ > As that puts it a "bus" is better known as a "subsystem". > > When we originally considered class vs bus for IIO, the view expressed > at the times was that the whole separation of the two didn't mean anything > and for non trivial cases bus was always preferred. It's nothing to do > with with whether the thing is a bus or not. Now I suppose it's possible > opinion has moved on this topic... However, I'd say there > is really 0 advantage in moving an existing subsystem even if opinion > has changed. > > +CC Greg in case he wants to add anything. Traditionally classes are a unified way of representing data to userspace, independant of the physical transport that the data came to userspace on (i.e. input devices are a class, it doesn't matter if they came on serial, USB, PS/2, or virtual busses.) A bus is traditionally a collection of drivers that all talk on a same physical transport, that then expose data from that transport to a specific userspace class. Again, think USB mice drivers, serial mice drivers, PS/2 mice drivers. Busses bind a driver to a device it creates based on that "bus". Classes create virtual devices that export data to userspace for a specific common protocol. Does that help? One can argue (and have properly in the past), that classes and busses really are not all that different, and there used to be code floating around that made them the same exact thing in the kernel, with loads of userspace sysfs symlinks to preserve things, but those are well out of date and I don't think anyone feels like reviving them. However I think systemd might still have code in it to work properly if that ever happens, haven't looked in a few years... thanks, greg k-h
On Sun, 3 May 2020 11:23:16 +0200 Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > On Sat, May 02, 2020 at 05:55:36PM +0100, Jonathan Cameron wrote: > > On Fri, 1 May 2020 11:46:10 -0400 > > William Breathitt Gray <vilhelm.gray@gmail.com> wrote: > > > > > On Thu, Apr 30, 2020 at 10:13:45PM +0200, Alexandre Belloni wrote: > > > > Hi, > > > > > > > > On 29/04/2020 14:11:34-0400, William Breathitt Gray wrote: > > > > > Over the past couple years we have noticed some shortcomings with the > > > > > Counter sysfs interface. Although useful in the majority of situations, > > > > > there are certain use-cases where interacting through sysfs attributes > > > > > can become cumbersome and inefficient. A desire to support more advanced > > > > > functionality such as timestamps, multi-axis positioning tables, and > > > > > other such latency-sensitive applications, has motivated a reevaluation > > > > > of the Counter subsystem. I believe a character device interface will be > > > > > helpful for this more niche area of counter device use. > > > > > > > > > > To quell any concerns from the offset: this patchset makes no changes to > > > > > the existing Counter sysfs userspace interface -- existing userspace > > > > > applications will continue to work with no modifications necessary. I > > > > > request that driver maintainers please test their applications to verify > > > > > that this is true, and report any discrepancies if they arise. > > > > > > > > > > > > > On that topic, I'm wondering why the counter subsystem uses /sys/bus > > > > instead of /sys/class that would be more natural for a class of devices. > > > > I can't see how counters would be considered busses. I think you should > > > > consider moving it over to /sys/class (even if deprecating > > > > /sys/bus/counter will be long). > > > > > > At the time I wasn't quite familiar with sysfs development so I was > > > following the iio sysfs code rather closely. However, I see now that > > > you're probably right: this isn't really a bus but rather a collection > > > of various types of counters -- i.e. a class of devices. > > > > > > Perhaps I should migrate this then to /sys/class/counter. Of course, the > > > /sys/bus/counter location will have to remain for compatibility with > > > existing applications, but I think a simple symlink to the new > > > /sys/class/counter location should suffice for that. > > > > > > If anyone sees an issue with this give me a heads up. > > To just address this point as I've not read the rest of the thread yet... > > > > I would resist moving it. This one is an old argument. > > > > Some info in https://lwn.net/Articles/645810/ > > As that puts it a "bus" is better known as a "subsystem". > > > > When we originally considered class vs bus for IIO, the view expressed > > at the times was that the whole separation of the two didn't mean anything > > and for non trivial cases bus was always preferred. It's nothing to do > > with with whether the thing is a bus or not. Now I suppose it's possible > > opinion has moved on this topic... However, I'd say there > > is really 0 advantage in moving an existing subsystem even if opinion > > has changed. > > > > +CC Greg in case he wants to add anything. > > Traditionally classes are a unified way of representing data to > userspace, independant of the physical transport that the data came to > userspace on (i.e. input devices are a class, it doesn't matter if they > came on serial, USB, PS/2, or virtual busses.) > > A bus is traditionally a collection of drivers that all talk on a same > physical transport, that then expose data from that transport to a > specific userspace class. Again, think USB mice drivers, serial mice > drivers, PS/2 mice drivers. > > Busses bind a driver to a device it creates based on that "bus". > Classes create virtual devices that export data to userspace for a > specific common protocol. > > Does that help? > > One can argue (and have properly in the past), that classes and busses > really are not all that different, and there used to be code floating > around that made them the same exact thing in the kernel, with loads of > userspace sysfs symlinks to preserve things, but those are well out of > date and I don't think anyone feels like reviving them. However I think > systemd might still have code in it to work properly if that ever > happens, haven't looked in a few years... > > thanks, > > greg k-h Thanks for the explanation. Here key thing to my mind is counters went in as a bus and should stay so because there is limited benefit in a move and it would be ABI breaking. Maybe it 'should' have been a class, but too late now. Jonathan
On Sun, May 03, 2020 at 01:54:58PM +0100, Jonathan Cameron wrote: > On Sun, 3 May 2020 11:23:16 +0200 > Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > > On Sat, May 02, 2020 at 05:55:36PM +0100, Jonathan Cameron wrote: > > > On Fri, 1 May 2020 11:46:10 -0400 > > > William Breathitt Gray <vilhelm.gray@gmail.com> wrote: > > > > > > > On Thu, Apr 30, 2020 at 10:13:45PM +0200, Alexandre Belloni wrote: > > > > > Hi, > > > > > > > > > > On 29/04/2020 14:11:34-0400, William Breathitt Gray wrote: > > > > > > Over the past couple years we have noticed some shortcomings with the > > > > > > Counter sysfs interface. Although useful in the majority of situations, > > > > > > there are certain use-cases where interacting through sysfs attributes > > > > > > can become cumbersome and inefficient. A desire to support more advanced > > > > > > functionality such as timestamps, multi-axis positioning tables, and > > > > > > other such latency-sensitive applications, has motivated a reevaluation > > > > > > of the Counter subsystem. I believe a character device interface will be > > > > > > helpful for this more niche area of counter device use. > > > > > > > > > > > > To quell any concerns from the offset: this patchset makes no changes to > > > > > > the existing Counter sysfs userspace interface -- existing userspace > > > > > > applications will continue to work with no modifications necessary. I > > > > > > request that driver maintainers please test their applications to verify > > > > > > that this is true, and report any discrepancies if they arise. > > > > > > > > > > > > > > > > On that topic, I'm wondering why the counter subsystem uses /sys/bus > > > > > instead of /sys/class that would be more natural for a class of devices. > > > > > I can't see how counters would be considered busses. I think you should > > > > > consider moving it over to /sys/class (even if deprecating > > > > > /sys/bus/counter will be long). > > > > > > > > At the time I wasn't quite familiar with sysfs development so I was > > > > following the iio sysfs code rather closely. However, I see now that > > > > you're probably right: this isn't really a bus but rather a collection > > > > of various types of counters -- i.e. a class of devices. > > > > > > > > Perhaps I should migrate this then to /sys/class/counter. Of course, the > > > > /sys/bus/counter location will have to remain for compatibility with > > > > existing applications, but I think a simple symlink to the new > > > > /sys/class/counter location should suffice for that. > > > > > > > > If anyone sees an issue with this give me a heads up. > > > To just address this point as I've not read the rest of the thread yet... > > > > > > I would resist moving it. This one is an old argument. > > > > > > Some info in https://lwn.net/Articles/645810/ > > > As that puts it a "bus" is better known as a "subsystem". > > > > > > When we originally considered class vs bus for IIO, the view expressed > > > at the times was that the whole separation of the two didn't mean anything > > > and for non trivial cases bus was always preferred. It's nothing to do > > > with with whether the thing is a bus or not. Now I suppose it's possible > > > opinion has moved on this topic... However, I'd say there > > > is really 0 advantage in moving an existing subsystem even if opinion > > > has changed. > > > > > > +CC Greg in case he wants to add anything. > > > > Traditionally classes are a unified way of representing data to > > userspace, independant of the physical transport that the data came to > > userspace on (i.e. input devices are a class, it doesn't matter if they > > came on serial, USB, PS/2, or virtual busses.) > > > > A bus is traditionally a collection of drivers that all talk on a same > > physical transport, that then expose data from that transport to a > > specific userspace class. Again, think USB mice drivers, serial mice > > drivers, PS/2 mice drivers. > > > > Busses bind a driver to a device it creates based on that "bus". > > Classes create virtual devices that export data to userspace for a > > specific common protocol. > > > > Does that help? > > > > One can argue (and have properly in the past), that classes and busses > > really are not all that different, and there used to be code floating > > around that made them the same exact thing in the kernel, with loads of > > userspace sysfs symlinks to preserve things, but those are well out of > > date and I don't think anyone feels like reviving them. However I think > > systemd might still have code in it to work properly if that ever > > happens, haven't looked in a few years... > > > > thanks, > > > > greg k-h > > Thanks for the explanation. Here key thing to my mind is counters went > in as a bus and should stay so because there is limited benefit in a move > and it would be ABI breaking. Maybe it 'should' have been a class, but > too late now. > > Jonathan Very well, that's an understandable reason to avoid incompatibility issues down the road, and userspace applications apparently care little about the difference between /sys/bus and /sys/class anyway, so I'll keep things as they are now and avoid those unnecessary changes. William Breathitt Gray
On Wed, 29 Apr 2020 14:11:34 -0400 William Breathitt Gray <vilhelm.gray@gmail.com> wrote: > Over the past couple years we have noticed some shortcomings with the > Counter sysfs interface. Although useful in the majority of situations, > there are certain use-cases where interacting through sysfs attributes > can become cumbersome and inefficient. A desire to support more advanced > functionality such as timestamps, multi-axis positioning tables, and > other such latency-sensitive applications, has motivated a reevaluation > of the Counter subsystem. I believe a character device interface will be > helpful for this more niche area of counter device use. > > To quell any concerns from the offset: this patchset makes no changes to > the existing Counter sysfs userspace interface -- existing userspace > applications will continue to work with no modifications necessary. I > request that driver maintainers please test their applications to verify > that this is true, and report any discrepancies if they arise. > > However, this patchset does contain a major reimplementation of the > Counter subsystem core and driver API. A reimplementation was necessary > in order to separate the sysfs code from the counter device drivers and > internalize it as a dedicated component of the core Counter subsystem > module. A minor benefit from all of this is that the sysfs interface is > now ensured a certain amount of consistency because the translation is > performed outside of individual counter device drivers. > > Essentially, the reimplementation has enabled counter device drivers to > pass and handle data as native C datatypes now rather than the sysfs > strings from before. A high-level view of how a count value is passed > down from a counter device driver can be exemplified by the following: > > ---------------------- > / Counter device \ > +----------------------+ > | Count register: 0x28 | > +----------------------+ > | > ----------------- > / raw count data / > ----------------- > | > V > +----------------------------+ > | Counter device driver |----------+ > +----------------------------+ | > | Processes data from device | ------------------- > |----------------------------| / driver callbacks / > | Type: unsigned long | ------------------- > | Value: 42 | | > +----------------------------+ | > | | > ---------------- | > / unsigned long / | > ---------------- | > | | > | 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: unsigned long | > | Value: "42" | | Value: 42 | > +--------------------+ +---------------------+ > | | > --------------- ---------------- > / const char * / / unsigned long / > --------------- ---------------- > | | > | V > | +-----------+ > | | ioctl | > | +-----------+ > | \ Count: 42 / > | ----------- > | > V > +--------------------------------------------------+ > | `/sys/bus/counter/devices/counterX/countY/count` | > +--------------------------------------------------+ > \ Count: "42" / > -------------------------------------------------- > > I am aware that an in-kernel API can simplify the data transfer between > counter device drivers and the userspace interfaces, but I want to > postpone that development until after the new Counter character device > ioctl commands are solidified. A userspace ABI is effectively immutable > so I want to make sure we get that right before working on an in-kernel > API that is more flexible to change. However, when we do develop an > in-kernel API, it will likely be housed as part of the Counter core > component, through which the userspace interfaces will then communicate. > > Interaction with Counter character devices occurs via ioctl commands. > This allows userspace applications to access and set counter data using > native C datatypes rather than working through string translations. > > Regarding the organization of this patchset, I have combined the counter > device driver changes with the first patch because the changes must all > be taken together in order to avoid compilation errors. I can see how > this can end up making it difficult to review so many changes at once, > so alternatively I can separate out the counter device driver changes > into their own dedicated patches -- with the understanding that the > patches must all be taken together. > > In addition, I anticipate the Microchip TCB capture counter driver to > break with this patchset. I'm not sure how that driver will be picked > up yet so I have avoided adding it to this patchset right now. But the > changes to support that driver are simple to make so I can add them in a > later revision of this patchset. > > The following are some questions I have about this patchset: > > 1. Should enums be used to represent standard counter component states > (e.g. COUNTER_SIGNAL_LOW), or would these be better defined as int? > > These standard counter component states are defined in the > counter-types.h file and serve as constants used by counter device > drivers and Counter subsystem components in order to ensure a > consistent interface. > > My concern is whether enum constants will cause problems when passed > to userspace via the Counter character device ioctl calls. Along the > same lines is whether the C bool datatype is safe to pass as well, > given that it is a more modern C datatype. For enums, I'd pass them as integers. Bool is probably fine either way. > > 2. Should device driver callbacks return int or long? I sometimes see > error values returned as long (e.g. PTR_ERR(), the file_operations > structure's ioctl callbacks, etc.); when is it necessary to return > long as opposed to int? In my view it doesn't really matter that much. For PTR_ERR it has to be a long because a long is always the same length as a pointer, but an int 'might' not be. However PTR_ERR returns a value that always fits in an integer anyway. https://www.oreilly.com/library/view/linux-device-drivers/0596005903/ch11.html Coding style in linux mostly use int for return values that might indicate an error. > > 3. I only implemented the unlocked_ioctl callback. Should I implement a > compat_ioctl callback as well? Depends if you need to deal with the 32bit userspace on 64 bit kernel corner cases. Looks like you only pass a pointer, in which case I think you can just use the ioctl_compat_ptr callback to handle it for you. > > 4. How much space should allot for name strings? Name strings hold the > names of components (ideally as they appear on datasheets), so I've > arbitrarily chosen a size of 32 for the character device interface. > > 5. Should the owning component of an extension be determined by the > device driver or Counter subsystem? > > A lot of the complexity in the counters-function-types.h file and the > sysfs-callbacks.c file is due to the function pointer casts required > in order to support three different ownership scenarios: the owning > component is the device, the owning component is a Count, the owning > component is a Signal. > > Because the Counter subsystem doesn't not know which scenario is > valid, it must manually check and provide for all three ownership > cases. On the other hand, device drivers do know exactly which case > applies because they are the ones providing the callbacks. > > The complexity in the Counter subsystem code can be eliminated if the > owning component is simply passed down to the callbacks as a void > pointer. The device drivers will then be responsible for casting to > the appropriate component type, but this should in theory not be a > problem since the device driver assigned the callback to the owning > component in the first place. > > William Breathitt Gray (4): > counter: Internalize sysfs interface code > docs: counter: Update to reflect sysfs internalization > counter: Add character device interface > docs: counter: Document character device interface > > Documentation/driver-api/generic-counter.rst | 259 ++- > .../userspace-api/ioctl/ioctl-number.rst | 1 + > MAINTAINERS | 3 +- > drivers/counter/104-quad-8.c | 437 +++-- > drivers/counter/Makefile | 2 + > drivers/counter/counter-chrdev.c | 1134 +++++++++++++ > drivers/counter/counter-chrdev.h | 16 + > drivers/counter/counter-core.c | 220 +++ > drivers/counter/counter-function-types.h | 81 + > drivers/counter/counter-strings.h | 46 + > drivers/counter/counter-sysfs-callbacks.c | 566 +++++++ > drivers/counter/counter-sysfs-callbacks.h | 28 + > drivers/counter/counter-sysfs.c | 524 ++++++ > drivers/counter/counter-sysfs.h | 14 + > drivers/counter/counter.c | 1496 ----------------- > drivers/counter/ftm-quaddec.c | 46 +- > drivers/counter/stm32-lptimer-cnt.c | 159 +- > drivers/counter/stm32-timer-cnt.c | 132 +- > drivers/counter/ti-eqep.c | 170 +- > include/linux/counter.h | 547 +++--- > include/linux/counter_enum.h | 45 - > include/uapi/linux/counter-types.h | 67 + > include/uapi/linux/counter.h | 313 ++++ > 23 files changed, 3816 insertions(+), 2490 deletions(-) > create mode 100644 drivers/counter/counter-chrdev.c > create mode 100644 drivers/counter/counter-chrdev.h > create mode 100644 drivers/counter/counter-core.c > create mode 100644 drivers/counter/counter-function-types.h > create mode 100644 drivers/counter/counter-strings.h > create mode 100644 drivers/counter/counter-sysfs-callbacks.c > create mode 100644 drivers/counter/counter-sysfs-callbacks.h > create mode 100644 drivers/counter/counter-sysfs.c > create mode 100644 drivers/counter/counter-sysfs.h > delete mode 100644 drivers/counter/counter.c > delete mode 100644 include/linux/counter_enum.h > create mode 100644 include/uapi/linux/counter-types.h > create mode 100644 include/uapi/linux/counter.h > > > base-commit: 00edef1ac058b3c754d29bcfd35ea998d9e7a339
From: Jonathan Cameron > Sent: 03 May 2020 15:13 ... > > The following are some questions I have about this patchset: > > > > 1. Should enums be used to represent standard counter component states > > (e.g. COUNTER_SIGNAL_LOW), or would these be better defined as int? > > > > These standard counter component states are defined in the > > counter-types.h file and serve as constants used by counter device > > drivers and Counter subsystem components in order to ensure a > > consistent interface. > > > > My concern is whether enum constants will cause problems when passed > > to userspace via the Counter character device ioctl calls. Along the > > same lines is whether the C bool datatype is safe to pass as well, > > given that it is a more modern C datatype. > > For enums, I'd pass them as integers. > > Bool is probably fine either way. Always use fixed size types in any API structures. Ensure that fields are always on their natural boundaries. So no enums and no bools. It may even be worth using uint64_t for any userspace pointers. At some point you'll live to regret anything else. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Wed, 29 Apr 2020 15:21:05 -0500 David Lechner <david@lechnology.com> wrote: > On 4/29/20 1:11 PM, William Breathitt Gray wrote: > > Over the past couple years we have noticed some shortcomings with the > > Counter sysfs interface. Although useful in the majority of situations, > > there are certain use-cases where interacting through sysfs attributes > > can become cumbersome and inefficient. A desire to support more advanced > > functionality such as timestamps, multi-axis positioning tables, and > > other such latency-sensitive applications, has motivated a reevaluation > > of the Counter subsystem. I believe a character device interface will be > > helpful for this more niche area of counter device use. > > Nice to see some progress being made. :-) > > > > > Interaction with Counter character devices occurs via ioctl commands. > > This allows userspace applications to access and set counter data using > > native C datatypes rather than working through string translations. > > For most aspects of the counter subsystem, this is not an issue since > configuring a counter is not a time-sensitive operation. Instead of > ioctls, I was expecting to just be able to read the character device > and receive counter events or poll to wait for events similar to how > the input subsystem works or how buffers work in the iio subsystem. > > I'm afraid I don't really see much use in having ioctls that do > exactly what sysfs already does. And my intuition tells me that the > extra work needed to maintain it will probably cost more than any > benefit gained. (Maybe other have a different experience that leads > to a different conclusion?) I agree with David here. The ioctls are currently doing what could have been done nicely with a userspace library. Moving away from the string based internal interface is a good move to my mind, because it ensures consistency in they sysfs interface and provides for in kernel users when they make sense. The step of then using that to simplify providing an IOCTL interface to do the same things doesn't seem particularly useful. So what do we gain? Jonathan
On Fri, 1 May 2020 11:46:10 -0400 William Breathitt Gray <vilhelm.gray@gmail.com> wrote: > On Thu, Apr 30, 2020 at 10:13:45PM +0200, Alexandre Belloni wrote: > > Hi, > > > > On 29/04/2020 14:11:34-0400, William Breathitt Gray wrote: > > > Over the past couple years we have noticed some shortcomings with the > > > Counter sysfs interface. Although useful in the majority of situations, > > > there are certain use-cases where interacting through sysfs attributes > > > can become cumbersome and inefficient. A desire to support more advanced > > > functionality such as timestamps, multi-axis positioning tables, and > > > other such latency-sensitive applications, has motivated a reevaluation > > > of the Counter subsystem. I believe a character device interface will be > > > helpful for this more niche area of counter device use. > > > > > > To quell any concerns from the offset: this patchset makes no changes to > > > the existing Counter sysfs userspace interface -- existing userspace > > > applications will continue to work with no modifications necessary. I > > > request that driver maintainers please test their applications to verify > > > that this is true, and report any discrepancies if they arise. > > > > > > > On that topic, I'm wondering why the counter subsystem uses /sys/bus > > instead of /sys/class that would be more natural for a class of devices. > > I can't see how counters would be considered busses. I think you should > > consider moving it over to /sys/class (even if deprecating > > /sys/bus/counter will be long). > > At the time I wasn't quite familiar with sysfs development so I was > following the iio sysfs code rather closely. However, I see now that > you're probably right: this isn't really a bus but rather a collection > of various types of counters -- i.e. a class of devices. > > Perhaps I should migrate this then to /sys/class/counter. Of course, the > /sys/bus/counter location will have to remain for compatibility with > existing applications, but I think a simple symlink to the new > /sys/class/counter location should suffice for that. > > If anyone sees an issue with this give me a heads up. > > > > Interaction with Counter character devices occurs via ioctl commands. > > > This allows userspace applications to access and set counter data using > > > native C datatypes rather than working through string translations. > > > > > > > I agree with David that you should consider using read to retrieve the > > counter data as this will simplify interrupt handling/polling and > > blocking/non-blocking reads can be used by an application. ABI wise, > > this can also be a good move as you could always consider having an > > ioctl requesting a specific format when reading the device so you are > > not stuck with the initial format you are going to choose. > > My hesitation to implement support for read/write calls is due to a > concern that we will end up with various incompatible formats between > counter drivers (thus requiring users to have intimate knowledge of the > drivers and therefore defeating the purpose of a subsystem). However, if > we can standardize on a format that is flexible enough to work for all > counter drivers, then read/write calls should not be a problem. Absolutely. So the different approaches that have been taken to this approach... 1) IIO, describe the format in sysfs. Highest performance option but heavily constrained in what the data flow can be. Was it a good idea? I think the jury is still out on that after 11 or more years :) 2) Input (evdev) - fixed length self describing records. High overhead, inflexible format, but just fine for the fairly sensible sorts of things that make up human input. https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/input.h#L28 > > I think a general format could be possible. For example, the counter > character device can return a standard header data at the start which > provides general information about the counter device: number of > counters, number or signals, number of extensions, etc. From this > information, offsets can be computed (or perhaps provided by the device) > to where the binary data for the count, extension, etc., can be read or > written. Interrupts can then be handled as blocking reads, as could > other types of events we implement. > > Would something like this work well? It's kind of somewhere between IIO and Input. Firstly think about what you might want to actually have out. Mostly I'm thinking timestamp + count value from devices that self clock. Perhaps additional flag to indicate a preset has been hit. Variable length records are a pain. Reality is neither IIO* nor input actually uses them (*once running :) ). Fixed length lets you push it through a kfifo to deal with userspace being slow to read it. So I'd think about doing it the input way and using multiple records for multiple counters. Timestamp can be used to work out they were at the same instant (or various other options such as an 'end' flag). More fun occurs if you want to start controlling 'triggers' etc as that leads to an explosion of the control interface that we still haven't gotten fully sorted in IIO. Good luck :) Jonathan > > William Breathitt Gray > > > > 2. Should device driver callbacks return int or long? I sometimes see > > > error values returned as long (e.g. PTR_ERR(), the file_operations > > > structure's ioctl callbacks, etc.); when is it necessary to return > > > long as opposed to int? > > > > > > > You should use a long if you ever have to return a point as it is > > guaranteed to have the correct size. Else, just stick to an int if you > > are not going to overflow it. > > > > > 3. I only implemented the unlocked_ioctl callback. Should I implement a > > > compat_ioctl callback as well? > > > > > > > The compat_ioctl is to handle 32bit userspace running on a 64bit kernel. > > If your structures have the same size in both cases, then you don't have > > to implement compat_ioctl. > > > > Have a look at Documentation/driver-api/ioctl.rst > > > > > > -- > > Alexandre Belloni, Bootlin > > Embedded Linux and Kernel engineering > > https://bootlin.com
Over the past couple years we have noticed some shortcomings with the Counter sysfs interface. Although useful in the majority of situations, there are certain use-cases where interacting through sysfs attributes can become cumbersome and inefficient. A desire to support more advanced functionality such as timestamps, multi-axis positioning tables, and other such latency-sensitive applications, has motivated a reevaluation of the Counter subsystem. I believe a character device interface will be helpful for this more niche area of counter device use. To quell any concerns from the offset: this patchset makes no changes to the existing Counter sysfs userspace interface -- existing userspace applications will continue to work with no modifications necessary. I request that driver maintainers please test their applications to verify that this is true, and report any discrepancies if they arise. However, this patchset does contain a major reimplementation of the Counter subsystem core and driver API. A reimplementation was necessary in order to separate the sysfs code from the counter device drivers and internalize it as a dedicated component of the core Counter subsystem module. A minor benefit from all of this is that the sysfs interface is now ensured a certain amount of consistency because the translation is performed outside of individual counter device drivers. Essentially, the reimplementation has enabled counter device drivers to pass and handle data as native C datatypes now rather than the sysfs strings from before. A high-level view of how a count value is passed down from a counter device driver can be exemplified by the following: ---------------------- / Counter device \ +----------------------+ | Count register: 0x28 | +----------------------+ | ----------------- / raw count data / ----------------- | V +----------------------------+ | Counter device driver |----------+ +----------------------------+ | | Processes data from device | ------------------- |----------------------------| / driver callbacks / | Type: unsigned long | ------------------- | Value: 42 | | +----------------------------+ | | | ---------------- | / unsigned long / | ---------------- | | | | 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: unsigned long | | Value: "42" | | Value: 42 | +--------------------+ +---------------------+ | | --------------- ---------------- / const char * / / unsigned long / --------------- ---------------- | | | V | +-----------+ | | ioctl | | +-----------+ | \ Count: 42 / | ----------- | V +--------------------------------------------------+ | `/sys/bus/counter/devices/counterX/countY/count` | +--------------------------------------------------+ \ Count: "42" / -------------------------------------------------- I am aware that an in-kernel API can simplify the data transfer between counter device drivers and the userspace interfaces, but I want to postpone that development until after the new Counter character device ioctl commands are solidified. A userspace ABI is effectively immutable so I want to make sure we get that right before working on an in-kernel API that is more flexible to change. However, when we do develop an in-kernel API, it will likely be housed as part of the Counter core component, through which the userspace interfaces will then communicate. Interaction with Counter character devices occurs via ioctl commands. This allows userspace applications to access and set counter data using native C datatypes rather than working through string translations. Regarding the organization of this patchset, I have combined the counter device driver changes with the first patch because the changes must all be taken together in order to avoid compilation errors. I can see how this can end up making it difficult to review so many changes at once, so alternatively I can separate out the counter device driver changes into their own dedicated patches -- with the understanding that the patches must all be taken together. In addition, I anticipate the Microchip TCB capture counter driver to break with this patchset. I'm not sure how that driver will be picked up yet so I have avoided adding it to this patchset right now. But the changes to support that driver are simple to make so I can add them in a later revision of this patchset. The following are some questions I have about this patchset: 1. Should enums be used to represent standard counter component states (e.g. COUNTER_SIGNAL_LOW), or would these be better defined as int? These standard counter component states are defined in the counter-types.h file and serve as constants used by counter device drivers and Counter subsystem components in order to ensure a consistent interface. My concern is whether enum constants will cause problems when passed to userspace via the Counter character device ioctl calls. Along the same lines is whether the C bool datatype is safe to pass as well, given that it is a more modern C datatype. 2. Should device driver callbacks return int or long? I sometimes see error values returned as long (e.g. PTR_ERR(), the file_operations structure's ioctl callbacks, etc.); when is it necessary to return long as opposed to int? 3. I only implemented the unlocked_ioctl callback. Should I implement a compat_ioctl callback as well? 4. How much space should allot for name strings? Name strings hold the names of components (ideally as they appear on datasheets), so I've arbitrarily chosen a size of 32 for the character device interface. 5. Should the owning component of an extension be determined by the device driver or Counter subsystem? A lot of the complexity in the counters-function-types.h file and the sysfs-callbacks.c file is due to the function pointer casts required in order to support three different ownership scenarios: the owning component is the device, the owning component is a Count, the owning component is a Signal. Because the Counter subsystem doesn't not know which scenario is valid, it must manually check and provide for all three ownership cases. On the other hand, device drivers do know exactly which case applies because they are the ones providing the callbacks. The complexity in the Counter subsystem code can be eliminated if the owning component is simply passed down to the callbacks as a void pointer. The device drivers will then be responsible for casting to the appropriate component type, but this should in theory not be a problem since the device driver assigned the callback to the owning component in the first place. William Breathitt Gray (4): counter: Internalize sysfs interface code docs: counter: Update to reflect sysfs internalization counter: Add character device interface docs: counter: Document character device interface Documentation/driver-api/generic-counter.rst | 259 ++- .../userspace-api/ioctl/ioctl-number.rst | 1 + MAINTAINERS | 3 +- drivers/counter/104-quad-8.c | 437 +++-- drivers/counter/Makefile | 2 + drivers/counter/counter-chrdev.c | 1134 +++++++++++++ drivers/counter/counter-chrdev.h | 16 + drivers/counter/counter-core.c | 220 +++ drivers/counter/counter-function-types.h | 81 + drivers/counter/counter-strings.h | 46 + drivers/counter/counter-sysfs-callbacks.c | 566 +++++++ drivers/counter/counter-sysfs-callbacks.h | 28 + drivers/counter/counter-sysfs.c | 524 ++++++ drivers/counter/counter-sysfs.h | 14 + drivers/counter/counter.c | 1496 ----------------- drivers/counter/ftm-quaddec.c | 46 +- drivers/counter/stm32-lptimer-cnt.c | 159 +- drivers/counter/stm32-timer-cnt.c | 132 +- drivers/counter/ti-eqep.c | 170 +- include/linux/counter.h | 547 +++--- include/linux/counter_enum.h | 45 - include/uapi/linux/counter-types.h | 67 + include/uapi/linux/counter.h | 313 ++++ 23 files changed, 3816 insertions(+), 2490 deletions(-) create mode 100644 drivers/counter/counter-chrdev.c create mode 100644 drivers/counter/counter-chrdev.h create mode 100644 drivers/counter/counter-core.c create mode 100644 drivers/counter/counter-function-types.h create mode 100644 drivers/counter/counter-strings.h create mode 100644 drivers/counter/counter-sysfs-callbacks.c create mode 100644 drivers/counter/counter-sysfs-callbacks.h create mode 100644 drivers/counter/counter-sysfs.c create mode 100644 drivers/counter/counter-sysfs.h delete mode 100644 drivers/counter/counter.c delete mode 100644 include/linux/counter_enum.h create mode 100644 include/uapi/linux/counter-types.h create mode 100644 include/uapi/linux/counter.h base-commit: 00edef1ac058b3c754d29bcfd35ea998d9e7a339