linux-kernel - Re: [PATCH v7 3/5] counter: Add character device interface

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <e9102ed7-d4e1-0c81-96f3-8d3c297d037f@lechnology.com>
Date:   Wed, 30 Dec 2020 15:36:35 -0600
From:   David Lechner <david@...hnology.com>
To:     William Breathitt Gray <vilhelm.gray@...il.com>, jic23@...nel.org
Cc:     kernel@...gutronix.de, linux-stm32@...md-mailman.stormreply.com,
        a.fatoum@...gutronix.de, kamel.bouhara@...tlin.com,
        gwendal@...omium.org, alexandre.belloni@...tlin.com,
        linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, syednwaris@...il.com,
        patrick.havelange@...ensium.com, fabrice.gasnier@...com,
        mcoquelin.stm32@...il.com, alexandre.torgue@...com,
        Dan Carpenter <dan.carpenter@...cle.com>
Subject: 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 000000000000..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 <linux/ioctl.h>
> +#include <linux/types.h>
> +
> +/* 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_ */
>