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

Date: Sat, 13 Apr 2024 11:22:33 +0200
From: Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
To: linux-pwm@...r.kernel.org
Cc: 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>, 
	David Lechner <dlechner@...libre.com>
Subject: Re: [PATCH 0/8] pwm: Add support for character devices

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

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)