| 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
| ||
|
Message-ID: <20200809144800.6b067dea@archlinux>
Date: Sun, 9 Aug 2020 14:48:00 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: William Breathitt Gray <vilhelm.gray@...il.com>
Cc: kamel.bouhara@...tlin.com, gwendal@...omium.org,
alexandre.belloni@...tlin.com, david@...hnology.com,
linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-stm32@...md-mailman.stormreply.com,
linux-arm-kernel@...ts.infradead.org, syednwaris@...il.com,
patrick.havelange@...ensium.com, fabrice.gasnier@...com,
mcoquelin.stm32@...il.com, alexandre.torgue@...com
Subject: Re: [PATCH v4 0/5] Introduce the Counter character device interface
On Tue, 21 Jul 2020 15:35:46 -0400
William Breathitt Gray <vilhelm.gray@...il.com> wrote:
> Changes in v4:
> - Reimplement character device interface to report Counter events
> - Implement Counter timestamps
> - Implement poll() support
> - Convert microchip-tcb-capture.c to new driver interface
> - Add IRQ support for the 104-quad-8 Counter driver
>
> Over the past couple years we have noticed some shortcomings with the
> Counter sysfs interface. Although useful in the majority of situations,
> there are certain use-cases where interacting through sysfs attributes
> can become cumbersome and inefficient. A desire to support more advanced
> functionality such as timestamps, multi-axes positioning tables, and
> other such latency-sensitive applications, has motivated a reevaluation
> of the Counter subsystem. I believe a character device interface will be
> helpful for this more niche area of counter device use.
>
> To quell any concerns from the offset: this patchset makes no changes to
> the existing Counter sysfs userspace interface -- existing userspace
> applications will continue to work with no modifications necessary. I
> request that driver maintainers please test their applications to verify
> that this is true, and report any discrepancies if they arise.
>
> However, this patchset does contain a major reimplementation of the
> Counter subsystem core and driver API. A reimplementation was necessary
> in order to separate the sysfs code from the counter device drivers and
> internalize it as a dedicated component of the core Counter subsystem
> module. A minor benefit from all of this is that the sysfs interface is
> now ensured a certain amount of consistency because the translation is
> performed outside of individual counter device drivers.
>
> Essentially, the reimplementation has enabled counter device drivers to
> pass and handle data as native C datatypes now rather than the sysfs
> strings from before. A high-level view of how a count value is passed
> down from a counter device driver can be exemplified by the following:
>
> ----------------------
> / Counter device \
> +----------------------+
> | Count register: 0x28 |
> +----------------------+
> |
> -----------------
> / raw count data /
> -----------------
> |
> V
> +----------------------------+
> | Counter device driver |----------+
> +----------------------------+ |
> | Processes data from device | -------------------
> |----------------------------| / driver callbacks /
> | Type: u64 | -------------------
> | Value: 42 | |
> +----------------------------+ |
> | |
> ---------- |
> / u64 / |
> ---------- |
> | |
> | V
> | +----------------------+
> | | Counter core |
> | +----------------------+
> | | Routes device driver |
> | | callbacks to the |
> | | userspace interfaces |
> | +----------------------+
> | |
> | -------------------
> | / driver callbacks /
> | -------------------
> | |
> +-------+---------------+ |
> | | |
> | +-------|-------+
> | | |
> V | V
> +--------------------+ | +---------------------+
> | Counter sysfs |<-+->| Counter chrdev |
> +--------------------+ +---------------------+
> | Translates to the | | Translates to the |
> | standard Counter | | standard Counter |
> | sysfs output | | character device |
> |--------------------| |---------------------+
> | Type: const char * | | Type: u64 |
> | Value: "42" | | Value: 42 |
> +--------------------+ +---------------------+
> | |
> --------------- -----------------------
> / const char * / / struct counter_event /
> --------------- -----------------------
> | |
> | V
> | +-----------+
> | | read |
> | +-----------+
> | \ Count: 42 /
> | -----------
> |
> V
> +--------------------------------------------------+
> | `/sys/bus/counter/devices/counterX/countY/count` |
> +--------------------------------------------------+
> \ Count: "42" /
> --------------------------------------------------
>
> Counter device data is exposed through standard character device read
> operations. Device data is gathered when a Counter event is pushed by
> the respective Counter device driver. Configuration is handled via ioctl
> operations on the respective Counter character device node.
>
> The following are some questions I have about this patchset:
>
> 1. Should I support multiple file descriptors for the character device
> in this introduction patchset?
>
> I intend to add support for multiple file descriptors to the Counter
> character device, but I restricted this patchset to a single file
> descriptor to simplify the code logic for the sake of review. If
> there is enough interest, I can add support for multiple file
> descriptors in the next revision; I anticipate that this should be
> simple to implement through the allocation of a kfifo for each file
> descriptor during the open callback.
What is the use case? I can conjecture one easily enough, but I'm not
sure how real it actually is. We've been around this question a few
times in IIO :)
Certainly makes sense to design an interface that would allow you to
add this support later if needed though.
>
> 2. Should struct counter_event have a union for different value types,
> or just a value u8 array?
>
> Currently I expose the event data value via a union containing the
> various possible Counter data types (value_u8 and value_u64). It is
> up to the user to select the right union member for the data they
> received. Would it make sense to return this data in a u8 array
> instead, with the expectation that the user will cast to the
> necessary data type?
Be careful on alignment if you do that. We would need to ensure that the
buffer is suitable aligned for a cast to work as expected.
>
> 3. How should errors be returned for Counter data reads performed by
> Counter events?
>
> Counter events are configured with a list of Counter data read
> operations to perform for the user. Any one of those data reads can
> return an error code, but not necessarily all of them. Currently, the
> code exits early when an error code is returned. Should the code
> instead continue on, saving the error code to the struct
> counter_event for userspace to handle?
I'd argue that errors are expected to be rare, so it isn't a problem
to just fault out hard on the first one.
>
> William Breathitt Gray (5):
> counter: Internalize sysfs interface code
> docs: counter: Update to reflect sysfs internalization
> counter: Add character device interface
> docs: counter: Document character device interface
> counter: 104-quad-8: Add IRQ support for the ACCES 104-QUAD-8
>
> .../ABI/testing/sysfs-bus-counter-104-quad-8 | 32 +
> Documentation/driver-api/generic-counter.rst | 363 +++-
> .../userspace-api/ioctl/ioctl-number.rst | 1 +
> MAINTAINERS | 2 +-
> drivers/counter/104-quad-8.c | 753 +++++----
> drivers/counter/Kconfig | 6 +-
> drivers/counter/Makefile | 1 +
> drivers/counter/counter-chrdev.c | 441 +++++
> drivers/counter/counter-chrdev.h | 16 +
> drivers/counter/counter-core.c | 188 +++
> drivers/counter/counter-sysfs.c | 849 ++++++++++
> drivers/counter/counter-sysfs.h | 14 +
> drivers/counter/counter.c | 1496 -----------------
> drivers/counter/ftm-quaddec.c | 59 +-
> drivers/counter/microchip-tcb-capture.c | 104 +-
> drivers/counter/stm32-lptimer-cnt.c | 161 +-
> drivers/counter/stm32-timer-cnt.c | 139 +-
> drivers/counter/ti-eqep.c | 211 +--
> include/linux/counter.h | 633 +++----
> include/linux/counter_enum.h | 45 -
> include/uapi/linux/counter.h | 90 +
> 21 files changed, 2919 insertions(+), 2685 deletions(-)
> create mode 100644 drivers/counter/counter-chrdev.c
> create mode 100644 drivers/counter/counter-chrdev.h
> create mode 100644 drivers/counter/counter-core.c
> create mode 100644 drivers/counter/counter-sysfs.c
> create mode 100644 drivers/counter/counter-sysfs.h
> delete mode 100644 drivers/counter/counter.c
> delete mode 100644 include/linux/counter_enum.h
> create mode 100644 include/uapi/linux/counter.h
>
Powered by blists - more mailing lists