| 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: <20200503151314.2ac1fc2e@archlinux>
Date: Sun, 3 May 2020 15:13:14 +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,
felipe.balbi@...ux.intel.com, fabien.lahoudere@...labora.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 0/4] Introduce the Counter character device interface
On Wed, 29 Apr 2020 14:11:34 -0400
William Breathitt Gray <vilhelm.gray@...il.com> wrote:
> 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-axis 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: unsigned long | -------------------
> | Value: 42 | |
> +----------------------------+ |
> | |
> ---------------- |
> / unsigned long / |
> ---------------- |
> | |
> | 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: unsigned long |
> | Value: "42" | | Value: 42 |
> +--------------------+ +---------------------+
> | |
> --------------- ----------------
> / const char * / / unsigned long /
> --------------- ----------------
> | |
> | V
> | +-----------+
> | | ioctl |
> | +-----------+
> | \ Count: 42 /
> | -----------
> |
> V
> +--------------------------------------------------+
> | `/sys/bus/counter/devices/counterX/countY/count` |
> +--------------------------------------------------+
> \ Count: "42" /
> --------------------------------------------------
>
> I am aware that an in-kernel API can simplify the data transfer between
> counter device drivers and the userspace interfaces, but I want to
> postpone that development until after the new Counter character device
> ioctl commands are solidified. A userspace ABI is effectively immutable
> so I want to make sure we get that right before working on an in-kernel
> API that is more flexible to change. However, when we do develop an
> in-kernel API, it will likely be housed as part of the Counter core
> component, through which the userspace interfaces will then communicate.
>
> Interaction with Counter character devices occurs via ioctl commands.
> This allows userspace applications to access and set counter data using
> native C datatypes rather than working through string translations.
>
> Regarding the organization of this patchset, I have combined the counter
> device driver changes with the first patch because the changes must all
> be taken together in order to avoid compilation errors. I can see how
> this can end up making it difficult to review so many changes at once,
> so alternatively I can separate out the counter device driver changes
> into their own dedicated patches -- with the understanding that the
> patches must all be taken together.
>
> In addition, I anticipate the Microchip TCB capture counter driver to
> break with this patchset. I'm not sure how that driver will be picked
> up yet so I have avoided adding it to this patchset right now. But the
> changes to support that driver are simple to make so I can add them in a
> later revision of this patchset.
>
> The following are some questions I have about this patchset:
>
> 1. Should enums be used to represent standard counter component states
> (e.g. COUNTER_SIGNAL_LOW), or would these be better defined as int?
>
> These standard counter component states are defined in the
> counter-types.h file and serve as constants used by counter device
> drivers and Counter subsystem components in order to ensure a
> consistent interface.
>
> My concern is whether enum constants will cause problems when passed
> to userspace via the Counter character device ioctl calls. Along the
> same lines is whether the C bool datatype is safe to pass as well,
> given that it is a more modern C datatype.
For enums, I'd pass them as integers.
Bool is probably fine either way.
>
> 2. Should device driver callbacks return int or long? I sometimes see
> error values returned as long (e.g. PTR_ERR(), the file_operations
> structure's ioctl callbacks, etc.); when is it necessary to return
> long as opposed to int?
In my view it doesn't really matter that much. For PTR_ERR it has to be
a long because a long is always the same length as a pointer, but an int
'might' not be. However PTR_ERR returns a value that always fits in an
integer anyway.
https://www.oreilly.com/library/view/linux-device-drivers/0596005903/ch11.html
Coding style in linux mostly use int for return values that might indicate
an error.
>
> 3. I only implemented the unlocked_ioctl callback. Should I implement a
> compat_ioctl callback as well?
Depends if you need to deal with the 32bit userspace on 64 bit kernel corner
cases. Looks like you only pass a pointer, in which case I think you
can just use the ioctl_compat_ptr callback to handle it for you.
>
> 4. How much space should allot for name strings? Name strings hold the
> names of components (ideally as they appear on datasheets), so I've
> arbitrarily chosen a size of 32 for the character device interface.
>
> 5. Should the owning component of an extension be determined by the
> device driver or Counter subsystem?
>
> A lot of the complexity in the counters-function-types.h file and the
> sysfs-callbacks.c file is due to the function pointer casts required
> in order to support three different ownership scenarios: the owning
> component is the device, the owning component is a Count, the owning
> component is a Signal.
>
> Because the Counter subsystem doesn't not know which scenario is
> valid, it must manually check and provide for all three ownership
> cases. On the other hand, device drivers do know exactly which case
> applies because they are the ones providing the callbacks.
>
> The complexity in the Counter subsystem code can be eliminated if the
> owning component is simply passed down to the callbacks as a void
> pointer. The device drivers will then be responsible for casting to
> the appropriate component type, but this should in theory not be a
> problem since the device driver assigned the callback to the owning
> component in the first place.
>
> William Breathitt Gray (4):
> counter: Internalize sysfs interface code
> docs: counter: Update to reflect sysfs internalization
> counter: Add character device interface
> docs: counter: Document character device interface
>
> Documentation/driver-api/generic-counter.rst | 259 ++-
> .../userspace-api/ioctl/ioctl-number.rst | 1 +
> MAINTAINERS | 3 +-
> drivers/counter/104-quad-8.c | 437 +++--
> drivers/counter/Makefile | 2 +
> drivers/counter/counter-chrdev.c | 1134 +++++++++++++
> drivers/counter/counter-chrdev.h | 16 +
> drivers/counter/counter-core.c | 220 +++
> drivers/counter/counter-function-types.h | 81 +
> drivers/counter/counter-strings.h | 46 +
> drivers/counter/counter-sysfs-callbacks.c | 566 +++++++
> drivers/counter/counter-sysfs-callbacks.h | 28 +
> drivers/counter/counter-sysfs.c | 524 ++++++
> drivers/counter/counter-sysfs.h | 14 +
> drivers/counter/counter.c | 1496 -----------------
> drivers/counter/ftm-quaddec.c | 46 +-
> drivers/counter/stm32-lptimer-cnt.c | 159 +-
> drivers/counter/stm32-timer-cnt.c | 132 +-
> drivers/counter/ti-eqep.c | 170 +-
> include/linux/counter.h | 547 +++---
> include/linux/counter_enum.h | 45 -
> include/uapi/linux/counter-types.h | 67 +
> include/uapi/linux/counter.h | 313 ++++
> 23 files changed, 3816 insertions(+), 2490 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-function-types.h
> create mode 100644 drivers/counter/counter-strings.h
> create mode 100644 drivers/counter/counter-sysfs-callbacks.c
> create mode 100644 drivers/counter/counter-sysfs-callbacks.h
> 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-types.h
> create mode 100644 include/uapi/linux/counter.h
>
>
> base-commit: 00edef1ac058b3c754d29bcfd35ea998d9e7a339
Powered by blists - more mailing lists