Re: [PATCH v7 3/5] counter: Add character device interface
On Wed, Dec 30, 2020 at 03:04:40PM +, Jonathan Cameron wrote:
> On Fri, 25 Dec 2020 19:15:36 -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
> > Cc: Gwendal Grignou
> > Cc: Dan Carpenter
> > Signed-off-by: William Breathitt Gray
>
> There are a few things in here that could profitably be pulled out as
> precursor
> patches. I don't really understand the connection of extension_name to the
> addition of a chardev for example. Might be needed to provide enough
> info to actually use the chardev, but does it have meaning without that?
> Either way, definitely feels like it can be done in a separate patch.
The extension_name attributes are needed so chrdev users have enough
info to identify which extension number corresponds to which extension.
I'll move this to change to a separate patch and provide an appropriate
explanation there to make things clearer.
> > +static long counter_chrdev_ioctl(struct file *filp, unsigned int cmd,
> > +unsigned long arg)
> > +{
> > + struct counter_device *const counter = filp->private_data;
> > + unsigned long flags;
> > + int err = 0;
> > +
> > + switch (cmd) {
> > + case COUNTER_CLEAR_WATCHES_IOCTL:
> > + return counter_clear_watches(counter);
> > + case COUNTER_ADD_WATCH_IOCTL:
> > + return counter_add_watch(counter, arg);
> > + case COUNTER_LOAD_WATCHES_IOCTL:
> > + raw_spin_lock_irqsave(>events_list_lock, flags);
> > +
> > + counter_events_list_free(>events_list);
> > + list_replace_init(>next_events_list,
> > + >events_list);
> > +
> > + if (counter->ops->events_configure)
> > + err = counter->ops->events_configure(counter);
> > +
> > + raw_spin_unlock_irqrestore(>events_list_lock, flags);
> > + break;
>
> return here.
Ack.
> > +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 = _node->comp;
> > + void *const parent = comp_node->parent;
> > + int err = 0;
> > + u8 value_u8 = 0;
> > + u32 value_u32 = 0;
> > +
> > + if (comp_node->component.type == COUNTER_COMPONENT_NONE)
> > + return 0;
> > +
> > + switch (comp->type) {
> > + case COUNTER_COMP_U8:
> > + case COUNTER_COMP_BOOL:
> > + switch (comp_node->component.scope) {
> > + case COUNTER_SCOPE_DEVICE:
> > + err = comp->device_u8_read(counter, _u8);
> > + break;
> > + case COUNTER_SCOPE_SIGNAL:
> > + err = comp->signal_u8_read(counter, parent, _u8);
> > + break;
> > + case COUNTER_SCOPE_COUNT:
> > + err = comp->count_u8_read(counter, parent, _u8);
> > + break;
> > + }
> > + *value = value_u8;
> > + break;
> > + 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_u32_read(counter, _u32);
> > + break;
> > + case COUNTER_SCOPE_SIGNAL:
> > + err = comp->signal_u32_read(counter, parent,
> > + _u32);
> > + break;
> > + case COUNTER_SCOPE_COUNT:
> > + err = comp->count_u32_read(counter, parent, _u32);
> > + break;
> > + }
> > + *value = value_u32;
>
> Seems like a return here would make more sense as no shared stuff to do at
> end of the switch. Same in other similar cases.
Ack.
> > + 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,
> > + _u32);
> > +
Re: [PATCH v7 3/5] counter: Add character device interface
On Thu, Jan 28, 2021 at 10:01:13AM +0100, Oleksij Rempel wrote:
> Hello William,
>
>
> On Fri, Dec 25, 2020 at 07:15:36PM -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
> > Cc: Gwendal Grignou
> > Cc: Dan Carpenter
> > Signed-off-by: William Breathitt Gray
> > ---
> ...
> > +struct counter_event {
> > + __aligned_u64 timestamp;
> > + __aligned_u64 value;
> > + struct counter_watch watch;
> > + __u8 errno;
>
> This variable clashed in user space, as soon as you include errno.h,
> with the libc's "magic" definition of errno. What about "err" instead.
> I'm not sure it an __u8 is the proper type, IIRC usually it's an int.
>
> Regards,
> Oleksij
> --
> 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- |
Sure, I can rename this to avoid a possible clash with libc's errno.
Maybe "status" would be more apt to indicate that this is an exit status
for the event -- the code returned may simply be a warning and not
necessarily a critical error.
Regarding the datatype for this value, I've opened up the discussion in
my reply to David Lechner [1], so perhaps we can continue it there.
[1] https://lkml.org/lkml/2021/1/30/5
William Breathitt Gray
signature.asc
Description: PGP signature
Re: [PATCH v7 3/5] counter: Add character device interface
On Wed, Dec 30, 2020 at 03:36:35PM -0600, David Lechner wrote:
> On 12/25/20 6:15 PM, William Breathitt Gray wrote:
>
> > diff --git a/include/uapi/linux/counter.h b/include/uapi/linux/counter.h
> > new file mode 100644
> > index ..7585dc9db19d
> > --- /dev/null
> > +++ b/include/uapi/linux/counter.h
> > @@ -0,0 +1,123 @@
> > +/* 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
> > +#include
> > +
> > +/* Component type definitions */
> > +enum counter_component_type {
> > + COUNTER_COMPONENT_NONE,
> > + COUNTER_COMPONENT_SIGNAL,
> > + COUNTER_COMPONENT_COUNT,
> > + COUNTER_COMPONENT_FUNCTION,
> > + COUNTER_COMPONENT_SYNAPSE_ACTION,
> > + COUNTER_COMPONENT_EXTENSION,
> > +};
> > +
> > +/* Component scope definitions */
> > +enum counter_scope {
>
> Do we need COUNTER_SCOPE_NONE to go with COUNTER_COMPONENT_NONE?
COUNTER_COMPONENT_NONE alone should be fine because it already indicates
that the 'component' member of the struct counter_watch is to be ignored
(i.e. type, scope, etc. will not be evaluated and that section of code
is bypassed).
> > + COUNTER_SCOPE_DEVICE,
> > + COUNTER_SCOPE_SIGNAL,
> > + COUNTER_SCOPE_COUNT,
> > +};
> > +
> > +/**
> > + * struct counter_component - Counter component identification
> > + * @type: component type (Count, extension, etc.)
>
> Instead of "Count, extension, etc.", it could be more helpful
> to say one of enum counter_component_type.
Ack.
> > + * @scope: component scope (Device, Count, or Signal)
>
> Same here. @scope must be one of enum counter_scope.
Ack.
> > + * @parent: parent component identification number
> > + * @id: component identification number
>
> It could be helpful to say that these id numbers match
> the X/Y/Z in the sysfs paths as described in the sysfs
> ABI.
Ack.
> > + */
> > +struct counter_component {
> > + __u8 type;
> > + __u8 scope;
> > + __u8 parent;
> > + __u8 id;
> > +};
> > +
> > +/* Event type definitions */
> > +enum counter_event_type {
> > + COUNTER_EVENT_OVERFLOW,
> > + COUNTER_EVENT_UNDERFLOW,
> > + COUNTER_EVENT_OVERFLOW_UNDERFLOW,
> > + COUNTER_EVENT_THRESHOLD,
> > + COUNTER_EVENT_INDEX,
> > +};
> > +
> > +/**
> > + * struct counter_watch - Counter component watch configuration
> > + * @component: component to watch when event triggers
> > + * @event: event that triggers
>
> It would be helpful to say that @event must be one of
> enum counter_event_type.
Ack.
> > + * @channel: event channel
>
> It would be useful to say that @channel should be 0 unless
> a component has more than one event of the same type.
I'll make this clearer.
> > + */
> > +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
> > + * @timestamp: best estimate of time of event occurrence, in nanoseconds
> > + * @value: component value
> > + * @watch: component watch configuration
> > + * @errno: system error number
> > + */
> > +struct counter_event {
> > + __aligned_u64 timestamp;
> > + __aligned_u64 value;
> > + struct counter_watch watch;
> > + __u8 errno;
>
> There are error codes larger than 255. Probably better
> make this __u32.
Are error codes larger than 255 actually useful in this case? I noticed
the exit() function will only return the least significant byte of
status to the parent: https://man7.org/linux/man-pages/man3/exit.3.html
William Breathitt Gray
signature.asc
Description: PGP signature
Re: [PATCH v7 3/5] counter: Add character device interface
Hello William,
On Fri, Dec 25, 2020 at 07:15:36PM -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
> Cc: Gwendal Grignou
> Cc: Dan Carpenter
> Signed-off-by: William Breathitt Gray
> ---
...
> +struct counter_event {
> + __aligned_u64 timestamp;
> + __aligned_u64 value;
> + struct counter_watch watch;
> + __u8 errno;
This variable clashed in user space, as soon as you include errno.h,
with the libc's "magic" definition of errno. What about "err" instead.
I'm not sure it an __u8 is the proper type, IIRC usually it's an int.
Regards,
Oleksij
--
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- |
Re: [PATCH v7 3/5] counter: Add character device interface
On 12/25/20 6:15 PM, William Breathitt Gray wrote:
diff --git a/include/uapi/linux/counter.h b/include/uapi/linux/counter.h
new file mode 100644
index ..7585dc9db19d
--- /dev/null
+++ b/include/uapi/linux/counter.h
@@ -0,0 +1,123 @@
+/* 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
+#include
+
+/* Component type definitions */
+enum counter_component_type {
+ COUNTER_COMPONENT_NONE,
+ COUNTER_COMPONENT_SIGNAL,
+ COUNTER_COMPONENT_COUNT,
+ COUNTER_COMPONENT_FUNCTION,
+ COUNTER_COMPONENT_SYNAPSE_ACTION,
+ COUNTER_COMPONENT_EXTENSION,
+};
+
+/* Component scope definitions */
+enum counter_scope {
Do we need COUNTER_SCOPE_NONE to go with COUNTER_COMPONENT_NONE?
+ COUNTER_SCOPE_DEVICE,
+ COUNTER_SCOPE_SIGNAL,
+ COUNTER_SCOPE_COUNT,
+};
+
+/**
+ * struct counter_component - Counter component identification
+ * @type: component type (Count, extension, etc.)
Instead of "Count, extension, etc.", it could be more helpful
to say one of enum counter_component_type.
+ * @scope: component scope (Device, Count, or Signal)
Same here. @scope must be one of enum counter_scope.
+ * @parent: parent component identification number
+ * @id: component identification number
It could be helpful to say that these id numbers match
the X/Y/Z in the sysfs paths as described in the sysfs
ABI.
+ */
+struct counter_component {
+ __u8 type;
+ __u8 scope;
+ __u8 parent;
+ __u8 id;
+};
+
+/* Event type definitions */
+enum counter_event_type {
+ COUNTER_EVENT_OVERFLOW,
+ COUNTER_EVENT_UNDERFLOW,
+ COUNTER_EVENT_OVERFLOW_UNDERFLOW,
+ COUNTER_EVENT_THRESHOLD,
+ COUNTER_EVENT_INDEX,
+};
+
+/**
+ * struct counter_watch - Counter component watch configuration
+ * @component: component to watch when event triggers
+ * @event: event that triggers
It would be helpful to say that @event must be one of
enum counter_event_type.
+ * @channel: event channel
It would be useful to say that @channel should be 0 unless
a component has more than one event of the same type.
+ */
+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
+ * @timestamp: best estimate of time of event occurrence, in nanoseconds
+ * @value: component value
+ * @watch: component watch configuration
+ * @errno: system error number
+ */
+struct counter_event {
+ __aligned_u64 timestamp;
+ __aligned_u64 value;
+ struct counter_watch watch;
+ __u8 errno;
There are error codes larger than 255. Probably better
make this __u32.
+};
+
+/* Count direction values */
+enum counter_count_direction {
+ COUNTER_COUNT_DIRECTION_FORWARD,
+ COUNTER_COUNT_DIRECTION_BACKWARD,
+};
+
+/* Count mode values */
+enum counter_count_mode {
+ COUNTER_COUNT_MODE_NORMAL,
+ COUNTER_COUNT_MODE_RANGE_LIMIT,
+ COUNTER_COUNT_MODE_NON_RECYCLE,
+ COUNTER_COUNT_MODE_MODULO_N,
+};
+
+/* Count function values */
+enum counter_function {
+ COUNTER_FUNCTION_INCREASE,
+ COUNTER_FUNCTION_DECREASE,
+ COUNTER_FUNCTION_PULSE_DIRECTION,
+ COUNTER_FUNCTION_QUADRATURE_X1_A,
+ COUNTER_FUNCTION_QUADRATURE_X1_B,
+ COUNTER_FUNCTION_QUADRATURE_X2_A,
+ COUNTER_FUNCTION_QUADRATURE_X2_B,
+ COUNTER_FUNCTION_QUADRATURE_X4,
+};
+
+/* Signal values */
+enum counter_signal_level {
+ COUNTER_SIGNAL_LEVEL_LOW,
+ COUNTER_SIGNAL_LEVEL_HIGH,
+};
+
+/* Action mode values */
+enum counter_synapse_action {
+ COUNTER_SYNAPSE_ACTION_NONE,
+ COUNTER_SYNAPSE_ACTION_RISING_EDGE,
+ COUNTER_SYNAPSE_ACTION_FALLING_EDGE,
+ COUNTER_SYNAPSE_ACTION_BOTH_EDGES,
+};
+
+#endif /* _UAPI_COUNTER_H_ */
Re: [PATCH v7 3/5] counter: Add character device interface
On Fri, 25 Dec 2020 19:15:36 -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
> Cc: Gwendal Grignou
> Cc: Dan Carpenter
> Signed-off-by: William Breathitt Gray
There are a few things in here that could profitably be pulled out as precursor
patches. I don't really understand the connection of extension_name to the
addition of a chardev for example. Might be needed to provide enough
info to actually use the chardev, but does it have meaning without that?
Either way, definitely feels like it can be done in a separate patch.
> ---
> MAINTAINERS | 1 +
> drivers/counter/Makefile | 2 +-
> drivers/counter/counter-chrdev.c | 490 +++
> drivers/counter/counter-chrdev.h | 16 +
> drivers/counter/counter-core.c | 37 ++-
> drivers/counter/counter-sysfs.c | 51 +++-
> include/linux/counter.h | 87 +++---
> include/uapi/linux/counter.h | 123
> 8 files changed, 751 insertions(+), 56 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 b64fa49d5796..3a240f70bdd4 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -4494,6 +4494,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
> 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 ..61b11989546a
> --- /dev/null
> +++ b/drivers/counter/counter-chrdev.c
> @@ -0,0 +1,490 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Generic Counter character device interface
> + * Copyright (C) 2020 William Breathitt Gray
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#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 int copied;
> +
> + if (len < sizeof(struct counter_event))
> + return -EINVAL;
> +
> + do {
> + if (kfifo_is_empty(>events)) {
> + if (filp->f_flags & O_NONBLOCK)
> + return -EAGAIN;
> +
> + err = wait_event_interruptible(counter->events_wait,
> + !kfifo_is_empty(>events));
> + if (err < 0)
> + return err;
> + }
> +
> + if (mutex_lock_interruptible(>events_lock))
> + return -ERESTARTSYS;
> + err = kfifo_to_user(>events, buf, len, );
> + mutex_unlock(>events_lock);
> + if (err < 0)
> + 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, >events_wait, pollt);
> +
> + if (!kfifo_is_empty(>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 */
>
