linux-kernel - Re: [PATCH 0/4] Introduce the Counter 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]

Date:   Thu, 30 Apr 2020 22:13:45 +0200
From:   Alexandre Belloni <alexandre.belloni@...tlin.com>
To:     William Breathitt Gray <vilhelm.gray@...il.com>
Cc:     jic23@...nel.org, kamel.bouhara@...tlin.com, gwendal@...omium.org,
        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

Hi,

On 29/04/2020 14:11:34-0400, William Breathitt Gray 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.
> 

On that topic, I'm wondering why the counter subsystem uses /sys/bus
instead of /sys/class that would be more natural for a class of devices.
I can't see how counters would be considered busses. I think you should
consider moving it over to /sys/class (even if deprecating
/sys/bus/counter will be long).

> 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.
> 

I agree with David that you should consider using read to retrieve the
counter data as this will simplify interrupt handling/polling and
blocking/non-blocking reads can be used by an application. ABI wise,
this can also be a good move as you could always consider having an
ioctl requesting a specific format when reading the device so you are
not stuck with the initial format you are going to choose.

> 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?
> 

You should use a long if you ever have to return a point as it is
guaranteed to have the correct size. Else, just stick to an int if you
are not going to overflow it.

> 3. I only implemented the unlocked_ioctl callback. Should I implement a
>    compat_ioctl callback as well?
> 

The compat_ioctl is to handle 32bit userspace running on a 64bit kernel.
If your structures have the same size in both cases, then you don't have
to implement compat_ioctl.

Have a look at Documentation/driver-api/ioctl.rst


-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com