From: William Breathitt Gray <hidden> Date: 2021-08-27 03:48:15
Changes in v16:
- Define magic numbers for stm32-lptimer-cnt clock polarities
- Define magic numbers for stm32-timer-cnt encoder modes
- Bump KernelVersion to 5.16 in sysfs-bus-counter ABI documentation
- Fix typos in driver API generic-counter.rst documentation file
For convenience, this patchset is also available on my personal git
repo: https://gitlab.com/vilhelmgray/iio/-/tree/counter_chrdev_v16
The patches preceding "counter: Internalize sysfs interface code" are
primarily cleanup and fixes that can be picked up and applied now to the
IIO tree if so desired. The "counter: Internalize sysfs interface code"
patch as well may be considered for pickup because it is relatively safe
and makes no changes to the userspace interface.
To summarize the main points of this patchset: there are no changes to
the existing Counter sysfs userspace interface; a Counter character
device interface is introduced that allows Counter events and associated
data to be read() by userspace; the events_configure() and
watch_validate() driver callbacks are introduced to support Counter
events; and IRQ support is added to the 104-QUAD-8 driver, serving as an
example of how to support the new Counter events functionality.
William Breathitt Gray (14):
counter: stm32-lptimer-cnt: Provide defines for clock polarities
counter: stm32-timer-cnt: Provide defines for slave mode selection
counter: Internalize sysfs interface code
counter: Update counter.h comments to reflect sysfs internalization
docs: counter: Update to reflect sysfs internalization
counter: Move counter enums to uapi header
counter: Add character device interface
docs: counter: Document character device interface
tools/counter: Create Counter tools
counter: Implement signalZ_action_component_id sysfs attribute
counter: Implement *_component_id sysfs attributes
counter: Implement events_queue_size sysfs attribute
counter: 104-quad-8: Replace mutex with spinlock
counter: 104-quad-8: Add IRQ support for the ACCES 104-QUAD-8
Documentation/ABI/testing/sysfs-bus-counter | 38 +-
Documentation/driver-api/generic-counter.rst | 358 +++-
.../userspace-api/ioctl/ioctl-number.rst | 1 +
MAINTAINERS | 3 +-
drivers/counter/104-quad-8.c | 699 ++++----
drivers/counter/Kconfig | 6 +-
drivers/counter/Makefile | 1 +
drivers/counter/counter-chrdev.c | 553 ++++++
drivers/counter/counter-chrdev.h | 14 +
drivers/counter/counter-core.c | 191 +++
drivers/counter/counter-sysfs.c | 960 +++++++++++
drivers/counter/counter-sysfs.h | 13 +
drivers/counter/counter.c | 1496 -----------------
drivers/counter/ftm-quaddec.c | 60 +-
drivers/counter/intel-qep.c | 144 +-
drivers/counter/interrupt-cnt.c | 62 +-
drivers/counter/microchip-tcb-capture.c | 91 +-
drivers/counter/stm32-lptimer-cnt.c | 212 ++-
drivers/counter/stm32-timer-cnt.c | 195 +--
drivers/counter/ti-eqep.c | 180 +-
include/linux/counter.h | 715 ++++----
include/linux/counter_enum.h | 45 -
include/linux/mfd/stm32-lptimer.h | 5 +
include/linux/mfd/stm32-timers.h | 4 +
include/uapi/linux/counter.h | 154 ++
tools/Makefile | 13 +-
tools/counter/Build | 1 +
tools/counter/Makefile | 53 +
tools/counter/counter_example.c | 93 +
29 files changed, 3569 insertions(+), 2791 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
create mode 100644 tools/counter/Build
create mode 100644 tools/counter/Makefile
create mode 100644 tools/counter/counter_example.c
base-commit: 5ffeb17c0d3dd44704b4aee83e297ec07666e4d6
--
2.32.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: William Breathitt Gray <hidden> Date: 2021-08-27 03:48:41
The Counter subsystem architecture and driver implementations have
changed in order to handle Counter sysfs interactions in a more
consistent way. This patch updates the Generic Counter interface
header file comments to reflect the changes.
Signed-off-by: William Breathitt Gray <redacted>
---
drivers/counter/counter-core.c | 3 ++
include/linux/counter.h | 62 ++++++++++++++++------------------
2 files changed, 33 insertions(+), 32 deletions(-)
From: William Breathitt Gray <hidden> Date: 2021-08-27 03:48:47
The Counter subsystem architecture and driver implementations have
changed in order to handle Counter sysfs interactions in a more
consistent way. This patch updates the Generic Counter interface
documentation to reflect the changes.
Reviewed-by: David Lechner <david@lechnology.com>
Signed-off-by: William Breathitt Gray <redacted>
---
Documentation/ABI/testing/sysfs-bus-counter | 9 +-
Documentation/driver-api/generic-counter.rst | 243 ++++++++++++++-----
2 files changed, 185 insertions(+), 67 deletions(-)
@@ -286,7 +286,14 @@ What: /sys/bus/counter/devices/counterX/signalY/signal KernelVersion: 5.2 Contact: linux-iio@vger.kernel.org Description:- Signal data of Signal Y represented as a string.+ Signal level state of Signal Y. The following signal level+ states are available:++ low:+ Low level state.++ high:+ High level state. What: /sys/bus/counter/devices/counterX/signalY/synchronous_mode KernelVersion: 5.2
@@ -250,8 +250,8 @@ for defining a counter device...kernel-doc:: drivers/counter/counter.c:export:-Implementation-==============+Driver Implementation+===================== To support a counter device, a driver must first allocate the available Counter Signals via counter_signal structures. These Signals should
@@ -267,25 +267,61 @@ respective counter_count structure. These counter_count structures are set to the counts array member of an allocated counter_device structure before the Counter is registered to the system.-Driver callbacks should be provided to the counter_device structure via-a constant counter_ops structure in order to communicate with the-device: to read and write various Signals and Counts, and to set and get-the "action mode" and "function mode" for various Synapses and Counts-respectively.+Driver callbacks must be provided to the counter_device structure in+order to communicate with the device: to read and write various Signals+and Counts, and to set and get the "action mode" and "function mode" for+various Synapses and Counts respectively. A defined counter_device structure may be registered to the system by passing it to the counter_register function, and unregistered by passing it to the counter_unregister function. Similarly, the-devm_counter_register and devm_counter_unregister functions may be used-if device memory-managed registration is desired.--Extension sysfs attributes can be created for auxiliary functionality-and data by passing in defined counter_device_ext, counter_count_ext,-and counter_signal_ext structures. In these cases, the-counter_device_ext structure is used for global/miscellaneous exposure-and configuration of the respective Counter device, while the-counter_count_ext and counter_signal_ext structures allow for auxiliary-exposure and configuration of a specific Count or Signal respectively.+devm_counter_register function may be used if device memory-managed+registration is desired.++The struct counter_comp structure is used to define counter extensions+for Signals, Synapses, and Counts.++The "type" member specifies the type of high-level data (e.g. BOOL,+COUNT_DIRECTION, etc.) handled by this extension. The "``*_read``" and+"``*_write``" members can then be set by the counter device driver with+callbacks to handle that data using native C data types (i.e. u8, u64,+etc.).++Convenience macros such as ``COUNTER_COMP_COUNT_U64`` are provided for+use by driver authors. In particular, driver authors are expected to use+the provided macros for standard Counter subsystem attributes in order+to maintain a consistent interface for userspace. For example, a counter+device driver may define several standard attributes like so::++ struct counter_comp count_ext[] = {+ COUNTER_COMP_DIRECTION(count_direction_read),+ COUNTER_COMP_ENABLE(count_enable_read, count_enable_write),+ COUNTER_COMP_CEILING(count_ceiling_read, count_ceiling_write),+ };++This makes it simple to see, add, and modify the attributes that are+supported by this driver ("direction", "enable", and "ceiling") and to+maintain this code without getting lost in a web of struct braces.++Callbacks must match the function type expected for the respective+component or extension. These function types are defined in the struct+counter_comp structure as the "``*_read``" and "``*_write``" union+members.++The corresponding callback prototypes for the extensions mentioned in+the previous example above would be::++ int count_direction_read(struct counter_device *counter,+ struct counter_count *count,+ enum counter_count_direction *direction);+ int count_enable_read(struct counter_device *counter,+ struct counter_count *count, u8 *enable);+ int count_enable_write(struct counter_device *counter,+ struct counter_count *count, u8 enable);+ int count_ceiling_read(struct counter_device *counter,+ struct counter_count *count, u64 *ceiling);+ int count_ceiling_write(struct counter_device *counter,+ struct counter_count *count, u64 ceiling); Determining the type of extension to create is a matter of scope.
@@ -313,52 +349,127 @@ Determining the type of extension to create is a matter of scope. chip overheated via a device extension called "error_overtemp": /sys/bus/counter/devices/counterX/error_overtemp-Architecture-============--When the Generic Counter interface counter module is loaded, the-counter_init function is called which registers a bus_type named-"counter" to the system. Subsequently, when the module is unloaded, the-counter_exit function is called which unregisters the bus_type named-"counter" from the system.--Counter devices are registered to the system via the counter_register-function, and later removed via the counter_unregister function. The-counter_register function establishes a unique ID for the Counter-device and creates a respective sysfs directory, where X is the-mentioned unique ID:-- /sys/bus/counter/devices/counterX--Sysfs attributes are created within the counterX directory to expose-functionality, configurations, and data relating to the Counts, Signals,-and Synapses of the Counter device, as well as options and information-for the Counter device itself.--Each Signal has a directory created to house its relevant sysfs-attributes, where Y is the unique ID of the respective Signal:-- /sys/bus/counter/devices/counterX/signalY--Similarly, each Count has a directory created to house its relevant-sysfs attributes, where Y is the unique ID of the respective Count:-- /sys/bus/counter/devices/counterX/countY--For a more detailed breakdown of the available Generic Counter interface-sysfs attributes, please refer to the-Documentation/ABI/testing/sysfs-bus-counter file.--The Signals and Counts associated with the Counter device are registered-to the system as well by the counter_register function. The-signal_read/signal_write driver callbacks are associated with their-respective Signal attributes, while the count_read/count_write and-function_get/function_set driver callbacks are associated with their-respective Count attributes; similarly, the same is true for the-action_get/action_set driver callbacks and their respective Synapse-attributes. If a driver callback is left undefined, then the respective-read/write permission is left disabled for the relevant attributes.--Similarly, extension sysfs attributes are created for the defined-counter_device_ext, counter_count_ext, and counter_signal_ext-structures that are passed in.+Subsystem Architecture+======================++Counter drivers pass and take data natively (i.e. ``u8``, ``u64``, etc.)+and the shared counter module handles the translation between the sysfs+interface. This guarantees a standard userspace interface for all+counter drivers, and enables a Generic Counter chrdev interface via a+generalized device driver ABI.++A high-level view of how a count value is passed down from a counter+driver is exemplified by the following. The driver callbacks are first+registered to the Counter core component for use by the Counter+userspace interface components::++ Driver callbacks registration:+ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~+ +----------------------------++| Counter device driver |+ +----------------------------++| Processes data from device |+ +----------------------------++ |+ -------------------+ / driver callbacks /+ -------------------+ |+ V+ +----------------------++| Counter core |+ +----------------------++| Routes device driver |+| callbacks to the |+| userspace interfaces |+ +----------------------++ |+ -------------------+ / driver callbacks /+ -------------------+ |+ +---------------++ |+ V+ +--------------------++| Counter sysfs |+ +--------------------++| Translates to the |+| standard Counter |+| sysfs output |+ +--------------------+++Thereafter, data can be transferred directly between the Counter device+driver and Counter userspace interface::++ Count data request:+ ~~~~~~~~~~~~~~~~~~~+ ----------------------+ / Counter device \+ +----------------------++| Count register: 0x28 |+ +----------------------++ |+ -----------------+ / raw count data /+ -----------------+ |+ V+ +----------------------------++| Counter device driver |+ +----------------------------++| Processes data from device |+ |----------------------------|+| Type: u64 |+| Value: 42 |+ +----------------------------++ |+ ----------+ / u64 /+ ----------+ |+ +---------------++ |+ V+ +--------------------++| Counter sysfs |+ +--------------------++| Translates to the |+| standard Counter |+| sysfs output |+ |--------------------|+| Type: const char * |+| Value: "42" |+ +--------------------++ |+ ---------------+ / const char * /+ ---------------+ |+ V+ +--------------------------------------------------++|`/sys/bus/counter/devices/counterX/countY/count` |+ +--------------------------------------------------++ \ Count: "42" /+ --------------------------------------------------++There are three primary components involved:++Counter device driver+---------------------+Communicates with the hardware device to read/write data; e.g. counter+drivers for quadrature encoders, timers, etc.++Counter core+------------+Registers the counter device driver to the system so that the respective+callbacks are called during userspace interaction.++Counter sysfs+-------------+Translates counter data to the standard Counter sysfs interface format+and vice versa.++Please refer to the ``Documentation/ABI/testing/sysfs-bus-counter`` file+for a detailed breakdown of the available Generic Counter interface+sysfs attributes.
--
2.32.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: William Breathitt Gray <hidden> Date: 2021-08-27 03:49:13
This is in preparation for a subsequent patch implementing a character
device interface for the Counter subsystem.
Reviewed-by: David Lechner <david@lechnology.com>
Signed-off-by: William Breathitt Gray <redacted>
---
MAINTAINERS | 1 +
include/linux/counter.h | 42 +--------------------------
include/uapi/linux/counter.h | 56 ++++++++++++++++++++++++++++++++++++
3 files changed, 58 insertions(+), 41 deletions(-)
create mode 100644 include/uapi/linux/counter.h
From: William Breathitt Gray <hidden> Date: 2021-08-27 03:49:16
This patch introduces a character device interface for the Counter
subsystem. Device data is exposed through standard character device read
operations. Device data is gathered when a Counter event is pushed by
the respective Counter device driver. Configuration is handled via ioctl
operations on the respective Counter character device node.
Cc: David Lechner <david@lechnology.com>
Cc: Gwendal Grignou <redacted>
Cc: Dan Carpenter <redacted>
Cc: Oleksij Rempel <o.rempel@pengutronix.de>
Signed-off-by: William Breathitt Gray <redacted>
---
drivers/counter/Makefile | 2 +-
drivers/counter/counter-chrdev.c | 553 +++++++++++++++++++++++++++++++
drivers/counter/counter-chrdev.h | 14 +
drivers/counter/counter-core.c | 60 +++-
include/linux/counter.h | 51 +++
include/uapi/linux/counter.h | 98 ++++++
6 files changed, 770 insertions(+), 8 deletions(-)
create mode 100644 drivers/counter/counter-chrdev.c
create mode 100644 drivers/counter/counter-chrdev.h
@@ -0,0 +1,553 @@+// SPDX-License-Identifier: GPL-2.0+/*+*GenericCountercharacterdeviceinterface+*Copyright(C)2020WilliamBreathittGray+*/+#include<linux/bitops.h>+#include<linux/cdev.h>+#include<linux/counter.h>+#include<linux/err.h>+#include<linux/errno.h>+#include<linux/export.h>+#include<linux/fs.h>+#include<linux/kfifo.h>+#include<linux/list.h>+#include<linux/mutex.h>+#include<linux/nospec.h>+#include<linux/poll.h>+#include<linux/slab.h>+#include<linux/spinlock.h>+#include<linux/timekeeping.h>+#include<linux/types.h>+#include<linux/uaccess.h>+#include<linux/wait.h>++#include"counter-chrdev.h"++structcounter_comp_node{+structlist_headl;+structcounter_componentcomponent;+structcounter_compcomp;+void*parent;+};++staticssize_tcounter_chrdev_read(structfile*filp,char__user*buf,+size_tlen,loff_t*f_ps)+{+structcounter_device*constcounter=filp->private_data;+interr;+unsignedintcopied;++if(!counter->ops)+return-ENODEV;++if(len<sizeof(structcounter_event))+return-EINVAL;++do{+if(kfifo_is_empty(&counter->events)){+if(filp->f_flags&O_NONBLOCK)+return-EAGAIN;++err=wait_event_interruptible(counter->events_wait,+!kfifo_is_empty(&counter->events)||+!counter->ops);+if(err<0)+returnerr;+if(!counter->ops)+return-ENODEV;+}++if(mutex_lock_interruptible(&counter->events_lock))+return-ERESTARTSYS;+err=kfifo_to_user(&counter->events,buf,len,&copied);+mutex_unlock(&counter->events_lock);+if(err<0)+returnerr;+}while(!copied);++returncopied;+}++static__poll_tcounter_chrdev_poll(structfile*filp,+structpoll_table_struct*pollt)+{+structcounter_device*constcounter=filp->private_data;+__poll_tevents=0;++if(!counter->ops)+returnevents;++poll_wait(filp,&counter->events_wait,pollt);++if(!kfifo_is_empty(&counter->events))+events=EPOLLIN|EPOLLRDNORM;++returnevents;+}++staticvoidcounter_events_list_free(structlist_head*constevents_list)+{+structcounter_event_node*p,*n;+structcounter_comp_node*q,*o;++list_for_each_entry_safe(p,n,events_list,l){+/* Free associated component nodes */+list_for_each_entry_safe(q,o,&p->comp_list,l){+list_del(&q->l);+kfree(q);+}++/* Free event node */+list_del(&p->l);+kfree(p);+}+}++staticintcounter_set_event_node(structcounter_device*constcounter,+structcounter_watch*constwatch,+conststructcounter_comp_node*constcfg)+{+structcounter_event_node*event_node;+interr=0;+structcounter_comp_node*comp_node;++/* Search for event in the list */+list_for_each_entry(event_node,&counter->next_events_list,l)+if(event_node->event==watch->event&&+event_node->channel==watch->channel)+break;++/* If event is not already in the list */+if(&event_node->l==&counter->next_events_list){+/* Allocate new event node */+event_node=kmalloc(sizeof(*event_node),GFP_KERNEL);+if(!event_node)+return-ENOMEM;++/* Configure event node and add to the list */+event_node->event=watch->event;+event_node->channel=watch->channel;+INIT_LIST_HEAD(&event_node->comp_list);+list_add(&event_node->l,&counter->next_events_list);+}++/* Check if component watch has already been set before */+list_for_each_entry(comp_node,&event_node->comp_list,l)+if(comp_node->parent==cfg->parent&&+comp_node->comp.count_u8_read==cfg->comp.count_u8_read){+err=-EINVAL;+gotoexit_free_event_node;+}++/* Allocate component node */+comp_node=kmalloc(sizeof(*comp_node),GFP_KERNEL);+if(!comp_node){+err=-ENOMEM;+gotoexit_free_event_node;+}+*comp_node=*cfg;++/* Add component node to event node */+list_add_tail(&comp_node->l,&event_node->comp_list);++exit_free_event_node:+/* Free event node if no one else is watching */+if(list_empty(&event_node->comp_list)){+list_del(&event_node->l);+kfree(event_node);+}++returnerr;+}++staticintcounter_enable_events(structcounter_device*constcounter)+{+unsignedlongflags;+interr=0;++mutex_lock(&counter->n_events_list_lock);+spin_lock_irqsave(&counter->events_list_lock,flags);++counter_events_list_free(&counter->events_list);+list_replace_init(&counter->next_events_list,+&counter->events_list);++if(counter->ops->events_configure)+err=counter->ops->events_configure(counter);++spin_unlock_irqrestore(&counter->events_list_lock,flags);+mutex_unlock(&counter->n_events_list_lock);++returnerr;+}++staticintcounter_disable_events(structcounter_device*constcounter)+{+unsignedlongflags;+interr=0;++spin_lock_irqsave(&counter->events_list_lock,flags);++counter_events_list_free(&counter->events_list);++if(counter->ops->events_configure)+err=counter->ops->events_configure(counter);++spin_unlock_irqrestore(&counter->events_list_lock,flags);++mutex_lock(&counter->n_events_list_lock);++counter_events_list_free(&counter->next_events_list);++mutex_unlock(&counter->n_events_list_lock);++returnerr;+}++staticintcounter_add_watch(structcounter_device*constcounter,+constunsignedlongarg)+{+void__user*constuwatch=(void__user*)arg;+structcounter_watchwatch;+structcounter_comp_nodecomp_node={};+size_tparent,id;+structcounter_comp*ext;+size_tnum_ext;+interr=0;++if(copy_from_user(&watch,uwatch,sizeof(watch)))+return-EFAULT;++if(watch.component.type==COUNTER_COMPONENT_NONE)+gotono_component;++parent=watch.component.parent;++/* Configure parent component info for comp node */+switch(watch.component.scope){+caseCOUNTER_SCOPE_DEVICE:+ext=counter->ext;+num_ext=counter->num_ext;+break;+caseCOUNTER_SCOPE_SIGNAL:+if(parent>=counter->num_signals)+return-EINVAL;+parent=array_index_nospec(parent,counter->num_signals);++comp_node.parent=counter->signals+parent;++ext=counter->signals[parent].ext;+num_ext=counter->signals[parent].num_ext;+break;+caseCOUNTER_SCOPE_COUNT:+if(parent>=counter->num_counts)+return-EINVAL;+parent=array_index_nospec(parent,counter->num_counts);++comp_node.parent=counter->counts+parent;++ext=counter->counts[parent].ext;+num_ext=counter->counts[parent].num_ext;+break;+default:+return-EINVAL;+}++id=watch.component.id;++/* Configure component info for comp node */+switch(watch.component.type){+caseCOUNTER_COMPONENT_SIGNAL:+if(watch.component.scope!=COUNTER_SCOPE_SIGNAL)+return-EINVAL;++comp_node.comp.type=COUNTER_COMP_SIGNAL_LEVEL;+comp_node.comp.signal_u32_read=counter->ops->signal_read;+break;+caseCOUNTER_COMPONENT_COUNT:+if(watch.component.scope!=COUNTER_SCOPE_COUNT)+return-EINVAL;++comp_node.comp.type=COUNTER_COMP_U64;+comp_node.comp.count_u64_read=counter->ops->count_read;+break;+caseCOUNTER_COMPONENT_FUNCTION:+if(watch.component.scope!=COUNTER_SCOPE_COUNT)+return-EINVAL;++comp_node.comp.type=COUNTER_COMP_FUNCTION;+comp_node.comp.count_u32_read=counter->ops->function_read;+break;+caseCOUNTER_COMPONENT_SYNAPSE_ACTION:+if(watch.component.scope!=COUNTER_SCOPE_COUNT)+return-EINVAL;+if(id>=counter->counts[parent].num_synapses)+return-EINVAL;+id=array_index_nospec(id,counter->counts[parent].num_synapses);++comp_node.comp.type=COUNTER_COMP_SYNAPSE_ACTION;+comp_node.comp.action_read=counter->ops->action_read;+comp_node.comp.priv=counter->counts[parent].synapses+id;+break;+caseCOUNTER_COMPONENT_EXTENSION:+if(id>=num_ext)+return-EINVAL;+id=array_index_nospec(id,num_ext);++comp_node.comp=ext[id];+break;+default:+return-EINVAL;+}+/* Check if any read callback is set; this is part of a union */+if(!comp_node.comp.count_u8_read)+return-EOPNOTSUPP;++no_component:+mutex_lock(&counter->n_events_list_lock);++if(counter->ops->watch_validate){+err=counter->ops->watch_validate(counter,&watch);+if(err<0)+gotoerr_exit;+}++comp_node.component=watch.component;++err=counter_set_event_node(counter,&watch,&comp_node);++err_exit:+mutex_unlock(&counter->n_events_list_lock);++returnerr;+}++staticlongcounter_chrdev_ioctl(structfile*filp,unsignedintcmd,+unsignedlongarg)+{+structcounter_device*constcounter=filp->private_data;+intret=-ENODEV;++mutex_lock(&counter->ops_exist_lock);++if(!counter->ops)+gotoout_unlock;++switch(cmd){+caseCOUNTER_ADD_WATCH_IOCTL:+ret=counter_add_watch(counter,arg);+break;+caseCOUNTER_ENABLE_EVENTS_IOCTL:+ret=counter_enable_events(counter);+break;+caseCOUNTER_DISABLE_EVENTS_IOCTL:+ret=counter_disable_events(counter);+break;+default:+ret=-ENOIOCTLCMD;+break;+}++out_unlock:+mutex_unlock(&counter->ops_exist_lock);++returnret;+}++staticintcounter_chrdev_open(structinode*inode,structfile*filp)+{+structcounter_device*constcounter=container_of(inode->i_cdev,+typeof(*counter),+chrdev);++if(test_and_set_bit_lock(0,counter->chrdev_lock))+return-EBUSY;++get_device(&counter->dev);+filp->private_data=counter;++returnnonseekable_open(inode,filp);+}++staticintcounter_chrdev_release(structinode*inode,structfile*filp)+{+structcounter_device*constcounter=filp->private_data;+intret=0;++mutex_lock(&counter->ops_exist_lock);++if(!counter->ops){+counter_events_list_free(&counter->events_list);+counter_events_list_free(&counter->next_events_list);+ret=-ENODEV;+gotoout_unlock;+}++ret=counter_disable_events(counter);+if(ret<0){+mutex_unlock(&counter->ops_exist_lock);+returnret;+}++out_unlock:+mutex_unlock(&counter->ops_exist_lock);++put_device(&counter->dev);+clear_bit_unlock(0,counter->chrdev_lock);++returnret;+}++staticconststructfile_operationscounter_fops={+.owner=THIS_MODULE,+.llseek=no_llseek,+.read=counter_chrdev_read,+.poll=counter_chrdev_poll,+.unlocked_ioctl=counter_chrdev_ioctl,+.open=counter_chrdev_open,+.release=counter_chrdev_release,+};++intcounter_chrdev_add(structcounter_device*constcounter)+{+/* Initialize Counter events lists */+INIT_LIST_HEAD(&counter->events_list);+INIT_LIST_HEAD(&counter->next_events_list);+spin_lock_init(&counter->events_list_lock);+mutex_init(&counter->n_events_list_lock);+init_waitqueue_head(&counter->events_wait);+mutex_init(&counter->events_lock);++/* Initialize character device */+clear_bit(0,counter->chrdev_lock);+cdev_init(&counter->chrdev,&counter_fops);++/* Allocate Counter events queue */+returnkfifo_alloc(&counter->events,64,GFP_KERNEL);+}++voidcounter_chrdev_remove(structcounter_device*constcounter)+{+kfifo_free(&counter->events);+}++staticintcounter_get_data(structcounter_device*constcounter,+conststructcounter_comp_node*constcomp_node,+u64*constvalue)+{+conststructcounter_comp*constcomp=&comp_node->comp;+void*constparent=comp_node->parent;+u8value_u8=0;+u32value_u32=0;+intret;++if(comp_node->component.type==COUNTER_COMPONENT_NONE)+return0;++switch(comp->type){+caseCOUNTER_COMP_U8:+caseCOUNTER_COMP_BOOL:+switch(comp_node->component.scope){+caseCOUNTER_SCOPE_DEVICE:+ret=comp->device_u8_read(counter,&value_u8);+break;+caseCOUNTER_SCOPE_SIGNAL:+ret=comp->signal_u8_read(counter,parent,&value_u8);+break;+caseCOUNTER_SCOPE_COUNT:+ret=comp->count_u8_read(counter,parent,&value_u8);+break;+}+*value=value_u8;+returnret;+caseCOUNTER_COMP_SIGNAL_LEVEL:+caseCOUNTER_COMP_FUNCTION:+caseCOUNTER_COMP_ENUM:+caseCOUNTER_COMP_COUNT_DIRECTION:+caseCOUNTER_COMP_COUNT_MODE:+switch(comp_node->component.scope){+caseCOUNTER_SCOPE_DEVICE:+ret=comp->device_u32_read(counter,&value_u32);+break;+caseCOUNTER_SCOPE_SIGNAL:+ret=comp->signal_u32_read(counter,parent,+&value_u32);+break;+caseCOUNTER_SCOPE_COUNT:+ret=comp->count_u32_read(counter,parent,&value_u32);+break;+}+*value=value_u32;+returnret;+caseCOUNTER_COMP_U64:+switch(comp_node->component.scope){+caseCOUNTER_SCOPE_DEVICE:+returncomp->device_u64_read(counter,value);+caseCOUNTER_SCOPE_SIGNAL:+returncomp->signal_u64_read(counter,parent,value);+caseCOUNTER_SCOPE_COUNT:+returncomp->count_u64_read(counter,parent,value);+default:+return-EINVAL;+}+caseCOUNTER_COMP_SYNAPSE_ACTION:+ret=comp->action_read(counter,parent,comp->priv,+&value_u32);+*value=value_u32;+returnret;+default:+return-EINVAL;+}+}++/**+*counter_push_event-queueeventforuserspacereading+*@counter:pointertoCounterstructure+*@event:triggeredevent+*@channel:eventchannel+*+*Note:Ifnooneiswatchingfortherespectiveevent,itissilently+*discarded.+*/+voidcounter_push_event(structcounter_device*constcounter,constu8event,+constu8channel)+{+structcounter_eventev;+unsignedintcopied=0;+unsignedlongflags;+structcounter_event_node*event_node;+structcounter_comp_node*comp_node;++ev.timestamp=ktime_get_ns();+ev.watch.event=event;+ev.watch.channel=channel;++/* Could be in an interrupt context, so use a spin lock */+spin_lock_irqsave(&counter->events_list_lock,flags);++/* Search for event in the list */+list_for_each_entry(event_node,&counter->events_list,l)+if(event_node->event==event&&+event_node->channel==channel)+break;++/* If event is not in the list */+if(&event_node->l==&counter->events_list)+gotoexit_early;++/* Read and queue relevant comp for userspace */+list_for_each_entry(comp_node,&event_node->comp_list,l){+ev.watch.component=comp_node->component;+ev.status=-counter_get_data(counter,comp_node,&ev.value);++copied+=kfifo_in(&counter->events,&ev,1);+}++exit_early:+spin_unlock_irqrestore(&counter->events_list_lock,flags);++if(copied)+wake_up_poll(&counter->events_wait,EPOLLIN);+}+EXPORT_SYMBOL_GPL(counter_push_event);
@@ -3,14 +3,22 @@*GenericCounterinterface*Copyright(C)2020WilliamBreathittGray*/+#include<linux/cdev.h>#include<linux/counter.h>#include<linux/device.h>+#include<linux/device/bus.h>#include<linux/export.h>+#include<linux/fs.h>#include<linux/gfp.h>#include<linux/idr.h>#include<linux/init.h>+#include<linux/kdev_t.h>#include<linux/module.h>+#include<linux/mutex.h>+#include<linux/types.h>+#include<linux/wait.h>+#include"counter-chrdev.h"#include"counter-sysfs.h"/* Provides a unique ID for each counter device */
@@ -13,6 +26,91 @@ enum counter_scope {COUNTER_SCOPE_COUNT,};+/**+*structcounter_component-Countercomponentidentification+*@type:componenttype(oneofenumcounter_component_type)+*@scope:componentscope(oneofenumcounter_scope)+*@parent:parentID(matchingtheIDsuffixoftherespectiveparentsysfs+*pathasdescribedbytheABIdocumentationfile+*Documentation/ABI/testing/sysfs-bus-counter)+*@id:componentID(matchingtheIDprovidedbytherespective*_component_id+*sysfsattributeofthedesiredcomponent)+*+*Forexample,iftheCount2ceilingextensionofCounterdevice4isdesired,+*settypeequaltoCOUNTER_COMPONENT_EXTENSION,scopeequalto+*COUNTER_COUNT_SCOPE,parentequalto2,andidequaltothevalueprovidedby+*therespective/sys/bus/counter/devices/counter4/count2/ceiling_component_id+*sysfsattribute.+*/+structcounter_component{+__u8type;+__u8scope;+__u8parent;+__u8id;+};++/* Event type definitions */+enumcounter_event_type{+/* Count value increased past ceiling */+COUNTER_EVENT_OVERFLOW,+/* Count value decreased past floor */+COUNTER_EVENT_UNDERFLOW,+/* Count value increased past ceiling, or decreased past floor */+COUNTER_EVENT_OVERFLOW_UNDERFLOW,+/* Count value reached threshold */+COUNTER_EVENT_THRESHOLD,+/* Index signal detected */+COUNTER_EVENT_INDEX,+};++/**+*structcounter_watch-Countercomponentwatchconfiguration+*@component:componenttowatchwheneventtriggers+*@event:eventthattriggers(oneofenumcounter_event_type)+*@channel:eventchannel(typically0unlessthedevicesupportsconcurrent+*eventsofthesametype)+*/+structcounter_watch{+structcounter_componentcomponent;+__u8event;+__u8channel;+};++/*+*QueuesaCounterwatchforthespecifiedevent.+*+*ThequeuedwatcheswillnotbeapplieduntilCOUNTER_ENABLE_EVENTS_IOCTLis+*called.+*/+#define COUNTER_ADD_WATCH_IOCTL _IOW(0x3E, 0x00, struct counter_watch)+/*+*EnablesmonitoringtheeventsspecifiedbytheCounterwatchesthatwere+*queuedbyCOUNTER_ADD_WATCH_IOCTL.+*+*Ifeventsarealreadyenabled,thenewsetofwatchesreplacestheoldone.+*Callingthisioctlalsohastheeffectofclearingthequeueofwatchesadded+*byCOUNTER_ADD_WATCH_IOCTL.+*/+#define COUNTER_ENABLE_EVENTS_IOCTL _IO(0x3E, 0x01)+/*+*Stopsmonitoringthepreviouslyenabledevents.+*/+#define COUNTER_DISABLE_EVENTS_IOCTL _IO(0x3E, 0x02)++/**+*structcounter_event-Countereventdata+*@timestamp:bestestimateoftimeofeventoccurrence,innanoseconds+*@value:componentvalue+*@watch:componentwatchconfiguration+*@status:returnstatus(systemerrornumber)+*/+structcounter_event{+__aligned_u64timestamp;+__aligned_u64value;+structcounter_watchwatch;+__u8status;+};+/* Count direction values */enumcounter_count_direction{COUNTER_COUNT_DIRECTION_FORWARD,
--
2.32.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
@@ -223,19 +223,6 @@ whether an input line is differential or single-ended) and instead focus on the core idea of what the data and process represent (e.g. position as interpreted from quadrature encoding data).-Userspace Interface-===================--Several sysfs attributes are generated by the Generic Counter interface,-and reside under the /sys/bus/counter/devices/counterX directory, where-counterX refers to the respective counter device. Please see-Documentation/ABI/testing/sysfs-bus-counter for detailed-information on each Generic Counter interface sysfs attribute.--Through these sysfs attributes, programs and scripts may interact with-the Generic Counter paradigm Counts, Signals, and Synapses of respective-counter devices.- Driver API ==========
@@ -388,16 +375,16 @@ userspace interface components:: / driver callbacks / ------------------- |- +---------------+- |- V- +--------------------+-| Counter sysfs |- +--------------------+-| Translates to the |-| standard Counter |-| sysfs output |- +--------------------++ +---------------+---------------++| |+ V V+ +--------------------+ +---------------------++| Counter sysfs | | Counter chrdev |+ +--------------------+ +---------------------++| Translates to the | | Translates to the |+| standard Counter | | standard Counter |+| sysfs output | | character device |+ +--------------------+ +---------------------+ Thereafter, data can be transferred directly between the Counter device driver and Counter userspace interface::
@@ -453,7 +447,7 @@ driver and Counter userspace interface:: \ Count: "42" / ---------------------------------------------------There are three primary components involved:+There are four primary components involved: Counter device driver ---------------------
@@ -473,3 +467,104 @@ and vice versa. Please refer to the ``Documentation/ABI/testing/sysfs-bus-counter`` file for a detailed breakdown of the available Generic Counter interface sysfs attributes.++Counter chrdev+--------------+Translates counter data to the standard Counter character device; data+is transferred via standard character device read calls, while Counter+events are configured via ioctl calls.++Sysfs Interface+===============++Several sysfs attributes are generated by the Generic Counter interface,+and reside under the ``/sys/bus/counter/devices/counterX`` directory,+where ``X`` is to the respective counter device id. Please see+``Documentation/ABI/testing/sysfs-bus-counter`` for detailed information+on each Generic Counter interface sysfs attribute.++Through these sysfs attributes, programs and scripts may interact with+the Generic Counter paradigm Counts, Signals, and Synapses of respective+counter devices.++Counter Character Device+========================++Counter character device nodes are created under the ``/dev`` directory+as ``counterX``, where ``X`` is the respective counter device id.+Defines for the standard Counter data types are exposed via the+userspace ``include/uapi/linux/counter.h`` file.++Counter events+--------------+Counter device drivers can support Counter events by utilizing the+``counter_push_event`` function::++ void counter_push_event(struct counter_device *const counter, const u8 event,+ const u8 channel);++The event id is specified by the ``event`` parameter; the event channel+id is specified by the ``channel`` parameter. When this function is+called, the Counter data associated with the respective event is+gathered, and a ``struct counter_event`` is generated for each datum and+pushed to userspace.++Counter events can be configured by users to report various Counter+data of interest. This can be conceptualized as a list of Counter+component read calls to perform. For example:++ +------------------------+------------------------++| COUNTER_EVENT_OVERFLOW | COUNTER_EVENT_INDEX |+ +========================+========================++| Channel 0 | Channel 0 |+ +------------------------+------------------------++|* Count 0 | * Signal 0 |+|* Count 1 | * Signal 0 Extension 0 |+|* Signal 3 | * Extension 4 |+| * Count 4 Extension 2 +------------------------++| * Signal 5 Extension 0 | Channel 1 |+| +------------------------++| | * Signal 4 |+| | * Signal 4 Extension 0 |+| | * Count 7 |+ +------------------------+------------------------+++When ``counter_push_event(counter, COUNTER_EVENT_INDEX, 1)`` is called+for example, it will go down the list for the ``COUNTER_EVENT_INDEX``+event channel 1 and execute the read callbacks for Signal 4, Signal 4+Extension 0, and Count 7 -- the data returned for each is pushed to a+kfifo as a ``struct counter_event``, which userspace can retrieve via a+standard read operation on the respective character device node.++Userspace+---------+Userspace applications can configure Counter events via ioctl operations+on the Counter character device node. There following ioctl codes are+supported and provided by the ``linux/counter.h`` userspace header file:++*:c:macro:`COUNTER_ADD_WATCH_IOCTL`++*:c:macro:`COUNTER_ENABLE_EVENTS_IOCTL`++*:c:macro:`COUNTER_DISABLE_EVENTS_IOCTL`++To configure events to gather Counter data, users first populate a+``struct counter_watch`` with the relevant event id, event channel id,+and the information for the desired Counter component from which to+read, and then pass it via the ``COUNTER_ADD_WATCH_IOCTL`` ioctl+command.++Note that an event can be watched without gathering Counter data by+setting the ``component.type`` member equal to+``COUNTER_COMPONENT_NONE``. With this configuration the Counter+character device will simply populate the event timestamps for those+respective ``struct counter_event`` elements and ignore the component+value.++The ``COUNTER_ADD_WATCH_IOCTL`` command will buffer these Counter+watches. When ready, the ``COUNTER_ENABLE_EVENTS_IOCTL`` ioctl command+may be used to activate these Counter watches.++Userspace applications can then execute a ``read`` operation (optionally+calling ``poll`` first) on the Counter character device node to retrieve+``struct counter_event`` elements with the desired data.
From: William Breathitt Gray <hidden> Date: 2021-08-27 03:49:25
The Generic Counter chrdev interface expects users to supply component
IDs in order to select extensions for requests. In order for users to
know what component ID belongs to which extension this information must
be exposed. The *_component_id attribute provides a way for users to
discover what component ID belongs to which respective extension.
Cc: David Lechner <david@lechnology.com>
Cc: Gwendal Grignou <redacted>
Cc: Dan Carpenter <redacted>
Signed-off-by: William Breathitt Gray <redacted>
---
Documentation/ABI/testing/sysfs-bus-counter | 16 +++++++++-
drivers/counter/counter-sysfs.c | 33 +++++++++++++++++----
2 files changed, 42 insertions(+), 7 deletions(-)
@@ -203,12 +203,26 @@ Description: both edges: Any state transition.+What: /sys/bus/counter/devices/counterX/countY/ceiling_component_id+What: /sys/bus/counter/devices/counterX/countY/floor_component_id+What: /sys/bus/counter/devices/counterX/countY/count_mode_component_id+What: /sys/bus/counter/devices/counterX/countY/direction_component_id+What: /sys/bus/counter/devices/counterX/countY/enable_component_id+What: /sys/bus/counter/devices/counterX/countY/error_noise_component_id+What: /sys/bus/counter/devices/counterX/countY/prescaler_component_id+What: /sys/bus/counter/devices/counterX/countY/preset_component_id+What: /sys/bus/counter/devices/counterX/countY/preset_enable_component_id What: /sys/bus/counter/devices/counterX/countY/signalZ_action_component_id+What: /sys/bus/counter/devices/counterX/signalY/cable_fault_component_id+What: /sys/bus/counter/devices/counterX/signalY/cable_fault_enable_component_id+What: /sys/bus/counter/devices/counterX/signalY/filter_clock_prescaler_component_id+What: /sys/bus/counter/devices/counterX/signalY/index_polarity_component_id+What: /sys/bus/counter/devices/counterX/signalY/synchronous_mode_component_id KernelVersion: 5.16 Contact: linux-iio@vger.kernel.org Description: Read-only attribute that indicates the component ID of the- respective Synapse of Count Y for Signal Z.+ respective extension or Synapse. What: /sys/bus/counter/devices/counterX/countY/spike_filter_ns KernelVersion: 5.14
@@ -586,6 +586,7 @@ static int counter_signal_attrs_create(struct counter_device *const counter,interr;structcounter_compcomp;size_ti;+structcounter_comp*ext;/* Create main Signal attribute */comp=counter_signal_comp;
@@ -601,8 +602,14 @@ static int counter_signal_attrs_create(struct counter_device *const counter,/* Create an attribute for each extension */for(i=0;i<signal->num_ext;i++){-err=counter_attr_create(dev,cattr_group,signal->ext+i,-scope,signal);+ext=&signal->ext[i];++err=counter_attr_create(dev,cattr_group,ext,scope,signal);+if(err<0)+returnerr;++err=counter_comp_id_attr_create(dev,cattr_group,ext->name,+i);if(err<0)returnerr;}
@@ -693,6 +700,7 @@ static int counter_count_attrs_create(struct counter_device *const counter,interr;structcounter_compcomp;size_ti;+structcounter_comp*ext;/* Create main Count attribute */comp=counter_count_comp;
@@ -717,8 +725,14 @@ static int counter_count_attrs_create(struct counter_device *const counter,/* Create an attribute for each extension */for(i=0;i<count->num_ext;i++){-err=counter_attr_create(dev,cattr_group,count->ext+i,-scope,count);+ext=&count->ext[i];++err=counter_attr_create(dev,cattr_group,ext,scope,count);+if(err<0)+returnerr;++err=counter_comp_id_attr_create(dev,cattr_group,ext->name,+i);if(err<0)returnerr;}
@@ -814,8 +829,14 @@ static int counter_sysfs_attr_add(struct counter_device *const counter,/* Create an attribute for each extension */for(i=0;i<counter->num_ext;i++){-err=counter_attr_create(dev,cattr_group,counter->ext+i,-scope,NULL);+ext=&counter->ext[i];++err=counter_attr_create(dev,cattr_group,ext,scope,NULL);+if(err<0)+returnerr;++err=counter_comp_id_attr_create(dev,cattr_group,ext->name,+i);if(err<0)returnerr;}
--
2.32.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: William Breathitt Gray <hidden> Date: 2021-08-27 03:49:29
The Generic Counter chrdev interface expects users to supply component
IDs in order to select Synapses for requests. In order for users to know
what component ID belongs to which Synapse this information must be
exposed. The signalZ_action_component_id attribute provides a way for
users to discover what component ID belongs to the respective Synapse.
Cc: Gwendal Grignou <redacted>
Cc: Dan Carpenter <redacted>
Reviewed-by: David Lechner <david@lechnology.com>
Signed-off-by: William Breathitt Gray <redacted>
---
Documentation/ABI/testing/sysfs-bus-counter | 7 ++++
drivers/counter/counter-sysfs.c | 45 +++++++++++++++++++++
2 files changed, 52 insertions(+)
@@ -203,6 +203,13 @@ Description: both edges: Any state transition.+What: /sys/bus/counter/devices/counterX/countY/signalZ_action_component_id+KernelVersion: 5.16+Contact: linux-iio@vger.kernel.org+Description:+ Read-only attribute that indicates the component ID of the+ respective Synapse of Count Y for Signal Z.+ What: /sys/bus/counter/devices/counterX/countY/spike_filter_ns KernelVersion: 5.14 Contact: linux-iio@vger.kernel.org
@@ -12,6 +12,7 @@ help:@echo' acpi - ACPI tools'@echo' bpf - misc BPF tools'@echo' cgroup - cgroup tools'+@echo' counter - counter tools'@echo' cpupower - a tool for all things x86 CPU power'@echo' debugging - tools for debugging'@echo' firewire - the userspace part of nosy, an IEEE-1394 traffic sniffer'
@@ -0,0 +1,53 @@+# SPDX-License-Identifier: GPL-2.0+include ../scripts/Makefile.include++bindir?=/usr/bin++ifeq ($(srctree),)+srctree:=$(patsubst%/,%,$(dir$(CURDIR)))+srctree:=$(patsubst%/,%,$(dir$(srctree)))+endif++# Do not use make's built-in rules+# (this improves performance and avoids hard-to-debug behaviour);+MAKEFLAGS+=-r++overrideCFLAGS+=-O2-Wall-g-D_GNU_SOURCE-I$(OUTPUT)include++ALL_TARGETS:=counter_example+ALL_PROGRAMS:=$(patsubst%,$(OUTPUT)%,$(ALL_TARGETS))++all:$(ALL_PROGRAMS)++exportsrctreeOUTPUTCCLDCFLAGS+include $(srctree)/tools/build/Makefile.include++#+# We need the following to be outside of kernel tree+#+$(OUTPUT)include/linux/counter.h:../../include/uapi/linux/counter.h+mkdir-p$(OUTPUT)include/linux2>&1||true+ln-sf$(CURDIR)/../../include/uapi/linux/counter.h$@++prepare:$(OUTPUT)include/linux/counter.h++COUNTER_EXAMPLE:=$(OUTPUT)counter_example.o+$(COUNTER_EXAMPLE):prepareFORCE+$(Q)$(MAKE)$(build)=counter_example+$(OUTPUT)counter_example:$(COUNTER_EXAMPLE)+$(QUIET_LINK)$(CC)$(CFLAGS)$(LDFLAGS)$<-o$@++clean:+rm-f$(ALL_PROGRAMS)+rm-rf$(OUTPUT)include/linux/counter.h+find$(if$(OUTPUT),$(OUTPUT),.)-name'*.o'-delete-o-name'\.*.d'-delete++install:$(ALL_PROGRAMS)+install-d-m755$(DESTDIR)$(bindir);\+forprogramin$(ALL_PROGRAMS);do\+install$$program$(DESTDIR)$(bindir);\+done++FORCE:++.PHONY:allinstallcleanFORCEprepare
From: William Breathitt Gray <hidden> Date: 2021-08-27 03:49:36
This patch replaces the mutex I/O lock with a spinlock. This is in
preparation for a subsequent patch adding IRQ support for 104-QUAD-8
devices; we can't sleep in an interrupt context, so we'll need to use a
spinlock instead.
Acked-by: Syed Nayyar Waris <redacted>
Signed-off-by: William Breathitt Gray <redacted>
---
drivers/counter/104-quad-8.c | 89 +++++++++++++++++++++---------------
1 file changed, 52 insertions(+), 37 deletions(-)
@@ -43,7 +44,7 @@ MODULE_PARM_DESC(base, "ACCES 104-QUAD-8 base addresses");*@base:baseportaddressofthedevice*/structquad8{-structmutexlock;+spinlock_tlock;structcounter_devicecounter;unsignedintfck_prescaler[QUAD8_NUM_COUNTERS];unsignedintpreset[QUAD8_NUM_COUNTERS];
@@ -124,6 +125,7 @@ static int quad8_count_read(struct counter_device *counter,unsignedintflags;unsignedintborrow;unsignedintcarry;+unsignedlongirqflags;inti;flags=inb(base_offset+1);
@@ -133,7 +135,7 @@ static int quad8_count_read(struct counter_device *counter,/* Borrow XOR Carry effectively doubles count range */*val=(unsignedlong)(borrow^carry)<<24;-mutex_lock(&priv->lock);+spin_lock_irqsave(&priv->lock,irqflags);/* Reset Byte Pointer; transfer Counter to Output Latch */outb(QUAD8_CTR_RLD|QUAD8_RLD_RESET_BP|QUAD8_RLD_CNTR_OUT,
@@ -142,7 +144,7 @@ static int quad8_count_read(struct counter_device *counter,for(i=0;i<3;i++)*val|=(unsignedlong)inb(base_offset)<<(8*i);-mutex_unlock(&priv->lock);+spin_unlock_irqrestore(&priv->lock,irqflags);return0;}
@@ -152,13 +154,14 @@ static int quad8_count_write(struct counter_device *counter,{structquad8*constpriv=counter->priv;constintbase_offset=priv->base+2*count->id;+unsignedlongirqflags;inti;/* Only 24-bit values are supported */if(val>0xFFFFFF)return-ERANGE;-mutex_lock(&priv->lock);+spin_lock_irqsave(&priv->lock,irqflags);/* Reset Byte Pointer */outb(QUAD8_CTR_RLD|QUAD8_RLD_RESET_BP,base_offset+1);
@@ -183,7 +186,7 @@ static int quad8_count_write(struct counter_device *counter,/* Reset Error flag */outb(QUAD8_CTR_RLD|QUAD8_RLD_RESET_E,base_offset+1);-mutex_unlock(&priv->lock);+spin_unlock_irqrestore(&priv->lock,irqflags);return0;}
@@ -201,8 +204,9 @@ static int quad8_function_read(struct counter_device *counter,{structquad8*constpriv=counter->priv;constintid=count->id;+unsignedlongirqflags;-mutex_lock(&priv->lock);+spin_lock_irqsave(&priv->lock,irqflags);if(priv->quadrature_mode[id])switch(priv->quadrature_scale[id]){
@@ -219,7 +223,7 @@ static int quad8_function_read(struct counter_device *counter,else*function=COUNTER_FUNCTION_PULSE_DIRECTION;-mutex_unlock(&priv->lock);+spin_unlock_irqrestore(&priv->lock,irqflags);return0;}
@@ -234,10 +238,11 @@ static int quad8_function_write(struct counter_device *counter,unsignedint*constscale=priv->quadrature_scale+id;unsignedint*constsynchronous_mode=priv->synchronous_mode+id;constintbase_offset=priv->base+2*id+1;+unsignedlongirqflags;unsignedintmode_cfg;unsignedintidr_cfg;-mutex_lock(&priv->lock);+spin_lock_irqsave(&priv->lock,irqflags);mode_cfg=priv->count_mode[id]<<1;idr_cfg=priv->index_polarity[id]<<1;
@@ -272,7 +277,7 @@ static int quad8_function_write(struct counter_device *counter,break;default:/* should never reach this path */-mutex_unlock(&priv->lock);+spin_unlock_irqrestore(&priv->lock,irqflags);return-EINVAL;}}
@@ -280,7 +285,7 @@ static int quad8_function_write(struct counter_device *counter,/* Load mode configuration to Counter Mode Register */outb(QUAD8_CTR_CMR|mode_cfg,base_offset);-mutex_unlock(&priv->lock);+spin_unlock_irqrestore(&priv->lock,irqflags);return0;}
@@ -406,9 +411,10 @@ static int quad8_index_polarity_set(struct counter_device *counter,structquad8*constpriv=counter->priv;constsize_tchannel_id=signal->id-16;constintbase_offset=priv->base+2*channel_id+1;+unsignedlongirqflags;unsignedintidr_cfg=index_polarity<<1;-mutex_lock(&priv->lock);+spin_lock_irqsave(&priv->lock,irqflags);idr_cfg|=priv->synchronous_mode[channel_id];
@@ -417,7 +423,7 @@ static int quad8_index_polarity_set(struct counter_device *counter,/* Load Index Control configuration to Index Control Register */outb(QUAD8_CTR_IDR|idr_cfg,base_offset);-mutex_unlock(&priv->lock);+spin_unlock_irqrestore(&priv->lock,irqflags);return0;}
@@ -446,15 +452,16 @@ static int quad8_synchronous_mode_set(struct counter_device *counter,structquad8*constpriv=counter->priv;constsize_tchannel_id=signal->id-16;constintbase_offset=priv->base+2*channel_id+1;+unsignedlongirqflags;unsignedintidr_cfg=synchronous_mode;-mutex_lock(&priv->lock);+spin_lock_irqsave(&priv->lock,irqflags);idr_cfg|=priv->index_polarity[channel_id]<<1;/* Index function must be non-synchronous in non-quadrature mode */if(synchronous_mode&&!priv->quadrature_mode[channel_id]){-mutex_unlock(&priv->lock);+spin_unlock_irqrestore(&priv->lock,irqflags);return-EINVAL;}
@@ -463,7 +470,7 @@ static int quad8_synchronous_mode_set(struct counter_device *counter,/* Load Index Control configuration to Index Control Register */outb(QUAD8_CTR_IDR|idr_cfg,base_offset);-mutex_unlock(&priv->lock);+spin_unlock_irqrestore(&priv->lock,irqflags);return0;}
@@ -510,6 +517,7 @@ static int quad8_count_mode_write(struct counter_device *counter,unsignedintcount_mode;unsignedintmode_cfg;constintbase_offset=priv->base+2*count->id+1;+unsignedlongirqflags;/* Map Generic Counter count mode to 104-QUAD-8 count mode */switch(cnt_mode){
@@ -530,7 +538,7 @@ static int quad8_count_mode_write(struct counter_device *counter,return-EINVAL;}-mutex_lock(&priv->lock);+spin_lock_irqsave(&priv->lock,irqflags);priv->count_mode[count->id]=count_mode;
@@ -544,7 +552,7 @@ static int quad8_count_mode_write(struct counter_device *counter,/* Load mode configuration to Counter Mode Register */outb(QUAD8_CTR_CMR|mode_cfg,base_offset);-mutex_unlock(&priv->lock);+spin_unlock_irqrestore(&priv->lock,irqflags);return0;}
@@ -564,9 +572,10 @@ static int quad8_count_enable_write(struct counter_device *counter,{structquad8*constpriv=counter->priv;constintbase_offset=priv->base+2*count->id;+unsignedlongirqflags;unsignedintior_cfg;-mutex_lock(&priv->lock);+spin_lock_irqsave(&priv->lock,irqflags);priv->ab_enable[count->id]=enable;
@@ -575,7 +584,7 @@ static int quad8_count_enable_write(struct counter_device *counter,/* Load I/O control configuration */outb(QUAD8_CTR_IOR|ior_cfg,base_offset+1);-mutex_unlock(&priv->lock);+spin_unlock_irqrestore(&priv->lock,irqflags);return0;}
@@ -626,16 +635,17 @@ static int quad8_count_preset_write(struct counter_device *counter,structcounter_count*count,u64preset){structquad8*constpriv=counter->priv;+unsignedlongirqflags;/* Only 24-bit values are supported */if(preset>0xFFFFFF)return-ERANGE;-mutex_lock(&priv->lock);+spin_lock_irqsave(&priv->lock,irqflags);quad8_preset_register_set(priv,count->id,preset);-mutex_unlock(&priv->lock);+spin_unlock_irqrestore(&priv->lock,irqflags);return0;}
@@ -644,8 +654,9 @@ static int quad8_count_ceiling_read(struct counter_device *counter,structcounter_count*count,u64*ceiling){structquad8*constpriv=counter->priv;+unsignedlongirqflags;-mutex_lock(&priv->lock);+spin_lock_irqsave(&priv->lock,irqflags);/* Range Limit and Modulo-N count modes use preset value as ceiling */switch(priv->count_mode[count->id]){
@@ -659,7 +670,7 @@ static int quad8_count_ceiling_read(struct counter_device *counter,break;}-mutex_unlock(&priv->lock);+spin_unlock_irqrestore(&priv->lock,irqflags);return0;}
@@ -668,23 +679,24 @@ static int quad8_count_ceiling_write(struct counter_device *counter,structcounter_count*count,u64ceiling){structquad8*constpriv=counter->priv;+unsignedlongirqflags;/* Only 24-bit values are supported */if(ceiling>0xFFFFFF)return-ERANGE;-mutex_lock(&priv->lock);+spin_lock_irqsave(&priv->lock,irqflags);/* Range Limit and Modulo-N count modes use preset value as ceiling */switch(priv->count_mode[count->id]){case1:case3:quad8_preset_register_set(priv,count->id,ceiling);-mutex_unlock(&priv->lock);+spin_unlock_irqrestore(&priv->lock,irqflags);return0;}-mutex_unlock(&priv->lock);+spin_unlock_irqrestore(&priv->lock,irqflags);return-EINVAL;}
@@ -706,12 +718,13 @@ static int quad8_count_preset_enable_write(struct counter_device *counter,{structquad8*constpriv=counter->priv;constintbase_offset=priv->base+2*count->id+1;+unsignedlongirqflags;unsignedintior_cfg;/* Preset enable is active low in Input/Output Control register */preset_enable=!preset_enable;-mutex_lock(&priv->lock);+spin_lock_irqsave(&priv->lock,irqflags);priv->preset_enable[count->id]=preset_enable;
@@ -720,7 +733,7 @@ static int quad8_count_preset_enable_write(struct counter_device *counter,/* Load I/O control configuration to Input / Output Control Register */outb(QUAD8_CTR_IOR|ior_cfg,base_offset);-mutex_unlock(&priv->lock);+spin_unlock_irqrestore(&priv->lock,irqflags);return0;}
@@ -772,9 +786,10 @@ static int quad8_signal_cable_fault_enable_write(struct counter_device *counter,{structquad8*constpriv=counter->priv;constsize_tchannel_id=signal->id/2;+unsignedlongirqflags;unsignedintcable_fault_enable;-mutex_lock(&priv->lock);+spin_lock_irqsave(&priv->lock,irqflags);if(enable)priv->cable_fault_enable|=BIT(channel_id);
@@ -786,7 +801,7 @@ static int quad8_signal_cable_fault_enable_write(struct counter_device *counter,outb(cable_fault_enable,priv->base+QUAD8_DIFF_ENCODER_CABLE_STATUS);-mutex_unlock(&priv->lock);+spin_unlock_irqrestore(&priv->lock,irqflags);return0;}
@@ -809,8 +824,9 @@ static int quad8_signal_fck_prescaler_write(struct counter_device *counter,structquad8*constpriv=counter->priv;constsize_tchannel_id=signal->id/2;constintbase_offset=priv->base+2*channel_id;+unsignedlongirqflags;-mutex_lock(&priv->lock);+spin_lock_irqsave(&priv->lock,irqflags);priv->fck_prescaler[channel_id]=prescaler;
@@ -822,7 +838,7 @@ static int quad8_signal_fck_prescaler_write(struct counter_device *counter,outb(QUAD8_CTR_RLD|QUAD8_RLD_RESET_BP|QUAD8_RLD_PRESET_PSC,base_offset+1);-mutex_unlock(&priv->lock);+spin_unlock_irqrestore(&priv->lock,irqflags);return0;}
@@ -991,8 +1007,7 @@ static int quad8_probe(struct device *dev, unsigned int id)priv->counter.priv=priv;priv->base=base[id];-/* Initialize mutex */-mutex_init(&priv->lock);+spin_lock_init(&priv->lock);/* Reset all counters and disable interrupt function */outb(QUAD8_CHAN_OP_RESET_COUNTERS,base[id]+QUAD8_REG_CHAN_OP);
--
2.32.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: William Breathitt Gray <hidden> Date: 2021-08-27 03:49:39
The events_queue_size sysfs attribute provides a way for users to
dynamically configure the Counter events queue size for the Counter
character device interface. The size is in number of struct
counter_event data structures. The number of elements will be rounded-up
to a power of 2 due to a requirement of the kfifo_alloc function called
during reallocation of the queue.
Cc: Oleksij Rempel <o.rempel@pengutronix.de>
Signed-off-by: William Breathitt Gray <redacted>
---
Documentation/ABI/testing/sysfs-bus-counter | 8 ++++
drivers/counter/counter-sysfs.c | 45 +++++++++++++++++++++
2 files changed, 53 insertions(+)
@@ -233,6 +233,14 @@ Description: shorter or equal to configured value are ignored. Value 0 means filter is disabled.+What: /sys/bus/counter/devices/counterX/events_queue_size+KernelVersion: 5.16+Contact: linux-iio@vger.kernel.org+Description:+ Size of the Counter events queue in number of struct+ counter_event data structures. The number of elements will be+ rounded-up to a power of 2.+ What: /sys/bus/counter/devices/counterX/name KernelVersion: 5.2 Contact: linux-iio@vger.kernel.org
@@ -783,12 +785,49 @@ static int counter_num_counts_read(struct counter_device *counter, u8 *val)return0;}+staticintcounter_events_queue_size_read(structcounter_device*counter,+u64*val)+{+*val=kfifo_size(&counter->events);+return0;+}++staticintcounter_events_queue_size_write(structcounter_device*counter,+u64val)+{+DECLARE_KFIFO_PTR(events,structcounter_event);+interr=0;++/* Verify chrdev is not currently being used */+if(test_and_set_bit_lock(0,counter->chrdev_lock))+return-EBUSY;++/* Allocate new events queue */+err=kfifo_alloc(&events,val,GFP_KERNEL);+if(err)+gotoexit_early;++/* Swap in new events queue */+kfifo_free(&counter->events);+counter->events.kfifo=events.kfifo;++exit_early:+clear_bit_unlock(0,counter->chrdev_lock);++returnerr;+}+staticstructcounter_compcounter_num_signals_comp=COUNTER_COMP_DEVICE_U8("num_signals",counter_num_signals_read,NULL);staticstructcounter_compcounter_num_counts_comp=COUNTER_COMP_DEVICE_U8("num_counts",counter_num_counts_read,NULL);+staticstructcounter_compcounter_events_queue_size_comp=+COUNTER_COMP_DEVICE_U64("events_queue_size",+counter_events_queue_size_read,+counter_events_queue_size_write);+staticintcounter_sysfs_attr_add(structcounter_device*constcounter,structcounter_attribute_group*cattr_group){
@@ -827,6 +866,12 @@ static int counter_sysfs_attr_add(struct counter_device *const counter,if(err<0)returnerr;+/* Create events_queue_size attribute */+err=counter_attr_create(dev,cattr_group,+&counter_events_queue_size_comp,scope,NULL);+if(err<0)+returnerr;+/* Create an attribute for each extension */for(i=0;i<counter->num_ext;i++){ext=&counter->ext[i];
--
2.32.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: William Breathitt Gray <hidden> Date: 2021-08-27 03:49:44
The LSI/CSI LS7266R1 chip provides programmable output via the FLG pins.
When interrupts are enabled on the ACCES 104-QUAD-8, they occur whenever
FLG1 is active. Four functions are available for the FLG1 signal: Carry,
Compare, Carry-Borrow, and Index.
Carry:
Interrupt generated on active low Carry signal. Carry
signal toggles every time the respective channel's
counter overflows.
Compare:
Interrupt generated on active low Compare signal.
Compare signal toggles every time respective channel's
preset register is equal to the respective channel's
counter.
Carry-Borrow:
Interrupt generated on active low Carry signal and
active low Borrow signal. Carry signal toggles every
time the respective channel's counter overflows. Borrow
signal toggles every time the respective channel's
counter underflows.
Index:
Interrupt generated on active high Index signal.
These four functions correspond respectivefly to the following four
Counter event types: COUNTER_EVENT_OVERFLOW, COUNTER_EVENT_THRESHOLD,
COUNTER_EVENT_OVERFLOW_UNDERFLOW, and COUNTER_EVENT_INDEX. Interrupts
push Counter events to event channel X, where 'X' is the respective
channel whose FLG1 activated.
This patch adds IRQ support for the ACCES 104-QUAD-8. The interrupt line
numbers for the devices may be configured via the irq array module
parameter.
Acked-by: Syed Nayyar Waris <redacted>
Signed-off-by: William Breathitt Gray <redacted>
---
drivers/counter/104-quad-8.c | 167 +++++++++++++++++++++++++++++++++--
drivers/counter/Kconfig | 6 +-
2 files changed, 164 insertions(+), 9 deletions(-)
@@ -378,13 +389,103 @@ static int quad8_action_read(struct counter_device *counter,}}+enum{+QUAD8_EVENT_NONE=-1,+QUAD8_EVENT_CARRY=0,+QUAD8_EVENT_COMPARE=1,+QUAD8_EVENT_CARRY_BORROW=2,+QUAD8_EVENT_INDEX=3,+};++staticintquad8_events_configure(structcounter_device*counter)+{+structquad8*constpriv=counter->priv;+unsignedlongirq_enabled=0;+unsignedlongirqflags;+size_tchannel;+unsignedlongior_cfg;+unsignedlongbase_offset;++spin_lock_irqsave(&priv->lock,irqflags);++/* Enable interrupts for the requested channels, disable for the rest */+for(channel=0;channel<QUAD8_NUM_COUNTERS;channel++){+if(priv->next_irq_trigger[channel]==QUAD8_EVENT_NONE)+continue;++if(priv->irq_trigger[channel]!=priv->next_irq_trigger[channel]){+/* Save new IRQ function configuration */+priv->irq_trigger[channel]=priv->next_irq_trigger[channel];++/* Load configuration to I/O Control Register */+ior_cfg=priv->ab_enable[channel]|+priv->preset_enable[channel]<<1|+priv->irq_trigger[channel]<<3;+base_offset=priv->base+2*channel+1;+outb(QUAD8_CTR_IOR|ior_cfg,base_offset);+}++/* Reset next IRQ trigger function configuration */+priv->next_irq_trigger[channel]=QUAD8_EVENT_NONE;++/* Enable IRQ line */+irq_enabled|=BIT(channel);+}++outb(irq_enabled,priv->base+QUAD8_REG_INDEX_INTERRUPT);++spin_unlock_irqrestore(&priv->lock,irqflags);++return0;+}++staticintquad8_watch_validate(structcounter_device*counter,+conststructcounter_watch*watch)+{+structquad8*constpriv=counter->priv;++if(watch->channel>QUAD8_NUM_COUNTERS-1)+return-EINVAL;++switch(watch->event){+caseCOUNTER_EVENT_OVERFLOW:+if(priv->next_irq_trigger[watch->channel]==QUAD8_EVENT_NONE)+priv->next_irq_trigger[watch->channel]=QUAD8_EVENT_CARRY;+elseif(priv->next_irq_trigger[watch->channel]!=QUAD8_EVENT_CARRY)+return-EINVAL;+return0;+caseCOUNTER_EVENT_THRESHOLD:+if(priv->next_irq_trigger[watch->channel]==QUAD8_EVENT_NONE)+priv->next_irq_trigger[watch->channel]=QUAD8_EVENT_COMPARE;+elseif(priv->next_irq_trigger[watch->channel]!=QUAD8_EVENT_COMPARE)+return-EINVAL;+return0;+caseCOUNTER_EVENT_OVERFLOW_UNDERFLOW:+if(priv->next_irq_trigger[watch->channel]==QUAD8_EVENT_NONE)+priv->next_irq_trigger[watch->channel]=QUAD8_EVENT_CARRY_BORROW;+elseif(priv->next_irq_trigger[watch->channel]!=QUAD8_EVENT_CARRY_BORROW)+return-EINVAL;+return0;+caseCOUNTER_EVENT_INDEX:+if(priv->next_irq_trigger[watch->channel]==QUAD8_EVENT_NONE)+priv->next_irq_trigger[watch->channel]=QUAD8_EVENT_INDEX;+elseif(priv->next_irq_trigger[watch->channel]!=QUAD8_EVENT_INDEX)+return-EINVAL;+return0;+default:+return-EINVAL;+}+}+staticconststructcounter_opsquad8_ops={.signal_read=quad8_signal_read,.count_read=quad8_count_read,.count_write=quad8_count_write,.function_read=quad8_function_read,.function_write=quad8_function_write,-.action_read=quad8_action_read+.action_read=quad8_action_read,+.events_configure=quad8_events_configure,+.watch_validate=quad8_watch_validate,};staticconstchar*constquad8_index_polarity_modes[]={
@@ -579,7 +680,8 @@ static int quad8_count_enable_write(struct counter_device *counter,priv->ab_enable[count->id]=enable;-ior_cfg=enable|priv->preset_enable[count->id]<<1;+ior_cfg=enable|priv->preset_enable[count->id]<<1|+priv->irq_trigger[count->id]<<3;/* Load I/O control configuration */outb(QUAD8_CTR_IOR|ior_cfg,base_offset+1);
@@ -728,7 +830,8 @@ static int quad8_count_preset_enable_write(struct counter_device *counter,priv->preset_enable[count->id]=preset_enable;-ior_cfg=priv->ab_enable[count->id]|preset_enable<<1;+ior_cfg=priv->ab_enable[count->id]|preset_enable<<1|+priv->irq_trigger[count->id]<<3;/* Load I/O control configuration to Input / Output Control Register */outb(QUAD8_CTR_IOR|ior_cfg,base_offset);
@@ -980,11 +1083,54 @@ static struct counter_count quad8_counts[] = {QUAD8_COUNT(7,"Channel 8 Count")};+staticirqreturn_tquad8_irq_handler(intirq,void*private)+{+structquad8*constpriv=private;+constunsignedlongbase=priv->base;+unsignedlongirq_status;+unsignedlongchannel;+u8event;++irq_status=inb(base+QUAD8_REG_INTERRUPT_STATUS);+if(!irq_status)+returnIRQ_NONE;++for_each_set_bit(channel,&irq_status,QUAD8_NUM_COUNTERS){+switch(priv->irq_trigger[channel]){+caseQUAD8_EVENT_CARRY:+event=COUNTER_EVENT_OVERFLOW;+break;+caseQUAD8_EVENT_COMPARE:+event=COUNTER_EVENT_THRESHOLD;+break;+caseQUAD8_EVENT_CARRY_BORROW:+event=COUNTER_EVENT_OVERFLOW_UNDERFLOW;+break;+caseQUAD8_EVENT_INDEX:+event=COUNTER_EVENT_INDEX;+break;+default:+/* should never reach this path */+WARN_ONCE(true,"invalid interrupt trigger function %u configured for channel %lu\n",+priv->irq_trigger[channel],channel);+continue;+}++counter_push_event(&priv->counter,event,channel);+}++/* Clear pending interrupts on device */+outb(QUAD8_CHAN_OP_ENABLE_INTERRUPT_FUNC,base+QUAD8_REG_CHAN_OP);++returnIRQ_HANDLED;+}+staticintquad8_probe(structdevice*dev,unsignedintid){structquad8*priv;inti,j;unsignedintbase_offset;+interr;if(!devm_request_region(dev,base[id],QUAD8_EXTENT,dev_name(dev))){dev_err(dev,"Unable to lock port addresses (0x%X-0x%X)\n",
@@ -1009,6 +1155,8 @@ static int quad8_probe(struct device *dev, unsigned int id)spin_lock_init(&priv->lock);+/* Reset Index/Interrupt Register */+outb(0x00,base[id]+QUAD8_REG_INDEX_INTERRUPT);/* Reset all counters and disable interrupt function */outb(QUAD8_CHAN_OP_RESET_COUNTERS,base[id]+QUAD8_REG_CHAN_OP);/* Set initial configuration for all counters */
@@ -1035,11 +1183,18 @@ static int quad8_probe(struct device *dev, unsigned int id)outb(QUAD8_CTR_IOR,base_offset+1);/* Disable index function; negative index polarity */outb(QUAD8_CTR_IDR,base_offset+1);+/* Initialize next IRQ trigger function configuration */+priv->next_irq_trigger[i]=QUAD8_EVENT_NONE;}/* Disable Differential Encoder Cable Status for all channels */outb(0xFF,base[id]+QUAD8_DIFF_ENCODER_CABLE_STATUS);-/* Enable all counters */-outb(QUAD8_CHAN_OP_ENABLE_COUNTERS,base[id]+QUAD8_REG_CHAN_OP);+/* Enable all counters and enable interrupt function */+outb(QUAD8_CHAN_OP_ENABLE_INTERRUPT_FUNC,base[id]+QUAD8_REG_CHAN_OP);++err=devm_request_irq(dev,irq[id],quad8_irq_handler,IRQF_SHARED,+priv->counter.name,priv);+if(err)+returnerr;returndevm_counter_register(dev,&priv->counter);}
From: Jonathan Cameron <jic23@kernel.org> Date: 2021-08-30 17:14:04
On Fri, 27 Aug 2021 12:47:44 +0900
William Breathitt Gray [off-list ref] wrote:
Changes in v16:
- Define magic numbers for stm32-lptimer-cnt clock polarities
- Define magic numbers for stm32-timer-cnt encoder modes
- Bump KernelVersion to 5.16 in sysfs-bus-counter ABI documentation
- Fix typos in driver API generic-counter.rst documentation file
For convenience, this patchset is also available on my personal git
repo: https://gitlab.com/vilhelmgray/iio/-/tree/counter_chrdev_v16
The patches preceding "counter: Internalize sysfs interface code" are
primarily cleanup and fixes that can be picked up and applied now to the
IIO tree if so desired. The "counter: Internalize sysfs interface code"
patch as well may be considered for pickup because it is relatively safe
and makes no changes to the userspace interface.
To summarize the main points of this patchset: there are no changes to
the existing Counter sysfs userspace interface; a Counter character
device interface is introduced that allows Counter events and associated
data to be read() by userspace; the events_configure() and
watch_validate() driver callbacks are introduced to support Counter
events; and IRQ support is added to the 104-QUAD-8 driver, serving as an
example of how to support the new Counter events functionality.
Hi William,
I'll aim to pick up the first part in a week (too tired today after a lot
of reviewing to even manage the basic sanity check on the changes).
For the rest...
What I'd really like to know is if anyone other than William and I is planning
to review them in depth? (particularly 7 and 8 which are the new interface
patch and docs)
So if anyone reading this is in that category please let me know. We can wait,
but conversely if no one is going to get time / inclination to do it then I
don't want to hold these up any longer and maximum time in linux-next may
be more useful than sitting unloved on the mailing list.
Jonathan
From: Jonathan Cameron <jic23@kernel.org> Date: 2021-09-08 17:44:45
On Fri, 27 Aug 2021 12:47:48 +0900
William Breathitt Gray [off-list ref] wrote:
The Counter subsystem architecture and driver implementations have
changed in order to handle Counter sysfs interactions in a more
consistent way. This patch updates the Generic Counter interface
header file comments to reflect the changes.
Signed-off-by: William Breathitt Gray <redacted>f
From: Jonathan Cameron <jic23@kernel.org> Date: 2021-09-08 17:44:45
On Fri, 27 Aug 2021 12:47:49 +0900
William Breathitt Gray [off-list ref] wrote:
The Counter subsystem architecture and driver implementations have
changed in order to handle Counter sysfs interactions in a more
consistent way. This patch updates the Generic Counter interface
documentation to reflect the changes.
Reviewed-by: David Lechner <david@lechnology.com>
Signed-off-by: William Breathitt Gray <redacted>
Applied.
I'm going to pause at this point. We are early in the cycle anyway
so there is no rush and I'd like to take another look at the remainder
of the series + leave a little longer for others to take a look at it!
Thanks,
Jonathan
@@ -286,7 +286,14 @@ What: /sys/bus/counter/devices/counterX/signalY/signal KernelVersion: 5.2 Contact: linux-iio@vger.kernel.org Description:- Signal data of Signal Y represented as a string.+ Signal level state of Signal Y. The following signal level+ states are available:++ low:+ Low level state.++ high:+ High level state. What: /sys/bus/counter/devices/counterX/signalY/synchronous_mode KernelVersion: 5.2
@@ -250,8 +250,8 @@ for defining a counter device...kernel-doc:: drivers/counter/counter.c:export:-Implementation-==============+Driver Implementation+===================== To support a counter device, a driver must first allocate the available Counter Signals via counter_signal structures. These Signals should
@@ -267,25 +267,61 @@ respective counter_count structure. These counter_count structures are set to the counts array member of an allocated counter_device structure before the Counter is registered to the system.-Driver callbacks should be provided to the counter_device structure via-a constant counter_ops structure in order to communicate with the-device: to read and write various Signals and Counts, and to set and get-the "action mode" and "function mode" for various Synapses and Counts-respectively.+Driver callbacks must be provided to the counter_device structure in+order to communicate with the device: to read and write various Signals+and Counts, and to set and get the "action mode" and "function mode" for+various Synapses and Counts respectively. A defined counter_device structure may be registered to the system by passing it to the counter_register function, and unregistered by passing it to the counter_unregister function. Similarly, the-devm_counter_register and devm_counter_unregister functions may be used-if device memory-managed registration is desired.--Extension sysfs attributes can be created for auxiliary functionality-and data by passing in defined counter_device_ext, counter_count_ext,-and counter_signal_ext structures. In these cases, the-counter_device_ext structure is used for global/miscellaneous exposure-and configuration of the respective Counter device, while the-counter_count_ext and counter_signal_ext structures allow for auxiliary-exposure and configuration of a specific Count or Signal respectively.+devm_counter_register function may be used if device memory-managed+registration is desired.++The struct counter_comp structure is used to define counter extensions+for Signals, Synapses, and Counts.++The "type" member specifies the type of high-level data (e.g. BOOL,+COUNT_DIRECTION, etc.) handled by this extension. The "``*_read``" and+"``*_write``" members can then be set by the counter device driver with+callbacks to handle that data using native C data types (i.e. u8, u64,+etc.).++Convenience macros such as ``COUNTER_COMP_COUNT_U64`` are provided for+use by driver authors. In particular, driver authors are expected to use+the provided macros for standard Counter subsystem attributes in order+to maintain a consistent interface for userspace. For example, a counter+device driver may define several standard attributes like so::++ struct counter_comp count_ext[] = {+ COUNTER_COMP_DIRECTION(count_direction_read),+ COUNTER_COMP_ENABLE(count_enable_read, count_enable_write),+ COUNTER_COMP_CEILING(count_ceiling_read, count_ceiling_write),+ };++This makes it simple to see, add, and modify the attributes that are+supported by this driver ("direction", "enable", and "ceiling") and to+maintain this code without getting lost in a web of struct braces.++Callbacks must match the function type expected for the respective+component or extension. These function types are defined in the struct+counter_comp structure as the "``*_read``" and "``*_write``" union+members.++The corresponding callback prototypes for the extensions mentioned in+the previous example above would be::++ int count_direction_read(struct counter_device *counter,+ struct counter_count *count,+ enum counter_count_direction *direction);+ int count_enable_read(struct counter_device *counter,+ struct counter_count *count, u8 *enable);+ int count_enable_write(struct counter_device *counter,+ struct counter_count *count, u8 enable);+ int count_ceiling_read(struct counter_device *counter,+ struct counter_count *count, u64 *ceiling);+ int count_ceiling_write(struct counter_device *counter,+ struct counter_count *count, u64 ceiling); Determining the type of extension to create is a matter of scope.
@@ -313,52 +349,127 @@ Determining the type of extension to create is a matter of scope. chip overheated via a device extension called "error_overtemp": /sys/bus/counter/devices/counterX/error_overtemp-Architecture-============--When the Generic Counter interface counter module is loaded, the-counter_init function is called which registers a bus_type named-"counter" to the system. Subsequently, when the module is unloaded, the-counter_exit function is called which unregisters the bus_type named-"counter" from the system.--Counter devices are registered to the system via the counter_register-function, and later removed via the counter_unregister function. The-counter_register function establishes a unique ID for the Counter-device and creates a respective sysfs directory, where X is the-mentioned unique ID:-- /sys/bus/counter/devices/counterX--Sysfs attributes are created within the counterX directory to expose-functionality, configurations, and data relating to the Counts, Signals,-and Synapses of the Counter device, as well as options and information-for the Counter device itself.--Each Signal has a directory created to house its relevant sysfs-attributes, where Y is the unique ID of the respective Signal:-- /sys/bus/counter/devices/counterX/signalY--Similarly, each Count has a directory created to house its relevant-sysfs attributes, where Y is the unique ID of the respective Count:-- /sys/bus/counter/devices/counterX/countY--For a more detailed breakdown of the available Generic Counter interface-sysfs attributes, please refer to the-Documentation/ABI/testing/sysfs-bus-counter file.--The Signals and Counts associated with the Counter device are registered-to the system as well by the counter_register function. The-signal_read/signal_write driver callbacks are associated with their-respective Signal attributes, while the count_read/count_write and-function_get/function_set driver callbacks are associated with their-respective Count attributes; similarly, the same is true for the-action_get/action_set driver callbacks and their respective Synapse-attributes. If a driver callback is left undefined, then the respective-read/write permission is left disabled for the relevant attributes.--Similarly, extension sysfs attributes are created for the defined-counter_device_ext, counter_count_ext, and counter_signal_ext-structures that are passed in.+Subsystem Architecture+======================++Counter drivers pass and take data natively (i.e. ``u8``, ``u64``, etc.)+and the shared counter module handles the translation between the sysfs+interface. This guarantees a standard userspace interface for all+counter drivers, and enables a Generic Counter chrdev interface via a+generalized device driver ABI.++A high-level view of how a count value is passed down from a counter+driver is exemplified by the following. The driver callbacks are first+registered to the Counter core component for use by the Counter+userspace interface components::++ Driver callbacks registration:+ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~+ +----------------------------++| Counter device driver |+ +----------------------------++| Processes data from device |+ +----------------------------++ |+ -------------------+ / driver callbacks /+ -------------------+ |+ V+ +----------------------++| Counter core |+ +----------------------++| Routes device driver |+| callbacks to the |+| userspace interfaces |+ +----------------------++ |+ -------------------+ / driver callbacks /+ -------------------+ |+ +---------------++ |+ V+ +--------------------++| Counter sysfs |+ +--------------------++| Translates to the |+| standard Counter |+| sysfs output |+ +--------------------+++Thereafter, data can be transferred directly between the Counter device+driver and Counter userspace interface::++ Count data request:+ ~~~~~~~~~~~~~~~~~~~+ ----------------------+ / Counter device \+ +----------------------++| Count register: 0x28 |+ +----------------------++ |+ -----------------+ / raw count data /+ -----------------+ |+ V+ +----------------------------++| Counter device driver |+ +----------------------------++| Processes data from device |+ |----------------------------|+| Type: u64 |+| Value: 42 |+ +----------------------------++ |+ ----------+ / u64 /+ ----------+ |+ +---------------++ |+ V+ +--------------------++| Counter sysfs |+ +--------------------++| Translates to the |+| standard Counter |+| sysfs output |+ |--------------------|+| Type: const char * |+| Value: "42" |+ +--------------------++ |+ ---------------+ / const char * /+ ---------------+ |+ V+ +--------------------------------------------------++|`/sys/bus/counter/devices/counterX/countY/count` |+ +--------------------------------------------------++ \ Count: "42" /+ --------------------------------------------------++There are three primary components involved:++Counter device driver+---------------------+Communicates with the hardware device to read/write data; e.g. counter+drivers for quadrature encoders, timers, etc.++Counter core+------------+Registers the counter device driver to the system so that the respective+callbacks are called during userspace interaction.++Counter sysfs+-------------+Translates counter data to the standard Counter sysfs interface format+and vice versa.++Please refer to the ``Documentation/ABI/testing/sysfs-bus-counter`` file+for a detailed breakdown of the available Generic Counter interface+sysfs attributes.
@@ -223,19 +223,6 @@ whether an input line is differential or single-ended) and instead focus on the core idea of what the data and process represent (e.g. position as interpreted from quadrature encoding data).-Userspace Interface-===================--Several sysfs attributes are generated by the Generic Counter interface,-and reside under the /sys/bus/counter/devices/counterX directory, where-counterX refers to the respective counter device. Please see-Documentation/ABI/testing/sysfs-bus-counter for detailed-information on each Generic Counter interface sysfs attribute.--Through these sysfs attributes, programs and scripts may interact with-the Generic Counter paradigm Counts, Signals, and Synapses of respective-counter devices.- Driver API ==========
@@ -388,16 +375,16 @@ userspace interface components:: / driver callbacks / ------------------- |- +---------------+- |- V- +--------------------+-| Counter sysfs |- +--------------------+-| Translates to the |-| standard Counter |-| sysfs output |- +--------------------++ +---------------+---------------++| |+ V V+ +--------------------+ +---------------------++| Counter sysfs | | Counter chrdev |+ +--------------------+ +---------------------++| Translates to the | | Translates to the |+| standard Counter | | standard Counter |+| sysfs output | | character device |+ +--------------------+ +---------------------+ Thereafter, data can be transferred directly between the Counter device driver and Counter userspace interface::
@@ -453,7 +447,7 @@ driver and Counter userspace interface:: \ Count: "42" / ---------------------------------------------------There are three primary components involved:+There are four primary components involved: Counter device driver ---------------------
@@ -473,3 +467,104 @@ and vice versa. Please refer to the ``Documentation/ABI/testing/sysfs-bus-counter`` file for a detailed breakdown of the available Generic Counter interface sysfs attributes.++Counter chrdev+--------------+Translates counter data to the standard Counter character device; data
Whilst it is clear when you read on, perhaps change "data" to "Counter
events" here.
quoted hunk
+is transferred via standard character device read calls, while Counter
+events are configured via ioctl calls.
+
+Sysfs Interface
+===============
+
+Several sysfs attributes are generated by the Generic Counter interface,
+and reside under the ``/sys/bus/counter/devices/counterX`` directory,
+where ``X`` is to the respective counter device id. Please see
+``Documentation/ABI/testing/sysfs-bus-counter`` for detailed information
+on each Generic Counter interface sysfs attribute.
+
+Through these sysfs attributes, programs and scripts may interact with
+the Generic Counter paradigm Counts, Signals, and Synapses of respective
+counter devices.
+
+Counter Character Device
+========================
+
+Counter character device nodes are created under the ``/dev`` directory
+as ``counterX``, where ``X`` is the respective counter device id.
+Defines for the standard Counter data types are exposed via the
+userspace ``include/uapi/linux/counter.h`` file.
+
+Counter events
+--------------
+Counter device drivers can support Counter events by utilizing the
+``counter_push_event`` function::
+
+ void counter_push_event(struct counter_device *const counter, const u8 event,
+ const u8 channel);
+
+The event id is specified by the ``event`` parameter; the event channel
+id is specified by the ``channel`` parameter. When this function is
+called, the Counter data associated with the respective event is
+gathered, and a ``struct counter_event`` is generated for each datum and
+pushed to userspace.
+
+Counter events can be configured by users to report various Counter
+data of interest. This can be conceptualized as a list of Counter
+component read calls to perform. For example:
+
+ +------------------------+------------------------+
+ | COUNTER_EVENT_OVERFLOW | COUNTER_EVENT_INDEX |
+ +========================+========================+
+ | Channel 0 | Channel 0 |
+ +------------------------+------------------------+
+ | * Count 0 | * Signal 0 |
+ | * Count 1 | * Signal 0 Extension 0 |
+ | * Signal 3 | * Extension 4 |
+ | * Count 4 Extension 2 +------------------------+
+ | * Signal 5 Extension 0 | Channel 1 |
+ | +------------------------+
+ | | * Signal 4 |
+ | | * Signal 4 Extension 0 |
+ | | * Count 7 |
+ +------------------------+------------------------+
+
+When ``counter_push_event(counter, COUNTER_EVENT_INDEX, 1)`` is called
+for example, it will go down the list for the ``COUNTER_EVENT_INDEX``
+event channel 1 and execute the read callbacks for Signal 4, Signal 4
+Extension 0, and Count 7 -- the data returned for each is pushed to a
+kfifo as a ``struct counter_event``, which userspace can retrieve via a
+standard read operation on the respective character device node.
+
+Userspace
+---------
+Userspace applications can configure Counter events via ioctl operations
+on the Counter character device node. There following ioctl codes are
+supported and provided by the ``linux/counter.h`` userspace header file:
+
+* :c:macro:`COUNTER_ADD_WATCH_IOCTL`
+
+* :c:macro:`COUNTER_ENABLE_EVENTS_IOCTL`
+
+* :c:macro:`COUNTER_DISABLE_EVENTS_IOCTL`
+
+To configure events to gather Counter data, users first populate a
+``struct counter_watch`` with the relevant event id, event channel id,
+and the information for the desired Counter component from which to
+read, and then pass it via the ``COUNTER_ADD_WATCH_IOCTL`` ioctl
+command.
+
+Note that an event can be watched without gathering Counter data by
+setting the ``component.type`` member equal to
+``COUNTER_COMPONENT_NONE``. With this configuration the Counter
+character device will simply populate the event timestamps for those
+respective ``struct counter_event`` elements and ignore the component
+value.
+
+The ``COUNTER_ADD_WATCH_IOCTL`` command will buffer these Counter
+watches. When ready, the ``COUNTER_ENABLE_EVENTS_IOCTL`` ioctl command
+may be used to activate these Counter watches.
+
+Userspace applications can then execute a ``read`` operation (optionally
+calling ``poll`` first) on the Counter character device node to retrieve
+``struct counter_event`` elements with the desired data.
From: Jonathan Cameron <jic23@kernel.org> Date: 2021-09-12 16:15:17
On Fri, 27 Aug 2021 12:47:51 +0900
William Breathitt Gray [off-list ref] wrote:
This patch introduces a character device interface for the Counter
subsystem. Device data is exposed through standard character device read
operations. Device data is gathered when a Counter event is pushed by
the respective Counter device driver. Configuration is handled via ioctl
operations on the respective Counter character device node.
Cc: David Lechner <david@lechnology.com>
Cc: Gwendal Grignou <redacted>
Cc: Dan Carpenter <redacted>
Cc: Oleksij Rempel <o.rempel@pengutronix.de>
Signed-off-by: William Breathitt Gray <redacted>
Hi William,
Why the bit based lock? It feels like a mutex_trylock() type approach or
spinlock_trylock() would be a more common solution to this problem.
There is precedence for doing what you have here though so I'm not that
worried about it.
There are a few more things inline.
I've now been through this patch with as fine toothed comb as I'm likely to
do so. Hence I won't do another review unless there are substantial changes.
I nearly applied it as it stands, but given we aren't in a rush (merge window
open), it's worth just a little more time to tidy up loose ends.
Jonathan
@@ -0,0 +1,553 @@+// SPDX-License-Identifier: GPL-2.0+/*+*GenericCountercharacterdeviceinterface+*Copyright(C)2020WilliamBreathittGray+*/+#include<linux/bitops.h>+#include<linux/cdev.h>+#include<linux/counter.h>+#include<linux/err.h>+#include<linux/errno.h>+#include<linux/export.h>+#include<linux/fs.h>+#include<linux/kfifo.h>+#include<linux/list.h>+#include<linux/mutex.h>+#include<linux/nospec.h>+#include<linux/poll.h>+#include<linux/slab.h>+#include<linux/spinlock.h>+#include<linux/timekeeping.h>+#include<linux/types.h>+#include<linux/uaccess.h>+#include<linux/wait.h>++#include"counter-chrdev.h"++structcounter_comp_node{+structlist_headl;+structcounter_componentcomponent;+structcounter_compcomp;+void*parent;+};++staticssize_tcounter_chrdev_read(structfile*filp,char__user*buf,+size_tlen,loff_t*f_ps)+{+structcounter_device*constcounter=filp->private_data;+interr;+unsignedintcopied;++if(!counter->ops)+return-ENODEV;++if(len<sizeof(structcounter_event))+return-EINVAL;++do{+if(kfifo_is_empty(&counter->events)){+if(filp->f_flags&O_NONBLOCK)+return-EAGAIN;++err=wait_event_interruptible(counter->events_wait,+!kfifo_is_empty(&counter->events)||+!counter->ops);+if(err<0)+returnerr;+if(!counter->ops)+return-ENODEV;+}++if(mutex_lock_interruptible(&counter->events_lock))+return-ERESTARTSYS;+err=kfifo_to_user(&counter->events,buf,len,&copied);+mutex_unlock(&counter->events_lock);+if(err<0)+returnerr;+}while(!copied);++returncopied;+}++static__poll_tcounter_chrdev_poll(structfile*filp,+structpoll_table_struct*pollt)+{+structcounter_device*constcounter=filp->private_data;+__poll_tevents=0;++if(!counter->ops)+returnevents;++poll_wait(filp,&counter->events_wait,pollt);++if(!kfifo_is_empty(&counter->events))+events=EPOLLIN|EPOLLRDNORM;++returnevents;+}++staticvoidcounter_events_list_free(structlist_head*constevents_list)+{+structcounter_event_node*p,*n;+structcounter_comp_node*q,*o;++list_for_each_entry_safe(p,n,events_list,l){+/* Free associated component nodes */+list_for_each_entry_safe(q,o,&p->comp_list,l){+list_del(&q->l);+kfree(q);+}++/* Free event node */+list_del(&p->l);+kfree(p);+}+}++staticintcounter_set_event_node(structcounter_device*constcounter,+structcounter_watch*constwatch,+conststructcounter_comp_node*constcfg)+{+structcounter_event_node*event_node;+interr=0;+structcounter_comp_node*comp_node;++/* Search for event in the list */+list_for_each_entry(event_node,&counter->next_events_list,l)+if(event_node->event==watch->event&&+event_node->channel==watch->channel)+break;++/* If event is not already in the list */+if(&event_node->l==&counter->next_events_list){+/* Allocate new event node */+event_node=kmalloc(sizeof(*event_node),GFP_KERNEL);+if(!event_node)+return-ENOMEM;++/* Configure event node and add to the list */+event_node->event=watch->event;+event_node->channel=watch->channel;+INIT_LIST_HEAD(&event_node->comp_list);+list_add(&event_node->l,&counter->next_events_list);+}++/* Check if component watch has already been set before */+list_for_each_entry(comp_node,&event_node->comp_list,l)+if(comp_node->parent==cfg->parent&&+comp_node->comp.count_u8_read==cfg->comp.count_u8_read){+err=-EINVAL;+gotoexit_free_event_node;+}++/* Allocate component node */+comp_node=kmalloc(sizeof(*comp_node),GFP_KERNEL);+if(!comp_node){+err=-ENOMEM;+gotoexit_free_event_node;+}+*comp_node=*cfg;++/* Add component node to event node */+list_add_tail(&comp_node->l,&event_node->comp_list);++exit_free_event_node:+/* Free event node if no one else is watching */+if(list_empty(&event_node->comp_list)){+list_del(&event_node->l);+kfree(event_node);+}++returnerr;+}++staticintcounter_enable_events(structcounter_device*constcounter)+{+unsignedlongflags;+interr=0;++mutex_lock(&counter->n_events_list_lock);+spin_lock_irqsave(&counter->events_list_lock,flags);++counter_events_list_free(&counter->events_list);+list_replace_init(&counter->next_events_list,+&counter->events_list);++if(counter->ops->events_configure)+err=counter->ops->events_configure(counter);++spin_unlock_irqrestore(&counter->events_list_lock,flags);+mutex_unlock(&counter->n_events_list_lock);++returnerr;+}++staticintcounter_disable_events(structcounter_device*constcounter)+{+unsignedlongflags;+interr=0;++spin_lock_irqsave(&counter->events_list_lock,flags);++counter_events_list_free(&counter->events_list);++if(counter->ops->events_configure)+err=counter->ops->events_configure(counter);++spin_unlock_irqrestore(&counter->events_list_lock,flags);++mutex_lock(&counter->n_events_list_lock);++counter_events_list_free(&counter->next_events_list);++mutex_unlock(&counter->n_events_list_lock);++returnerr;+}++staticintcounter_add_watch(structcounter_device*constcounter,+constunsignedlongarg)+{+void__user*constuwatch=(void__user*)arg;+structcounter_watchwatch;+structcounter_comp_nodecomp_node={};+size_tparent,id;+structcounter_comp*ext;+size_tnum_ext;+interr=0;++if(copy_from_user(&watch,uwatch,sizeof(watch)))+return-EFAULT;++if(watch.component.type==COUNTER_COMPONENT_NONE)+gotono_component;++parent=watch.component.parent;++/* Configure parent component info for comp node */+switch(watch.component.scope){+caseCOUNTER_SCOPE_DEVICE:+ext=counter->ext;+num_ext=counter->num_ext;+break;+caseCOUNTER_SCOPE_SIGNAL:+if(parent>=counter->num_signals)+return-EINVAL;+parent=array_index_nospec(parent,counter->num_signals);++comp_node.parent=counter->signals+parent;++ext=counter->signals[parent].ext;+num_ext=counter->signals[parent].num_ext;+break;+caseCOUNTER_SCOPE_COUNT:+if(parent>=counter->num_counts)+return-EINVAL;+parent=array_index_nospec(parent,counter->num_counts);++comp_node.parent=counter->counts+parent;++ext=counter->counts[parent].ext;+num_ext=counter->counts[parent].num_ext;+break;+default:+return-EINVAL;+}++id=watch.component.id;++/* Configure component info for comp node */+switch(watch.component.type){+caseCOUNTER_COMPONENT_SIGNAL:+if(watch.component.scope!=COUNTER_SCOPE_SIGNAL)+return-EINVAL;++comp_node.comp.type=COUNTER_COMP_SIGNAL_LEVEL;+comp_node.comp.signal_u32_read=counter->ops->signal_read;+break;+caseCOUNTER_COMPONENT_COUNT:+if(watch.component.scope!=COUNTER_SCOPE_COUNT)+return-EINVAL;++comp_node.comp.type=COUNTER_COMP_U64;+comp_node.comp.count_u64_read=counter->ops->count_read;+break;+caseCOUNTER_COMPONENT_FUNCTION:+if(watch.component.scope!=COUNTER_SCOPE_COUNT)+return-EINVAL;++comp_node.comp.type=COUNTER_COMP_FUNCTION;+comp_node.comp.count_u32_read=counter->ops->function_read;+break;+caseCOUNTER_COMPONENT_SYNAPSE_ACTION:+if(watch.component.scope!=COUNTER_SCOPE_COUNT)+return-EINVAL;+if(id>=counter->counts[parent].num_synapses)+return-EINVAL;+id=array_index_nospec(id,counter->counts[parent].num_synapses);++comp_node.comp.type=COUNTER_COMP_SYNAPSE_ACTION;+comp_node.comp.action_read=counter->ops->action_read;+comp_node.comp.priv=counter->counts[parent].synapses+id;+break;+caseCOUNTER_COMPONENT_EXTENSION:+if(id>=num_ext)+return-EINVAL;+id=array_index_nospec(id,num_ext);++comp_node.comp=ext[id];+break;+default:+return-EINVAL;+}+/* Check if any read callback is set; this is part of a union */+if(!comp_node.comp.count_u8_read)
This isn't valid for C. The compiler is allowed to treat the elements
of a union as not being in the same memory etc unless they are of the same
type (which isn't true here - I think anyway)...
So whilst it seems silly you need to check all the callbacks in this if
statement. It will 'almost' always work as you have it, but it's possible
some future optimisation by the compiler will mean that u8_read would be
set by this point, but it delayed setting e.g. u32_read until after
this check.
+ return -EOPNOTSUPP;
+
+no_component:
+ mutex_lock(&counter->n_events_list_lock);
+
+ if (counter->ops->watch_validate) {
+ err = counter->ops->watch_validate(counter, &watch);
+ if (err < 0)
+ goto err_exit;
+ }
+
+ comp_node.component = watch.component;
+
+ err = counter_set_event_node(counter, &watch, &comp_node);
+
+err_exit:
+ mutex_unlock(&counter->n_events_list_lock);
+
+ return err;
+}
+
+static long counter_chrdev_ioctl(struct file *filp, unsigned int cmd,
+ unsigned long arg)
+{
+ struct counter_device *const counter = filp->private_data;
+ int ret = -ENODEV;
+
+ mutex_lock(&counter->ops_exist_lock);
+
+ if (!counter->ops)
+ goto out_unlock;
+
+ switch (cmd) {
+ case COUNTER_ADD_WATCH_IOCTL:
+ ret = counter_add_watch(counter, arg);
+ break;
+ case COUNTER_ENABLE_EVENTS_IOCTL:
+ ret = counter_enable_events(counter);
+ break;
+ case COUNTER_DISABLE_EVENTS_IOCTL:
+ ret = counter_disable_events(counter);
+ break;
+ default:
+ ret = -ENOIOCTLCMD;
+ break;
+ }
+
+out_unlock:
+ mutex_unlock(&counter->ops_exist_lock);
+
+ return ret;
+}
+
+static int counter_chrdev_open(struct inode *inode, struct file *filp)
+{
+ struct counter_device *const counter = container_of(inode->i_cdev,
+ typeof(*counter),
+ chrdev);
+
+ if (test_and_set_bit_lock(0, counter->chrdev_lock))
+ return -EBUSY;
+
+ get_device(&counter->dev);
+ filp->private_data = counter;
+
+ return nonseekable_open(inode, filp);
+}
+
+static int counter_chrdev_release(struct inode *inode, struct file *filp)
+{
+ struct counter_device *const counter = filp->private_data;
+ int ret = 0;
+
+ mutex_lock(&counter->ops_exist_lock);
+
+ if (!counter->ops) {
This needs a comment to explain how you would get here and
why these two lists need cleaning up here if we do.
Superficially it feels to me like you could just add a counter->ops check in
counter_disable_events() and then call that directly. I'm guessing
I am missing a deadlock or similar however.
To think about: This isn't doing the add. So would counter_chrdev_init() be more
appropriate? The fact it can fail due to the kfifo_alloc makes that
naming less than ideal though.
+{
+ /* Initialize Counter events lists */
+ INIT_LIST_HEAD(&counter->events_list);
+ INIT_LIST_HEAD(&counter->next_events_list);
+ spin_lock_init(&counter->events_list_lock);
+ mutex_init(&counter->n_events_list_lock);
+ init_waitqueue_head(&counter->events_wait);
+ mutex_init(&counter->events_lock);
+
+ /* Initialize character device */
+ clear_bit(0, counter->chrdev_lock);
+ cdev_init(&counter->chrdev, &counter_fops);
+
+ /* Allocate Counter events queue */
+ return kfifo_alloc(&counter->events, 64, GFP_KERNEL);
+}
+
+void counter_chrdev_remove(struct counter_device *const counter)
+{
+ kfifo_free(&counter->events);
+}
+
+static int counter_get_data(struct counter_device *const counter,
+ const struct counter_comp_node *const comp_node,
+ u64 *const value)
+{
+ const struct counter_comp *const comp = &comp_node->comp;
+ void *const parent = comp_node->parent;
+ u8 value_u8 = 0;
+ u32 value_u32 = 0;
+ int ret;
+
+ if (comp_node->component.type == COUNTER_COMPONENT_NONE)
+ return 0;
+
+ switch (comp->type) {
+ case COUNTER_COMP_U8:
+ case COUNTER_COMP_BOOL:
+ switch (comp_node->component.scope) {
+ case COUNTER_SCOPE_DEVICE:
+ ret = comp->device_u8_read(counter, &value_u8);
+ break;
+ case COUNTER_SCOPE_SIGNAL:
+ ret = comp->signal_u8_read(counter, parent, &value_u8);
+ break;
+ case COUNTER_SCOPE_COUNT:
+ ret = comp->count_u8_read(counter, parent, &value_u8);
+ break;
+ }
+ *value = value_u8;
+ return ret;
+ case COUNTER_COMP_SIGNAL_LEVEL:
+ case COUNTER_COMP_FUNCTION:
+ case COUNTER_COMP_ENUM:
+ case COUNTER_COMP_COUNT_DIRECTION:
+ case COUNTER_COMP_COUNT_MODE:
+ switch (comp_node->component.scope) {
+ case COUNTER_SCOPE_DEVICE:
+ ret = comp->device_u32_read(counter, &value_u32);
+ break;
+ case COUNTER_SCOPE_SIGNAL:
+ ret = comp->signal_u32_read(counter, parent,
+ &value_u32);
+ break;
+ case COUNTER_SCOPE_COUNT:
+ ret = comp->count_u32_read(counter, parent, &value_u32);
+ break;
+ }
+ *value = value_u32;
+ return ret;
+ case COUNTER_COMP_U64:
+ switch (comp_node->component.scope) {
+ case COUNTER_SCOPE_DEVICE:
+ return comp->device_u64_read(counter, value);
+ case COUNTER_SCOPE_SIGNAL:
+ return comp->signal_u64_read(counter, parent, value);
+ case COUNTER_SCOPE_COUNT:
+ return comp->count_u64_read(counter, parent, value);
+ default:
+ return -EINVAL;
+ }
+ case COUNTER_COMP_SYNAPSE_ACTION:
+ ret = comp->action_read(counter, parent, comp->priv,
+ &value_u32);
+ *value = value_u32;
+ return ret;
+ default:
+ return -EINVAL;
+ }
+}
+
+/**
+ * counter_push_event - queue event for userspace reading
+ * @counter: pointer to Counter structure
+ * @event: triggered event
+ * @channel: event channel
+ *
+ * Note: If no one is watching for the respective event, it is silently
+ * discarded.
+ */
+void counter_push_event(struct counter_device *const counter, const u8 event,
+ const u8 channel)
+{
+ struct counter_event ev;
+ unsigned int copied = 0;
+ unsigned long flags;
+ struct counter_event_node *event_node;
+ struct counter_comp_node *comp_node;
+
+ ev.timestamp = ktime_get_ns();
+ ev.watch.event = event;
+ ev.watch.channel = channel;
+
+ /* Could be in an interrupt context, so use a spin lock */
+ spin_lock_irqsave(&counter->events_list_lock, flags);
+
+ /* Search for event in the list */
Worth keeping in mind that searching a list is not exactly a scalable
solution even if it's fine for now.
+ list_for_each_entry(event_node, &counter->events_list, l)
+ if (event_node->event == event &&
+ event_node->channel == channel)
+ break;
+
+ /* If event is not in the list */
+ if (&event_node->l == &counter->events_list)
+ goto exit_early;
+
+ /* Read and queue relevant comp for userspace */
+ list_for_each_entry(comp_node, &event_node->comp_list, l) {
+ ev.watch.component = comp_node->component;
+ ev.status = -counter_get_data(counter, comp_node, &ev.value);
+
+ copied += kfifo_in(&counter->events, &ev, 1);
I had a short debate with myself on whether this or kfifo_put() was more appropriate
(as fixed record length). I think this particular case they end up the same anyway
so it doesn't matter (nothing to do here!)
@@ -3,14 +3,22 @@*GenericCounterinterface*Copyright(C)2020WilliamBreathittGray*/+#include<linux/cdev.h>#include<linux/counter.h>#include<linux/device.h>+#include<linux/device/bus.h>#include<linux/export.h>+#include<linux/fs.h>#include<linux/gfp.h>#include<linux/idr.h>#include<linux/init.h>+#include<linux/kdev_t.h>#include<linux/module.h>+#include<linux/mutex.h>+#include<linux/types.h>+#include<linux/wait.h>+#include"counter-chrdev.h"#include"counter-sysfs.h"/* Provides a unique ID for each counter device */
@@ -53,10 +66,13 @@ int counter_register(struct counter_device *const counter)if(id<0)returnid;+mutex_init(&counter->ops_exist_lock);+/* Configure device structure for Counter */dev->id=id;dev->type=&counter_device_type;dev->bus=&counter_bus_type;+dev->devt=MKDEV(MAJOR(counter_devt),id);if(counter->parent){dev->parent=counter->parent;dev->of_node=counter->parent->of_node;
@@ -64,18 +80,22 @@ int counter_register(struct counter_device *const counter)device_initialize(dev);dev_set_drvdata(dev,counter);-/* Add Counter sysfs attributes */-err=counter_sysfs_add(counter);+err=counter_chrdev_add(counter);if(err<0)gotoerr_free_id;-/* Add device to system */-err=device_add(dev);+err=counter_sysfs_add(counter);if(err<0)-gotoerr_free_id;+gotoerr_remove_chrdev;++err=cdev_device_add(&counter->chrdev,dev);+if(err<0)+gotoerr_remove_chrdev;return0;+err_remove_chrdev:+counter_chrdev_remove(counter);
Totally trivial, but the sysfs cleanup is managed by devm_ whilst this counter_chrdev_remove()
is manual. That leaves us in a case where the unwind order is subtly different from the
reverse of the setup order. Is there any reason to not do the sysfs part of register first
and hence have the ordering match?
@@ -127,13 +156,30 @@ int devm_counter_register(struct device *dev, } EXPORT_SYMBOL_GPL(devm_counter_register);+#define COUNTER_DEV_MAX 256+ static int __init counter_init(void) {- return bus_register(&counter_bus_type);+ int err;++ err = bus_register(&counter_bus_type);
Hmm. This isn't ideal in IIO either, but logically bus_register() is the part that
exposes the infrastructure for other drivers to attach to so should be last thing
in init. However, I'm fairly sure the dependency handing will ensure this init()
is finished before it tries to register any counter drivers. Hence we are fine
anyway.
So in conclusion - nothing needs changing here...
From: Jonathan Cameron <jic23@kernel.org> Date: 2021-09-12 16:23:13
On Fri, 27 Aug 2021 12:47:53 +0900
William Breathitt Gray [off-list ref] wrote:
This creates an example Counter program under tools/counter/*
to exemplify the Counter character device interface.
Cc: Pavel Machek <redacted>
Signed-off-by: William Breathitt Gray <redacted>
It's great to have this example, but it very much an example rather than
a generic tool. You may want to revisit and provide a more generic
tool in the future.
A trivial comment about using a loop inline.
Jonathan
@@ -12,6 +12,7 @@ help:@echo' acpi - ACPI tools'@echo' bpf - misc BPF tools'@echo' cgroup - cgroup tools'+@echo' counter - counter tools'@echo' cpupower - a tool for all things x86 CPU power'@echo' debugging - tools for debugging'@echo' firewire - the userspace part of nosy, an IEEE-1394 traffic sniffer'
@@ -0,0 +1,53 @@+# SPDX-License-Identifier: GPL-2.0+include ../scripts/Makefile.include++bindir?=/usr/bin++ifeq ($(srctree),)+srctree:=$(patsubst%/,%,$(dir$(CURDIR)))+srctree:=$(patsubst%/,%,$(dir$(srctree)))+endif++# Do not use make's built-in rules+# (this improves performance and avoids hard-to-debug behaviour);+MAKEFLAGS+=-r++overrideCFLAGS+=-O2-Wall-g-D_GNU_SOURCE-I$(OUTPUT)include++ALL_TARGETS:=counter_example+ALL_PROGRAMS:=$(patsubst%,$(OUTPUT)%,$(ALL_TARGETS))++all:$(ALL_PROGRAMS)++exportsrctreeOUTPUTCCLDCFLAGS+include $(srctree)/tools/build/Makefile.include++#+# We need the following to be outside of kernel tree+#+$(OUTPUT)include/linux/counter.h:../../include/uapi/linux/counter.h+mkdir-p$(OUTPUT)include/linux2>&1||true+ln-sf$(CURDIR)/../../include/uapi/linux/counter.h$@++prepare:$(OUTPUT)include/linux/counter.h++COUNTER_EXAMPLE:=$(OUTPUT)counter_example.o+$(COUNTER_EXAMPLE):prepareFORCE+$(Q)$(MAKE)$(build)=counter_example+$(OUTPUT)counter_example:$(COUNTER_EXAMPLE)+$(QUIET_LINK)$(CC)$(CFLAGS)$(LDFLAGS)$<-o$@++clean:+rm-f$(ALL_PROGRAMS)+rm-rf$(OUTPUT)include/linux/counter.h+find$(if$(OUTPUT),$(OUTPUT),.)-name'*.o'-delete-o-name'\.*.d'-delete++install:$(ALL_PROGRAMS)+install-d-m755$(DESTDIR)$(bindir);\+forprogramin$(ALL_PROGRAMS);do\+install$$program$(DESTDIR)$(bindir);\+done++FORCE:++.PHONY:allinstallcleanFORCEprepare
From: Jonathan Cameron <jic23@kernel.org> Date: 2021-09-12 16:33:13
On Mon, 30 Aug 2021 18:17:06 +0100
Jonathan Cameron [off-list ref] wrote:
On Fri, 27 Aug 2021 12:47:44 +0900
William Breathitt Gray [off-list ref] wrote:
quoted
Changes in v16:
- Define magic numbers for stm32-lptimer-cnt clock polarities
- Define magic numbers for stm32-timer-cnt encoder modes
- Bump KernelVersion to 5.16 in sysfs-bus-counter ABI documentation
- Fix typos in driver API generic-counter.rst documentation file
For convenience, this patchset is also available on my personal git
repo: https://gitlab.com/vilhelmgray/iio/-/tree/counter_chrdev_v16
The patches preceding "counter: Internalize sysfs interface code" are
primarily cleanup and fixes that can be picked up and applied now to the
IIO tree if so desired. The "counter: Internalize sysfs interface code"
patch as well may be considered for pickup because it is relatively safe
and makes no changes to the userspace interface.
To summarize the main points of this patchset: there are no changes to
the existing Counter sysfs userspace interface; a Counter character
device interface is introduced that allows Counter events and associated
data to be read() by userspace; the events_configure() and
watch_validate() driver callbacks are introduced to support Counter
events; and IRQ support is added to the 104-QUAD-8 driver, serving as an
example of how to support the new Counter events functionality.
Hi William,
I'll aim to pick up the first part in a week (too tired today after a lot
of reviewing to even manage the basic sanity check on the changes).
For the rest...
What I'd really like to know is if anyone other than William and I is planning
to review them in depth? (particularly 7 and 8 which are the new interface
patch and docs)
So if anyone reading this is in that category please let me know. We can wait,
but conversely if no one is going to get time / inclination to do it then I
don't want to hold these up any longer and maximum time in linux-next may
be more useful than sitting unloved on the mailing list.
Ah well, looks like it's just the two of us for the chrdev core patches :)
Anyhow, I found time for a more thorough review. I'm not 100% convinced on
the model for the chrdev but you know a lot more about this sort of hardware than
I do and it definitely seems reasonable - if anything it might be more flexible
than it needs to be.
I've highlighted a few small things in the patches. With those fixed I'm happy
to apply the remainder of this series unless someone shouts in the meantime.
There has been plenty of time for review, so fingers crossed that anyone
who hasn't commented, but cares, is happy with how you have done it.
So, lucky v17! Persistence pays off in the end.
Thanks,
Jonathan
From: William Breathitt Gray <hidden> Date: 2021-09-20 10:09:25
On Sun, Sep 12, 2021 at 05:18:42PM +0100, Jonathan Cameron wrote:
On Fri, 27 Aug 2021 12:47:51 +0900
William Breathitt Gray [off-list ref] wrote:
quoted
This patch introduces a character device interface for the Counter
subsystem. Device data is exposed through standard character device read
operations. Device data is gathered when a Counter event is pushed by
the respective Counter device driver. Configuration is handled via ioctl
operations on the respective Counter character device node.
Cc: David Lechner <david@lechnology.com>
Cc: Gwendal Grignou <redacted>
Cc: Dan Carpenter <redacted>
Cc: Oleksij Rempel <o.rempel@pengutronix.de>
Signed-off-by: William Breathitt Gray <redacted>
Hi William,
Why the bit based lock? It feels like a mutex_trylock() type approach or
spinlock_trylock() would be a more common solution to this problem.
There is precedence for doing what you have here though so I'm not that
worried about it.
There are a few more things inline.
I've now been through this patch with as fine toothed comb as I'm likely to
do so. Hence I won't do another review unless there are substantial changes.
I nearly applied it as it stands, but given we aren't in a rush (merge window
open), it's worth just a little more time to tidy up loose ends.
Jonathan
@@ -0,0 +1,553 @@+// SPDX-License-Identifier: GPL-2.0+/*+*GenericCountercharacterdeviceinterface+*Copyright(C)2020WilliamBreathittGray+*/+#include<linux/bitops.h>+#include<linux/cdev.h>+#include<linux/counter.h>+#include<linux/err.h>+#include<linux/errno.h>+#include<linux/export.h>+#include<linux/fs.h>+#include<linux/kfifo.h>+#include<linux/list.h>+#include<linux/mutex.h>+#include<linux/nospec.h>+#include<linux/poll.h>+#include<linux/slab.h>+#include<linux/spinlock.h>+#include<linux/timekeeping.h>+#include<linux/types.h>+#include<linux/uaccess.h>+#include<linux/wait.h>++#include"counter-chrdev.h"++structcounter_comp_node{+structlist_headl;+structcounter_componentcomponent;+structcounter_compcomp;+void*parent;+};++staticssize_tcounter_chrdev_read(structfile*filp,char__user*buf,+size_tlen,loff_t*f_ps)+{+structcounter_device*constcounter=filp->private_data;+interr;+unsignedintcopied;++if(!counter->ops)+return-ENODEV;++if(len<sizeof(structcounter_event))+return-EINVAL;++do{+if(kfifo_is_empty(&counter->events)){+if(filp->f_flags&O_NONBLOCK)+return-EAGAIN;++err=wait_event_interruptible(counter->events_wait,+!kfifo_is_empty(&counter->events)||+!counter->ops);+if(err<0)+returnerr;+if(!counter->ops)+return-ENODEV;+}++if(mutex_lock_interruptible(&counter->events_lock))+return-ERESTARTSYS;+err=kfifo_to_user(&counter->events,buf,len,&copied);+mutex_unlock(&counter->events_lock);+if(err<0)+returnerr;+}while(!copied);++returncopied;+}++static__poll_tcounter_chrdev_poll(structfile*filp,+structpoll_table_struct*pollt)+{+structcounter_device*constcounter=filp->private_data;+__poll_tevents=0;++if(!counter->ops)+returnevents;++poll_wait(filp,&counter->events_wait,pollt);++if(!kfifo_is_empty(&counter->events))+events=EPOLLIN|EPOLLRDNORM;++returnevents;+}++staticvoidcounter_events_list_free(structlist_head*constevents_list)+{+structcounter_event_node*p,*n;+structcounter_comp_node*q,*o;++list_for_each_entry_safe(p,n,events_list,l){+/* Free associated component nodes */+list_for_each_entry_safe(q,o,&p->comp_list,l){+list_del(&q->l);+kfree(q);+}++/* Free event node */+list_del(&p->l);+kfree(p);+}+}++staticintcounter_set_event_node(structcounter_device*constcounter,+structcounter_watch*constwatch,+conststructcounter_comp_node*constcfg)+{+structcounter_event_node*event_node;+interr=0;+structcounter_comp_node*comp_node;++/* Search for event in the list */+list_for_each_entry(event_node,&counter->next_events_list,l)+if(event_node->event==watch->event&&+event_node->channel==watch->channel)+break;++/* If event is not already in the list */+if(&event_node->l==&counter->next_events_list){+/* Allocate new event node */+event_node=kmalloc(sizeof(*event_node),GFP_KERNEL);+if(!event_node)+return-ENOMEM;++/* Configure event node and add to the list */+event_node->event=watch->event;+event_node->channel=watch->channel;+INIT_LIST_HEAD(&event_node->comp_list);+list_add(&event_node->l,&counter->next_events_list);+}++/* Check if component watch has already been set before */+list_for_each_entry(comp_node,&event_node->comp_list,l)+if(comp_node->parent==cfg->parent&&+comp_node->comp.count_u8_read==cfg->comp.count_u8_read){+err=-EINVAL;+gotoexit_free_event_node;+}++/* Allocate component node */+comp_node=kmalloc(sizeof(*comp_node),GFP_KERNEL);+if(!comp_node){+err=-ENOMEM;+gotoexit_free_event_node;+}+*comp_node=*cfg;++/* Add component node to event node */+list_add_tail(&comp_node->l,&event_node->comp_list);++exit_free_event_node:+/* Free event node if no one else is watching */+if(list_empty(&event_node->comp_list)){+list_del(&event_node->l);+kfree(event_node);+}++returnerr;+}++staticintcounter_enable_events(structcounter_device*constcounter)+{+unsignedlongflags;+interr=0;++mutex_lock(&counter->n_events_list_lock);+spin_lock_irqsave(&counter->events_list_lock,flags);++counter_events_list_free(&counter->events_list);+list_replace_init(&counter->next_events_list,+&counter->events_list);++if(counter->ops->events_configure)+err=counter->ops->events_configure(counter);++spin_unlock_irqrestore(&counter->events_list_lock,flags);+mutex_unlock(&counter->n_events_list_lock);++returnerr;+}++staticintcounter_disable_events(structcounter_device*constcounter)+{+unsignedlongflags;+interr=0;++spin_lock_irqsave(&counter->events_list_lock,flags);++counter_events_list_free(&counter->events_list);++if(counter->ops->events_configure)+err=counter->ops->events_configure(counter);++spin_unlock_irqrestore(&counter->events_list_lock,flags);++mutex_lock(&counter->n_events_list_lock);++counter_events_list_free(&counter->next_events_list);++mutex_unlock(&counter->n_events_list_lock);++returnerr;+}++staticintcounter_add_watch(structcounter_device*constcounter,+constunsignedlongarg)+{+void__user*constuwatch=(void__user*)arg;+structcounter_watchwatch;+structcounter_comp_nodecomp_node={};+size_tparent,id;+structcounter_comp*ext;+size_tnum_ext;+interr=0;++if(copy_from_user(&watch,uwatch,sizeof(watch)))+return-EFAULT;++if(watch.component.type==COUNTER_COMPONENT_NONE)+gotono_component;++parent=watch.component.parent;++/* Configure parent component info for comp node */+switch(watch.component.scope){+caseCOUNTER_SCOPE_DEVICE:+ext=counter->ext;+num_ext=counter->num_ext;+break;+caseCOUNTER_SCOPE_SIGNAL:+if(parent>=counter->num_signals)+return-EINVAL;+parent=array_index_nospec(parent,counter->num_signals);++comp_node.parent=counter->signals+parent;++ext=counter->signals[parent].ext;+num_ext=counter->signals[parent].num_ext;+break;+caseCOUNTER_SCOPE_COUNT:+if(parent>=counter->num_counts)+return-EINVAL;+parent=array_index_nospec(parent,counter->num_counts);++comp_node.parent=counter->counts+parent;++ext=counter->counts[parent].ext;+num_ext=counter->counts[parent].num_ext;+break;+default:+return-EINVAL;+}++id=watch.component.id;++/* Configure component info for comp node */+switch(watch.component.type){+caseCOUNTER_COMPONENT_SIGNAL:+if(watch.component.scope!=COUNTER_SCOPE_SIGNAL)+return-EINVAL;++comp_node.comp.type=COUNTER_COMP_SIGNAL_LEVEL;+comp_node.comp.signal_u32_read=counter->ops->signal_read;+break;+caseCOUNTER_COMPONENT_COUNT:+if(watch.component.scope!=COUNTER_SCOPE_COUNT)+return-EINVAL;++comp_node.comp.type=COUNTER_COMP_U64;+comp_node.comp.count_u64_read=counter->ops->count_read;+break;+caseCOUNTER_COMPONENT_FUNCTION:+if(watch.component.scope!=COUNTER_SCOPE_COUNT)+return-EINVAL;++comp_node.comp.type=COUNTER_COMP_FUNCTION;+comp_node.comp.count_u32_read=counter->ops->function_read;+break;+caseCOUNTER_COMPONENT_SYNAPSE_ACTION:+if(watch.component.scope!=COUNTER_SCOPE_COUNT)+return-EINVAL;+if(id>=counter->counts[parent].num_synapses)+return-EINVAL;+id=array_index_nospec(id,counter->counts[parent].num_synapses);++comp_node.comp.type=COUNTER_COMP_SYNAPSE_ACTION;+comp_node.comp.action_read=counter->ops->action_read;+comp_node.comp.priv=counter->counts[parent].synapses+id;+break;+caseCOUNTER_COMPONENT_EXTENSION:+if(id>=num_ext)+return-EINVAL;+id=array_index_nospec(id,num_ext);++comp_node.comp=ext[id];+break;+default:+return-EINVAL;+}+/* Check if any read callback is set; this is part of a union */+if(!comp_node.comp.count_u8_read)
This isn't valid for C. The compiler is allowed to treat the elements
of a union as not being in the same memory etc unless they are of the same
type (which isn't true here - I think anyway)...
So whilst it seems silly you need to check all the callbacks in this if
statement. It will 'almost' always work as you have it, but it's possible
some future optimisation by the compiler will mean that u8_read would be
set by this point, but it delayed setting e.g. u32_read until after
this check.
Ack, I'll recreate a couple macros to handle these checks.
quoted
+ return -EOPNOTSUPP;
+
+no_component:
+ mutex_lock(&counter->n_events_list_lock);
+
+ if (counter->ops->watch_validate) {
+ err = counter->ops->watch_validate(counter, &watch);
+ if (err < 0)
+ goto err_exit;
+ }
+
+ comp_node.component = watch.component;
+
+ err = counter_set_event_node(counter, &watch, &comp_node);
+
+err_exit:
+ mutex_unlock(&counter->n_events_list_lock);
+
+ return err;
+}
+
+static long counter_chrdev_ioctl(struct file *filp, unsigned int cmd,
+ unsigned long arg)
+{
+ struct counter_device *const counter = filp->private_data;
+ int ret = -ENODEV;
+
+ mutex_lock(&counter->ops_exist_lock);
+
+ if (!counter->ops)
+ goto out_unlock;
+
+ switch (cmd) {
+ case COUNTER_ADD_WATCH_IOCTL:
+ ret = counter_add_watch(counter, arg);
+ break;
+ case COUNTER_ENABLE_EVENTS_IOCTL:
+ ret = counter_enable_events(counter);
+ break;
+ case COUNTER_DISABLE_EVENTS_IOCTL:
+ ret = counter_disable_events(counter);
+ break;
+ default:
+ ret = -ENOIOCTLCMD;
+ break;
+ }
+
+out_unlock:
+ mutex_unlock(&counter->ops_exist_lock);
+
+ return ret;
+}
+
+static int counter_chrdev_open(struct inode *inode, struct file *filp)
+{
+ struct counter_device *const counter = container_of(inode->i_cdev,
+ typeof(*counter),
+ chrdev);
+
+ if (test_and_set_bit_lock(0, counter->chrdev_lock))
+ return -EBUSY;
+
+ get_device(&counter->dev);
+ filp->private_data = counter;
+
+ return nonseekable_open(inode, filp);
+}
+
+static int counter_chrdev_release(struct inode *inode, struct file *filp)
+{
+ struct counter_device *const counter = filp->private_data;
+ int ret = 0;
+
+ mutex_lock(&counter->ops_exist_lock);
+
+ if (!counter->ops) {
This needs a comment to explain how you would get here and
why these two lists need cleaning up here if we do.
Superficially it feels to me like you could just add a counter->ops check in
counter_disable_events() and then call that directly. I'm guessing
I am missing a deadlock or similar however.
I don't believe there is a risk of a deadlock, I just felt this
conditional check isn't really related to the counter_disable_events()
path so I kept it seperate; the events lists are freed in
counter_disable_events() but that's incidental rather than the purpose
of the function. My reasoning for the separation is that there are two
scenarios where counter_chrdev_release is called: the first is when a
user closes the chrdev, while the second is when the Counter driver is
removed.
For the first scenario, the counter_disable_events() function is called
to stop events from firing off when noone has the chrdev open. In the
second scenario, we are not interested in disabling events, we know
we're no longer going to be interacting with this device again so we
want to free any held memory.
I want to keep the intention of these code paths clear because the
distinction is important if we start managing additional memory in the
future (i.e. memory unrelated to the events list that would not be freed
in counter_disable_events()), so I'll add a comment explaining what's
happening in this path. Alternatively, I could define a
counter_chrdev_free() function and toss those list free calls into
there, but perhaps that would be overkill right now for just two calls.
To think about: This isn't doing the add. So would counter_chrdev_init() be more
appropriate? The fact it can fail due to the kfifo_alloc makes that
naming less than ideal though.
I've been using the "add" here to refer to adding the chrdev to the
counter_device structure rather than to the rest of the system. I used a
similar naming convention for the counter-sysfs.c file so I think
"counter_chrdev_add" here should be all right and is consistent with the
existing "counter_sysfs_add" function.
quoted
+{
+ /* Initialize Counter events lists */
+ INIT_LIST_HEAD(&counter->events_list);
+ INIT_LIST_HEAD(&counter->next_events_list);
+ spin_lock_init(&counter->events_list_lock);
+ mutex_init(&counter->n_events_list_lock);
+ init_waitqueue_head(&counter->events_wait);
+ mutex_init(&counter->events_lock);
+
+ /* Initialize character device */
+ clear_bit(0, counter->chrdev_lock);
+ cdev_init(&counter->chrdev, &counter_fops);
+
+ /* Allocate Counter events queue */
+ return kfifo_alloc(&counter->events, 64, GFP_KERNEL);
+}
+
+void counter_chrdev_remove(struct counter_device *const counter)
+{
+ kfifo_free(&counter->events);
+}
+
+static int counter_get_data(struct counter_device *const counter,
+ const struct counter_comp_node *const comp_node,
+ u64 *const value)
+{
+ const struct counter_comp *const comp = &comp_node->comp;
+ void *const parent = comp_node->parent;
+ u8 value_u8 = 0;
+ u32 value_u32 = 0;
+ int ret;
+
+ if (comp_node->component.type == COUNTER_COMPONENT_NONE)
+ return 0;
+
+ switch (comp->type) {
+ case COUNTER_COMP_U8:
+ case COUNTER_COMP_BOOL:
+ switch (comp_node->component.scope) {
+ case COUNTER_SCOPE_DEVICE:
+ ret = comp->device_u8_read(counter, &value_u8);
+ break;
+ case COUNTER_SCOPE_SIGNAL:
+ ret = comp->signal_u8_read(counter, parent, &value_u8);
+ break;
+ case COUNTER_SCOPE_COUNT:
+ ret = comp->count_u8_read(counter, parent, &value_u8);
+ break;
+ }
+ *value = value_u8;
+ return ret;
+ case COUNTER_COMP_SIGNAL_LEVEL:
+ case COUNTER_COMP_FUNCTION:
+ case COUNTER_COMP_ENUM:
+ case COUNTER_COMP_COUNT_DIRECTION:
+ case COUNTER_COMP_COUNT_MODE:
+ switch (comp_node->component.scope) {
+ case COUNTER_SCOPE_DEVICE:
+ ret = comp->device_u32_read(counter, &value_u32);
+ break;
+ case COUNTER_SCOPE_SIGNAL:
+ ret = comp->signal_u32_read(counter, parent,
+ &value_u32);
+ break;
+ case COUNTER_SCOPE_COUNT:
+ ret = comp->count_u32_read(counter, parent, &value_u32);
+ break;
+ }
+ *value = value_u32;
+ return ret;
+ case COUNTER_COMP_U64:
+ switch (comp_node->component.scope) {
+ case COUNTER_SCOPE_DEVICE:
+ return comp->device_u64_read(counter, value);
+ case COUNTER_SCOPE_SIGNAL:
+ return comp->signal_u64_read(counter, parent, value);
+ case COUNTER_SCOPE_COUNT:
+ return comp->count_u64_read(counter, parent, value);
+ default:
+ return -EINVAL;
+ }
+ case COUNTER_COMP_SYNAPSE_ACTION:
+ ret = comp->action_read(counter, parent, comp->priv,
+ &value_u32);
+ *value = value_u32;
+ return ret;
+ default:
+ return -EINVAL;
+ }
+}
+
+/**
+ * counter_push_event - queue event for userspace reading
+ * @counter: pointer to Counter structure
+ * @event: triggered event
+ * @channel: event channel
+ *
+ * Note: If no one is watching for the respective event, it is silently
+ * discarded.
+ */
+void counter_push_event(struct counter_device *const counter, const u8 event,
+ const u8 channel)
+{
+ struct counter_event ev;
+ unsigned int copied = 0;
+ unsigned long flags;
+ struct counter_event_node *event_node;
+ struct counter_comp_node *comp_node;
+
+ ev.timestamp = ktime_get_ns();
+ ev.watch.event = event;
+ ev.watch.channel = channel;
+
+ /* Could be in an interrupt context, so use a spin lock */
+ spin_lock_irqsave(&counter->events_list_lock, flags);
+
+ /* Search for event in the list */
Worth keeping in mind that searching a list is not exactly a scalable
solution even if it's fine for now.
quoted
+ list_for_each_entry(event_node, &counter->events_list, l)
+ if (event_node->event == event &&
+ event_node->channel == channel)
+ break;
+
+ /* If event is not in the list */
+ if (&event_node->l == &counter->events_list)
+ goto exit_early;
+
+ /* Read and queue relevant comp for userspace */
+ list_for_each_entry(comp_node, &event_node->comp_list, l) {
+ ev.watch.component = comp_node->component;
+ ev.status = -counter_get_data(counter, comp_node, &ev.value);
+
+ copied += kfifo_in(&counter->events, &ev, 1);
I had a short debate with myself on whether this or kfifo_put() was more appropriate
(as fixed record length). I think this particular case they end up the same anyway
so it doesn't matter (nothing to do here!)
@@ -3,14 +3,22 @@*GenericCounterinterface*Copyright(C)2020WilliamBreathittGray*/+#include<linux/cdev.h>#include<linux/counter.h>#include<linux/device.h>+#include<linux/device/bus.h>#include<linux/export.h>+#include<linux/fs.h>#include<linux/gfp.h>#include<linux/idr.h>#include<linux/init.h>+#include<linux/kdev_t.h>#include<linux/module.h>+#include<linux/mutex.h>+#include<linux/types.h>+#include<linux/wait.h>+#include"counter-chrdev.h"#include"counter-sysfs.h"/* Provides a unique ID for each counter device */
@@ -53,10 +66,13 @@ int counter_register(struct counter_device *const counter)if(id<0)returnid;+mutex_init(&counter->ops_exist_lock);+/* Configure device structure for Counter */dev->id=id;dev->type=&counter_device_type;dev->bus=&counter_bus_type;+dev->devt=MKDEV(MAJOR(counter_devt),id);if(counter->parent){dev->parent=counter->parent;dev->of_node=counter->parent->of_node;
@@ -64,18 +80,22 @@ int counter_register(struct counter_device *const counter)device_initialize(dev);dev_set_drvdata(dev,counter);-/* Add Counter sysfs attributes */-err=counter_sysfs_add(counter);+err=counter_chrdev_add(counter);if(err<0)gotoerr_free_id;-/* Add device to system */-err=device_add(dev);+err=counter_sysfs_add(counter);if(err<0)-gotoerr_free_id;+gotoerr_remove_chrdev;++err=cdev_device_add(&counter->chrdev,dev);+if(err<0)+gotoerr_remove_chrdev;return0;+err_remove_chrdev:+counter_chrdev_remove(counter);
Totally trivial, but the sysfs cleanup is managed by devm_ whilst this counter_chrdev_remove()
is manual. That leaves us in a case where the unwind order is subtly different from the
reverse of the setup order. Is there any reason to not do the sysfs part of register first
and hence have the ordering match?
I think in an earlier revision of this patchset counter-chrdev had a
dependency on counter-sysfs, but that's no longer the case so we should
be able to reorder these calls so that the unwind order matches.
@@ -127,13 +156,30 @@ int devm_counter_register(struct device *dev, } EXPORT_SYMBOL_GPL(devm_counter_register);+#define COUNTER_DEV_MAX 256+ static int __init counter_init(void) {- return bus_register(&counter_bus_type);+ int err;++ err = bus_register(&counter_bus_type);
Hmm. This isn't ideal in IIO either, but logically bus_register() is the part that
exposes the infrastructure for other drivers to attach to so should be last thing
in init. However, I'm fairly sure the dependency handing will ensure this init()
is finished before it tries to register any counter drivers. Hence we are fine
anyway.
So in conclusion - nothing needs changing here...
Ack. Given that things work all right rigth now, I'll revisit this again
in the future as a general cleanup improvment after this patchset is
merged so that we don't risk complicating the Counter character device
functionality introduction any further.
From: Jonathan Cameron <jic23@kernel.org> Date: 2021-09-26 15:12:58
On Mon, 20 Sep 2021 19:09:13 +0900
William Breathitt Gray [off-list ref] wrote:
On Sun, Sep 12, 2021 at 05:18:42PM +0100, Jonathan Cameron wrote:
quoted
On Fri, 27 Aug 2021 12:47:51 +0900
William Breathitt Gray [off-list ref] wrote:
quoted
This patch introduces a character device interface for the Counter
subsystem. Device data is exposed through standard character device read
operations. Device data is gathered when a Counter event is pushed by
the respective Counter device driver. Configuration is handled via ioctl
operations on the respective Counter character device node.
Cc: David Lechner <david@lechnology.com>
Cc: Gwendal Grignou <redacted>
Cc: Dan Carpenter <redacted>
Cc: Oleksij Rempel <o.rempel@pengutronix.de>
Signed-off-by: William Breathitt Gray <redacted>
Hi William,
Why the bit based lock? It feels like a mutex_trylock() type approach or
spinlock_trylock() would be a more common solution to this problem.
There is precedence for doing what you have here though so I'm not that
worried about it.
Ok. I'm not sure bit lock was quite what was intended (as there is only one of them)
but I suppose it doesn't greatly matter.
quoted
There are a few more things inline.
I've now been through this patch with as fine toothed comb as I'm likely to
do so. Hence I won't do another review unless there are substantial changes.
I nearly applied it as it stands, but given we aren't in a rush (merge window
open), it's worth just a little more time to tidy up loose ends.
Jonathan
Responses follow below.
...
...
quoted
quoted
+static int counter_chrdev_release(struct inode *inode, struct file *filp)
+{
+ struct counter_device *const counter = filp->private_data;
+ int ret = 0;
+
+ mutex_lock(&counter->ops_exist_lock);
+
+ if (!counter->ops) {
This needs a comment to explain how you would get here and
why these two lists need cleaning up here if we do.
Superficially it feels to me like you could just add a counter->ops check in
counter_disable_events() and then call that directly. I'm guessing
I am missing a deadlock or similar however.
I don't believe there is a risk of a deadlock, I just felt this
conditional check isn't really related to the counter_disable_events()
path so I kept it seperate; the events lists are freed in
counter_disable_events() but that's incidental rather than the purpose
of the function. My reasoning for the separation is that there are two
scenarios where counter_chrdev_release is called: the first is when a
user closes the chrdev, while the second is when the Counter driver is
removed.
For the first scenario, the counter_disable_events() function is called
to stop events from firing off when noone has the chrdev open. In the
second scenario, we are not interested in disabling events, we know
we're no longer going to be interacting with this device again so we
want to free any held memory.
I want to keep the intention of these code paths clear because the
distinction is important if we start managing additional memory in the
future (i.e. memory unrelated to the events list that would not be freed
in counter_disable_events()), so I'll add a comment explaining what's
happening in this path. Alternatively, I could define a
counter_chrdev_free() function and toss those list free calls into
there, but perhaps that would be overkill right now for just two calls.
To think about: This isn't doing the add. So would counter_chrdev_init() be more
appropriate? The fact it can fail due to the kfifo_alloc makes that
naming less than ideal though.
I've been using the "add" here to refer to adding the chrdev to the
counter_device structure rather than to the rest of the system. I used a
similar naming convention for the counter-sysfs.c file so I think
"counter_chrdev_add" here should be all right and is consistent with the
existing "counter_sysfs_add" function.
Hmm. I'll go with maybe and reserve the right to say I told you so if
it ever causes problem (and I can remember this discussion which is
fairly unlikely!).
...
...
quoted
quoted
/**
@@ -260,6 +289,16 @@ struct counter_ops { * @num_ext: number of Counter device extensions specified in @ext * @priv: optional private data supplied by driver * @dev: internal device structure+ * @chrdev: internal character device structure+ * @events_list: list of current watching Counter events+ * @events_list_lock: lock to protect Counter events list operations+ * @next_events_list: list of next watching Counter events+ * @n_events_list_lock: lock to protect Counter next events list operations+ * @events: queue of detected Counter events+ * @events_wait: wait queue to allow blocking reads of Counter events+ * @events_lock: lock to protect Counter events queue read operations+ * @chrdev_lock: lock to limit chrdev to a single open at a time+ * @ops_exist_lock: lock to prevent use during removal */ struct counter_device { const char *name;
From: William Breathitt Gray <hidden> Date: 2021-09-27 10:21:31
On Sun, Sep 26, 2021 at 04:15:42PM +0100, Jonathan Cameron wrote:
On Mon, 20 Sep 2021 19:09:13 +0900
William Breathitt Gray [off-list ref] wrote:
quoted
On Sun, Sep 12, 2021 at 05:18:42PM +0100, Jonathan Cameron wrote:
quoted
On Fri, 27 Aug 2021 12:47:51 +0900
William Breathitt Gray [off-list ref] wrote:
quoted
This patch introduces a character device interface for the Counter
subsystem. Device data is exposed through standard character device read
operations. Device data is gathered when a Counter event is pushed by
the respective Counter device driver. Configuration is handled via ioctl
operations on the respective Counter character device node.
Cc: David Lechner <david@lechnology.com>
Cc: Gwendal Grignou <redacted>
Cc: Dan Carpenter <redacted>
Cc: Oleksij Rempel <o.rempel@pengutronix.de>
Signed-off-by: William Breathitt Gray <redacted>
Hi William,
Why the bit based lock? It feels like a mutex_trylock() type approach or
spinlock_trylock() would be a more common solution to this problem.
There is precedence for doing what you have here though so I'm not that
worried about it.
Ok. I'm not sure bit lock was quite what was intended (as there is only one of them)
but I suppose it doesn't greatly matter.
It didn't cross my mind before, but would declaring chrdev_lock as an
atomic_t be a more appropriate solution here because we have only one
flag?
William Breathitt Gray
From: Jonathan Cameron <Jonathan.Cameron@Huawei.com> Date: 2021-09-27 11:20:21
On Mon, 27 Sep 2021 19:21:17 +0900
William Breathitt Gray [off-list ref] wrote:
On Sun, Sep 26, 2021 at 04:15:42PM +0100, Jonathan Cameron wrote:
quoted
On Mon, 20 Sep 2021 19:09:13 +0900
William Breathitt Gray [off-list ref] wrote:
quoted
On Sun, Sep 12, 2021 at 05:18:42PM +0100, Jonathan Cameron wrote:
quoted
On Fri, 27 Aug 2021 12:47:51 +0900
William Breathitt Gray [off-list ref] wrote:
quoted
This patch introduces a character device interface for the Counter
subsystem. Device data is exposed through standard character device read
operations. Device data is gathered when a Counter event is pushed by
the respective Counter device driver. Configuration is handled via ioctl
operations on the respective Counter character device node.
Cc: David Lechner <david@lechnology.com>
Cc: Gwendal Grignou <redacted>
Cc: Dan Carpenter <redacted>
Cc: Oleksij Rempel <o.rempel@pengutronix.de>
Signed-off-by: William Breathitt Gray <redacted>
Hi William,
Why the bit based lock? It feels like a mutex_trylock() type approach or
spinlock_trylock() would be a more common solution to this problem.
There is precedence for doing what you have here though so I'm not that
worried about it.
Ok. I'm not sure bit lock was quite what was intended (as there is only one of them)
but I suppose it doesn't greatly matter.
It didn't cross my mind before, but would declaring chrdev_lock as an
atomic_t be a more appropriate solution here because we have only one
flag?
William Breathitt Gray
It would be less esoteric. This was the first time I've ever come across the bitlock stuff
whereas atomics are an every day thing.
Thanks,
Jonathan
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: William Breathitt Gray <hidden> Date: 2021-09-27 11:33:18
On Mon, Sep 27, 2021 at 12:20:00PM +0100, Jonathan Cameron wrote:
On Mon, 27 Sep 2021 19:21:17 +0900
William Breathitt Gray [off-list ref] wrote:
quoted
On Sun, Sep 26, 2021 at 04:15:42PM +0100, Jonathan Cameron wrote:
quoted
On Mon, 20 Sep 2021 19:09:13 +0900
William Breathitt Gray [off-list ref] wrote:
quoted
On Sun, Sep 12, 2021 at 05:18:42PM +0100, Jonathan Cameron wrote:
quoted
On Fri, 27 Aug 2021 12:47:51 +0900
William Breathitt Gray [off-list ref] wrote:
quoted
This patch introduces a character device interface for the Counter
subsystem. Device data is exposed through standard character device read
operations. Device data is gathered when a Counter event is pushed by
the respective Counter device driver. Configuration is handled via ioctl
operations on the respective Counter character device node.
Cc: David Lechner <david@lechnology.com>
Cc: Gwendal Grignou <redacted>
Cc: Dan Carpenter <redacted>
Cc: Oleksij Rempel <o.rempel@pengutronix.de>
Signed-off-by: William Breathitt Gray <redacted>
Hi William,
Why the bit based lock? It feels like a mutex_trylock() type approach or
spinlock_trylock() would be a more common solution to this problem.
There is precedence for doing what you have here though so I'm not that
worried about it.
Ok. I'm not sure bit lock was quite what was intended (as there is only one of them)
but I suppose it doesn't greatly matter.
It didn't cross my mind before, but would declaring chrdev_lock as an
atomic_t be a more appropriate solution here because we have only one
flag?
William Breathitt Gray
It would be less esoteric. This was the first time I've ever come across the bitlock stuff
whereas atomics are an every day thing.
Thanks,
Jonathan
I agree. I'll try that out then and reimplement this using
atomic_inc_and_test() instead of test_and_set_bit_lock().
William Breathitt Gray