| Message ID | cover.1595358237.git.vilhelm.gray@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Introduce the Counter character device interface | expand |
On 8/2/20 4:04 PM, William Breathitt Gray wrote: > On Tue, Jul 28, 2020 at 05:45:53PM -0500, David Lechner wrote: >> On 7/21/20 2:35 PM, William Breathitt Gray wrote: >>> This is a reimplementation of the Generic Counter driver interface. ... >>> -F: include/linux/counter_enum.h >>> +F: include/uapi/linux/counter.h >> >> Seems odd to be introducing a uapi header here since this patch doesn't >> make any changes to userspace. > > These defines are needed by userspace for the character device > interface, but I see your point that at this point in the patchset they > don't need to be exposed yet. > > I could create temporary include/linux/counter_types.h to house these > defines, and then later move them to include/uapi/linux/counter.h in the > character device interface introduction patch. Do you think I should do > so? Since this patch is independent of the chardev changes and probably ready to merge after one more round of review, I would say it probably makes sense to just leave them in counter.h for now and move them to uapi when the chardev interface is finalized. This way, we can just merge this patch as soon as it is ready. > >>> >>> CPMAC ETHERNET DRIVER >>> M: Florian Fainelli <f.fainelli@gmail.com> >>> diff --git a/drivers/counter/104-quad-8.c b/drivers/counter/104-quad-8.c >>> index 78766b6ec271..0f20920073d6 100644 >>> --- a/drivers/counter/104-quad-8.c >>> +++ b/drivers/counter/104-quad-8.c >>> @@ -621,7 +621,7 @@ static const struct iio_chan_spec quad8_channels[] = { >>> }; >>> >>> static int quad8_signal_read(struct counter_device *counter, >>> - struct counter_signal *signal, enum counter_signal_value *val) >>> + struct counter_signal *signal, u8 *val) >> >> I'm not a fan of replacing enum types with u8 everywhere in this patch. >> But if we have to for technical reasons (e.g. causes compiler error if >> we don't) then it would be helpful to add comments giving the enum type >> everywhere like this instance where u8 is actually an enum value. >> >> If we use u32 as the generic type for enums instead of u8, I think the >> compiler will happlily let us use enum type and u32 interchangeably and >> not complain. > > I switched to fixed-width types after the suggestion by David Laight: > https://lkml.org/lkml/2020/5/3/159. I'll CC David Laight just in case he > wants to chime in again. > > Enum types would be nice for making the valid values explicit, but there > is one benefit I have appreciated from the move to fixed-width types: > there has been a significant reduction of duplicate code; before, we had > a different read function for each different enum type, but now we use a > single function to handle them all. Yes, what I was trying to explain is that by using u32 instead of u8, I think we can actually do both. The function pointers in struct counter_device *counter would use u32 as a generic enum value in the declaration, but then the actual implementations could still use the proper enum type. > >>> + device_del(&counter->dev); >>> + counter_sysfs_free(counter); >> >> Should sysfs be freed before deleting device? I think sysfs might be >> using dev still. > > I think it's the other way around isn't it? The Counter sysfs memory > should stay alive for the lifetime of the device. Once the device is > deleted, there's nothing left to access those struct attributes, so that > memory can now be freed. Correct me if my reasoning is wrong here. I think you are right. I was thinking that device_del() would free memory, but it doesn't. It also looks like other drivers call device_put() after this, so maybe needed here too? >>> +static ssize_t counter_data_u8_show(struct device *dev, >>> + struct device_attribute *attr, char *buf) >>> +{ >>> + const struct counter_attribute *const a = to_counter_attribute(attr); >>> + struct counter_device *const counter = dev_get_drvdata(dev); >>> + const struct counter_available *const avail = a->data.priv; >>> + int err; >>> + u8 data; >>> + >>> + switch (a->type) { >> >> I don't understand the use of the word "owner" here. What is being "owned"? >> >> Perhaps "component" would be a better choice? > > I wasn't too set on calling this "owner" either, but I'm not sure if > "component" would make sense either because I wouldn't label a device > attribute as belonging to any particular component (in fact it's quite > the opposite). > > Perhaps the word "scope" would be better. What do you think? Or would > that be too vague as well. "scope" makes sense to me. >>> -/** >>> - * struct counter_signal_ext - Counter Signal extensions >>> - * @name: attribute name >>> - * @read: read callback for this attribute; may be NULL >>> - * @write: write callback for this attribute; may be NULL >>> - * @priv: data private to the driver >>> - */ >>> -struct counter_signal_ext { >>> +enum counter_data_type { >>> + COUNTER_DATA_TYPE_U8, >>> + COUNTER_DATA_TYPE_U64, >>> + COUNTER_DATA_TYPE_BOOL, >>> + COUNTER_DATA_TYPE_SIGNAL, >> >> Does this mean signal name? > > This represents the signal values "high" or "low". With the introduction > of this patchset, these values are no longer strings internally so I > gave them their own data type here. Ah, OK. So maybe COUNTER_DATA_TYPE_SIGNAL_LEVEL would be a better name. > >>> + COUNTER_DATA_TYPE_COUNT_FUNCTION, >>> + COUNTER_DATA_TYPE_SYNAPSE_ACTION, >>> + COUNTER_DATA_TYPE_ENUM, >> >> Why do some enums get their own type while others use a common >> generic ENUM type? > > COUNTER_DATA_TYPE_ENUM is intended for driver-specific Counter enums. > This allows driver authors to define their own Counter enums so that we > don't pollute the Generic Counter interface with enums that are unique > to individual drivers. > >>> + COUNTER_DATA_TYPE_COUNT_DIRECTION, >>> + COUNTER_DATA_TYPE_COUNT_MODE, >> >> Would be nice to group all COUNTER_DATA_TYPE_COUNT_* together > > I assume you're referring to COUNTER_DATA_TYPE_COUNT_FUNCTION being > separate from these two. That's because a "count function" is actually > part of the Generic Counter paradigm: it's the trigger operation for the > Synapse. > > In retrospect, I should have named it "trigger operation" or something > similar when I developed the paradigm originally, but hindsight is > 20/20 (I'd probably rename "Synapse" to something else too if I could). > It's unfortunately too late to rename this because we've exposed it to > userspace already as a named sysfs attribute. > > Perhaps I can rename this enum constant however to > COUNTER_DATA_TYPE_FUNCTION, or similar, to differentiate it from the > Count extensions. > Yes, I think COUNTER_DATA_TYPE_FUNCTION would be sufficient and avoid confusion. >>> /** >>> * struct counter_device - Counter data structure >>> - * @name: name of the device as it appears in the datasheet >>> + * @name: name of the device >>> * @parent: optional parent device providing the counters >>> - * @device_state: internal device state container >>> - * @ops: callbacks from driver >>> + * @signal_read: optional read callback for Signals. The read value of >>> + * the respective Signal should be passed back via the >>> + * value parameter. >>> + * @count_read: optional read callback for Counts. The read value of the >>> + * respective Count should be passed back via the value >>> + * parameter. >>> + * @count_write: optional write callback for Counts. The write value for >>> + * the respective Count is passed in via the value >>> + * parameter. >>> + * @function_read: optional read callback the Count function modes. The >>> + * read function mode of the respective Count should be >>> + * passed back via the function parameter. >>> + * @function_write: option write callback for Count function modes. The >>> + * function mode to write for the respective Count is >>> + * passed in via the function parameter. >>> + * @action_read: optional read callback the Synapse action modes. The >>> + * read action mode of the respective Synapse should be >>> + * passed back via the action parameter. >>> + * @action_write: option write callback for Synapse action modes. The >>> + * action mode to write for the respective Synapse is >>> + * passed in via the action parameter. >>> * @signals: array of Signals >> >> Why not keep the ops struct? > > Defining static ops structures in the drivers seemed to have no > advantage when those callbacks are always used via the counter_device > structure. I decided it'd be simpler to just set them directly in the > counter_device structure then. > > I could reorganize them into an ops structure again if there's enough > interest. I've been working on really constrained systems lately where every byte counts, so this stuck out to me since there would be a copy of all functions for each counter instance. But probably not that big of a deal in the Linux kernel. :-)
On Mon, 3 Aug 2020 15:00:49 -0500 David Lechner <david@lechnology.com> wrote: > On 8/2/20 4:04 PM, William Breathitt Gray wrote: > > On Tue, Jul 28, 2020 at 05:45:53PM -0500, David Lechner wrote: > >> On 7/21/20 2:35 PM, William Breathitt Gray wrote: > >>> This is a reimplementation of the Generic Counter driver interface. > > ... > > >>> -F: include/linux/counter_enum.h > >>> +F: include/uapi/linux/counter.h > >> > >> Seems odd to be introducing a uapi header here since this patch doesn't > >> make any changes to userspace. > > > > These defines are needed by userspace for the character device > > interface, but I see your point that at this point in the patchset they > > don't need to be exposed yet. > > > > I could create temporary include/linux/counter_types.h to house these > > defines, and then later move them to include/uapi/linux/counter.h in the > > character device interface introduction patch. Do you think I should do > > so? > > Since this patch is independent of the chardev changes and probably ready > to merge after one more round of review, I would say it probably makes > sense to just leave them in counter.h for now and move them to uapi when > the chardev interface is finalized. This way, we can just merge this patch > as soon as it is ready. > Agreed. ... > >>> /** > >>> * struct counter_device - Counter data structure > >>> - * @name: name of the device as it appears in the datasheet > >>> + * @name: name of the device > >>> * @parent: optional parent device providing the counters > >>> - * @device_state: internal device state container > >>> - * @ops: callbacks from driver > >>> + * @signal_read: optional read callback for Signals. The read value of > >>> + * the respective Signal should be passed back via the > >>> + * value parameter. > >>> + * @count_read: optional read callback for Counts. The read value of the > >>> + * respective Count should be passed back via the value > >>> + * parameter. > >>> + * @count_write: optional write callback for Counts. The write value for > >>> + * the respective Count is passed in via the value > >>> + * parameter. > >>> + * @function_read: optional read callback the Count function modes. The > >>> + * read function mode of the respective Count should be > >>> + * passed back via the function parameter. > >>> + * @function_write: option write callback for Count function modes. The > >>> + * function mode to write for the respective Count is > >>> + * passed in via the function parameter. > >>> + * @action_read: optional read callback the Synapse action modes. The > >>> + * read action mode of the respective Synapse should be > >>> + * passed back via the action parameter. > >>> + * @action_write: option write callback for Synapse action modes. The > >>> + * action mode to write for the respective Synapse is > >>> + * passed in via the action parameter. > >>> * @signals: array of Signals > >> > >> Why not keep the ops struct? > > > > Defining static ops structures in the drivers seemed to have no > > advantage when those callbacks are always used via the counter_device > > structure. I decided it'd be simpler to just set them directly in the > > counter_device structure then. > > > > I could reorganize them into an ops structure again if there's enough > > interest. > > I've been working on really constrained systems lately where every byte > counts, so this stuck out to me since there would be a copy of all > functions for each counter instance. But probably not that big of a deal > in the Linux kernel. :-) > In addition to that.. There are other advantages to keeping an ops structure including easy function order randomization (for security), plus the fact that we want to make any function pointers build time assignments if we possibly can. Makes them harder to attack. So in more recent kernel code we try to use ops structures wherever possible. Jonathan
On Tue, 21 Jul 2020 15:35:46 -0400 William Breathitt Gray <vilhelm.gray@gmail.com> wrote: > Changes in v4: > - Reimplement character device interface to report Counter events > - Implement Counter timestamps > - Implement poll() support > - Convert microchip-tcb-capture.c to new driver interface > - Add IRQ support for the 104-quad-8 Counter driver > > 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-axes 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: 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 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. > > The following are some questions I have about this patchset: > > 1. Should I support multiple file descriptors for the character device > in this introduction patchset? > > I intend to add support for multiple file descriptors to the Counter > character device, but I restricted this patchset to a single file > descriptor to simplify the code logic for the sake of review. If > there is enough interest, I can add support for multiple file > descriptors in the next revision; I anticipate that this should be > simple to implement through the allocation of a kfifo for each file > descriptor during the open callback. What is the use case? I can conjecture one easily enough, but I'm not sure how real it actually is. We've been around this question a few times in IIO :) Certainly makes sense to design an interface that would allow you to add this support later if needed though. > > 2. Should struct counter_event have a union for different value types, > or just a value u8 array? > > Currently I expose the event data value via a union containing the > various possible Counter data types (value_u8 and value_u64). It is > up to the user to select the right union member for the data they > received. Would it make sense to return this data in a u8 array > instead, with the expectation that the user will cast to the > necessary data type? Be careful on alignment if you do that. We would need to ensure that the buffer is suitable aligned for a cast to work as expected. > > 3. How should errors be returned for Counter data reads performed by > Counter events? > > Counter events are configured with a list of Counter data read > operations to perform for the user. Any one of those data reads can > return an error code, but not necessarily all of them. Currently, the > code exits early when an error code is returned. Should the code > instead continue on, saving the error code to the struct > counter_event for userspace to handle? I'd argue that errors are expected to be rare, so it isn't a problem to just fault out hard on the first one. > > William Breathitt Gray (5): > counter: Internalize sysfs interface code > docs: counter: Update to reflect sysfs internalization > counter: Add character device interface > docs: counter: Document character device interface > counter: 104-quad-8: Add IRQ support for the ACCES 104-QUAD-8 > > .../ABI/testing/sysfs-bus-counter-104-quad-8 | 32 + > Documentation/driver-api/generic-counter.rst | 363 +++- > .../userspace-api/ioctl/ioctl-number.rst | 1 + > MAINTAINERS | 2 +- > drivers/counter/104-quad-8.c | 753 +++++---- > drivers/counter/Kconfig | 6 +- > drivers/counter/Makefile | 1 + > drivers/counter/counter-chrdev.c | 441 +++++ > drivers/counter/counter-chrdev.h | 16 + > drivers/counter/counter-core.c | 188 +++ > drivers/counter/counter-sysfs.c | 849 ++++++++++ > drivers/counter/counter-sysfs.h | 14 + > drivers/counter/counter.c | 1496 ----------------- > drivers/counter/ftm-quaddec.c | 59 +- > drivers/counter/microchip-tcb-capture.c | 104 +- > drivers/counter/stm32-lptimer-cnt.c | 161 +- > drivers/counter/stm32-timer-cnt.c | 139 +- > drivers/counter/ti-eqep.c | 211 +-- > include/linux/counter.h | 633 +++---- > include/linux/counter_enum.h | 45 - > include/uapi/linux/counter.h | 90 + > 21 files changed, 2919 insertions(+), 2685 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-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.h >
On Sun, Aug 09, 2020 at 02:48:00PM +0100, Jonathan Cameron wrote: > On Tue, 21 Jul 2020 15:35:46 -0400 > William Breathitt Gray <vilhelm.gray@gmail.com> wrote: > > The following are some questions I have about this patchset: > > > > 1. Should I support multiple file descriptors for the character device > > in this introduction patchset? > > > > I intend to add support for multiple file descriptors to the Counter > > character device, but I restricted this patchset to a single file > > descriptor to simplify the code logic for the sake of review. If > > there is enough interest, I can add support for multiple file > > descriptors in the next revision; I anticipate that this should be > > simple to implement through the allocation of a kfifo for each file > > descriptor during the open callback. > > What is the use case? I can conjecture one easily enough, but I'm not > sure how real it actually is. We've been around this question a few > times in IIO :) > > Certainly makes sense to design an interface that would allow you to > add this support later if needed though. I don't have any particular use case in mind, but I figured it would be useful. For example, a counter device can have multiple channels with their own events, but any particular channel might be counting the signals of an independent device unrelated to the other channels; in this scenario, two independent user applications might need access to the same counter device. Of course, supporting multiple file descriptors is something that can be added later so perhaps it's best for us to wait until the need arises with a real-life use case. > > > > 2. Should struct counter_event have a union for different value types, > > or just a value u8 array? > > > > Currently I expose the event data value via a union containing the > > various possible Counter data types (value_u8 and value_u64). It is > > up to the user to select the right union member for the data they > > received. Would it make sense to return this data in a u8 array > > instead, with the expectation that the user will cast to the > > necessary data type? > > Be careful on alignment if you do that. We would need to ensure that the > buffer is suitable aligned for a cast to work as expected. That's a fair point. It's probably safer to continue with a union which also has the benefit of making the possible returned types clearer to see in the code. > > > > 3. How should errors be returned for Counter data reads performed by > > Counter events? > > > > Counter events are configured with a list of Counter data read > > operations to perform for the user. Any one of those data reads can > > return an error code, but not necessarily all of them. Currently, the > > code exits early when an error code is returned. Should the code > > instead continue on, saving the error code to the struct > > counter_event for userspace to handle? > > I'd argue that errors are expected to be rare, so it isn't a problem > to just fault out hard on the first one. All right, that should help keep the error logic simple too then. William Breathitt Gray
On Mon, Aug 03, 2020 at 03:00:49PM -0500, David Lechner wrote: > On 8/2/20 4:04 PM, William Breathitt Gray wrote: > > On Tue, Jul 28, 2020 at 05:45:53PM -0500, David Lechner wrote: > >> On 7/21/20 2:35 PM, William Breathitt Gray wrote: > >>> This is a reimplementation of the Generic Counter driver interface. > > ... > > >>> -F: include/linux/counter_enum.h > >>> +F: include/uapi/linux/counter.h > >> > >> Seems odd to be introducing a uapi header here since this patch doesn't > >> make any changes to userspace. > > > > These defines are needed by userspace for the character device > > interface, but I see your point that at this point in the patchset they > > don't need to be exposed yet. > > > > I could create temporary include/linux/counter_types.h to house these > > defines, and then later move them to include/uapi/linux/counter.h in the > > character device interface introduction patch. Do you think I should do > > so? > > Since this patch is independent of the chardev changes and probably ready > to merge after one more round of review, I would say it probably makes > sense to just leave them in counter.h for now and move them to uapi when > the chardev interface is finalized. This way, we can just merge this patch > as soon as it is ready. It would be good to isolate out that patch since it's so large. Okay I'll put these defines in counter.h then and move them to uapi in the later patch. > > > >>> > >>> CPMAC ETHERNET DRIVER > >>> M: Florian Fainelli <f.fainelli@gmail.com> > >>> diff --git a/drivers/counter/104-quad-8.c b/drivers/counter/104-quad-8.c > >>> index 78766b6ec271..0f20920073d6 100644 > >>> --- a/drivers/counter/104-quad-8.c > >>> +++ b/drivers/counter/104-quad-8.c > >>> @@ -621,7 +621,7 @@ static const struct iio_chan_spec quad8_channels[] = { > >>> }; > >>> > >>> static int quad8_signal_read(struct counter_device *counter, > >>> - struct counter_signal *signal, enum counter_signal_value *val) > >>> + struct counter_signal *signal, u8 *val) > >> > >> I'm not a fan of replacing enum types with u8 everywhere in this patch. > >> But if we have to for technical reasons (e.g. causes compiler error if > >> we don't) then it would be helpful to add comments giving the enum type > >> everywhere like this instance where u8 is actually an enum value. > >> > >> If we use u32 as the generic type for enums instead of u8, I think the > >> compiler will happlily let us use enum type and u32 interchangeably and > >> not complain. > > > > I switched to fixed-width types after the suggestion by David Laight: > > https://lkml.org/lkml/2020/5/3/159. I'll CC David Laight just in case he > > wants to chime in again. > > > > Enum types would be nice for making the valid values explicit, but there > > is one benefit I have appreciated from the move to fixed-width types: > > there has been a significant reduction of duplicate code; before, we had > > a different read function for each different enum type, but now we use a > > single function to handle them all. > > Yes, what I was trying to explain is that by using u32 instead of u8, I > think we can actually do both. > > The function pointers in struct counter_device *counter would use u32 as a > generic enum value in the declaration, but then the actual implementations > could still use the proper enum type. Oh, I see what you mean now. So for example: int (*signal_read)(struct counter_device *counter, struct counter_signal *signal, u8 *val) This will become instead: int (*signal_read)(struct counter_device *counter, struct counter_signal *signal, u32 *val) Then in the driver callback implementation we use the enum type we need: enum counter_signal_level signal_level = COUNTER_SIGNAL_HIGH; ... *val = signal_level; Is that what you have in mind? > > > >>> + device_del(&counter->dev); > >>> + counter_sysfs_free(counter); > >> > >> Should sysfs be freed before deleting device? I think sysfs might be > >> using dev still. > > > > I think it's the other way around isn't it? The Counter sysfs memory > > should stay alive for the lifetime of the device. Once the device is > > deleted, there's nothing left to access those struct attributes, so that > > memory can now be freed. Correct me if my reasoning is wrong here. > > I think you are right. I was thinking that device_del() would free > memory, but it doesn't. It also looks like other drivers call > device_put() after this, so maybe needed here too? Do you mean put_device()? Hmm, I think you might be right; the documentation comment states that put_device() should always be used to give up the reference after a device_add() call. At the very least, I need to call put_device() after a device_add() failure. > >>> +static ssize_t counter_data_u8_show(struct device *dev, > >>> + struct device_attribute *attr, char *buf) > >>> +{ > >>> + const struct counter_attribute *const a = to_counter_attribute(attr); > >>> + struct counter_device *const counter = dev_get_drvdata(dev); > >>> + const struct counter_available *const avail = a->data.priv; > >>> + int err; > >>> + u8 data; > >>> + > >>> + switch (a->type) { > >> > >> I don't understand the use of the word "owner" here. What is being "owned"? > >> > >> Perhaps "component" would be a better choice? > > > > I wasn't too set on calling this "owner" either, but I'm not sure if > > "component" would make sense either because I wouldn't label a device > > attribute as belonging to any particular component (in fact it's quite > > the opposite). > > > > Perhaps the word "scope" would be better. What do you think? Or would > > that be too vague as well. > > "scope" makes sense to me. Okay, I'll make this change then. > >>> -/** > >>> - * struct counter_signal_ext - Counter Signal extensions > >>> - * @name: attribute name > >>> - * @read: read callback for this attribute; may be NULL > >>> - * @write: write callback for this attribute; may be NULL > >>> - * @priv: data private to the driver > >>> - */ > >>> -struct counter_signal_ext { > >>> +enum counter_data_type { > >>> + COUNTER_DATA_TYPE_U8, > >>> + COUNTER_DATA_TYPE_U64, > >>> + COUNTER_DATA_TYPE_BOOL, > >>> + COUNTER_DATA_TYPE_SIGNAL, > >> > >> Does this mean signal name? > > > > This represents the signal values "high" or "low". With the introduction > > of this patchset, these values are no longer strings internally so I > > gave them their own data type here. > > Ah, OK. So maybe COUNTER_DATA_TYPE_SIGNAL_LEVEL would be a better name. Sure, that name seems sensible to me. > > > >>> + COUNTER_DATA_TYPE_COUNT_FUNCTION, > >>> + COUNTER_DATA_TYPE_SYNAPSE_ACTION, > >>> + COUNTER_DATA_TYPE_ENUM, > >> > >> Why do some enums get their own type while others use a common > >> generic ENUM type? > > > > COUNTER_DATA_TYPE_ENUM is intended for driver-specific Counter enums. > > This allows driver authors to define their own Counter enums so that we > > don't pollute the Generic Counter interface with enums that are unique > > to individual drivers. > > > >>> + COUNTER_DATA_TYPE_COUNT_DIRECTION, > >>> + COUNTER_DATA_TYPE_COUNT_MODE, > >> > >> Would be nice to group all COUNTER_DATA_TYPE_COUNT_* together > > > > I assume you're referring to COUNTER_DATA_TYPE_COUNT_FUNCTION being > > separate from these two. That's because a "count function" is actually > > part of the Generic Counter paradigm: it's the trigger operation for the > > Synapse. > > > > In retrospect, I should have named it "trigger operation" or something > > similar when I developed the paradigm originally, but hindsight is > > 20/20 (I'd probably rename "Synapse" to something else too if I could). > > It's unfortunately too late to rename this because we've exposed it to > > userspace already as a named sysfs attribute. > > > > Perhaps I can rename this enum constant however to > > COUNTER_DATA_TYPE_FUNCTION, or similar, to differentiate it from the > > Count extensions. > > > > Yes, I think COUNTER_DATA_TYPE_FUNCTION would be sufficient and avoid > confusion. Okay, I'll make this change then. > >>> /** > >>> * struct counter_device - Counter data structure > >>> - * @name: name of the device as it appears in the datasheet > >>> + * @name: name of the device > >>> * @parent: optional parent device providing the counters > >>> - * @device_state: internal device state container > >>> - * @ops: callbacks from driver > >>> + * @signal_read: optional read callback for Signals. The read value of > >>> + * the respective Signal should be passed back via the > >>> + * value parameter. > >>> + * @count_read: optional read callback for Counts. The read value of the > >>> + * respective Count should be passed back via the value > >>> + * parameter. > >>> + * @count_write: optional write callback for Counts. The write value for > >>> + * the respective Count is passed in via the value > >>> + * parameter. > >>> + * @function_read: optional read callback the Count function modes. The > >>> + * read function mode of the respective Count should be > >>> + * passed back via the function parameter. > >>> + * @function_write: option write callback for Count function modes. The > >>> + * function mode to write for the respective Count is > >>> + * passed in via the function parameter. > >>> + * @action_read: optional read callback the Synapse action modes. The > >>> + * read action mode of the respective Synapse should be > >>> + * passed back via the action parameter. > >>> + * @action_write: option write callback for Synapse action modes. The > >>> + * action mode to write for the respective Synapse is > >>> + * passed in via the action parameter. > >>> * @signals: array of Signals > >> > >> Why not keep the ops struct? > > > > Defining static ops structures in the drivers seemed to have no > > advantage when those callbacks are always used via the counter_device > > structure. I decided it'd be simpler to just set them directly in the > > counter_device structure then. > > > > I could reorganize them into an ops structure again if there's enough > > interest. > > I've been working on really constrained systems lately where every byte > counts, so this stuck out to me since there would be a copy of all > functions for each counter instance. But probably not that big of a deal > in the Linux kernel. :-) I hadn't considered this before, but that's a decent point. In addition, considering Jonathan Cameron's comments in the other message about the benefit of security with making the function pointers build time assignments, I think I'll bring back the static ops structure afterall. William Breathitt Gray
>>>>> >>>>> CPMAC ETHERNET DRIVER >>>>> M: Florian Fainelli <f.fainelli@gmail.com> >>>>> diff --git a/drivers/counter/104-quad-8.c b/drivers/counter/104-quad-8.c >>>>> index 78766b6ec271..0f20920073d6 100644 >>>>> --- a/drivers/counter/104-quad-8.c >>>>> +++ b/drivers/counter/104-quad-8.c >>>>> @@ -621,7 +621,7 @@ static const struct iio_chan_spec quad8_channels[] = { >>>>> }; >>>>> >>>>> static int quad8_signal_read(struct counter_device *counter, >>>>> - struct counter_signal *signal, enum counter_signal_value *val) >>>>> + struct counter_signal *signal, u8 *val) >>>> >>>> I'm not a fan of replacing enum types with u8 everywhere in this patch. >>>> But if we have to for technical reasons (e.g. causes compiler error if >>>> we don't) then it would be helpful to add comments giving the enum type >>>> everywhere like this instance where u8 is actually an enum value. >>>> >>>> If we use u32 as the generic type for enums instead of u8, I think the >>>> compiler will happlily let us use enum type and u32 interchangeably and >>>> not complain. >>> >>> I switched to fixed-width types after the suggestion by David Laight: >>> https://lkml.org/lkml/2020/5/3/159. I'll CC David Laight just in case he >>> wants to chime in again. >>> >>> Enum types would be nice for making the valid values explicit, but there >>> is one benefit I have appreciated from the move to fixed-width types: >>> there has been a significant reduction of duplicate code; before, we had >>> a different read function for each different enum type, but now we use a >>> single function to handle them all. >> >> Yes, what I was trying to explain is that by using u32 instead of u8, I >> think we can actually do both. >> >> The function pointers in struct counter_device *counter would use u32 as a >> generic enum value in the declaration, but then the actual implementations >> could still use the proper enum type. > > Oh, I see what you mean now. So for example: > > int (*signal_read)(struct counter_device *counter, > struct counter_signal *signal, u8 *val) > > This will become instead: > > int (*signal_read)(struct counter_device *counter, > struct counter_signal *signal, u32 *val) > > Then in the driver callback implementation we use the enum type we need: > > enum counter_signal_level signal_level = COUNTER_SIGNAL_HIGH; > ... > *val = signal_level; > > Is that what you have in mind? > Yes. Additionally, if we have... int (*x_write)(struct counter_device *counter, ..., u32 val) We should be able to define the implementation as: static int my_driver_x_write(struct counter_device *counter, ..., enum some_type val) { ... } Not sure if it works if val is a pointer though. Little- endian systems would probably be fine, but maybe not big- endian combined with -fshort-enums compiler flag. int (*x_read)(struct counter_device *counter, ..., u32 *val) static int my_driver_x_read(struct counter_device *counter, ..., enum some_type *val) { ... }
On Mon, Aug 10, 2020 at 05:48:07PM -0500, David Lechner wrote: > > >>>>> > >>>>> CPMAC ETHERNET DRIVER > >>>>> M: Florian Fainelli <f.fainelli@gmail.com> > >>>>> diff --git a/drivers/counter/104-quad-8.c b/drivers/counter/104-quad-8.c > >>>>> index 78766b6ec271..0f20920073d6 100644 > >>>>> --- a/drivers/counter/104-quad-8.c > >>>>> +++ b/drivers/counter/104-quad-8.c > >>>>> @@ -621,7 +621,7 @@ static const struct iio_chan_spec quad8_channels[] = { > >>>>> }; > >>>>> > >>>>> static int quad8_signal_read(struct counter_device *counter, > >>>>> - struct counter_signal *signal, enum counter_signal_value *val) > >>>>> + struct counter_signal *signal, u8 *val) > >>>> > >>>> I'm not a fan of replacing enum types with u8 everywhere in this patch. > >>>> But if we have to for technical reasons (e.g. causes compiler error if > >>>> we don't) then it would be helpful to add comments giving the enum type > >>>> everywhere like this instance where u8 is actually an enum value. > >>>> > >>>> If we use u32 as the generic type for enums instead of u8, I think the > >>>> compiler will happlily let us use enum type and u32 interchangeably and > >>>> not complain. > >>> > >>> I switched to fixed-width types after the suggestion by David Laight: > >>> https://lkml.org/lkml/2020/5/3/159. I'll CC David Laight just in case he > >>> wants to chime in again. > >>> > >>> Enum types would be nice for making the valid values explicit, but there > >>> is one benefit I have appreciated from the move to fixed-width types: > >>> there has been a significant reduction of duplicate code; before, we had > >>> a different read function for each different enum type, but now we use a > >>> single function to handle them all. > >> > >> Yes, what I was trying to explain is that by using u32 instead of u8, I > >> think we can actually do both. > >> > >> The function pointers in struct counter_device *counter would use u32 as a > >> generic enum value in the declaration, but then the actual implementations > >> could still use the proper enum type. > > > > Oh, I see what you mean now. So for example: > > > > int (*signal_read)(struct counter_device *counter, > > struct counter_signal *signal, u8 *val) > > > > This will become instead: > > > > int (*signal_read)(struct counter_device *counter, > > struct counter_signal *signal, u32 *val) > > > > Then in the driver callback implementation we use the enum type we need: > > > > enum counter_signal_level signal_level = COUNTER_SIGNAL_HIGH; > > ... > > *val = signal_level; > > > > Is that what you have in mind? > > > > Yes. > > Additionally, if we have... > > > int (*x_write)(struct counter_device *counter, > ..., u32 val) > > We should be able to define the implementation as: > > static int my_driver_x_write(struct counter_device *counter, > ..., enum some_type val) > { > ... > } > > Not sure if it works if val is a pointer though. Little- > endian systems would probably be fine, but maybe not big- > endian combined with -fshort-enums compiler flag. > > > int (*x_read)(struct counter_device *counter, > ..., u32 *val) > > > static int my_driver_x_read(struct counter_device *counter, > ..., enum some_type *val) > { > ... > } Regardless of endianness for pointers, will targets that have -fshort-enums enabled by default present a problem here? I imagine that in these cases enum some_type will have a size of unsigned char because that is the first type that can represent all the values: https://gcc.gnu.org/onlinedocs/gcc/Structures-unions-enumerations-and-bit-fields-implementation.html What I'm worried about is whether we can gurantee u32 val can be swapped out with enum some_type val -- or if this is not possible because some architectures will be built with -fshort-enums enabled? William Breathitt Gray
Changes in v4: - Reimplement character device interface to report Counter events - Implement Counter timestamps - Implement poll() support - Convert microchip-tcb-capture.c to new driver interface - Add IRQ support for the 104-quad-8 Counter driver 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-axes 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: 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 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. The following are some questions I have about this patchset: 1. Should I support multiple file descriptors for the character device in this introduction patchset? I intend to add support for multiple file descriptors to the Counter character device, but I restricted this patchset to a single file descriptor to simplify the code logic for the sake of review. If there is enough interest, I can add support for multiple file descriptors in the next revision; I anticipate that this should be simple to implement through the allocation of a kfifo for each file descriptor during the open callback. 2. Should struct counter_event have a union for different value types, or just a value u8 array? Currently I expose the event data value via a union containing the various possible Counter data types (value_u8 and value_u64). It is up to the user to select the right union member for the data they received. Would it make sense to return this data in a u8 array instead, with the expectation that the user will cast to the necessary data type? 3. How should errors be returned for Counter data reads performed by Counter events? Counter events are configured with a list of Counter data read operations to perform for the user. Any one of those data reads can return an error code, but not necessarily all of them. Currently, the code exits early when an error code is returned. Should the code instead continue on, saving the error code to the struct counter_event for userspace to handle? William Breathitt Gray (5): counter: Internalize sysfs interface code docs: counter: Update to reflect sysfs internalization counter: Add character device interface docs: counter: Document character device interface counter: 104-quad-8: Add IRQ support for the ACCES 104-QUAD-8 .../ABI/testing/sysfs-bus-counter-104-quad-8 | 32 + Documentation/driver-api/generic-counter.rst | 363 +++- .../userspace-api/ioctl/ioctl-number.rst | 1 + MAINTAINERS | 2 +- drivers/counter/104-quad-8.c | 753 +++++---- drivers/counter/Kconfig | 6 +- drivers/counter/Makefile | 1 + drivers/counter/counter-chrdev.c | 441 +++++ drivers/counter/counter-chrdev.h | 16 + drivers/counter/counter-core.c | 188 +++ drivers/counter/counter-sysfs.c | 849 ++++++++++ drivers/counter/counter-sysfs.h | 14 + drivers/counter/counter.c | 1496 ----------------- drivers/counter/ftm-quaddec.c | 59 +- drivers/counter/microchip-tcb-capture.c | 104 +- drivers/counter/stm32-lptimer-cnt.c | 161 +- drivers/counter/stm32-timer-cnt.c | 139 +- drivers/counter/ti-eqep.c | 211 +-- include/linux/counter.h | 633 +++---- include/linux/counter_enum.h | 45 - include/uapi/linux/counter.h | 90 + 21 files changed, 2919 insertions(+), 2685 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-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.h