linux-hardening - Re: [PATCH 0/8] pwm: Add support for character devices

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] [day] [month] [year] [list]

Date: Mon, 22 Apr 2024 13:59:51 -0500
From: David Lechner <dlechner@...libre.com>
To: Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
Cc: linux-pwm@...r.kernel.org, linux-hardening@...r.kernel.org, 
	Kees Cook <keescook@...omium.org>, kernel@...gutronix.de, 
	"Gustavo A. R. Silva" <gustavoars@...nel.org>, John Ernberg <john.ernberg@...ia.se>, 
	Thorsten Scherer <T.Scherer@...elmann.de>, Marek Szyprowski <m.szyprowski@...sung.com>, 
	Trevor Gamblin <tgamblin@...libre.com>
Subject: Re: [PATCH 0/8] pwm: Add support for character devices

On Sat, Apr 13, 2024 at 4:22 AM Uwe Kleine-König
<u.kleine-koenig@...gutronix.de> wrote:
>
> Hello,
>
> On Sun, Mar 17, 2024 at 11:40:31AM +0100, Uwe Kleine-König wrote:
> > After the necessary changes to the lowlevel drivers got in for v6.9-rc1
> > here come some changes to the core to implement /dev/pwmchipX character
> > devices.
> >
> > In my tests on an ARM STM32MP1 programming a PWM using the character
> > device is ~4 times faster than just changing duty_cycle via the sysfs
> > API. It also has the advantage that (similar to pwm_apply_*) the target
> > state is provided to the kernel with a single call, instead of having to
> > program the individual settings one after another via sysfs (in the
> > right order to not cross states not supported by the driver).
> >
> > Note the representation of a PWM waveform is different here compared to
> > the in-kernel representation. A PWM waveform is represented using:
> >
> >       period
> >       duty_cycle
> >       duty_offset
> >
> > A disabled PWM is represented by period = 0. For an inversed wave use:
> >
> >       duty_offset = duty_cycle
> >       duty_cycle = period - duty_cycle;
> >
> > . However there are some difficulties yet that make it hard to provide a
> > consistent API to userspace and so for now duty_offset isn't (fully)
> > supported yet. That needs some more consideration and can be added
> > later.
> >
> > A userspace lib together with some simple test programs making use of
> > this new API can be found at
> >
> >       https://git.kernel.org/pub/scm/linux/kernel/git/ukleinek/libpwm.git
> >
> > .
> >
> > The start of the series is some cleanup and preparation. The lifetime
> > and locking patches are needed to not crash the kernel when a character
> > device is open while a lowlevel driver goes away.
>
> This series is already in next for some time, but I'm not sure that I
> want to really send it to Linus in the next merge window as there are a
> few issues with it:
>
>  - A (false positive) lockdep warning reported by Marek Szyprowski.
>    See https://lore.kernel.org/all/5a49cadd-21b7-4384-9e7d-9105ccc288b3@samsung.com
>
>  - A speculation warning flagged by smatch that I don't understand
>    completely yet (and failed to attract attention by people that know
>    more about about it)
>    See https://lore.kernel.org/all/1e3dc81d-dcd4-4b04-85b1-23937e2f0acd@moroto.mountain
>
>  - I'm a bit unhappy about the rounding behaviour. Actually I'd like to
>    only provide userspace access via the character device to drivers
>    that adhere to the rounding rules for new drivers (that is: First
>    pick the maximal period that isn't bigger than the requested period.
>    Then for the chosen period pick the maximal duty_cycle that isn't
>    bigger than the requested one) to give a consistent behaviour. This
>    is further complicated by the fact that the character device exposes
>    a more flexible API (involving a duty_offset instead of polarity) and
>    the natural extension for the rounding rules with duty_offset is
>    different than for inverted polarity configurations.
>
> I currently consider introducing a new callback that in the long run
> should replace .apply() and that properly implements the duty_offset
> stuff. Then the character device could only be provided for the drivers
> implementing .apply2().
>
> I'm open for feedback, e.g. suggestions for a better name for .apply2().
>

Waiting to merge this and giving this some more thought first does
seem like a wise idea.