* [PATCH v6 0/5] Introduce the Counter character device interface
@ 2020-11-22 20:29 William Breathitt Gray
2020-11-22 20:29 ` [PATCH v6 2/5] docs: counter: Update to reflect sysfs internalization William Breathitt Gray
` (5 more replies)
0 siblings, 6 replies; 17+ messages in thread
From: William Breathitt Gray @ 2020-11-22 20:29 UTC (permalink / raw)
To: jic23
Cc: kernel, linux-stm32, a.fatoum, kamel.bouhara, gwendal,
alexandre.belloni, david, linux-iio, linux-kernel,
linux-arm-kernel, syednwaris, patrick.havelange, fabrice.gasnier,
mcoquelin.stm32, alexandre.torgue, William Breathitt Gray
Changes in v6:
- Consolidated the value member of struct counter_event down to a
single u64; u64 should be capable of representing all component
values
- Removed extension width sysfs attributes; no longer needed when value
is always u64
- Implemented COUNTER_COMPONENT_DUMMY to allow timestamp grabs without
component data reads
- Implemented events_config() callback; called during
COUNTER_CLEAR_WATCHES_IOCTL and COUNTER_LOAD_WATCHES_IOCTL in order
to allow devices a chance to adjust (enable/disable IRQ, etc.) for
the new events configuration requested by the user
- Simplified example code in Documentation by removing confusing use of
poll() call
- Removed redundant ida_simple_remove() from counter_register()
- Renamed devm_counter_unreg() to devm_counter_unregister()
- Renamed functions in counter-sysfs.c to be clearer
- Fixed miscellaneous typos throughout files
- Added more kernel doc comments; I've left some defines without
comments if they seemed obvious -- but please let me know if further
documentation is needed
- Refactored quad8_irq_handler() to use WARN_ONCE() instead of
returning on error; this should prevent interrupts from entering an
endless loop
- General refactoring and additional comments for clarity
- Returns EOPNOTSUPP instead of EFAULT now if a Counter watch is added
for unsupported component
- Renamed COUNTER_SET_WATCH_IOCTL TO COUNTER_ADD_WATCH_IOCTL to make
the use clear
- Reimplemented the parent and id members of struct counter_component
as __u8 instead of __u64; it's unlikely we'll ever have a device that
supports more than 255 components
- Reimplement __u64 variables in include/uapi/linux/counter.h as
__aligned_u64 to prevent 32-bit vs 64-bit alignment issues
- Fixed return value bug in counter_comp_u8_store(); enums set to a
value with index > 0 should now work correctly
- Fixed spectre issues in counter-chrdev.c
- Removed redundant get_device() call from counter_register()
- Moved put_device() to after the events_list is freed lest we leak
memory
I'm skipping the introduction blurb because it was just a rehashing of
information included in the documentation patches within this patchset.
Instead I will focus this cover letter on discussions about this
patchset and the userspace interface implications.
1. Should standard Counter component data types be defined as u8 or u32?
Many standard Counter component types such COUNTER_COMP_SIGNAL_LEVEL
have standard values defined (e.g. COUNTER_SIGNAL_LEVEL_LOW and
COUNTER_SIGNAL_LEVEL_HIGH). These values are currently handled by the
Counter subsystem code as u8 data types.
If u32 is used for these values instead, C enum structures could be
used by driver authors to implicitly cast these values via the driver
callback parameters.
This question is primarily addressed to David Lechner. I'm somewhat
confused about how this setup would look in device drivers. I've gone
ahead and refactored the code to support u32 enums, and pushed it to
a separate branch on my repository called counter_chrdev_v6_u32_enum:
https://gitlab.com/vilhelmgray/iio/-/tree/counter_chrdev_v6_u32_enum
Please check it out and let me know what you think. Is this the
support you had in mind? I'm curious to see an example of how would
your driver callback functions would look in this case. Is everything
works out fine, then I'll submit this branch as v7 of this patchset.
2. How should we handle "raw" timestamps?
Ahmad Fatoum brought up the possibility of returning "raw" timestamps
similar to what the network stack offers (see the network stack
SOF_TIMESTAMPING_{RAW,SYS}_HARDWARE support).
I'm not very familiar with the networking stack code, but if I
understand correctly the SOF_TIMESTAMPING_RAW_HARDWARE timestamps are
values returned from the device. If so, I suspect we would be able to
support these "raw" timestamps by defining them as Counter Extensions
and returning them in struct counter_event elements similar to the
other Extension values.
William Breathitt Gray (5):
counter: Internalize sysfs interface code
docs: counter: Update to reflect sysfs internalization
counter: Add character device interface
docs: counter: Document character device interface
counter: 104-quad-8: Add IRQ support for the ACCES 104-QUAD-8
Documentation/ABI/testing/sysfs-bus-counter | 18 +-
.../ABI/testing/sysfs-bus-counter-104-quad-8 | 32 +
Documentation/driver-api/generic-counter.rst | 411 ++++-
.../userspace-api/ioctl/ioctl-number.rst | 1 +
MAINTAINERS | 2 +-
drivers/counter/104-quad-8.c | 778 +++++----
drivers/counter/Kconfig | 6 +-
drivers/counter/Makefile | 1 +
drivers/counter/counter-chrdev.c | 476 ++++++
drivers/counter/counter-chrdev.h | 16 +
drivers/counter/counter-core.c | 183 ++
drivers/counter/counter-sysfs.c | 806 +++++++++
drivers/counter/counter-sysfs.h | 13 +
drivers/counter/counter.c | 1496 -----------------
drivers/counter/ftm-quaddec.c | 60 +-
drivers/counter/microchip-tcb-capture.c | 114 +-
drivers/counter/stm32-lptimer-cnt.c | 175 +-
drivers/counter/stm32-timer-cnt.c | 145 +-
drivers/counter/ti-eqep.c | 224 +--
include/linux/counter.h | 676 ++++----
include/linux/counter_enum.h | 45 -
include/uapi/linux/counter.h | 105 ++
22 files changed, 3094 insertions(+), 2689 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
--
2.29.2
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v6 2/5] docs: counter: Update to reflect sysfs internalization
2020-11-22 20:29 [PATCH v6 0/5] Introduce the Counter character device interface William Breathitt Gray
@ 2020-11-22 20:29 ` William Breathitt Gray
2020-11-22 20:29 ` [PATCH v6 3/5] counter: Add character device interface William Breathitt Gray
` (4 subsequent siblings)
5 siblings, 0 replies; 17+ messages in thread
From: William Breathitt Gray @ 2020-11-22 20:29 UTC (permalink / raw)
To: jic23
Cc: kernel, linux-stm32, a.fatoum, kamel.bouhara, gwendal,
alexandre.belloni, david, linux-iio, linux-kernel,
linux-arm-kernel, syednwaris, patrick.havelange, fabrice.gasnier,
mcoquelin.stm32, alexandre.torgue, William Breathitt Gray
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.
Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
---
Documentation/ABI/testing/sysfs-bus-counter | 9 +-
Documentation/driver-api/generic-counter.rst | 242 ++++++++++++++-----
2 files changed, 184 insertions(+), 67 deletions(-)
diff --git a/Documentation/ABI/testing/sysfs-bus-counter b/Documentation/ABI/testing/sysfs-bus-counter
index 566bd99fe0a5..1820ce2f9183 100644
--- a/Documentation/ABI/testing/sysfs-bus-counter
+++ b/Documentation/ABI/testing/sysfs-bus-counter
@@ -219,7 +219,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/name
KernelVersion: 5.2
diff --git a/Documentation/driver-api/generic-counter.rst b/Documentation/driver-api/generic-counter.rst
index b02c52cd69d6..b842ddbbd8a0 100644
--- a/Documentation/driver-api/generic-counter.rst
+++ b/Documentation/driver-api/generic-counter.rst
@@ -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,59 @@ 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, u8 *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 +347,128 @@ 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 gurantees a standard userspace interface for all counter
+drivers, and helps generalize the Generic Counter driver ABI in order to
+support the Generic Counter chrdev interface without significant changes
+to the existing counter drivers.
+
+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.29.2
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v6 3/5] counter: Add character device interface
2020-11-22 20:29 [PATCH v6 0/5] Introduce the Counter character device interface William Breathitt Gray
2020-11-22 20:29 ` [PATCH v6 2/5] docs: counter: Update to reflect sysfs internalization William Breathitt Gray
@ 2020-11-22 20:29 ` William Breathitt Gray
2020-12-13 23:58 ` David Lechner
2021-01-19 9:20 ` Oleksij Rempel
2020-11-22 20:29 ` [PATCH v6 4/5] docs: counter: Document " William Breathitt Gray
` (3 subsequent siblings)
5 siblings, 2 replies; 17+ messages in thread
From: William Breathitt Gray @ 2020-11-22 20:29 UTC (permalink / raw)
To: jic23
Cc: kernel, linux-stm32, a.fatoum, kamel.bouhara, gwendal,
alexandre.belloni, david, linux-iio, linux-kernel,
linux-arm-kernel, syednwaris, patrick.havelange, fabrice.gasnier,
mcoquelin.stm32, alexandre.torgue, William Breathitt Gray,
Dan Carpenter
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 <gwendal@chromium.org>
Cc: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
---
MAINTAINERS | 1 +
drivers/counter/Makefile | 2 +-
drivers/counter/counter-chrdev.c | 476 +++++++++++++++++++++++++++++++
drivers/counter/counter-chrdev.h | 16 ++
drivers/counter/counter-core.c | 38 ++-
drivers/counter/counter-sysfs.c | 51 +++-
include/linux/counter.h | 66 +++--
include/uapi/linux/counter.h | 105 +++++++
8 files changed, 711 insertions(+), 44 deletions(-)
create mode 100644 drivers/counter/counter-chrdev.c
create mode 100644 drivers/counter/counter-chrdev.h
create mode 100644 include/uapi/linux/counter.h
diff --git a/MAINTAINERS b/MAINTAINERS
index 5e0d8ca70912..beda3366b64c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4453,6 +4453,7 @@ F: Documentation/ABI/testing/sysfs-bus-counter*
F: Documentation/driver-api/generic-counter.rst
F: drivers/counter/
F: include/linux/counter.h
+F: include/uapi/linux/counter.h
CPMAC ETHERNET DRIVER
M: Florian Fainelli <f.fainelli@gmail.com>
diff --git a/drivers/counter/Makefile b/drivers/counter/Makefile
index cbe1d06af6a9..c4870eb5b1dd 100644
--- a/drivers/counter/Makefile
+++ b/drivers/counter/Makefile
@@ -4,7 +4,7 @@
#
obj-$(CONFIG_COUNTER) += counter.o
-counter-y := counter-core.o counter-sysfs.o
+counter-y := counter-core.o counter-sysfs.o counter-chrdev.o
obj-$(CONFIG_104_QUAD_8) += 104-quad-8.o
obj-$(CONFIG_STM32_TIMER_CNT) += stm32-timer-cnt.o
diff --git a/drivers/counter/counter-chrdev.c b/drivers/counter/counter-chrdev.c
new file mode 100644
index 000000000000..96fa7fbeef92
--- /dev/null
+++ b/drivers/counter/counter-chrdev.c
@@ -0,0 +1,476 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Generic Counter character device interface
+ * Copyright (C) 2020 William Breathitt Gray
+ */
+
+#include <linux/cdev.h>
+#include <linux/counter.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/errno.h>
+#include <linux/export.h>
+#include <linux/fs.h>
+#include <linux/list.h>
+#include <linux/nospec.h>
+#include <linux/poll.h>
+#include <linux/kdev_t.h>
+#include <linux/kfifo.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+#include <linux/timekeeping.h>
+#include <linux/types.h>
+#include <linux/wait.h>
+#include <linux/uaccess.h>
+
+#include "counter-chrdev.h"
+
+struct counter_comp_node {
+ struct list_head l;
+ struct counter_component component;
+ struct counter_comp comp;
+ void *parent;
+};
+
+static ssize_t counter_chrdev_read(struct file *filp, char __user *buf,
+ size_t len, loff_t *f_ps)
+{
+ struct counter_device *const counter = filp->private_data;
+ int err;
+ unsigned long flags;
+ unsigned int copied;
+
+ if (len < sizeof(struct counter_event))
+ return -EINVAL;
+
+ do {
+ if (kfifo_is_empty(&counter->events)) {
+ if (filp->f_flags & O_NONBLOCK)
+ return -EAGAIN;
+
+ err = wait_event_interruptible(counter->events_wait,
+ !kfifo_is_empty(&counter->events));
+ if (err)
+ return err;
+ }
+
+ raw_spin_lock_irqsave(&counter->events_lock, flags);
+ err = kfifo_to_user(&counter->events, buf, len, &copied);
+ raw_spin_unlock_irqrestore(&counter->events_lock, flags);
+ if (err)
+ return err;
+ } while (!copied);
+
+ return copied;
+}
+
+static __poll_t counter_chrdev_poll(struct file *filp,
+ struct poll_table_struct *pollt)
+{
+ struct counter_device *const counter = filp->private_data;
+ __poll_t events = 0;
+
+ poll_wait(filp, &counter->events_wait, pollt);
+
+ if (!kfifo_is_empty(&counter->events))
+ events = EPOLLIN | EPOLLRDNORM;
+
+ return events;
+}
+
+static void counter_events_list_free(struct list_head *const events_list)
+{
+ struct counter_event_node *p, *n;
+ struct counter_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);
+ }
+}
+
+static int counter_set_event_node(struct counter_device *const counter,
+ struct counter_watch *const watch,
+ const struct counter_comp_node *const cfg)
+{
+ struct counter_event_node *event_node;
+ struct counter_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_ATOMIC);
+ 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)
+ return -EINVAL;
+
+ /* Allocate component node */
+ comp_node = kmalloc(sizeof(*comp_node), GFP_ATOMIC);
+ if (!comp_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);
+ }
+ return -ENOMEM;
+ }
+ *comp_node = *cfg;
+
+ /* Add component node to event node */
+ list_add_tail(&comp_node->l, &event_node->comp_list);
+
+ return 0;
+}
+
+static int counter_add_watch(struct counter_device *const counter,
+ const unsigned long arg)
+{
+ void __user *const uwatch = (void __user *)arg;
+ struct counter_watch watch;
+ struct counter_comp_node comp_node = {0};
+ size_t parent, id;
+ struct counter_comp *ext;
+ size_t num_ext;
+
+ if (copy_from_user(&watch, uwatch, sizeof(watch)))
+ return -EFAULT;
+
+ /* Dummy components can skip evaluation */
+ if (watch.component.type == COUNTER_COMPONENT_DUMMY)
+ goto dummy_component;
+
+ parent = watch.component.parent;
+
+ /* Configure parent component info for comp node */
+ switch (watch.component.scope) {
+ case COUNTER_SCOPE_DEVICE:
+ ext = counter->ext;
+ num_ext = counter->num_ext;
+ break;
+ case COUNTER_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;
+ case COUNTER_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) {
+ case COUNTER_COMPONENT_SIGNAL:
+ if (watch.component.scope != COUNTER_SCOPE_SIGNAL)
+ return -EINVAL;
+
+ comp_node.comp.type = COUNTER_COMP_SIGNAL_LEVEL;
+ comp_node.comp.signal_u8_read = counter->ops->signal_read;
+ break;
+ case COUNTER_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;
+ case COUNTER_COMPONENT_FUNCTION:
+ if (watch.component.scope != COUNTER_SCOPE_COUNT)
+ return -EINVAL;
+
+ comp_node.comp.type = COUNTER_COMP_FUNCTION;
+ comp_node.comp.count_u8_read = counter->ops->function_read;
+ break;
+ case COUNTER_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;
+ case COUNTER_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;
+
+dummy_component:
+ comp_node.component = watch.component;
+
+ return counter_set_event_node(counter, &watch, &comp_node);
+}
+
+static long counter_chrdev_ioctl(struct file *filp, unsigned int cmd,
+ unsigned long arg)
+{
+ struct counter_device *const counter = filp->private_data;
+ raw_spinlock_t *const events_lock = &counter->events_lock;
+ unsigned long flags;
+ struct list_head *const events_list = &counter->events_list;
+ struct list_head *const next_events_list = &counter->next_events_list;
+ int err = 0;
+
+ switch (cmd) {
+ case COUNTER_CLEAR_WATCHES_IOCTL:
+ raw_spin_lock_irqsave(events_lock, flags);
+
+ counter_events_list_free(events_list);
+
+ if (counter->ops->events_config)
+ err = counter->ops->events_config(counter);
+
+ raw_spin_unlock_irqrestore(events_lock, flags);
+
+ counter_events_list_free(next_events_list);
+ break;
+ case COUNTER_ADD_WATCH_IOCTL:
+ return counter_add_watch(counter, arg);
+ case COUNTER_LOAD_WATCHES_IOCTL:
+ raw_spin_lock_irqsave(events_lock, flags);
+
+ counter_events_list_free(events_list);
+ list_replace_init(next_events_list, events_list);
+
+ if (counter->ops->events_config)
+ err = counter->ops->events_config(counter);
+
+ raw_spin_unlock_irqrestore(events_lock, flags);
+ break;
+ default:
+ return -ENOIOCTLCMD;
+ }
+
+ return err;
+}
+
+static int counter_chrdev_open(struct inode *inode, struct file *filp)
+{
+ struct counter_device *const counter = container_of(inode->i_cdev,
+ typeof(*counter),
+ chrdev);
+
+ get_device(&counter->dev);
+ filp->private_data = counter;
+
+ return nonseekable_open(inode, filp);
+}
+
+static int counter_chrdev_release(struct inode *inode, struct file *filp)
+{
+ struct counter_device *const counter = filp->private_data;
+ unsigned long flags;
+
+ raw_spin_lock_irqsave(&counter->events_lock, flags);
+ counter_events_list_free(&counter->events_list);
+ raw_spin_unlock_irqrestore(&counter->events_lock, flags);
+ counter_events_list_free(&counter->next_events_list);
+
+ put_device(&counter->dev);
+
+ return 0;
+}
+
+static const struct file_operations counter_fops = {
+ .llseek = no_llseek,
+ .read = counter_chrdev_read,
+ .poll = counter_chrdev_poll,
+ .unlocked_ioctl = counter_chrdev_ioctl,
+ .open = counter_chrdev_open,
+ .release = counter_chrdev_release,
+};
+
+int counter_chrdev_add(struct counter_device *const counter,
+ const dev_t counter_devt)
+{
+ struct device *const dev = &counter->dev;
+ struct cdev *const chrdev = &counter->chrdev;
+
+ /* Initialize Counter events lists */
+ INIT_LIST_HEAD(&counter->events_list);
+ INIT_LIST_HEAD(&counter->next_events_list);
+ raw_spin_lock_init(&counter->events_lock);
+
+ /* Initialize Counter events queue */
+ INIT_KFIFO(counter->events);
+ init_waitqueue_head(&counter->events_wait);
+
+ /* Initialize character device */
+ cdev_init(chrdev, &counter_fops);
+ dev->devt = MKDEV(MAJOR(counter_devt), counter->id);
+ cdev_set_parent(chrdev, &dev->kobj);
+
+ return cdev_add(chrdev, dev->devt, 1);
+}
+
+void counter_chrdev_remove(struct counter_device *const counter)
+{
+ cdev_del(&counter->chrdev);
+}
+
+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;
+ int err = 0;
+ u8 value_u8;
+
+ /* Dummy components don't have data so return early */
+ if (comp_node->component.type == COUNTER_COMPONENT_DUMMY)
+ return 0;
+
+ switch (comp->type) {
+ case COUNTER_COMP_U8:
+ case COUNTER_COMP_BOOL:
+ 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:
+ err = comp->device_u8_read(counter, &value_u8);
+ break;
+ case COUNTER_SCOPE_SIGNAL:
+ err = comp->signal_u8_read(counter, parent, &value_u8);
+ break;
+ case COUNTER_SCOPE_COUNT:
+ err = comp->count_u8_read(counter, parent, &value_u8);
+ break;
+ }
+ if (err)
+ return err;
+ *value = value_u8;
+ break;
+ 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);
+ }
+ break;
+ case COUNTER_COMP_SYNAPSE_ACTION:
+ err = comp->action_read(counter, parent, comp->priv, &value_u8);
+ if (err)
+ return err;
+ *value = value_u8;
+ break;
+ }
+
+ return 0;
+}
+
+/**
+ * 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.
+ *
+ * RETURNS:
+ * 0 on success, negative error number on failure.
+ */
+int counter_push_event(struct counter_device *const counter, const u8 event,
+ const u8 channel)
+{
+ struct counter_event ev = {0};
+ unsigned int copied = 0;
+ unsigned long flags;
+ struct counter_event_node *event_node;
+ struct counter_comp_node *comp_node;
+ int err = 0;
+
+ ev.timestamp = ktime_get_ns();
+ ev.watch.event = event;
+ ev.watch.channel = channel;
+
+ raw_spin_lock_irqsave(&counter->events_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)
+ goto exit_early;
+
+ /* Read and queue relevant comp for userspace */
+ list_for_each_entry(comp_node, &event_node->comp_list, l) {
+ err = counter_get_data(counter, comp_node, &ev.value);
+ if (err)
+ goto exit_early;
+
+ ev.watch.component = comp_node->component;
+
+ copied += kfifo_put(&counter->events, ev);
+ }
+
+ if (copied)
+ wake_up_poll(&counter->events_wait, EPOLLIN);
+
+exit_early:
+ raw_spin_unlock_irqrestore(&counter->events_lock, flags);
+
+ return err;
+}
+EXPORT_SYMBOL_GPL(counter_push_event);
diff --git a/drivers/counter/counter-chrdev.h b/drivers/counter/counter-chrdev.h
new file mode 100644
index 000000000000..cf5a318fe540
--- /dev/null
+++ b/drivers/counter/counter-chrdev.h
@@ -0,0 +1,16 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Counter character device interface
+ * Copyright (C) 2020 William Breathitt Gray
+ */
+#ifndef _COUNTER_CHRDEV_H_
+#define _COUNTER_CHRDEV_H_
+
+#include <linux/counter.h>
+#include <linux/types.h>
+
+int counter_chrdev_add(struct counter_device *const counter,
+ const dev_t counter_devt);
+void counter_chrdev_remove(struct counter_device *const counter);
+
+#endif /* _COUNTER_CHRDEV_H_ */
diff --git a/drivers/counter/counter-core.c b/drivers/counter/counter-core.c
index 251627b95a3a..088886e93898 100644
--- a/drivers/counter/counter-core.c
+++ b/drivers/counter/counter-core.c
@@ -5,12 +5,16 @@
*/
#include <linux/counter.h>
#include <linux/device.h>
+#include <linux/device/bus.h>
#include <linux/export.h>
+#include <linux/fs.h>
#include <linux/gfp.h>
#include <linux/idr.h>
#include <linux/init.h>
#include <linux/module.h>
+#include <linux/types.h>
+#include "counter-chrdev.h"
#include "counter-sysfs.h"
/* Provides a unique ID for each counter device */
@@ -33,6 +37,8 @@ static struct bus_type counter_bus_type = {
.name = "counter"
};
+static dev_t counter_devt;
+
/**
* counter_register - register Counter to the system
* @counter: pointer to Counter to register
@@ -51,7 +57,6 @@ int counter_register(struct counter_device *const counter)
if (counter->id < 0)
return counter->id;
- /* Configure device structure for Counter */
dev->type = &counter_device_type;
dev->bus = &counter_bus_type;
if (counter->parent) {
@@ -62,18 +67,25 @@ int counter_register(struct counter_device *const counter)
device_initialize(dev);
dev_set_drvdata(dev, counter);
+ /* Add Counter character device */
+ err = counter_chrdev_add(counter, counter_devt);
+ if (err)
+ goto err_free_id;
+
/* Add Counter sysfs attributes */
err = counter_sysfs_add(counter);
if (err)
- goto err_free_id;
+ goto err_remove_chrdev;
/* Add device to system */
err = device_add(dev);
if (err)
- goto err_free_id;
+ goto err_remove_chrdev;
return 0;
+err_remove_chrdev:
+ counter_chrdev_remove(counter);
err_free_id:
put_device(dev);
return err;
@@ -92,6 +104,7 @@ void counter_unregister(struct counter_device *const counter)
return;
device_unregister(&counter->dev);
+ counter_chrdev_remove(counter);
}
EXPORT_SYMBOL_GPL(counter_unregister);
@@ -135,13 +148,30 @@ int devm_counter_register(struct device *dev,
}
EXPORT_SYMBOL_GPL(devm_counter_register);
+#define COUNTER_DEV_MAX 256
+
static int __init counter_init(void)
{
- return bus_register(&counter_bus_type);
+ int err;
+
+ err = bus_register(&counter_bus_type);
+ if (err < 0)
+ return err;
+
+ err = alloc_chrdev_region(&counter_devt, 0, COUNTER_DEV_MAX, "counter");
+ if (err < 0)
+ goto err_unregister_bus;
+
+ return 0;
+
+err_unregister_bus:
+ bus_unregister(&counter_bus_type);
+ return err;
}
static void __exit counter_exit(void)
{
+ unregister_chrdev_region(counter_devt, COUNTER_DEV_MAX);
bus_unregister(&counter_bus_type);
}
diff --git a/drivers/counter/counter-sysfs.c b/drivers/counter/counter-sysfs.c
index c767754d0310..7b677c7de251 100644
--- a/drivers/counter/counter-sysfs.c
+++ b/drivers/counter/counter-sysfs.c
@@ -437,6 +437,7 @@ static ssize_t counter_comp_name_show(struct device *dev,
static int counter_name_attr_create(struct device *const dev,
struct counter_attribute_group *const group,
+ const char *const attr_name,
const char *const name)
{
struct counter_attribute *counter_attr;
@@ -451,7 +452,7 @@ static int counter_name_attr_create(struct device *const dev,
/* Configure device attribute */
sysfs_attr_init(&counter_attr->dev_attr.attr);
- counter_attr->dev_attr.attr.name = "name";
+ counter_attr->dev_attr.attr.name = attr_name;
counter_attr->dev_attr.attr.mode = 0444;
counter_attr->dev_attr.show = counter_comp_name_show;
@@ -462,6 +463,18 @@ static int counter_name_attr_create(struct device *const dev,
return 0;
}
+static int counter_ext_name_attr_create(struct device *const dev,
+ struct counter_attribute_group *const group, const size_t i,
+ const char *const name)
+{
+ const char *attr_name;
+
+ attr_name = devm_kasprintf(dev, GFP_KERNEL, "extension%zu_name", i);
+ if (!attr_name)
+ return -ENOMEM;
+
+ return counter_name_attr_create(dev, group, attr_name, name);
+}
static struct counter_comp counter_signal_comp = {
.type = COUNTER_COMP_SIGNAL_LEVEL,
@@ -477,6 +490,7 @@ static int counter_signal_attrs_create(struct counter_device *const counter,
int err;
struct counter_comp comp;
size_t i;
+ struct counter_comp *ext;
/* Create main Signal attribute */
comp = counter_signal_comp;
@@ -486,14 +500,19 @@ static int counter_signal_attrs_create(struct counter_device *const counter,
return err;
/* Create Signal name attribute */
- err = counter_name_attr_create(dev, group, signal->name);
+ err = counter_name_attr_create(dev, group, "name", signal->name);
if (err)
return err;
/* Create an attribute for each extension */
for (i = 0; i < signal->num_ext; i++) {
- err = counter_attr_create(dev, group, signal->ext + i, scope,
- signal);
+ ext = signal->ext + i;
+
+ err = counter_attr_create(dev, group, ext, scope, signal);
+ if (err)
+ return err;
+
+ err = counter_ext_name_attr_create(dev, group, i, ext->name);
if (err)
return err;
}
@@ -578,6 +597,7 @@ static int counter_count_attrs_create(struct counter_device *const counter,
int err;
struct counter_comp comp;
size_t i;
+ struct counter_comp *ext;
/* Create main Count attribute */
comp = counter_count_comp;
@@ -588,7 +608,7 @@ static int counter_count_attrs_create(struct counter_device *const counter,
return err;
/* Create Count name attribute */
- err = counter_name_attr_create(dev, group, count->name);
+ err = counter_name_attr_create(dev, group, "name", count->name);
if (err)
return err;
@@ -602,8 +622,13 @@ 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, group, count->ext + i, scope,
- count);
+ ext = count->ext + i;
+
+ err = counter_attr_create(dev, group, ext, scope, count);
+ if (err)
+ return err;
+
+ err = counter_ext_name_attr_create(dev, group, i, ext->name);
if (err)
return err;
}
@@ -667,6 +692,7 @@ static int counter_sysfs_attr_add(struct counter_device *const counter,
struct device *const dev = &counter->dev;
int err;
size_t i;
+ struct counter_comp *ext;
/* Add Signals sysfs attributes */
err = counter_sysfs_signals_add(counter, group);
@@ -681,7 +707,7 @@ static int counter_sysfs_attr_add(struct counter_device *const counter,
group += counter->num_counts;
/* Create name attribute */
- err = counter_name_attr_create(dev, group, counter->name);
+ err = counter_name_attr_create(dev, group, "name", counter->name);
if (err)
return err;
@@ -699,8 +725,13 @@ 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, group, counter->ext + i, scope,
- NULL);
+ ext = counter->ext + i;
+
+ err = counter_attr_create(dev, group, ext, scope, NULL);
+ if (err)
+ return err;
+
+ err = counter_ext_name_attr_create(dev, group, i, ext->name);
if (err)
return err;
}
diff --git a/include/linux/counter.h b/include/linux/counter.h
index 3f3f8ba6c1b4..98cd7c035968 100644
--- a/include/linux/counter.h
+++ b/include/linux/counter.h
@@ -6,9 +6,14 @@
#ifndef _COUNTER_H_
#define _COUNTER_H_
+#include <linux/cdev.h>
#include <linux/device.h>
#include <linux/kernel.h>
+#include <linux/kfifo.h>
+#include <linux/spinlock_types.h>
#include <linux/types.h>
+#include <linux/wait.h>
+#include <uapi/linux/counter.h>
struct counter_device;
struct counter_count;
@@ -27,35 +32,6 @@ enum counter_comp_type {
COUNTER_COMP_COUNT_MODE,
};
-#define COUNTER_SCOPE_DEVICE 0
-#define COUNTER_SCOPE_SIGNAL 1
-#define COUNTER_SCOPE_COUNT 2
-
-#define COUNTER_COUNT_DIRECTION_FORWARD 0
-#define COUNTER_COUNT_DIRECTION_BACKWARD 1
-
-#define COUNTER_COUNT_MODE_NORMAL 0
-#define COUNTER_COUNT_MODE_RANGE_LIMIT 1
-#define COUNTER_COUNT_MODE_NON_RECYCLE 2
-#define COUNTER_COUNT_MODE_MODULO_N 3
-
-#define COUNTER_FUNCTION_INCREASE 0
-#define COUNTER_FUNCTION_DECREASE 1
-#define COUNTER_FUNCTION_PULSE_DIRECTION 2
-#define COUNTER_FUNCTION_QUADRATURE_X1_A 3
-#define COUNTER_FUNCTION_QUADRATURE_X1_B 4
-#define COUNTER_FUNCTION_QUADRATURE_X2_A 5
-#define COUNTER_FUNCTION_QUADRATURE_X2_B 6
-#define COUNTER_FUNCTION_QUADRATURE_X4 7
-
-#define COUNTER_SIGNAL_LEVEL_LOW 0
-#define COUNTER_SIGNAL_LEVEL_HIGH 1
-
-#define COUNTER_SYNAPSE_ACTION_NONE 0
-#define COUNTER_SYNAPSE_ACTION_RISING_EDGE 1
-#define COUNTER_SYNAPSE_ACTION_FALLING_EDGE 2
-#define COUNTER_SYNAPSE_ACTION_BOTH_EDGES 3
-
/**
* struct counter_comp - Counter component node
* @type: Counter component data type
@@ -195,6 +171,20 @@ struct counter_count {
size_t num_ext;
};
+/**
+ * struct counter_event_node - Counter Event node
+ * @l: list of current watching Counter events
+ * @event: event that triggers
+ * @channel: event channel
+ * @comp_list: list of components to watch when event triggers
+ */
+struct counter_event_node {
+ struct list_head l;
+ u8 event;
+ u8 channel;
+ struct list_head comp_list;
+};
+
/**
* struct counter_ops - Callbacks from driver
* @signal_read: read callback for Signals. The read value of the
@@ -217,6 +207,9 @@ struct counter_count {
* @action_write: write callback for Synapse action modes. The action mode
* to write for the respective Synapse is passed in via the
* action parameter.
+ * @events_config: write callback to configure events. The list of struct
+ * counter_event_node may be accessed via the events_list
+ * member of the counter parameter.
*/
struct counter_ops {
int (*signal_read)(struct counter_device *counter,
@@ -235,6 +228,7 @@ struct counter_ops {
int (*action_write)(struct counter_device *counter,
struct counter_count *count,
struct counter_synapse *synapse, u8 action);
+ int (*events_config)(struct counter_device *counter);
};
/**
@@ -251,6 +245,12 @@ struct counter_ops {
* @priv: optional private data supplied by driver
* @id: unique ID used to identify the Counter
* @dev: internal device structure
+ * @chrdev: internal character device structure
+ * @events_lock: synchronization lock for Counter events
+ * @events_list: list of current watching Counter events
+ * @next_events_list: list of next watching Counter events
+ * @events: queue of detected Counter events
+ * @events_wait: wait queue to allow blocking reads of Counter events
*/
struct counter_device {
const char *name;
@@ -270,12 +270,20 @@ struct counter_device {
int id;
struct device dev;
+ struct cdev chrdev;
+ raw_spinlock_t events_lock;
+ struct list_head events_list;
+ struct list_head next_events_list;
+ DECLARE_KFIFO(events, struct counter_event, 64);
+ wait_queue_head_t events_wait;
};
int counter_register(struct counter_device *const counter);
void counter_unregister(struct counter_device *const counter);
int devm_counter_register(struct device *dev,
struct counter_device *const counter);
+int counter_push_event(struct counter_device *const counter, const u8 event,
+ const u8 channel);
#define COUNTER_COMP_DEVICE_U8(_name, _read, _write) \
{ \
diff --git a/include/uapi/linux/counter.h b/include/uapi/linux/counter.h
new file mode 100644
index 000000000000..0a3a412c7061
--- /dev/null
+++ b/include/uapi/linux/counter.h
@@ -0,0 +1,105 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+/*
+ * Userspace ABI for Counter character devices
+ * Copyright (C) 2020 William Breathitt Gray
+ */
+#ifndef _UAPI_COUNTER_H_
+#define _UAPI_COUNTER_H_
+
+#include <linux/ioctl.h>
+#include <linux/types.h>
+
+/* Component type definitions */
+#define COUNTER_COMPONENT_DUMMY 0
+#define COUNTER_COMPONENT_SIGNAL 1
+#define COUNTER_COMPONENT_COUNT 2
+#define COUNTER_COMPONENT_FUNCTION 3
+#define COUNTER_COMPONENT_SYNAPSE_ACTION 4
+#define COUNTER_COMPONENT_EXTENSION 5
+
+/* Component scope definitions */
+#define COUNTER_SCOPE_DEVICE 0
+#define COUNTER_SCOPE_SIGNAL 1
+#define COUNTER_SCOPE_COUNT 2
+
+/**
+ * struct counter_component - Counter component identification
+ * @type: component type (Count, extension, etc.)
+ * @scope: component scope (Device, Count, or Signal)
+ * @parent: parent component identification number
+ * @id: component identification number
+ */
+struct counter_component {
+ __u8 type;
+ __u8 scope;
+ __u8 parent;
+ __u8 id;
+};
+
+/* Event type definitions */
+#define COUNTER_EVENT_OVERFLOW 0
+#define COUNTER_EVENT_UNDERFLOW 1
+#define COUNTER_EVENT_OVERFLOW_UNDERFLOW 2
+#define COUNTER_EVENT_THRESHOLD 3
+#define COUNTER_EVENT_INDEX 4
+
+/**
+ * struct counter_watch - Counter component watch configuration
+ * @component: component to watch when event triggers
+ * @event: event that triggers
+ * @channel: event channel
+ */
+struct counter_watch {
+ struct counter_component component;
+ __u8 event;
+ __u8 channel;
+};
+
+/* ioctl commands */
+#define COUNTER_CLEAR_WATCHES_IOCTL _IO(0x3E, 0x00)
+#define COUNTER_ADD_WATCH_IOCTL _IOW(0x3E, 0x01, struct counter_watch)
+#define COUNTER_LOAD_WATCHES_IOCTL _IO(0x3E, 0x02)
+
+/**
+ * struct counter_event - Counter event data
+ * @watch: component watch configuration
+ * @timestamp: best estimate of time of event occurrence, in nanoseconds
+ * @value: component value
+ */
+struct counter_event {
+ struct counter_watch watch;
+ __aligned_u64 timestamp;
+ __aligned_u64 value;
+};
+
+/* Count direction values */
+#define COUNTER_COUNT_DIRECTION_FORWARD 0
+#define COUNTER_COUNT_DIRECTION_BACKWARD 1
+
+/* Count mode values */
+#define COUNTER_COUNT_MODE_NORMAL 0
+#define COUNTER_COUNT_MODE_RANGE_LIMIT 1
+#define COUNTER_COUNT_MODE_NON_RECYCLE 2
+#define COUNTER_COUNT_MODE_MODULO_N 3
+
+/* Count function values */
+#define COUNTER_FUNCTION_INCREASE 0
+#define COUNTER_FUNCTION_DECREASE 1
+#define COUNTER_FUNCTION_PULSE_DIRECTION 2
+#define COUNTER_FUNCTION_QUADRATURE_X1_A 3
+#define COUNTER_FUNCTION_QUADRATURE_X1_B 4
+#define COUNTER_FUNCTION_QUADRATURE_X2_A 5
+#define COUNTER_FUNCTION_QUADRATURE_X2_B 6
+#define COUNTER_FUNCTION_QUADRATURE_X4 7
+
+/* Signal values */
+#define COUNTER_SIGNAL_LEVEL_LOW 0
+#define COUNTER_SIGNAL_LEVEL_HIGH 1
+
+/* Action mode values */
+#define COUNTER_SYNAPSE_ACTION_NONE 0
+#define COUNTER_SYNAPSE_ACTION_RISING_EDGE 1
+#define COUNTER_SYNAPSE_ACTION_FALLING_EDGE 2
+#define COUNTER_SYNAPSE_ACTION_BOTH_EDGES 3
+
+#endif /* _UAPI_COUNTER_H_ */
--
2.29.2
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v6 4/5] docs: counter: Document character device interface
2020-11-22 20:29 [PATCH v6 0/5] Introduce the Counter character device interface William Breathitt Gray
2020-11-22 20:29 ` [PATCH v6 2/5] docs: counter: Update to reflect sysfs internalization William Breathitt Gray
2020-11-22 20:29 ` [PATCH v6 3/5] counter: Add character device interface William Breathitt Gray
@ 2020-11-22 20:29 ` William Breathitt Gray
2020-11-22 20:29 ` [PATCH v6 5/5] counter: 104-quad-8: Add IRQ support for the ACCES 104-QUAD-8 William Breathitt Gray
` (2 subsequent siblings)
5 siblings, 0 replies; 17+ messages in thread
From: William Breathitt Gray @ 2020-11-22 20:29 UTC (permalink / raw)
To: jic23
Cc: kernel, linux-stm32, a.fatoum, kamel.bouhara, gwendal,
alexandre.belloni, david, linux-iio, linux-kernel,
linux-arm-kernel, syednwaris, patrick.havelange, fabrice.gasnier,
mcoquelin.stm32, alexandre.torgue, William Breathitt Gray
This patch adds high-level documentation about the Counter subsystem
character device interface.
Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
---
Documentation/ABI/testing/sysfs-bus-counter | 9 +
Documentation/driver-api/generic-counter.rst | 231 ++++++++++++++----
.../userspace-api/ioctl/ioctl-number.rst | 1 +
3 files changed, 200 insertions(+), 41 deletions(-)
diff --git a/Documentation/ABI/testing/sysfs-bus-counter b/Documentation/ABI/testing/sysfs-bus-counter
index 1820ce2f9183..c8abe7c03a82 100644
--- a/Documentation/ABI/testing/sysfs-bus-counter
+++ b/Documentation/ABI/testing/sysfs-bus-counter
@@ -99,6 +99,15 @@ Description:
Read-only attribute that indicates whether excessive noise is
present at the channel Y counter inputs.
+What: /sys/bus/counter/devices/counterX/countY/extensionZ_name
+What: /sys/bus/counter/devices/counterX/extensionZ_name
+What: /sys/bus/counter/devices/counterX/signalY/extensionZ_name
+KernelVersion: 5.11
+Contact: linux-iio@vger.kernel.org
+Description:
+ Read-only attribute that indicates the component name of
+ Extension Z.
+
What: /sys/bus/counter/devices/counterX/countY/function
KernelVersion: 5.2
Contact: linux-iio@vger.kernel.org
diff --git a/Documentation/driver-api/generic-counter.rst b/Documentation/driver-api/generic-counter.rst
index b842ddbbd8a0..7c96d80f05e2 100644
--- a/Documentation/driver-api/generic-counter.rst
+++ b/Documentation/driver-api/generic-counter.rst
@@ -223,19 +223,6 @@ whether an input line is differential or single-ended) and instead focus
on the core idea of what the data and process represent (e.g. position
as interpreted from quadrature encoding data).
-Userspace Interface
-===================
-
-Several sysfs attributes are generated by the Generic Counter interface,
-and reside under the /sys/bus/counter/devices/counterX directory, where
-counterX refers to the respective counter device. Please see
-Documentation/ABI/testing/sysfs-bus-counter for detailed
-information on each Generic Counter interface sysfs attribute.
-
-Through these sysfs attributes, programs and scripts may interact with
-the Generic Counter paradigm Counts, Signals, and Synapses of respective
-counter devices.
-
Driver API
==========
@@ -387,16 +374,16 @@ userspace interface components::
/ driver callbacks /
-------------------
|
- +---------------+
- |
- V
- +--------------------+
- | Counter sysfs |
- +--------------------+
- | Translates to the |
- | standard Counter |
- | sysfs output |
- +--------------------+
+ +---------------+---------------+
+ | |
+ V V
+ +--------------------+ +---------------------+
+ | Counter sysfs | | Counter chrdev |
+ +--------------------+ +---------------------+
+ | Translates to the | | Translates to the |
+ | standard Counter | | standard Counter |
+ | sysfs output | | character device |
+ +--------------------+ +---------------------+
Thereafter, data can be transferred directly between the Counter device
driver and Counter userspace interface::
@@ -427,23 +414,30 @@ driver and Counter userspace interface::
/ u64 /
----------
|
- +---------------+
- |
- V
- +--------------------+
- | Counter sysfs |
- +--------------------+
- | Translates to the |
- | standard Counter |
- | sysfs output |
- |--------------------|
- | Type: const char * |
- | Value: "42" |
- +--------------------+
- |
- ---------------
- / const char * /
- ---------------
+ +---------------+---------------+
+ | |
+ V V
+ +--------------------+ +---------------------+
+ | Counter sysfs | | Counter chrdev |
+ +--------------------+ +---------------------+
+ | Translates to the | | Translates to the |
+ | standard Counter | | standard Counter |
+ | sysfs output | | character device |
+ |--------------------| |---------------------|
+ | Type: const char * | | Type: u64 |
+ | Value: "42" | | Value: 42 |
+ +--------------------+ +---------------------+
+ | |
+ --------------- -----------------------
+ / const char * / / struct counter_event /
+ --------------- -----------------------
+ | |
+ | V
+ | +-----------+
+ | | read |
+ | +-----------+
+ | \ Count: 42 /
+ | -----------
|
V
+--------------------------------------------------+
@@ -452,7 +446,7 @@ driver and Counter userspace interface::
\ Count: "42" /
--------------------------------------------------
-There are three primary components involved:
+There are four primary components involved:
Counter device driver
---------------------
@@ -472,3 +466,158 @@ and vice versa.
Please refer to the `Documentation/ABI/testing/sysfs-bus-counter` file
for a detailed breakdown of the available Generic Counter interface
sysfs attributes.
+
+Counter chrdev
+--------------
+Translates counter data to the standard Counter character device; data
+is transferred via standard character device read calls, while Counter
+events are configured via ioctl calls.
+
+Sysfs Interface
+===============
+
+Several sysfs attributes are generated by the Generic Counter interface,
+and reside under the `/sys/bus/counter/devices/counterX` directory,
+where `X` is to the respective counter device id. Please see
+`Documentation/ABI/testing/sysfs-bus-counter` for detailed information
+on each Generic Counter interface sysfs attribute.
+
+Through these sysfs attributes, programs and scripts may interact with
+the Generic Counter paradigm Counts, Signals, and Synapses of respective
+counter devices.
+
+Counter Character Device
+========================
+
+Counter character device nodes are created under the `/dev` directory as
+`counterX`, where `X` is the respective counter device id. Defines for
+the standard Counter data types are exposed via the userspace
+`include/uapi/linux/counter.h` file.
+
+Counter events
+--------------
+Counter device drivers can support Counter events by utilizing the
+`counter_push_event` function::
+
+ int counter_push_event(struct counter_device *const counter, const u8 event,
+ const u8 channel);
+
+The event id is specified by the `event` parameter; the event channel id
+is specified by the `channel` parameter. When this function is called,
+the Counter data associated with the respective event is gathered, and a
+`struct counter_event` is generated for each datum and pushed to
+userspace.
+
+Counter events can be configured by users to report various Counter
+data of interest. This can be conceptualized as a list of Counter
+component read calls to perform. For example::
+
+ +~~~~~~~~~~~~~~~~~~~~~~~~+~~~~~~~~~~~~~~~~~~~~~~~~+
+ | COUNTER_EVENT_OVERFLOW | COUNTER_EVENT_INDEX |
+ +~~~~~~~~~~~~~~~~~~~~~~~~+~~~~~~~~~~~~~~~~~~~~~~~~+
+ | Channel 0 | Channel 0 |
+ +------------------------+------------------------+
+ | * Count 0 | * Signal 0 |
+ | * Count 1 | * Signal 0 Extension 0 |
+ | * Signal 3 | * Extension 4 |
+ | * Count 4 Extension 2 +------------------------+
+ | * Signal 5 Extension 0 | Channel 1 |
+ | +------------------------+
+ | | * Signal 4 |
+ | | * Signal 4 Extension 0 |
+ | | * Count 7 |
+ +------------------------+------------------------+
+
+When `counter_push_event(counter, COUNTER_EVENT_INDEX, 1)` is called for
+example, it will go down the list for the `COUNTER_EVENT_INDEX` event
+channel 1 and execute the read callbacks for Signal 4, Signal 4
+Extension 0, and Count 4 -- the data returned for each is pushed to a
+kfifo as a `struct counter_event`, which userspace can retrieve via a
+standard read operation on the respective character device node.
+
+Userspace
+---------
+Userspace applications can configure Counter events via ioctl operations
+on the Counter character device node. There following ioctl codes are
+supported and provided by the `linux/counter.h` userspace header file:
+
+* COUNTER_CLEAR_WATCHES_IOCTL:
+ Clear all Counter watches from all events
+
+* COUNTER_ADD_WATCH_IOCTL:
+ Add a Counter watch for the specified event
+
+* COUNTER_LOAD_WATCHES_IOCTL:
+ Activates the Counter watches added earlier
+
+To configure events to gather Counter data, users first populate a
+`struct counter_watch` with the relevant event id, event channel id, and
+the information for the desired Counter component from which to read,
+and then pass it via the `COUNTER_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_DUMMY`.
+This configures the component as a dummy: the Counter character device
+node 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_LOAD_WATCHES_IOCTL` ioctl command may be used
+to activate these Counter watches.
+
+Userspace applications can then execute a `read` operation (optionally
+calling `poll` first) on the Counter character device node to retrieve
+`struct counter_event` elements with the desired data.
+
+For example, the following userspace code opens `/dev/counter0`,
+configures the `COUNTER_EVENT_INDEX` event channel 0 to gather Count 0
+and Count 1, and prints out the data as it becomes available on the
+character device node::
+
+ #include <fcntl.h>
+ #include <linux/counter.h>
+ #include <stdio.h>
+ #include <sys/ioctl.h>
+ #include <unistd.h>
+
+ struct counter_watch watches[2] = {
+ {
+ .component.type = COUNTER_COMPONENT_COUNT,
+ .component.scope = COUNTER_SCOPE_COUNT,
+ .component.parent = 0,
+ .event = COUNTER_EVENT_INDEX,
+ .channel = 0,
+ },
+ {
+ .component.type = COUNTER_COMPONENT_COUNT,
+ .component.scope = COUNTER_SCOPE_COUNT,
+ .component.parent = 1,
+ .event = COUNTER_EVENT_INDEX,
+ .channel = 0,
+ },
+ };
+
+ int main(void)
+ {
+ int fd;
+ struct counter_event event_data[2];
+
+ fd = open("/dev/counter0", O_RDWR);
+
+ ioctl(fd, COUNTER_ADD_WATCH_IOCTL, watches);
+ ioctl(fd, COUNTER_ADD_WATCH_IOCTL, watches + 1);
+ ioctl(fd, COUNTER_LOAD_WATCHES_IOCTL);
+
+ for (;;) {
+ read(fd, event_data, sizeof(event_data));
+
+ printf("Count 0: %llu\nTimestamp 0: %llu\n"
+ "Count 1: %llu\nTimestamp 1: %llu\n",
+ (unsigned long long)event_data[0].value,
+ (unsigned long long)event_data[0].timestamp,
+ (unsigned long long)event_data[1].value,
+ (unsigned long long)event_data[1].timestamp);
+ }
+
+ return 0;
+ }
diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst b/Documentation/userspace-api/ioctl/ioctl-number.rst
index 2a198838fca9..f6e96bb780cd 100644
--- a/Documentation/userspace-api/ioctl/ioctl-number.rst
+++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
@@ -88,6 +88,7 @@ Code Seq# Include File Comments
<http://infiniband.sourceforge.net/>
0x20 all drivers/cdrom/cm206.h
0x22 all scsi/sg.h
+0x3E 00-0F linux/counter.h <mailto:linux-iio@vger.kernel.org>
'!' 00-1F uapi/linux/seccomp.h
'#' 00-3F IEEE 1394 Subsystem
Block for the entire subsystem
--
2.29.2
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v6 5/5] counter: 104-quad-8: Add IRQ support for the ACCES 104-QUAD-8
2020-11-22 20:29 [PATCH v6 0/5] Introduce the Counter character device interface William Breathitt Gray
` (2 preceding siblings ...)
2020-11-22 20:29 ` [PATCH v6 4/5] docs: counter: Document " William Breathitt Gray
@ 2020-11-22 20:29 ` William Breathitt Gray
[not found] ` <950660d49af7d12b09bc9d3b1db6f8ff74209c26.1606075915.git.vilhelm.gray@gmail.com>
2020-12-13 23:15 ` [PATCH v6 0/5] Introduce the Counter character device interface David Lechner
5 siblings, 0 replies; 17+ messages in thread
From: William Breathitt Gray @ 2020-11-22 20:29 UTC (permalink / raw)
To: jic23
Cc: kernel, linux-stm32, a.fatoum, kamel.bouhara, gwendal,
alexandre.belloni, david, linux-iio, linux-kernel,
linux-arm-kernel, syednwaris, patrick.havelange, fabrice.gasnier,
mcoquelin.stm32, alexandre.torgue, William Breathitt Gray
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.
The irq_trigger Count extension is introduced to allow the selection of
the desired IRQ trigger function per channel. The irq_trigger_enable
Count extension is introduced to allow the enablement of interrupts for
a respective channel. 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.
Reviewed-by: Syed Nayyar Waris <syednwaris@gmail.com>
Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
---
.../ABI/testing/sysfs-bus-counter-104-quad-8 | 32 ++
drivers/counter/104-quad-8.c | 306 ++++++++++++++----
drivers/counter/Kconfig | 6 +-
3 files changed, 273 insertions(+), 71 deletions(-)
diff --git a/Documentation/ABI/testing/sysfs-bus-counter-104-quad-8 b/Documentation/ABI/testing/sysfs-bus-counter-104-quad-8
index eac32180c40d..995bf08d365d 100644
--- a/Documentation/ABI/testing/sysfs-bus-counter-104-quad-8
+++ b/Documentation/ABI/testing/sysfs-bus-counter-104-quad-8
@@ -1,3 +1,35 @@
+What: /sys/bus/counter/devices/counterX/countY/irq_trigger
+KernelVersion: 5.11
+Contact: linux-iio@vger.kernel.org
+Description:
+ IRQ trigger function for channel Y. Four trigger functions are
+ available: carry, compare, carry-borrow, and index.
+
+ carry:
+ Interrupt generated on active low Carry signal. Carry
+ signal toggles every time channel Y counter overflows.
+
+ compare:
+ Interrupt generated on active low Compare signal.
+ Compare signal toggles every time channel Y preset
+ register is equal to channel Y counter.
+
+ carry-borrow:
+ Interrupt generated on active low Carry signal and
+ active low Borrow signal. Carry signal toggles every
+ time channel Y counter overflows. Borrow signal toggles
+ every time channel Y counter underflows.
+
+ index:
+ Interrupt generated on active high Index signal.
+
+What: /sys/bus/counter/devices/counterX/countY/irq_trigger_enable
+KernelVersion: 5.11
+Contact: linux-iio@vger.kernel.org
+Description:
+ Whether generation of interrupts is enabled for channel Y. Valid
+ attribute values are boolean.
+
What: /sys/bus/counter/devices/counterX/signalY/cable_fault
KernelVersion: 5.7
Contact: linux-iio@vger.kernel.org
diff --git a/drivers/counter/104-quad-8.c b/drivers/counter/104-quad-8.c
index 92a3809609e9..c6948e31c6d6 100644
--- a/drivers/counter/104-quad-8.c
+++ b/drivers/counter/104-quad-8.c
@@ -13,23 +13,30 @@
#include <linux/iio/types.h>
#include <linux/io.h>
#include <linux/ioport.h>
+#include <linux/interrupt.h>
#include <linux/isa.h>
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/moduleparam.h>
#include <linux/types.h>
+#include <linux/spinlock.h>
#define QUAD8_EXTENT 32
static unsigned int base[max_num_isa_dev(QUAD8_EXTENT)];
static unsigned int num_quad8;
-module_param_array(base, uint, &num_quad8, 0);
+module_param_hw_array(base, uint, ioport, &num_quad8, 0);
MODULE_PARM_DESC(base, "ACCES 104-QUAD-8 base addresses");
+static unsigned int irq[max_num_isa_dev(QUAD8_EXTENT)];
+module_param_hw_array(irq, uint, irq, NULL, 0);
+MODULE_PARM_DESC(irq, "ACCES 104-QUAD-8 interrupt line numbers");
+
#define QUAD8_NUM_COUNTERS 8
/**
* struct quad8_iio - IIO device private data structure
+ * @lock: synchronization lock to prevent I/O race conditions
* @counter: instance of the counter_device
* @fck_prescaler: array of filter clock prescaler configurations
* @preset: array of preset values
@@ -38,13 +45,14 @@ MODULE_PARM_DESC(base, "ACCES 104-QUAD-8 base addresses");
* @quadrature_scale: array of quadrature mode scale configurations
* @ab_enable: array of A and B inputs enable configurations
* @preset_enable: array of set_to_preset_on_index attribute configurations
+ * @irq_trigger: array of interrupt trigger function configurations
* @synchronous_mode: array of index function synchronous mode configurations
* @index_polarity: array of index function polarity configurations
* @cable_fault_enable: differential encoder cable status enable configurations
* @base: base port address of the IIO device
*/
struct quad8_iio {
- struct mutex lock;
+ raw_spinlock_t lock;
struct counter_device counter;
unsigned int fck_prescaler[QUAD8_NUM_COUNTERS];
unsigned int preset[QUAD8_NUM_COUNTERS];
@@ -53,13 +61,16 @@ struct quad8_iio {
unsigned int quadrature_scale[QUAD8_NUM_COUNTERS];
unsigned int ab_enable[QUAD8_NUM_COUNTERS];
unsigned int preset_enable[QUAD8_NUM_COUNTERS];
+ unsigned int irq_trigger[QUAD8_NUM_COUNTERS];
unsigned int synchronous_mode[QUAD8_NUM_COUNTERS];
unsigned int index_polarity[QUAD8_NUM_COUNTERS];
unsigned int cable_fault_enable;
unsigned int base;
};
+#define QUAD8_REG_INTERRUPT_STATUS 0x10
#define QUAD8_REG_CHAN_OP 0x11
+#define QUAD8_REG_INDEX_INTERRUPT 0x12
#define QUAD8_REG_INDEX_INPUT_LEVELS 0x16
#define QUAD8_DIFF_ENCODER_CABLE_STATUS 0x17
/* Borrow Toggle flip-flop */
@@ -92,8 +103,8 @@ struct quad8_iio {
#define QUAD8_RLD_CNTR_OUT 0x10
/* Transfer Preset Register LSB to FCK Prescaler */
#define QUAD8_RLD_PRESET_PSC 0x18
-#define QUAD8_CHAN_OP_ENABLE_COUNTERS 0x00
#define QUAD8_CHAN_OP_RESET_COUNTERS 0x01
+#define QUAD8_CHAN_OP_ENABLE_INTERRUPT_FUNC 0x04
#define QUAD8_CMR_QUADRATURE_X1 0x08
#define QUAD8_CMR_QUADRATURE_X2 0x10
#define QUAD8_CMR_QUADRATURE_X4 0x18
@@ -107,6 +118,7 @@ static int quad8_read_raw(struct iio_dev *indio_dev,
unsigned int flags;
unsigned int borrow;
unsigned int carry;
+ unsigned long irqflags;
int i;
switch (mask) {
@@ -124,7 +136,7 @@ static int quad8_read_raw(struct iio_dev *indio_dev,
/* Borrow XOR Carry effectively doubles count range */
*val = (borrow ^ carry) << 24;
- mutex_lock(&priv->lock);
+ raw_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,
@@ -133,7 +145,7 @@ static int quad8_read_raw(struct iio_dev *indio_dev,
for (i = 0; i < 3; i++)
*val |= (unsigned int)inb(base_offset) << (8 * i);
- mutex_unlock(&priv->lock);
+ raw_spin_unlock_irqrestore(&priv->lock, irqflags);
return IIO_VAL_INT;
case IIO_CHAN_INFO_ENABLE:
@@ -153,6 +165,7 @@ static int quad8_write_raw(struct iio_dev *indio_dev,
{
struct quad8_iio *const priv = iio_priv(indio_dev);
const int base_offset = priv->base + 2 * chan->channel;
+ unsigned long flags;
int i;
unsigned int ior_cfg;
@@ -165,7 +178,7 @@ static int quad8_write_raw(struct iio_dev *indio_dev,
if ((unsigned int)val > 0xFFFFFF)
return -EINVAL;
- mutex_lock(&priv->lock);
+ raw_spin_lock_irqsave(&priv->lock, flags);
/* Reset Byte Pointer */
outb(QUAD8_CTR_RLD | QUAD8_RLD_RESET_BP, base_offset + 1);
@@ -190,7 +203,7 @@ static int quad8_write_raw(struct iio_dev *indio_dev,
/* Reset Error flag */
outb(QUAD8_CTR_RLD | QUAD8_RLD_RESET_E, base_offset + 1);
- mutex_unlock(&priv->lock);
+ raw_spin_unlock_irqrestore(&priv->lock, flags);
return 0;
case IIO_CHAN_INFO_ENABLE:
@@ -198,25 +211,26 @@ static int quad8_write_raw(struct iio_dev *indio_dev,
if (val < 0 || val > 1)
return -EINVAL;
- mutex_lock(&priv->lock);
+ raw_spin_lock_irqsave(&priv->lock, flags);
priv->ab_enable[chan->channel] = val;
- ior_cfg = val | priv->preset_enable[chan->channel] << 1;
+ ior_cfg = val | priv->preset_enable[chan->channel] << 1 |
+ priv->irq_trigger[chan->channel] << 3;
/* Load I/O control configuration */
outb(QUAD8_CTR_IOR | ior_cfg, base_offset + 1);
- mutex_unlock(&priv->lock);
+ raw_spin_unlock_irqrestore(&priv->lock, flags);
return 0;
case IIO_CHAN_INFO_SCALE:
- mutex_lock(&priv->lock);
+ raw_spin_lock_irqsave(&priv->lock, flags);
/* Quadrature scaling only available in quadrature mode */
if (!priv->quadrature_mode[chan->channel] &&
(val2 || val != 1)) {
- mutex_unlock(&priv->lock);
+ raw_spin_unlock_irqrestore(&priv->lock, flags);
return -EINVAL;
}
@@ -232,15 +246,15 @@ static int quad8_write_raw(struct iio_dev *indio_dev,
priv->quadrature_scale[chan->channel] = 2;
break;
default:
- mutex_unlock(&priv->lock);
+ raw_spin_unlock_irqrestore(&priv->lock, flags);
return -EINVAL;
}
else {
- mutex_unlock(&priv->lock);
+ raw_spin_unlock_irqrestore(&priv->lock, flags);
return -EINVAL;
}
- mutex_unlock(&priv->lock);
+ raw_spin_unlock_irqrestore(&priv->lock, flags);
return 0;
}
@@ -266,6 +280,7 @@ static ssize_t quad8_write_preset(struct iio_dev *indio_dev, uintptr_t private,
struct quad8_iio *const priv = iio_priv(indio_dev);
const int base_offset = priv->base + 2 * chan->channel;
unsigned int preset;
+ unsigned long irqflags;
int ret;
int i;
@@ -277,7 +292,7 @@ static ssize_t quad8_write_preset(struct iio_dev *indio_dev, uintptr_t private,
if (preset > 0xFFFFFF)
return -EINVAL;
- mutex_lock(&priv->lock);
+ raw_spin_lock_irqsave(&priv->lock, irqflags);
priv->preset[chan->channel] = preset;
@@ -288,7 +303,7 @@ static ssize_t quad8_write_preset(struct iio_dev *indio_dev, uintptr_t private,
for (i = 0; i < 3; i++)
outb(preset >> (8 * i), base_offset);
- mutex_unlock(&priv->lock);
+ raw_spin_unlock_irqrestore(&priv->lock, irqflags);
return len;
}
@@ -309,6 +324,7 @@ static ssize_t quad8_write_set_to_preset_on_index(struct iio_dev *indio_dev,
struct quad8_iio *const priv = iio_priv(indio_dev);
const int base_offset = priv->base + 2 * chan->channel + 1;
bool preset_enable;
+ unsigned long irqflags;
int ret;
unsigned int ior_cfg;
@@ -319,17 +335,18 @@ static ssize_t quad8_write_set_to_preset_on_index(struct iio_dev *indio_dev,
/* Preset enable is active low in Input/Output Control register */
preset_enable = !preset_enable;
- mutex_lock(&priv->lock);
+ raw_spin_lock_irqsave(&priv->lock, irqflags);
priv->preset_enable[chan->channel] = preset_enable;
ior_cfg = priv->ab_enable[chan->channel] |
- (unsigned int)preset_enable << 1;
+ (unsigned int)preset_enable << 1 |
+ priv->irq_trigger[chan->channel] << 3;
/* Load I/O control configuration to Input / Output Control Register */
outb(QUAD8_CTR_IOR | ior_cfg, base_offset);
- mutex_unlock(&priv->lock);
+ raw_spin_unlock_irqrestore(&priv->lock, irqflags);
return len;
}
@@ -387,8 +404,9 @@ static int quad8_set_count_mode(struct iio_dev *indio_dev,
struct quad8_iio *const priv = iio_priv(indio_dev);
unsigned int mode_cfg = cnt_mode << 1;
const int base_offset = priv->base + 2 * chan->channel + 1;
+ unsigned long irqflags;
- mutex_lock(&priv->lock);
+ raw_spin_lock_irqsave(&priv->lock, irqflags);
priv->count_mode[chan->channel] = cnt_mode;
@@ -399,7 +417,7 @@ static int quad8_set_count_mode(struct iio_dev *indio_dev,
/* Load mode configuration to Counter Mode Register */
outb(QUAD8_CTR_CMR | mode_cfg, base_offset);
- mutex_unlock(&priv->lock);
+ raw_spin_unlock_irqrestore(&priv->lock, irqflags);
return 0;
}
@@ -430,14 +448,15 @@ static int quad8_set_synchronous_mode(struct iio_dev *indio_dev,
struct quad8_iio *const priv = iio_priv(indio_dev);
const int base_offset = priv->base + 2 * chan->channel + 1;
unsigned int idr_cfg = synchronous_mode;
+ unsigned long irqflags;
- mutex_lock(&priv->lock);
+ raw_spin_lock_irqsave(&priv->lock, irqflags);
idr_cfg |= priv->index_polarity[chan->channel] << 1;
/* Index function must be non-synchronous in non-quadrature mode */
if (synchronous_mode && !priv->quadrature_mode[chan->channel]) {
- mutex_unlock(&priv->lock);
+ raw_spin_unlock_irqrestore(&priv->lock, irqflags);
return -EINVAL;
}
@@ -446,7 +465,7 @@ static int quad8_set_synchronous_mode(struct iio_dev *indio_dev,
/* Load Index Control configuration to Index Control Register */
outb(QUAD8_CTR_IDR | idr_cfg, base_offset);
- mutex_unlock(&priv->lock);
+ raw_spin_unlock_irqrestore(&priv->lock, irqflags);
return 0;
}
@@ -476,9 +495,10 @@ static int quad8_set_quadrature_mode(struct iio_dev *indio_dev,
{
struct quad8_iio *const priv = iio_priv(indio_dev);
const int base_offset = priv->base + 2 * chan->channel + 1;
+ unsigned long irqflags;
unsigned int mode_cfg;
- mutex_lock(&priv->lock);
+ raw_spin_lock_irqsave(&priv->lock, irqflags);
mode_cfg = priv->count_mode[chan->channel] << 1;
@@ -498,7 +518,7 @@ static int quad8_set_quadrature_mode(struct iio_dev *indio_dev,
/* Load mode configuration to Counter Mode Register */
outb(QUAD8_CTR_CMR | mode_cfg, base_offset);
- mutex_unlock(&priv->lock);
+ raw_spin_unlock_irqrestore(&priv->lock, irqflags);
return 0;
}
@@ -529,8 +549,9 @@ static int quad8_set_index_polarity(struct iio_dev *indio_dev,
struct quad8_iio *const priv = iio_priv(indio_dev);
const int base_offset = priv->base + 2 * chan->channel + 1;
unsigned int idr_cfg = index_polarity << 1;
+ unsigned long irqflags;
- mutex_lock(&priv->lock);
+ raw_spin_lock_irqsave(&priv->lock, irqflags);
idr_cfg |= priv->synchronous_mode[chan->channel];
@@ -539,7 +560,7 @@ static int quad8_set_index_polarity(struct iio_dev *indio_dev,
/* Load Index Control configuration to Index Control Register */
outb(QUAD8_CTR_IDR | idr_cfg, base_offset);
- mutex_unlock(&priv->lock);
+ raw_spin_unlock_irqrestore(&priv->lock, irqflags);
return 0;
}
@@ -646,6 +667,7 @@ static int quad8_count_read(struct counter_device *counter,
unsigned int flags;
unsigned int borrow;
unsigned int carry;
+ unsigned long irqflags;
int i;
flags = inb(base_offset + 1);
@@ -655,7 +677,7 @@ static int quad8_count_read(struct counter_device *counter,
/* Borrow XOR Carry effectively doubles count range */
*val = (unsigned long)(borrow ^ carry) << 24;
- mutex_lock(&priv->lock);
+ raw_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,
@@ -664,7 +686,7 @@ static int quad8_count_read(struct counter_device *counter,
for (i = 0; i < 3; i++)
*val |= (unsigned long)inb(base_offset) << (8 * i);
- mutex_unlock(&priv->lock);
+ raw_spin_unlock_irqrestore(&priv->lock, irqflags);
return 0;
}
@@ -674,13 +696,14 @@ static int quad8_count_write(struct counter_device *counter,
{
struct quad8_iio *const priv = counter->priv;
const int base_offset = priv->base + 2 * count->id;
+ unsigned long irqflags;
int i;
/* Only 24-bit values are supported */
if (val > 0xFFFFFF)
return -EINVAL;
- mutex_lock(&priv->lock);
+ raw_spin_lock_irqsave(&priv->lock, irqflags);
/* Reset Byte Pointer */
outb(QUAD8_CTR_RLD | QUAD8_RLD_RESET_BP, base_offset + 1);
@@ -705,7 +728,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);
+ raw_spin_unlock_irqrestore(&priv->lock, irqflags);
return 0;
}
@@ -723,8 +746,9 @@ static int quad8_function_read(struct counter_device *counter,
{
struct quad8_iio *const priv = counter->priv;
const int id = count->id;
+ unsigned long irqflags;
- mutex_lock(&priv->lock);
+ raw_spin_lock_irqsave(&priv->lock, irqflags);
if (priv->quadrature_mode[id])
switch (priv->quadrature_scale[id]) {
@@ -741,7 +765,7 @@ static int quad8_function_read(struct counter_device *counter,
else
*function = COUNTER_FUNCTION_PULSE_DIRECTION;
- mutex_unlock(&priv->lock);
+ raw_spin_unlock_irqrestore(&priv->lock, irqflags);
return 0;
}
@@ -755,10 +779,11 @@ static int quad8_function_write(struct counter_device *counter,
unsigned int *const scale = priv->quadrature_scale + id;
unsigned int *const synchronous_mode = priv->synchronous_mode + id;
const int base_offset = priv->base + 2 * id + 1;
+ unsigned long irqflags;
unsigned int mode_cfg;
unsigned int idr_cfg;
- mutex_lock(&priv->lock);
+ raw_spin_lock_irqsave(&priv->lock, irqflags);
mode_cfg = priv->count_mode[id] << 1;
idr_cfg = priv->index_polarity[id] << 1;
@@ -797,7 +822,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);
+ raw_spin_unlock_irqrestore(&priv->lock, irqflags);
return 0;
}
@@ -915,9 +940,10 @@ static int quad8_index_polarity_set(struct counter_device *counter,
struct quad8_iio *const priv = counter->priv;
const size_t channel_id = signal->id - 16;
const int base_offset = priv->base + 2 * channel_id + 1;
+ unsigned long irqflags;
unsigned int idr_cfg = index_polarity << 1;
- mutex_lock(&priv->lock);
+ raw_spin_lock_irqsave(&priv->lock, irqflags);
idr_cfg |= priv->synchronous_mode[channel_id];
@@ -926,7 +952,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);
+ raw_spin_unlock_irqrestore(&priv->lock, irqflags);
return 0;
}
@@ -950,15 +976,16 @@ static int quad8_synchronous_mode_set(struct counter_device *counter,
struct quad8_iio *const priv = counter->priv;
const size_t channel_id = signal->id - 16;
const int base_offset = priv->base + 2 * channel_id + 1;
+ unsigned long irqflags;
unsigned int idr_cfg = synchronous_mode;
- mutex_lock(&priv->lock);
+ raw_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);
+ raw_spin_unlock_irqrestore(&priv->lock, irqflags);
return -EINVAL;
}
@@ -967,7 +994,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);
+ raw_spin_unlock_irqrestore(&priv->lock, irqflags);
return 0;
}
@@ -1012,6 +1039,7 @@ static int quad8_count_mode_write(struct counter_device *counter,
unsigned int count_mode;
unsigned int mode_cfg;
const int base_offset = priv->base + 2 * count->id + 1;
+ unsigned long irqflags;
/* Map Generic Counter count mode to 104-QUAD-8 count mode */
switch (cnt_mode) {
@@ -1029,7 +1057,7 @@ static int quad8_count_mode_write(struct counter_device *counter,
break;
}
- mutex_lock(&priv->lock);
+ raw_spin_lock_irqsave(&priv->lock, irqflags);
priv->count_mode[count->id] = count_mode;
@@ -1043,7 +1071,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);
+ raw_spin_unlock_irqrestore(&priv->lock, irqflags);
return 0;
}
@@ -1063,18 +1091,20 @@ static int quad8_count_enable_write(struct counter_device *counter,
{
struct quad8_iio *const priv = counter->priv;
const int base_offset = priv->base + 2 * count->id;
+ unsigned long irqflags;
unsigned int ior_cfg;
- mutex_lock(&priv->lock);
+ raw_spin_lock_irqsave(&priv->lock, irqflags);
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);
- mutex_unlock(&priv->lock);
+ raw_spin_unlock_irqrestore(&priv->lock, irqflags);
return 0;
}
@@ -1120,16 +1150,17 @@ static int quad8_count_preset_write(struct counter_device *counter,
struct counter_count *count, u64 preset)
{
struct quad8_iio *const priv = counter->priv;
+ unsigned long irqflags;
/* Only 24-bit values are supported */
if (preset > 0xFFFFFF)
return -EINVAL;
- mutex_lock(&priv->lock);
+ raw_spin_lock_irqsave(&priv->lock, irqflags);
quad8_preset_register_set(priv, count->id, preset);
- mutex_unlock(&priv->lock);
+ raw_spin_unlock_irqrestore(&priv->lock, irqflags);
return 0;
}
@@ -1138,8 +1169,9 @@ static int quad8_count_ceiling_read(struct counter_device *counter,
struct counter_count *count, u64 *ceiling)
{
struct quad8_iio *const priv = counter->priv;
+ unsigned long irqflags;
- mutex_lock(&priv->lock);
+ raw_spin_lock_irqsave(&priv->lock, irqflags);
/* Range Limit and Modulo-N count modes use preset value as ceiling */
switch (priv->count_mode[count->id]) {
@@ -1153,7 +1185,7 @@ static int quad8_count_ceiling_read(struct counter_device *counter,
break;
}
- mutex_unlock(&priv->lock);
+ raw_spin_unlock_irqrestore(&priv->lock, irqflags);
return 0;
}
@@ -1162,12 +1194,13 @@ static int quad8_count_ceiling_write(struct counter_device *counter,
struct counter_count *count, u64 ceiling)
{
struct quad8_iio *const priv = counter->priv;
+ unsigned long irqflags;
/* Only 24-bit values are supported */
if (ceiling > 0xFFFFFF)
return -EINVAL;
- mutex_lock(&priv->lock);
+ raw_spin_lock_irqsave(&priv->lock, irqflags);
/* Range Limit and Modulo-N count modes use preset value as ceiling */
switch (priv->count_mode[count->id]) {
@@ -1177,7 +1210,7 @@ static int quad8_count_ceiling_write(struct counter_device *counter,
break;
}
- mutex_unlock(&priv->lock);
+ raw_spin_unlock_irqrestore(&priv->lock, irqflags);
return -EINVAL;
}
@@ -1199,21 +1232,91 @@ static int quad8_count_preset_enable_write(struct counter_device *counter,
{
struct quad8_iio *const priv = counter->priv;
const int base_offset = priv->base + 2 * count->id + 1;
+ unsigned long irqflags;
unsigned int ior_cfg;
/* Preset enable is active low in Input/Output Control register */
preset_enable = !preset_enable;
- mutex_lock(&priv->lock);
+ raw_spin_lock_irqsave(&priv->lock, irqflags);
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);
- mutex_unlock(&priv->lock);
+ raw_spin_unlock_irqrestore(&priv->lock, irqflags);
+
+ return 0;
+}
+
+static int quad8_irq_trigger_get(struct counter_device *counter,
+ struct counter_count *count, u8 *irq_trigger)
+{
+ const struct quad8_iio *const priv = counter->priv;
+
+ *irq_trigger = priv->irq_trigger[count->id];
+
+ return 0;
+}
+
+static int quad8_irq_trigger_set(struct counter_device *counter,
+ struct counter_count *count, u8 irq_trigger)
+{
+ struct quad8_iio *const priv = counter->priv;
+ const unsigned long base_offset = priv->base + 2 * count->id + 1;
+ unsigned long irqflags;
+ unsigned long ior_cfg;
+
+ raw_spin_lock_irqsave(&priv->lock, irqflags);
+
+ priv->irq_trigger[count->id] = irq_trigger;
+
+ ior_cfg = priv->ab_enable[count->id] |
+ priv->preset_enable[count->id] << 1 | irq_trigger << 3;
+
+ /* Load I/O control configuration to Input / Output Control Register */
+ outb(QUAD8_CTR_IOR | ior_cfg, base_offset);
+
+ raw_spin_unlock_irqrestore(&priv->lock, irqflags);
+
+ return 0;
+}
+
+static int quad8_irq_trigger_enable_read(struct counter_device *counter,
+ struct counter_count *count, u8 *state)
+{
+ const struct quad8_iio *const priv = counter->priv;
+ unsigned long irq_enabled;
+
+ irq_enabled = inb(priv->base + QUAD8_REG_INDEX_INTERRUPT);
+ *state = !!(irq_enabled & BIT(count->id));
+
+ return 0;
+}
+
+static int quad8_irq_trigger_enable_write(struct counter_device *counter,
+ struct counter_count *count, u8 state)
+{
+ struct quad8_iio *const priv = counter->priv;
+ unsigned long irqflags;
+ unsigned long irq_enabled;
+
+ raw_spin_lock_irqsave(&priv->lock, irqflags);
+
+ irq_enabled = inb(priv->base + QUAD8_REG_INDEX_INTERRUPT);
+
+ if (state)
+ irq_enabled |= BIT(count->id);
+ else
+ irq_enabled &= ~BIT(count->id);
+
+ outb(irq_enabled, priv->base + QUAD8_REG_INDEX_INTERRUPT);
+
+ raw_spin_unlock_irqrestore(&priv->lock, irqflags);
return 0;
}
@@ -1224,22 +1327,23 @@ static int quad8_signal_cable_fault_read(struct counter_device *counter,
{
struct quad8_iio *const priv = counter->priv;
const size_t channel_id = signal->id / 2;
+ unsigned long irqflags;
bool disabled;
unsigned int status;
- mutex_lock(&priv->lock);
+ raw_spin_lock_irqsave(&priv->lock, irqflags);
disabled = !(priv->cable_fault_enable & BIT(channel_id));
if (disabled) {
- mutex_unlock(&priv->lock);
+ raw_spin_unlock_irqrestore(&priv->lock, irqflags);
return -EINVAL;
}
/* Logic 0 = cable fault */
status = inb(priv->base + QUAD8_DIFF_ENCODER_CABLE_STATUS);
- mutex_unlock(&priv->lock);
+ raw_spin_unlock_irqrestore(&priv->lock, irqflags);
/* Mask respective channel and invert logic */
*cable_fault = !(status & BIT(channel_id));
@@ -1265,9 +1369,10 @@ static int quad8_signal_cable_fault_enable_write(struct counter_device *counter,
{
struct quad8_iio *const priv = counter->priv;
const size_t channel_id = signal->id / 2;
+ unsigned long irqflags;
unsigned int cable_fault_enable;
- mutex_lock(&priv->lock);
+ raw_spin_lock_irqsave(&priv->lock, irqflags);
if (enable)
priv->cable_fault_enable |= BIT(channel_id);
@@ -1279,7 +1384,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);
+ raw_spin_unlock_irqrestore(&priv->lock, irqflags);
return 0;
}
@@ -1302,8 +1407,9 @@ static int quad8_signal_fck_prescaler_write(struct counter_device *counter,
struct quad8_iio *const priv = counter->priv;
const size_t channel_id = signal->id / 2;
const int base_offset = priv->base + 2 * channel_id;
+ unsigned long irqflags;
- mutex_lock(&priv->lock);
+ raw_spin_lock_irqsave(&priv->lock, irqflags);
priv->fck_prescaler[channel_id] = prescaler;
@@ -1315,7 +1421,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);
+ raw_spin_unlock_irqrestore(&priv->lock, irqflags);
return 0;
}
@@ -1418,7 +1524,15 @@ static const u8 quad8_cnt_modes[] = {
static DEFINE_COUNTER_AVAILABLE(quad8_count_mode_available, quad8_cnt_modes);
+static const char *const quad8_irq_trigger_states[] = {
+ "carry",
+ "compare",
+ "carry-borrow",
+ "index",
+};
+
static DEFINE_COUNTER_ENUM(quad8_error_noise_enum, quad8_noise_error_states);
+static DEFINE_COUNTER_ENUM(quad8_irq_trigger_enum, quad8_irq_trigger_states);
static struct counter_comp quad8_count_ext[] = {
COUNTER_COMP_CEILING(quad8_count_ceiling_read,
@@ -1433,6 +1547,11 @@ static struct counter_comp quad8_count_ext[] = {
COUNTER_COMP_PRESET(quad8_count_preset_read, quad8_count_preset_write),
COUNTER_COMP_PRESET_ENABLE(quad8_count_preset_enable_read,
quad8_count_preset_enable_write),
+ COUNTER_COMP_COUNT_ENUM("irq_trigger", quad8_irq_trigger_get,
+ quad8_irq_trigger_set, quad8_irq_trigger_enum),
+ COUNTER_COMP_COUNT_BOOL("irq_trigger_enable",
+ quad8_irq_trigger_enable_read,
+ quad8_irq_trigger_enable_write),
};
#define QUAD8_COUNT(_id, _cntname) { \
@@ -1457,6 +1576,51 @@ static struct counter_count quad8_counts[] = {
QUAD8_COUNT(7, "Channel 8 Count")
};
+static irqreturn_t quad8_irq_handler(int irq, void *quad8iio)
+{
+ struct quad8_iio *const priv = quad8iio;
+ const unsigned long base = priv->base;
+ unsigned long irq_status;
+ unsigned long channel;
+ u8 event;
+ int err;
+
+ irq_status = inb(base + QUAD8_REG_INTERRUPT_STATUS);
+ if (!irq_status)
+ return IRQ_NONE;
+
+ for_each_set_bit(channel, &irq_status, QUAD8_NUM_COUNTERS) {
+ switch (priv->irq_trigger[channel]) {
+ case 0:
+ event = COUNTER_EVENT_OVERFLOW;
+ break;
+ case 1:
+ event = COUNTER_EVENT_THRESHOLD;
+ break;
+ case 2:
+ event = COUNTER_EVENT_OVERFLOW_UNDERFLOW;
+ break;
+ case 3:
+ event = COUNTER_EVENT_INDEX;
+ break;
+ default:
+ /* We should never reach here */
+ WARN_ONCE(true, "invalid interrupt trigger function %u configured for channel %lu\n",
+ priv->irq_trigger[channel], channel);
+ continue;
+ }
+ err = counter_push_event(&priv->counter, event, channel);
+ if (err)
+ WARN_ONCE(true, "unable to push counter event %lu for channel %lu (error code %d)\n",
+ (unsigned long)event, channel, err);
+ }
+
+ /* Clear pending interrupts on device */
+ outb(QUAD8_CHAN_OP_ENABLE_INTERRUPT_FUNC, base + QUAD8_REG_CHAN_OP);
+
+ return IRQ_HANDLED;
+}
+
static int quad8_probe(struct device *dev, unsigned int id)
{
struct iio_dev *indio_dev;
@@ -1495,9 +1659,10 @@ static int quad8_probe(struct device *dev, unsigned int id)
quad8iio->counter.priv = quad8iio;
quad8iio->base = base[id];
- /* Initialize mutex */
- mutex_init(&quad8iio->lock);
+ raw_spin_lock_init(&quad8iio->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 */
@@ -1527,8 +1692,8 @@ static int quad8_probe(struct device *dev, unsigned int id)
}
/* 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);
/* Register IIO device */
err = devm_iio_device_register(dev, indio_dev);
@@ -1536,7 +1701,12 @@ static int quad8_probe(struct device *dev, unsigned int id)
return err;
/* Register Counter device */
- return devm_counter_register(dev, &quad8iio->counter);
+ err = devm_counter_register(dev, &quad8iio->counter);
+ if (err)
+ return err;
+
+ return devm_request_irq(dev, irq[id], quad8_irq_handler, IRQF_SHARED,
+ quad8iio->counter.name, quad8iio);
}
static struct isa_driver quad8_driver = {
diff --git a/drivers/counter/Kconfig b/drivers/counter/Kconfig
index 2de53ab0dd25..bd42df98f522 100644
--- a/drivers/counter/Kconfig
+++ b/drivers/counter/Kconfig
@@ -23,11 +23,11 @@ config 104_QUAD_8
A counter's respective error flag may be cleared by performing a write
operation on the respective count value attribute. Although the
104-QUAD-8 counters have a 25-bit range, only the lower 24 bits may be
- set, either directly or via the counter's preset attribute. Interrupts
- are not supported by this driver.
+ set, either directly or via the counter's preset attribute.
The base port addresses for the devices may be configured via the base
- array module parameter.
+ array module parameter. The interrupt line numbers for the devices may
+ be configured via the irq array module parameter.
config STM32_TIMER_CNT
tristate "STM32 Timer encoder counter driver"
--
2.29.2
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v6 1/5] counter: Internalize sysfs interface code
[not found] ` <950660d49af7d12b09bc9d3b1db6f8ff74209c26.1606075915.git.vilhelm.gray@gmail.com>
@ 2020-11-25 13:07 ` William Breathitt Gray
2020-12-13 23:15 ` David Lechner
1 sibling, 0 replies; 17+ messages in thread
From: William Breathitt Gray @ 2020-11-25 13:07 UTC (permalink / raw)
To: david
Cc: kernel, linux-stm32, a.fatoum, kamel.bouhara, gwendal,
alexandre.belloni, jic23, linux-iio, linux-kernel,
linux-arm-kernel, syednwaris, patrick.havelange, fabrice.gasnier,
mcoquelin.stm32, alexandre.torgue, Dan Carpenter
[-- Attachment #1: Type: text/plain, Size: 757 bytes --]
On Sun, Nov 22, 2020 at 03:29:52PM -0500, William Breathitt Gray wrote:
> @@ -117,62 +112,95 @@ static int ti_eqep_count_write(struct counter_device *counter,
> return regmap_write(priv->regmap32, QPOSCNT, val);
> }
>
> -static int ti_eqep_function_get(struct counter_device *counter,
> - struct counter_count *count, size_t *function)
> +static const u8 ti_qep_t2c_functions_map[] = {
> +};
Just a heads-up: this ti_qep_t2c_functions_map array is left over from
some code I was testing. It's not used at all -- I simply forgot to
remove it -- so I'll make sure to take it out in the next patchset. I'll
give this v6 patchset some more time for people to review and comment
before I submit the v7 revision.
William Breathitt Gray
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v6 1/5] counter: Internalize sysfs interface code
[not found] ` <950660d49af7d12b09bc9d3b1db6f8ff74209c26.1606075915.git.vilhelm.gray@gmail.com>
2020-11-25 13:07 ` [PATCH v6 1/5] counter: Internalize sysfs interface code William Breathitt Gray
@ 2020-12-13 23:15 ` David Lechner
2020-12-20 22:11 ` William Breathitt Gray
1 sibling, 1 reply; 17+ messages in thread
From: David Lechner @ 2020-12-13 23:15 UTC (permalink / raw)
To: William Breathitt Gray, jic23
Cc: kernel, linux-stm32, a.fatoum, kamel.bouhara, gwendal,
alexandre.belloni, linux-iio, linux-kernel, linux-arm-kernel,
syednwaris, patrick.havelange, fabrice.gasnier, mcoquelin.stm32,
alexandre.torgue, Dan Carpenter
On 11/22/20 2:29 PM, William Breathitt Gray wrote:
> 14 files changed, 1806 insertions(+), 2546 deletions(-)
It would be really nice if we could break this down into smaller
pieces and start getting it merged. It is really tough to keep
reviewing this much code in one patch over and over again.
Here are some initial findings from testing:
> +static void counter_device_release(struct device *dev)
> +{
> + struct counter_device *const counter = dev_get_drvdata(dev);
> +
> + counter_chrdev_remove(counter);
> + ida_simple_remove(&counter_ida, counter->id);
> +}
I got the following error after `modprobe -r ti-eqep`:
[ 1186.045766] ------------[ cut here ]------------
[ 1186.050647] WARNING: CPU: 0 PID: 2625 at lib/refcount.c:28 counter_device_release+0x10/0x24 [counter]
[ 1186.059976] refcount_t: underflow; use-after-free.
[ 1186.064790] Modules linked in: aes_arm_bs(+) crypto_simd cryptd ccm usb_f_mass_storage usb_f_acm u_serial usb_f_ecm rfcomm usb_f_rndis u_ether libcomposite aes_arm aes_generic cmac bnep wl18xx wlcore mac80211 libarc4 sha256_generic libsha256 sha256_arm cfg80211 ti_am335x_adc kfifo_buf omap_aes_driver omap_crypto omap_sham crypto_engine pm33xx ti_emif_sram hci_uart omap_rng btbcm rng_core ti_eqep(-) counter bluetooth c_can_platform c_can ecdh_generic bmp280_spi ecc can_dev libaes bmp280_i2c bmp280 industrialio omap_mailbox musb_dsps wlcore_sdio musb_hdrc udc_core usbcore wkup_m3_ipc at24 omap_wdt phy_am335x watchdog phy_am335x_control ti_am335x_tscadc phy_generic wkup_m3_rproc usb_common cppi41 rtc_omap leds_gpio led_class cpufreq_dt pwm_tiehrpwm autofs4
[ 1186.132376] CPU: 0 PID: 2625 Comm: modprobe Not tainted 5.10.0-rc7bone-counter+ #23
[ 1186.140070] Hardware name: Generic AM33XX (Flattened Device Tree)
[ 1186.146225] [<c0110d70>] (unwind_backtrace) from [<c010b640>] (show_stack+0x10/0x14)
[ 1186.154017] [<c010b640>] (show_stack) from [<c09a0c98>] (dump_stack+0xc4/0xe4)
[ 1186.161285] [<c09a0c98>] (dump_stack) from [<c0137ba0>] (__warn+0xd8/0x100)
[ 1186.168284] [<c0137ba0>] (__warn) from [<c099c8e4>] (warn_slowpath_fmt+0x94/0xbc)
[ 1186.175814] [<c099c8e4>] (warn_slowpath_fmt) from [<bf10b0e8>] (counter_device_release+0x10/0x24 [counter])
[ 1186.185632] [<bf10b0e8>] (counter_device_release [counter]) from [<c0667118>] (device_release+0x30/0xa4)
[ 1186.195163] [<c0667118>] (device_release) from [<c057f73c>] (kobject_put+0x94/0x104)
[ 1186.202944] [<c057f73c>] (kobject_put) from [<c057f73c>] (kobject_put+0x94/0x104)
[ 1186.210472] [<c057f73c>] (kobject_put) from [<bf19004c>] (ti_eqep_remove+0x10/0x30 [ti_eqep])
[ 1186.219047] [<bf19004c>] (ti_eqep_remove [ti_eqep]) from [<c066f390>] (platform_drv_remove+0x24/0x3c)
[ 1186.228313] [<c066f390>] (platform_drv_remove) from [<c066d934>] (device_release_driver_internal+0xfc/0x1d0)
[ 1186.238187] [<c066d934>] (device_release_driver_internal) from [<c066da78>] (driver_detach+0x58/0xa8)
[ 1186.247456] [<c066da78>] (driver_detach) from [<c066c5ec>] (bus_remove_driver+0x4c/0xa0)
[ 1186.255594] [<c066c5ec>] (bus_remove_driver) from [<c01dd150>] (sys_delete_module+0x180/0x264)
[ 1186.264250] [<c01dd150>] (sys_delete_module) from [<c0100080>] (ret_fast_syscall+0x0/0x54)
[ 1186.272551] Exception stack(0xd247ffa8 to 0xd247fff0)
[ 1186.277629] ffa0: 004fb478 004fb478 004fb4b4 00000800 b3bfcf00 00000000
[ 1186.285847] ffc0: 004fb478 004fb478 004fb478 00000081 00000000 be974900 be974a55 004fb478
[ 1186.294062] ffe0: 004f8f5c be97352c 004ddd97 b6d11d68
[ 1186.299253] ---[ end trace e1c61dea091f1078 ]---
> +static ssize_t counter_comp_u8_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t len)
> +{
> + const struct counter_attribute *const a = to_counter_attribute(attr);
> + struct counter_device *const counter = dev_get_drvdata(dev);
> + struct counter_count *const count = a->parent;
> + struct counter_synapse *const synapse = a->comp.priv;
> + const struct counter_available *const avail = a->comp.priv;
> + int err;
> + bool bool_data;
> + int idx;
> + u8 data;
> +
> + switch (a->comp.type) {
> + case COUNTER_COMP_BOOL:
> + err = kstrtobool(buf, &bool_data);
> + data = bool_data;
> + break;
> + case COUNTER_COMP_FUNCTION:
> + err = find_in_string_array(&data, count->functions_list,
> + count->num_functions, buf,
> + counter_function_str);
> + break;
> + case COUNTER_COMP_SYNAPSE_ACTION:
> + err = find_in_string_array(&data, synapse->actions_list,
> + synapse->num_actions, buf,
> + counter_synapse_action_str);
> + break;
> + case COUNTER_COMP_ENUM:
> + idx = __sysfs_match_string(avail->strs, avail->num_items, buf);
> + if (idx < 0)
> + return idx;
> + data = idx;
> + break;
> + case COUNTER_COMP_COUNT_MODE:
> + err = find_in_string_array(&data, avail->enums,
> + avail->num_items, buf,
> + counter_count_mode_str);
> + break;
> + default:
> + err = kstrtou8(buf, 0, &data);
> + break;
> + }
> + if (err)
This needs to be `if (err < 0)`. There are cases where the functions
above return positive values. (And to be overly safe, it probably wouldn't
hurt to use err < 0 everywhere - not just in this function.)
> + return err;
> +
> + switch (a->scope) {
> + case COUNTER_SCOPE_DEVICE:
> + err = a->comp.device_u8_write(counter, data);
> + break;
> + case COUNTER_SCOPE_SIGNAL:
> + err = a->comp.signal_u8_write(counter, a->parent, data);
> + break;
> + case COUNTER_SCOPE_COUNT:
> + if (a->comp.type == COUNTER_COMP_SYNAPSE_ACTION)
> + err = a->comp.action_write(counter, count, synapse,
> + data);
> + else
> + err = a->comp.count_u8_write(counter, count, data);
> + break;
> + }
> + if (err)
> + return err;
> +
> + return len;
> +}
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v6 0/5] Introduce the Counter character device interface
2020-11-22 20:29 [PATCH v6 0/5] Introduce the Counter character device interface William Breathitt Gray
` (4 preceding siblings ...)
[not found] ` <950660d49af7d12b09bc9d3b1db6f8ff74209c26.1606075915.git.vilhelm.gray@gmail.com>
@ 2020-12-13 23:15 ` David Lechner
2020-12-20 21:44 ` William Breathitt Gray
5 siblings, 1 reply; 17+ messages in thread
From: David Lechner @ 2020-12-13 23:15 UTC (permalink / raw)
To: William Breathitt Gray, jic23
Cc: kernel, linux-stm32, a.fatoum, kamel.bouhara, gwendal,
alexandre.belloni, linux-iio, linux-kernel, linux-arm-kernel,
syednwaris, patrick.havelange, fabrice.gasnier, mcoquelin.stm32,
alexandre.torgue
On 11/22/20 2:29 PM, William Breathitt Gray wrote:
>
> 1. Should standard Counter component data types be defined as u8 or u32?
>
> Many standard Counter component types such COUNTER_COMP_SIGNAL_LEVEL
> have standard values defined (e.g. COUNTER_SIGNAL_LEVEL_LOW and
> COUNTER_SIGNAL_LEVEL_HIGH). These values are currently handled by the
> Counter subsystem code as u8 data types.
>
> If u32 is used for these values instead, C enum structures could be
> used by driver authors to implicitly cast these values via the driver
> callback parameters.
>
> This question is primarily addressed to David Lechner. I'm somewhat
> confused about how this setup would look in device drivers. I've gone
> ahead and refactored the code to support u32 enums, and pushed it to
> a separate branch on my repository called counter_chrdev_v6_u32_enum:
> https://gitlab.com/vilhelmgray/iio/-/tree/counter_chrdev_v6_u32_enum
>
> Please check it out and let me know what you think. Is this the
> support you had in mind? I'm curious to see an example of how would
> your driver callback functions would look in this case. If everything
> works out fine, then I'll submit this branch as v7 of this patchset.
I haven't had time to look at this in depth, but just superficially looking
at it, it is mostly there. The driver callback would just use the enum type
in place of u32. For example:
static int ti_eqep_function_write(struct counter_device *counter,
struct counter_count *count,
enum counter_function function)
and the COUNTER_FUNCTION_* constants would be defined as:
enum counter_function {
COUNTER_FUNCTION_INCREASE,
...
};
instead of using #define macros.
One advantage I see to using u8, at least in the user API data structures,
is that it increases the number of events that fit in the kfifo buffer by
a significant factor.
And that is not to say that we couldn't do both: have the user API structs
use u8 for enum values and still use u32/strong enum types internally in
the callback functions.
>
> 2. How should we handle "raw" timestamps?
>
> Ahmad Fatoum brought up the possibility of returning "raw" timestamps
> similar to what the network stack offers (see the network stack
> SOF_TIMESTAMPING_{RAW,SYS}_HARDWARE support).
>
> I'm not very familiar with the networking stack code, but if I
> understand correctly the SOF_TIMESTAMPING_RAW_HARDWARE timestamps are
> values returned from the device. If so, I suspect we would be able to
> support these "raw" timestamps by defining them as Counter Extensions
> and returning them in struct counter_event elements similar to the
> other Extension values.
Is nanosecond resolution good enough? In the TI eQEP driver I considered
returning the raw timer value, but quickly realized that it would not be
very nice to expect the user code to know the clock rate of the timer. It
was very easy to get the clock rate in the kernel and just convert the
timer value to nanoseconds before returning it to userspace.
So if there is some specialized case where it can be solved no other way
besides using raw timestamps, then sure, include it. Otherwise I think we
should stick with nanoseconds for time values when possible.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v6 3/5] counter: Add character device interface
2020-11-22 20:29 ` [PATCH v6 3/5] counter: Add character device interface William Breathitt Gray
@ 2020-12-13 23:58 ` David Lechner
2020-12-25 17:30 ` William Breathitt Gray
2021-01-19 9:20 ` Oleksij Rempel
1 sibling, 1 reply; 17+ messages in thread
From: David Lechner @ 2020-12-13 23:58 UTC (permalink / raw)
To: William Breathitt Gray, jic23
Cc: kernel, linux-stm32, a.fatoum, kamel.bouhara, gwendal,
alexandre.belloni, linux-iio, linux-kernel, linux-arm-kernel,
syednwaris, patrick.havelange, fabrice.gasnier, mcoquelin.stm32,
alexandre.torgue, Dan Carpenter
On 11/22/20 2:29 PM, William Breathitt Gray wrote:
> This patch introduces a character device interface for the Counter
> subsystem. Device data is exposed through standard character device read
> operations. Device data is gathered when a Counter event is pushed by
> the respective Counter device driver. Configuration is handled via ioctl
> operations on the respective Counter character device node.
>
> Cc: David Lechner <david@lechnology.com>
> Cc: Gwendal Grignou <gwendal@chromium.org>
> Cc: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
> ---
> diff --git a/drivers/counter/counter-chrdev.c b/drivers/counter/counter-chrdev.c
> new file mode 100644
> index 000000000000..96fa7fbeef92
> --- /dev/null
> +++ b/drivers/counter/counter-chrdev.c
> +static ssize_t counter_chrdev_read(struct file *filp, char __user *buf,
> + size_t len, loff_t *f_ps)
> +{
> + struct counter_device *const counter = filp->private_data;
> + int err;
> + unsigned long flags;
> + unsigned int copied;
> +
> + if (len < sizeof(struct counter_event))
> + return -EINVAL;
> +
> + do {
> + if (kfifo_is_empty(&counter->events)) {
> + if (filp->f_flags & O_NONBLOCK)
> + return -EAGAIN;
> +
> + err = wait_event_interruptible(counter->events_wait,
> + !kfifo_is_empty(&counter->events));
> + if (err)
> + return err;
> + }
> +
> + raw_spin_lock_irqsave(&counter->events_lock, flags);
As mentioned in my previous review, I think it would be fine to use a
mutex here instead of disabling interrupts. In fact, I think copy_to_user()
might sleep, in which case we really don't want to call this with
interrupts disabled.
> + err = kfifo_to_user(&counter->events, buf, len, &copied);
> + raw_spin_unlock_irqrestore(&counter->events_lock, flags);
> + if (err)
> + return err;
> + } while (!copied);
> +
> + return copied;
> +}
> +
> +static int counter_add_watch(struct counter_device *const counter,
> + const unsigned long arg)
> +{
> + void __user *const uwatch = (void __user *)arg;
> + struct counter_watch watch;
> + struct counter_comp_node comp_node = {0};
> + size_t parent, id;
> + struct counter_comp *ext;
> + size_t num_ext;
> +
> + if (copy_from_user(&watch, uwatch, sizeof(watch)))
> + return -EFAULT;
> +
> + /* Dummy components can skip evaluation */
> + if (watch.component.type == COUNTER_COMPONENT_DUMMY)
I think "none" would be a better name than "dummy". Then it just
naturally makes sense why we would skip the evaluation.
> + goto dummy_component;
> +
> + parent = watch.component.parent;
> +
> + /* Configure parent component info for comp node */
> + switch (watch.component.scope) {
> + case COUNTER_SCOPE_DEVICE:
> + ext = counter->ext;
> + num_ext = counter->num_ext;
> + break;
> + case COUNTER_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;
> + case COUNTER_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) {
> + case COUNTER_COMPONENT_SIGNAL:
> + if (watch.component.scope != COUNTER_SCOPE_SIGNAL)
> + return -EINVAL;
> +
> + comp_node.comp.type = COUNTER_COMP_SIGNAL_LEVEL;
> + comp_node.comp.signal_u8_read = counter->ops->signal_read;
> + break;
> + case COUNTER_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;
> + case COUNTER_COMPONENT_FUNCTION:
> + if (watch.component.scope != COUNTER_SCOPE_COUNT)
> + return -EINVAL;
> +
> + comp_node.comp.type = COUNTER_COMP_FUNCTION;
> + comp_node.comp.count_u8_read = counter->ops->function_read;
> + break;
> + case COUNTER_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;
> + case COUNTER_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;
> +
> +dummy_component:
> + comp_node.component = watch.component;
In my experiments, I added a events_validate driver callback here to
validate each event as it is added. This way the user can know exactly
which event caused the problem rather than waiting for the event_config
callback.
> +
> + return counter_set_event_node(counter, &watch, &comp_node);
> +}
> +
> +static int counter_chrdev_open(struct inode *inode, struct file *filp)
> +{
> + struct counter_device *const counter = container_of(inode->i_cdev,
> + typeof(*counter),
> + chrdev);
> +
> + get_device(&counter->dev);
> + filp->private_data = counter;
> +
> + return nonseekable_open(inode, filp);
> +}
> +
> +static int counter_chrdev_release(struct inode *inode, struct file *filp)
> +{
> + struct counter_device *const counter = filp->private_data;
> + unsigned long flags;
> +
> + raw_spin_lock_irqsave(&counter->events_lock, flags);
> + counter_events_list_free(&counter->events_list);
Do we need to call the events_config callback here?
> + raw_spin_unlock_irqrestore(&counter->events_lock, flags);
> + counter_events_list_free(&counter->next_events_list);
> +
> + put_device(&counter->dev);
> +
> + return 0;
> +}
> +/**
> + * 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.
> + *
> + * RETURNS:
> + * 0 on success, negative error number on failure.
> + */
> +int counter_push_event(struct counter_device *const counter, const u8 event,
> + const u8 channel)
> +{
> + struct counter_event ev = {0};
> + unsigned int copied = 0;
> + unsigned long flags;
> + struct counter_event_node *event_node;
> + struct counter_comp_node *comp_node;
> + int err = 0;
> +
> + ev.timestamp = ktime_get_ns();
> + ev.watch.event = event;
> + ev.watch.channel = channel;
> +
> + raw_spin_lock_irqsave(&counter->events_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)
> + goto exit_early;
> +
> + /* Read and queue relevant comp for userspace */
> + list_for_each_entry(comp_node, &event_node->comp_list, l) {
> + err = counter_get_data(counter, comp_node, &ev.value);
> + if (err)
> + goto exit_early;
It looks like this will skip an event even if just one watch value
has an error. This doesn't seem so great.
> +
> + ev.watch.component = comp_node->component;
> +
> + copied += kfifo_put(&counter->events, ev);
> + }
> +
> + if (copied)
> + wake_up_poll(&counter->events_wait, EPOLLIN);
> +
> +exit_early:
> + raw_spin_unlock_irqrestore(&counter->events_lock, flags);
> +
> + return err;
Interrupt handlers can't really do anything about the error here.
It could be nice to instead add an error field to the userspace
event struct, then userspace would know when an error occurred
and could do something about it. And it also fixes the one-error-
skips-all problem I mentioned above.
> +}
> +EXPORT_SYMBOL_GPL(counter_push_event);
> diff --git a/include/linux/counter.h b/include/linux/counter.h
> index 3f3f8ba6c1b4..98cd7c035968 100644
> --- a/include/linux/counter.h
>
> +/**
> + * struct counter_event_node - Counter Event node
> + * @l: list of current watching Counter events
> + * @event: event that triggers
> + * @channel: event channel
> + * @comp_list: list of components to watch when event triggers
> + */
> +struct counter_event_node {
> + struct list_head l;
> + u8 event;
> + u8 channel;
> + struct list_head comp_list;
> +};
> +
Unless this is needed outside of the drivers/counter/ directory, I
would suggest putting it in drivers/counter/counter-chrdev.h instead
of include/linux/counter.h.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v6 0/5] Introduce the Counter character device interface
2020-12-13 23:15 ` [PATCH v6 0/5] Introduce the Counter character device interface David Lechner
@ 2020-12-20 21:44 ` William Breathitt Gray
2020-12-21 15:19 ` David Lechner
0 siblings, 1 reply; 17+ messages in thread
From: William Breathitt Gray @ 2020-12-20 21:44 UTC (permalink / raw)
To: David Lechner, David.Laight
Cc: jic23, kernel, linux-stm32, a.fatoum, kamel.bouhara, gwendal,
alexandre.belloni, linux-iio, linux-kernel, linux-arm-kernel,
syednwaris, patrick.havelange, fabrice.gasnier, mcoquelin.stm32,
alexandre.torgue
[-- Attachment #1: Type: text/plain, Size: 4999 bytes --]
On Sun, Dec 13, 2020 at 05:15:14PM -0600, David Lechner wrote:
> On 11/22/20 2:29 PM, William Breathitt Gray wrote:
> >
> > 1. Should standard Counter component data types be defined as u8 or u32?
> >
> > Many standard Counter component types such COUNTER_COMP_SIGNAL_LEVEL
> > have standard values defined (e.g. COUNTER_SIGNAL_LEVEL_LOW and
> > COUNTER_SIGNAL_LEVEL_HIGH). These values are currently handled by the
> > Counter subsystem code as u8 data types.
> >
> > If u32 is used for these values instead, C enum structures could be
> > used by driver authors to implicitly cast these values via the driver
> > callback parameters.
> >
> > This question is primarily addressed to David Lechner. I'm somewhat
> > confused about how this setup would look in device drivers. I've gone
> > ahead and refactored the code to support u32 enums, and pushed it to
> > a separate branch on my repository called counter_chrdev_v6_u32_enum:
> > https://gitlab.com/vilhelmgray/iio/-/tree/counter_chrdev_v6_u32_enum
> >
> > Please check it out and let me know what you think. Is this the
> > support you had in mind? I'm curious to see an example of how would
> > your driver callback functions would look in this case. If everything
> > works out fine, then I'll submit this branch as v7 of this patchset.
>
> I haven't had time to look at this in depth, but just superficially looking
> at it, it is mostly there. The driver callback would just use the enum type
> in place of u32. For example:
>
> static int ti_eqep_function_write(struct counter_device *counter,
> struct counter_count *count,
> enum counter_function function)
>
> and the COUNTER_FUNCTION_* constants would be defined as:
>
> enum counter_function {
> COUNTER_FUNCTION_INCREASE,
> ...
> };
>
> instead of using #define macros.
>
> One advantage I see to using u8, at least in the user API data structures,
> is that it increases the number of events that fit in the kfifo buffer by
> a significant factor.
>
> And that is not to say that we couldn't do both: have the user API structs
> use u8 for enum values and still use u32/strong enum types internally in
> the callback functions.
I'm including David Laight because he initially opposed enums in favor
of fixed size types when we discussed this in an earlier revision:
https://lkml.org/lkml/2020/5/3/159
However, there have been significant changes to this patchset so the
context now is different than those earlier discussions (i.e. we're no
longer discussing ioctl calls).
I think reimplementing these constants as enums as described could work.
If we do so, should the enum constants be given specific values? For
example:
enum counter_function {
COUNTER_FUNCTION_INCREASE = 0,
COUNTER_FUNCTION_DECREASE = 1,
...
};
>
> >
> > 2. How should we handle "raw" timestamps?
> >
> > Ahmad Fatoum brought up the possibility of returning "raw" timestamps
> > similar to what the network stack offers (see the network stack
> > SOF_TIMESTAMPING_{RAW,SYS}_HARDWARE support).
> >
> > I'm not very familiar with the networking stack code, but if I
> > understand correctly the SOF_TIMESTAMPING_RAW_HARDWARE timestamps are
> > values returned from the device. If so, I suspect we would be able to
> > support these "raw" timestamps by defining them as Counter Extensions
> > and returning them in struct counter_event elements similar to the
> > other Extension values.
>
> Is nanosecond resolution good enough? In the TI eQEP driver I considered
> returning the raw timer value, but quickly realized that it would not be
> very nice to expect the user code to know the clock rate of the timer. It
> was very easy to get the clock rate in the kernel and just convert the
> timer value to nanoseconds before returning it to userspace.
>
> So if there is some specialized case where it can be solved no other way
> besides using raw timestamps, then sure, include it. Otherwise I think we
> should stick with nanoseconds for time values when possible.
Given that the struct counter_event 'timestamp' member serves as the
identification vessel for correlating component values to a single event
(i.e. component values of a given event will share the same unique
timestamp), I believe it's prudent to standardize this timestamp format
on the kernel monotonic time as we have currently done so via our
ktime_get_ns() call.
There are cases where it is understandably better to use a timestamp
provided directly by the hardware (e.g. keeping timestamping close to
data collection). For these cases, we can retrieve these "raw"
timestamps via a Counter Extension: users would get their "raw"
timestamp via the struct counter_event 'value' member, and just treat
the 'timestamp' member as a unique event identification number.
William Breathitt Gray
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v6 1/5] counter: Internalize sysfs interface code
2020-12-13 23:15 ` David Lechner
@ 2020-12-20 22:11 ` William Breathitt Gray
2020-12-21 15:26 ` David Lechner
0 siblings, 1 reply; 17+ messages in thread
From: William Breathitt Gray @ 2020-12-20 22:11 UTC (permalink / raw)
To: David Lechner
Cc: jic23, kernel, linux-stm32, a.fatoum, kamel.bouhara, gwendal,
alexandre.belloni, linux-iio, linux-kernel, linux-arm-kernel,
syednwaris, patrick.havelange, fabrice.gasnier, mcoquelin.stm32,
alexandre.torgue, Dan Carpenter
[-- Attachment #1: Type: text/plain, Size: 6478 bytes --]
On Sun, Dec 13, 2020 at 05:15:00PM -0600, David Lechner wrote:
> On 11/22/20 2:29 PM, William Breathitt Gray wrote:
>
> > 14 files changed, 1806 insertions(+), 2546 deletions(-)
>
> It would be really nice if we could break this down into smaller
> pieces and start getting it merged. It is really tough to keep
> reviewing this much code in one patch over and over again.
Yes, this is a pretty massive patch. I could break this across the
individual files affected to make it simpler to review, but in the end
all those patches would need to end up squashed together before merge
again (for the sake of git bisect), so the effort feels somewhat moot.
Luckily, I don't think there will be much change in the next revision
since it's looking like it'll mainly be a few bug fixes; hopefully this
coming version 7 will be the final revision before merge.
> Here are some initial findings from testing:
>
>
> > +static void counter_device_release(struct device *dev)
> > +{
> > + struct counter_device *const counter = dev_get_drvdata(dev);
> > +
> > + counter_chrdev_remove(counter);
> > + ida_simple_remove(&counter_ida, counter->id);
> > +}
>
>
> I got the following error after `modprobe -r ti-eqep`:
>
> [ 1186.045766] ------------[ cut here ]------------
> [ 1186.050647] WARNING: CPU: 0 PID: 2625 at lib/refcount.c:28 counter_device_release+0x10/0x24 [counter]
> [ 1186.059976] refcount_t: underflow; use-after-free.
> [ 1186.064790] Modules linked in: aes_arm_bs(+) crypto_simd cryptd ccm usb_f_mass_storage usb_f_acm u_serial usb_f_ecm rfcomm usb_f_rndis u_ether libcomposite aes_arm aes_generic cmac bnep wl18xx wlcore mac80211 libarc4 sha256_generic libsha256 sha256_arm cfg80211 ti_am335x_adc kfifo_buf omap_aes_driver omap_crypto omap_sham crypto_engine pm33xx ti_emif_sram hci_uart omap_rng btbcm rng_core ti_eqep(-) counter bluetooth c_can_platform c_can ecdh_generic bmp280_spi ecc can_dev libaes bmp280_i2c bmp280 industrialio omap_mailbox musb_dsps wlcore_sdio musb_hdrc udc_core usbcore wkup_m3_ipc at24 omap_wdt phy_am335x watchdog phy_am335x_control ti_am335x_tscadc phy_generic wkup_m3_rproc usb_common cppi41 rtc_omap leds_gpio led_class cpufreq_dt pwm_tiehrpwm autofs4
> [ 1186.132376] CPU: 0 PID: 2625 Comm: modprobe Not tainted 5.10.0-rc7bone-counter+ #23
> [ 1186.140070] Hardware name: Generic AM33XX (Flattened Device Tree)
> [ 1186.146225] [<c0110d70>] (unwind_backtrace) from [<c010b640>] (show_stack+0x10/0x14)
> [ 1186.154017] [<c010b640>] (show_stack) from [<c09a0c98>] (dump_stack+0xc4/0xe4)
> [ 1186.161285] [<c09a0c98>] (dump_stack) from [<c0137ba0>] (__warn+0xd8/0x100)
> [ 1186.168284] [<c0137ba0>] (__warn) from [<c099c8e4>] (warn_slowpath_fmt+0x94/0xbc)
> [ 1186.175814] [<c099c8e4>] (warn_slowpath_fmt) from [<bf10b0e8>] (counter_device_release+0x10/0x24 [counter])
> [ 1186.185632] [<bf10b0e8>] (counter_device_release [counter]) from [<c0667118>] (device_release+0x30/0xa4)
> [ 1186.195163] [<c0667118>] (device_release) from [<c057f73c>] (kobject_put+0x94/0x104)
> [ 1186.202944] [<c057f73c>] (kobject_put) from [<c057f73c>] (kobject_put+0x94/0x104)
> [ 1186.210472] [<c057f73c>] (kobject_put) from [<bf19004c>] (ti_eqep_remove+0x10/0x30 [ti_eqep])
> [ 1186.219047] [<bf19004c>] (ti_eqep_remove [ti_eqep]) from [<c066f390>] (platform_drv_remove+0x24/0x3c)
> [ 1186.228313] [<c066f390>] (platform_drv_remove) from [<c066d934>] (device_release_driver_internal+0xfc/0x1d0)
> [ 1186.238187] [<c066d934>] (device_release_driver_internal) from [<c066da78>] (driver_detach+0x58/0xa8)
> [ 1186.247456] [<c066da78>] (driver_detach) from [<c066c5ec>] (bus_remove_driver+0x4c/0xa0)
> [ 1186.255594] [<c066c5ec>] (bus_remove_driver) from [<c01dd150>] (sys_delete_module+0x180/0x264)
> [ 1186.264250] [<c01dd150>] (sys_delete_module) from [<c0100080>] (ret_fast_syscall+0x0/0x54)
> [ 1186.272551] Exception stack(0xd247ffa8 to 0xd247fff0)
> [ 1186.277629] ffa0: 004fb478 004fb478 004fb4b4 00000800 b3bfcf00 00000000
> [ 1186.285847] ffc0: 004fb478 004fb478 004fb478 00000081 00000000 be974900 be974a55 004fb478
> [ 1186.294062] ffe0: 004f8f5c be97352c 004ddd97 b6d11d68
> [ 1186.299253] ---[ end trace e1c61dea091f1078 ]---
I noticed that I'm calling counter_chrdev_remove() twice: once in
counter_unregister(), and again in counter_device_release(). I suspect
this is what's causing the refcount to underflow. I'll test and verify
that this is the culprit.
In fact, I don't think I need to define a counter_device_release()
callback at all, would I? These cleanup function calls could be moved to
counter_unregister() instead.
> > +static ssize_t counter_comp_u8_store(struct device *dev,
> > + struct device_attribute *attr,
> > + const char *buf, size_t len)
> > +{
> > + const struct counter_attribute *const a = to_counter_attribute(attr);
> > + struct counter_device *const counter = dev_get_drvdata(dev);
> > + struct counter_count *const count = a->parent;
> > + struct counter_synapse *const synapse = a->comp.priv;
> > + const struct counter_available *const avail = a->comp.priv;
> > + int err;
> > + bool bool_data;
> > + int idx;
> > + u8 data;
> > +
> > + switch (a->comp.type) {
> > + case COUNTER_COMP_BOOL:
> > + err = kstrtobool(buf, &bool_data);
> > + data = bool_data;
> > + break;
> > + case COUNTER_COMP_FUNCTION:
> > + err = find_in_string_array(&data, count->functions_list,
> > + count->num_functions, buf,
> > + counter_function_str);
> > + break;
> > + case COUNTER_COMP_SYNAPSE_ACTION:
> > + err = find_in_string_array(&data, synapse->actions_list,
> > + synapse->num_actions, buf,
> > + counter_synapse_action_str);
> > + break;
> > + case COUNTER_COMP_ENUM:
> > + idx = __sysfs_match_string(avail->strs, avail->num_items, buf);
> > + if (idx < 0)
> > + return idx;
> > + data = idx;
> > + break;
> > + case COUNTER_COMP_COUNT_MODE:
> > + err = find_in_string_array(&data, avail->enums,
> > + avail->num_items, buf,
> > + counter_count_mode_str);
> > + break;
> > + default:
> > + err = kstrtou8(buf, 0, &data);
> > + break;
> > + }
> > + if (err)
>
> This needs to be `if (err < 0)`. There are cases where the functions
> above return positive values. (And to be overly safe, it probably wouldn't
> hurt to use err < 0 everywhere - not just in this function.)
Ack.
William Breathitt Gray
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v6 0/5] Introduce the Counter character device interface
2020-12-20 21:44 ` William Breathitt Gray
@ 2020-12-21 15:19 ` David Lechner
0 siblings, 0 replies; 17+ messages in thread
From: David Lechner @ 2020-12-21 15:19 UTC (permalink / raw)
To: William Breathitt Gray, David.Laight
Cc: jic23, kernel, linux-stm32, a.fatoum, kamel.bouhara, gwendal,
alexandre.belloni, linux-iio, linux-kernel, linux-arm-kernel,
syednwaris, patrick.havelange, fabrice.gasnier, mcoquelin.stm32,
alexandre.torgue
On 12/20/20 3:44 PM, William Breathitt Gray wrote:
> On Sun, Dec 13, 2020 at 05:15:14PM -0600, David Lechner wrote:
>> On 11/22/20 2:29 PM, William Breathitt Gray wrote:
>>>
>>> 1. Should standard Counter component data types be defined as u8 or u32?
>>>
>>> Many standard Counter component types such COUNTER_COMP_SIGNAL_LEVEL
>>> have standard values defined (e.g. COUNTER_SIGNAL_LEVEL_LOW and
>>> COUNTER_SIGNAL_LEVEL_HIGH). These values are currently handled by the
>>> Counter subsystem code as u8 data types.
>>>
>>> If u32 is used for these values instead, C enum structures could be
>>> used by driver authors to implicitly cast these values via the driver
>>> callback parameters.
>>>
>>> This question is primarily addressed to David Lechner. I'm somewhat
>>> confused about how this setup would look in device drivers. I've gone
>>> ahead and refactored the code to support u32 enums, and pushed it to
>>> a separate branch on my repository called counter_chrdev_v6_u32_enum:
>>> https://gitlab.com/vilhelmgray/iio/-/tree/counter_chrdev_v6_u32_enum
>>>
>>> Please check it out and let me know what you think. Is this the
>>> support you had in mind? I'm curious to see an example of how would
>>> your driver callback functions would look in this case. If everything
>>> works out fine, then I'll submit this branch as v7 of this patchset.
>>
>> I haven't had time to look at this in depth, but just superficially looking
>> at it, it is mostly there. The driver callback would just use the enum type
>> in place of u32. For example:
>>
>> static int ti_eqep_function_write(struct counter_device *counter,
>> struct counter_count *count,
>> enum counter_function function)
>>
>> and the COUNTER_FUNCTION_* constants would be defined as:
>>
>> enum counter_function {
>> COUNTER_FUNCTION_INCREASE,
>> ...
>> };
>>
>> instead of using #define macros.
>>
>> One advantage I see to using u8, at least in the user API data structures,
>> is that it increases the number of events that fit in the kfifo buffer by
>> a significant factor.
>>
>> And that is not to say that we couldn't do both: have the user API structs
>> use u8 for enum values and still use u32/strong enum types internally in
>> the callback functions.
>
> I'm including David Laight because he initially opposed enums in favor
> of fixed size types when we discussed this in an earlier revision:
> https://lkml.org/lkml/2020/5/3/159
>
> However, there have been significant changes to this patchset so the
> context now is different than those earlier discussions (i.e. we're no
> longer discussing ioctl calls).
>
> I think reimplementing these constants as enums as described could work.
> If we do so, should the enum constants be given specific values? For
> example:
>
> enum counter_function {
> COUNTER_FUNCTION_INCREASE = 0,
> COUNTER_FUNCTION_DECREASE = 1,
> ...
> };
I would say no on the explicit values since they don't have
any significant meaning.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v6 1/5] counter: Internalize sysfs interface code
2020-12-20 22:11 ` William Breathitt Gray
@ 2020-12-21 15:26 ` David Lechner
0 siblings, 0 replies; 17+ messages in thread
From: David Lechner @ 2020-12-21 15:26 UTC (permalink / raw)
To: William Breathitt Gray
Cc: jic23, kernel, linux-stm32, a.fatoum, kamel.bouhara, gwendal,
alexandre.belloni, linux-iio, linux-kernel, linux-arm-kernel,
syednwaris, patrick.havelange, fabrice.gasnier, mcoquelin.stm32,
alexandre.torgue, Dan Carpenter
On 12/20/20 4:11 PM, William Breathitt Gray wrote:
> On Sun, Dec 13, 2020 at 05:15:00PM -0600, David Lechner wrote:
>> On 11/22/20 2:29 PM, William Breathitt Gray wrote:
>>
>>> 14 files changed, 1806 insertions(+), 2546 deletions(-)
>>
>> It would be really nice if we could break this down into smaller
>> pieces and start getting it merged. It is really tough to keep
>> reviewing this much code in one patch over and over again.
>
> Yes, this is a pretty massive patch. I could break this across the
> individual files affected to make it simpler to review, but in the end
> all those patches would need to end up squashed together before merge
> again (for the sake of git bisect), so the effort feels somewhat moot.
>
> Luckily, I don't think there will be much change in the next revision
> since it's looking like it'll mainly be a few bug fixes; hopefully this
> coming version 7 will be the final revision before merge.
>
>> Here are some initial findings from testing:
>>
>>
>>> +static void counter_device_release(struct device *dev)
>>> +{
>>> + struct counter_device *const counter = dev_get_drvdata(dev);
>>> +
>>> + counter_chrdev_remove(counter);
>>> + ida_simple_remove(&counter_ida, counter->id);
>>> +}
>>
>>
>> I got the following error after `modprobe -r ti-eqep`:
>>
>> [ 1186.045766] ------------[ cut here ]------------
>> [ 1186.050647] WARNING: CPU: 0 PID: 2625 at lib/refcount.c:28 counter_device_release+0x10/0x24 [counter]
>> [ 1186.059976] refcount_t: underflow; use-after-free.
>> [ 1186.064790] Modules linked in: aes_arm_bs(+) crypto_simd cryptd ccm usb_f_mass_storage usb_f_acm u_serial usb_f_ecm rfcomm usb_f_rndis u_ether libcomposite aes_arm aes_generic cmac bnep wl18xx wlcore mac80211 libarc4 sha256_generic libsha256 sha256_arm cfg80211 ti_am335x_adc kfifo_buf omap_aes_driver omap_crypto omap_sham crypto_engine pm33xx ti_emif_sram hci_uart omap_rng btbcm rng_core ti_eqep(-) counter bluetooth c_can_platform c_can ecdh_generic bmp280_spi ecc can_dev libaes bmp280_i2c bmp280 industrialio omap_mailbox musb_dsps wlcore_sdio musb_hdrc udc_core usbcore wkup_m3_ipc at24 omap_wdt phy_am335x watchdog phy_am335x_control ti_am335x_tscadc phy_generic wkup_m3_rproc usb_common cppi41 rtc_omap leds_gpio led_class cpufreq_dt pwm_tiehrpwm autofs4
>> [ 1186.132376] CPU: 0 PID: 2625 Comm: modprobe Not tainted 5.10.0-rc7bone-counter+ #23
>> [ 1186.140070] Hardware name: Generic AM33XX (Flattened Device Tree)
>> [ 1186.146225] [<c0110d70>] (unwind_backtrace) from [<c010b640>] (show_stack+0x10/0x14)
>> [ 1186.154017] [<c010b640>] (show_stack) from [<c09a0c98>] (dump_stack+0xc4/0xe4)
>> [ 1186.161285] [<c09a0c98>] (dump_stack) from [<c0137ba0>] (__warn+0xd8/0x100)
>> [ 1186.168284] [<c0137ba0>] (__warn) from [<c099c8e4>] (warn_slowpath_fmt+0x94/0xbc)
>> [ 1186.175814] [<c099c8e4>] (warn_slowpath_fmt) from [<bf10b0e8>] (counter_device_release+0x10/0x24 [counter])
>> [ 1186.185632] [<bf10b0e8>] (counter_device_release [counter]) from [<c0667118>] (device_release+0x30/0xa4)
>> [ 1186.195163] [<c0667118>] (device_release) from [<c057f73c>] (kobject_put+0x94/0x104)
>> [ 1186.202944] [<c057f73c>] (kobject_put) from [<c057f73c>] (kobject_put+0x94/0x104)
>> [ 1186.210472] [<c057f73c>] (kobject_put) from [<bf19004c>] (ti_eqep_remove+0x10/0x30 [ti_eqep])
>> [ 1186.219047] [<bf19004c>] (ti_eqep_remove [ti_eqep]) from [<c066f390>] (platform_drv_remove+0x24/0x3c)
>> [ 1186.228313] [<c066f390>] (platform_drv_remove) from [<c066d934>] (device_release_driver_internal+0xfc/0x1d0)
>> [ 1186.238187] [<c066d934>] (device_release_driver_internal) from [<c066da78>] (driver_detach+0x58/0xa8)
>> [ 1186.247456] [<c066da78>] (driver_detach) from [<c066c5ec>] (bus_remove_driver+0x4c/0xa0)
>> [ 1186.255594] [<c066c5ec>] (bus_remove_driver) from [<c01dd150>] (sys_delete_module+0x180/0x264)
>> [ 1186.264250] [<c01dd150>] (sys_delete_module) from [<c0100080>] (ret_fast_syscall+0x0/0x54)
>> [ 1186.272551] Exception stack(0xd247ffa8 to 0xd247fff0)
>> [ 1186.277629] ffa0: 004fb478 004fb478 004fb4b4 00000800 b3bfcf00 00000000
>> [ 1186.285847] ffc0: 004fb478 004fb478 004fb478 00000081 00000000 be974900 be974a55 004fb478
>> [ 1186.294062] ffe0: 004f8f5c be97352c 004ddd97 b6d11d68
>> [ 1186.299253] ---[ end trace e1c61dea091f1078 ]---
>
> I noticed that I'm calling counter_chrdev_remove() twice: once in
> counter_unregister(), and again in counter_device_release(). I suspect
> this is what's causing the refcount to underflow. I'll test and verify
> that this is the culprit.
>
> In fact, I don't think I need to define a counter_device_release()
> callback at all, would I? These cleanup function calls could be moved to
> counter_unregister() instead.
As long as a user program keeps a chrdev open, it holds a reference to
the device, so I think it needs to be the other way around. (Unless it
is impossible to call counter_unregister() before all references have
been released - but I don't think that is the case - not 100% sure.)
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v6 3/5] counter: Add character device interface
2020-12-13 23:58 ` David Lechner
@ 2020-12-25 17:30 ` William Breathitt Gray
0 siblings, 0 replies; 17+ messages in thread
From: William Breathitt Gray @ 2020-12-25 17:30 UTC (permalink / raw)
To: David Lechner
Cc: jic23, kernel, linux-stm32, a.fatoum, kamel.bouhara, gwendal,
alexandre.belloni, linux-iio, linux-kernel, linux-arm-kernel,
syednwaris, patrick.havelange, fabrice.gasnier, mcoquelin.stm32,
alexandre.torgue, Dan Carpenter
[-- Attachment #1: Type: text/plain, Size: 2007 bytes --]
Hi David,
I agree with your suggested changes -- just a couple select comments
following below.
On Sun, Dec 13, 2020 at 05:58:26PM -0600, David Lechner wrote:
> > +static int counter_add_watch(struct counter_device *const counter,
> > + const unsigned long arg)
> > +{
[...]
> > +
> > +dummy_component:
> > + comp_node.component = watch.component;
>
>
> In my experiments, I added a events_validate driver callback here to
> validate each event as it is added. This way the user can know exactly
> which event caused the problem rather than waiting for the event_config
> callback.
Yes, this is a good idea and I have use for this in the 104-quad-8
driver as well. I'm going to name this "watch_validate" however, because
I need to validate the requested channel as well as the requested event
here (both part of the struct counter_watch).
> > diff --git a/include/linux/counter.h b/include/linux/counter.h
> > index 3f3f8ba6c1b4..98cd7c035968 100644
> > --- a/include/linux/counter.h
>
>
> >
> > +/**
> > + * struct counter_event_node - Counter Event node
> > + * @l: list of current watching Counter events
> > + * @event: event that triggers
> > + * @channel: event channel
> > + * @comp_list: list of components to watch when event triggers
> > + */
> > +struct counter_event_node {
> > + struct list_head l;
> > + u8 event;
> > + u8 channel;
> > + struct list_head comp_list;
> > +};
> > +
>
>
> Unless this is needed outside of the drivers/counter/ directory, I
> would suggest putting it in drivers/counter/counter-chrdev.h instead
> of include/linux/counter.h.
The "events_list" member of the struct counter_device is a list of
struct counter_event_node. The events_configure() callback should parse
through this list to determine the current events configuration request.
As such, driver authors will need this structure available via
include/linux/counter.h so they can parse "events_list".
William Breathitt Gray
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v6 3/5] counter: Add character device interface
2020-11-22 20:29 ` [PATCH v6 3/5] counter: Add character device interface William Breathitt Gray
2020-12-13 23:58 ` David Lechner
@ 2021-01-19 9:20 ` Oleksij Rempel
2021-01-21 8:03 ` William Breathitt Gray
1 sibling, 1 reply; 17+ messages in thread
From: Oleksij Rempel @ 2021-01-19 9:20 UTC (permalink / raw)
To: William Breathitt Gray
Cc: jic23, kamel.bouhara, gwendal, a.fatoum, david, linux-iio,
patrick.havelange, alexandre.belloni, mcoquelin.stm32,
linux-kernel, Dan Carpenter, kernel, fabrice.gasnier, syednwaris,
linux-stm32, linux-arm-kernel, alexandre.torgue, David Jander
On Sun, Nov 22, 2020 at 03:29:54PM -0500, William Breathitt Gray wrote:
> This patch introduces a character device interface for the Counter
> subsystem. Device data is exposed through standard character device read
> operations. Device data is gathered when a Counter event is pushed by
> the respective Counter device driver. Configuration is handled via ioctl
> operations on the respective Counter character device node.
>
> Cc: David Lechner <david@lechnology.com>
> Cc: Gwendal Grignou <gwendal@chromium.org>
> Cc: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
> ---
Hello William,
the series looks quite interesting, we have some thoughts... see below:
[...]
> +/**
> + * 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.
> + *
> + * RETURNS:
> + * 0 on success, negative error number on failure.
> + */
> +int counter_push_event(struct counter_device *const counter, const u8 event,
> + const u8 channel)
> +{
> + struct counter_event ev = {0};
> + unsigned int copied = 0;
> + unsigned long flags;
> + struct counter_event_node *event_node;
> + struct counter_comp_node *comp_node;
> + int err = 0;
> +
> + ev.timestamp = ktime_get_ns();
> + ev.watch.event = event;
> + ev.watch.channel = channel;
> +
> + raw_spin_lock_irqsave(&counter->events_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)
> + goto exit_early;
> +
> + /* Read and queue relevant comp for userspace */
> + list_for_each_entry(comp_node, &event_node->comp_list, l) {
> + err = counter_get_data(counter, comp_node, &ev.value);
> + if (err)
> + goto exit_early;
> +
> + ev.watch.component = comp_node->component;
> +
> + copied += kfifo_put(&counter->events, ev);
We want to calculate the frequency of some IRQ pulses in user space and
counter values with time stamps really fits well here. As the pulses are
from a physical system (rotating wheel), they will only change at a
certain rate. We want to have the possibility to read from the counter
device less often, we intentionally want to skip (meaning miss)
events.
When reading we're interested in the newest events. The kfifo implements
a "tail" drop FIFO, which means new values are added at the end, and if
the FIFO is full, they are dropped. We need a "head" drop FIFO which
discards the oldest events, keeping only the recent ones.
As far as we know, kfifo doesn't offer a head drop mode, but I think
this can be added.
[...]
> struct counter_device {
> const char *name;
> @@ -270,12 +270,20 @@ struct counter_device {
>
> int id;
> struct device dev;
> + struct cdev chrdev;
> + raw_spinlock_t events_lock;
> + struct list_head events_list;
> + struct list_head next_events_list;
> + DECLARE_KFIFO(events, struct counter_event, 64);
Do you plan to make the size of the FIFO configurable?
regards,
Oleksij & Marc
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v6 3/5] counter: Add character device interface
2021-01-19 9:20 ` Oleksij Rempel
@ 2021-01-21 8:03 ` William Breathitt Gray
2021-01-21 18:26 ` Jonathan Cameron
0 siblings, 1 reply; 17+ messages in thread
From: William Breathitt Gray @ 2021-01-21 8:03 UTC (permalink / raw)
To: Oleksij Rempel
Cc: jic23, kamel.bouhara, gwendal, a.fatoum, david, linux-iio,
patrick.havelange, alexandre.belloni, mcoquelin.stm32,
linux-kernel, Dan Carpenter, kernel, fabrice.gasnier, syednwaris,
linux-stm32, linux-arm-kernel, alexandre.torgue, David Jander
[-- Attachment #1: Type: text/plain, Size: 4151 bytes --]
On Tue, Jan 19, 2021 at 10:20:22AM +0100, Oleksij Rempel wrote:
> On Sun, Nov 22, 2020 at 03:29:54PM -0500, William Breathitt Gray wrote:
> > This patch introduces a character device interface for the Counter
> > subsystem. Device data is exposed through standard character device read
> > operations. Device data is gathered when a Counter event is pushed by
> > the respective Counter device driver. Configuration is handled via ioctl
> > operations on the respective Counter character device node.
> >
> > Cc: David Lechner <david@lechnology.com>
> > Cc: Gwendal Grignou <gwendal@chromium.org>
> > Cc: Dan Carpenter <dan.carpenter@oracle.com>
> > Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
> > ---
>
> Hello William,
>
> the series looks quite interesting, we have some thoughts... see below:
>
> [...]
> > +/**
> > + * 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.
> > + *
> > + * RETURNS:
> > + * 0 on success, negative error number on failure.
> > + */
> > +int counter_push_event(struct counter_device *const counter, const u8 event,
> > + const u8 channel)
> > +{
> > + struct counter_event ev = {0};
> > + unsigned int copied = 0;
> > + unsigned long flags;
> > + struct counter_event_node *event_node;
> > + struct counter_comp_node *comp_node;
> > + int err = 0;
> > +
> > + ev.timestamp = ktime_get_ns();
> > + ev.watch.event = event;
> > + ev.watch.channel = channel;
> > +
> > + raw_spin_lock_irqsave(&counter->events_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)
> > + goto exit_early;
> > +
> > + /* Read and queue relevant comp for userspace */
> > + list_for_each_entry(comp_node, &event_node->comp_list, l) {
> > + err = counter_get_data(counter, comp_node, &ev.value);
> > + if (err)
> > + goto exit_early;
> > +
> > + ev.watch.component = comp_node->component;
> > +
> > + copied += kfifo_put(&counter->events, ev);
>
> We want to calculate the frequency of some IRQ pulses in user space and
> counter values with time stamps really fits well here. As the pulses are
> from a physical system (rotating wheel), they will only change at a
> certain rate. We want to have the possibility to read from the counter
> device less often, we intentionally want to skip (meaning miss)
> events.
>
> When reading we're interested in the newest events. The kfifo implements
> a "tail" drop FIFO, which means new values are added at the end, and if
> the FIFO is full, they are dropped. We need a "head" drop FIFO which
> discards the oldest events, keeping only the recent ones.
>
> As far as we know, kfifo doesn't offer a head drop mode, but I think
> this can be added.
I'm not sure if kfifo has this kind of mode, but it seems like something
that should be there if it is not already -- I imagine this kind of
operation mode would be pretty common. Perhaps someone knows how to
achieve this and will share here.
>
> [...]
>
> > struct counter_device {
> > const char *name;
> > @@ -270,12 +270,20 @@ struct counter_device {
> >
> > int id;
> > struct device dev;
> > + struct cdev chrdev;
> > + raw_spinlock_t events_lock;
> > + struct list_head events_list;
> > + struct list_head next_events_list;
> > + DECLARE_KFIFO(events, struct counter_event, 64);
>
> Do you plan to make the size of the FIFO configurable?
>
> regards,
> Oleksij & Marc
I suppose it wouldn't be a problem to make this a configurable setting;
I think I can implement this via kfifo_alloc() and kfifo_free(). How
would users control this -- maybe using a sysfs attribute?
William Breathitt Gray
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v6 3/5] counter: Add character device interface
2021-01-21 8:03 ` William Breathitt Gray
@ 2021-01-21 18:26 ` Jonathan Cameron
0 siblings, 0 replies; 17+ messages in thread
From: Jonathan Cameron @ 2021-01-21 18:26 UTC (permalink / raw)
To: William Breathitt Gray
Cc: Oleksij Rempel, linux-arm-kernel, kamel.bouhara, gwendal,
a.fatoum, david, linux-iio, patrick.havelange, linux-stm32,
alexandre.belloni, linux-kernel, Dan Carpenter, mcoquelin.stm32,
David Jander, fabrice.gasnier, syednwaris, kernel, jic23,
alexandre.torgue
On Thu, 21 Jan 2021 17:03:11 +0900
William Breathitt Gray <vilhelm.gray@gmail.com> wrote:
> On Tue, Jan 19, 2021 at 10:20:22AM +0100, Oleksij Rempel wrote:
> > On Sun, Nov 22, 2020 at 03:29:54PM -0500, William Breathitt Gray wrote:
> > > This patch introduces a character device interface for the Counter
> > > subsystem. Device data is exposed through standard character device read
> > > operations. Device data is gathered when a Counter event is pushed by
> > > the respective Counter device driver. Configuration is handled via ioctl
> > > operations on the respective Counter character device node.
> > >
> > > Cc: David Lechner <david@lechnology.com>
> > > Cc: Gwendal Grignou <gwendal@chromium.org>
> > > Cc: Dan Carpenter <dan.carpenter@oracle.com>
> > > Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
> > > ---
> >
> > Hello William,
> >
> > the series looks quite interesting, we have some thoughts... see below:
> >
> > [...]
> > > +/**
> > > + * 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.
> > > + *
> > > + * RETURNS:
> > > + * 0 on success, negative error number on failure.
> > > + */
> > > +int counter_push_event(struct counter_device *const counter, const u8 event,
> > > + const u8 channel)
> > > +{
> > > + struct counter_event ev = {0};
> > > + unsigned int copied = 0;
> > > + unsigned long flags;
> > > + struct counter_event_node *event_node;
> > > + struct counter_comp_node *comp_node;
> > > + int err = 0;
> > > +
> > > + ev.timestamp = ktime_get_ns();
> > > + ev.watch.event = event;
> > > + ev.watch.channel = channel;
> > > +
> > > + raw_spin_lock_irqsave(&counter->events_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)
> > > + goto exit_early;
> > > +
> > > + /* Read and queue relevant comp for userspace */
> > > + list_for_each_entry(comp_node, &event_node->comp_list, l) {
> > > + err = counter_get_data(counter, comp_node, &ev.value);
> > > + if (err)
> > > + goto exit_early;
> > > +
> > > + ev.watch.component = comp_node->component;
> > > +
> > > + copied += kfifo_put(&counter->events, ev);
> >
> > We want to calculate the frequency of some IRQ pulses in user space and
> > counter values with time stamps really fits well here. As the pulses are
> > from a physical system (rotating wheel), they will only change at a
> > certain rate. We want to have the possibility to read from the counter
> > device less often, we intentionally want to skip (meaning miss)
> > events.
> >
> > When reading we're interested in the newest events. The kfifo implements
> > a "tail" drop FIFO, which means new values are added at the end, and if
> > the FIFO is full, they are dropped. We need a "head" drop FIFO which
> > discards the oldest events, keeping only the recent ones.
> >
> > As far as we know, kfifo doesn't offer a head drop mode, but I think
> > this can be added.
>
> I'm not sure if kfifo has this kind of mode, but it seems like something
> that should be there if it is not already -- I imagine this kind of
> operation mode would be pretty common. Perhaps someone knows how to
> achieve this and will share here.
>
I don't think it does. In IIO we started with a ring (effectively this)
- hopefully no one else remembers the horrible mess I made of it ;),
but eventually decided that there were very few usecases that
actually demanded it so just switched over to kfifo.
It's actually pretty rare that you aren't better off draining to
a backing store of some type.
Not had any demand to bring a ring back to IIO since we dropped it.
> >
> > [...]
> >
> > > struct counter_device {
> > > const char *name;
> > > @@ -270,12 +270,20 @@ struct counter_device {
> > >
> > > int id;
> > > struct device dev;
> > > + struct cdev chrdev;
> > > + raw_spinlock_t events_lock;
> > > + struct list_head events_list;
> > > + struct list_head next_events_list;
> > > + DECLARE_KFIFO(events, struct counter_event, 64);
> >
> > Do you plan to make the size of the FIFO configurable?
> >
> > regards,
> > Oleksij & Marc
>
> I suppose it wouldn't be a problem to make this a configurable setting;
> I think I can implement this via kfifo_alloc() and kfifo_free(). How
> would users control this -- maybe using a sysfs attribute?
Would make sense to do it from sysfs.
Jonathan
>
> William Breathitt Gray
>
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2021-01-21 18:35 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-22 20:29 [PATCH v6 0/5] Introduce the Counter character device interface William Breathitt Gray
2020-11-22 20:29 ` [PATCH v6 2/5] docs: counter: Update to reflect sysfs internalization William Breathitt Gray
2020-11-22 20:29 ` [PATCH v6 3/5] counter: Add character device interface William Breathitt Gray
2020-12-13 23:58 ` David Lechner
2020-12-25 17:30 ` William Breathitt Gray
2021-01-19 9:20 ` Oleksij Rempel
2021-01-21 8:03 ` William Breathitt Gray
2021-01-21 18:26 ` Jonathan Cameron
2020-11-22 20:29 ` [PATCH v6 4/5] docs: counter: Document " William Breathitt Gray
2020-11-22 20:29 ` [PATCH v6 5/5] counter: 104-quad-8: Add IRQ support for the ACCES 104-QUAD-8 William Breathitt Gray
[not found] ` <950660d49af7d12b09bc9d3b1db6f8ff74209c26.1606075915.git.vilhelm.gray@gmail.com>
2020-11-25 13:07 ` [PATCH v6 1/5] counter: Internalize sysfs interface code William Breathitt Gray
2020-12-13 23:15 ` David Lechner
2020-12-20 22:11 ` William Breathitt Gray
2020-12-21 15:26 ` David Lechner
2020-12-13 23:15 ` [PATCH v6 0/5] Introduce the Counter character device interface David Lechner
2020-12-20 21:44 ` William Breathitt Gray
2020-12-21 15:19 ` David Lechner
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).