linux-kernel - Re: [PATCH v5 1/9] rust: pwm: Add Kconfig and basic data structures

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]

Message-ID: <ayp32pdwvpko3zuatgt2jgtfxgcmrmc5aujkx6twjchmyazpz7@yeo3kxgxnpda>
Date: Sun, 29 Jun 2025 11:23:28 +0200
From: Uwe Kleine-König <ukleinek@...nel.org>
To: Michal Wilczynski <m.wilczynski@...sung.com>
Cc: Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>, 
	Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>, 
	Björn Roy Baron <bjorn3_gh@...tonmail.com>, Andreas Hindborg <a.hindborg@...nel.org>, 
	Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>, 
	Danilo Krummrich <dakr@...nel.org>, Drew Fustini <drew@...7.com>, Guo Ren <guoren@...nel.org>, 
	Fu Wei <wefu@...hat.com>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Paul Walmsley <paul.walmsley@...ive.com>, Palmer Dabbelt <palmer@...belt.com>, 
	Albert Ou <aou@...s.berkeley.edu>, Alexandre Ghiti <alex@...ti.fr>, 
	Marek Szyprowski <m.szyprowski@...sung.com>, Benno Lossin <lossin@...nel.org>, 
	Michael Turquette <mturquette@...libre.com>, Stephen Boyd <sboyd@...nel.org>, linux-kernel@...r.kernel.org, 
	linux-pwm@...r.kernel.org, rust-for-linux@...r.kernel.org, linux-riscv@...ts.infradead.org, 
	devicetree@...r.kernel.org, linux-clk@...r.kernel.org
Subject: Re: [PATCH v5 1/9] rust: pwm: Add Kconfig and basic data structures

Hello Michal,

On Sat, Jun 28, 2025 at 04:38:15PM +0200, Michal Wilczynski wrote:
> On 6/27/25 17:10, Uwe Kleine-König wrote:
> > On Mon, Jun 23, 2025 at 08:08:49PM +0200, Michal Wilczynski wrote:
> >> +/// From C: `#define WFHWSIZE 20`
> >> +pub const WFHW_MAX_SIZE: usize = 20;
> > 
> > Can we somehow enforce that this doesn't diverge if the C define is
> > increased?
> 
> You are absolutely right. The hardcoded value is a maintenance risk. The
> #define is in core.c, so bindgen cannot see it.
> 
> I can address this by submitting a patch to move the #define WFHWSIZE to
> include/linux/pwm.h. This will make it part of the public API, allow
> bindgen to generate a binding for it, and ensure the Rust code can never
> diverge. Is this fine ?

I wonder if that is the opportunity to create a file
include/linux/pwm-provider.h. In that file we could collect all the bits
that are only relevant for drivers (pwm_ops, pwm_chip, pwmchip_parent,
pwmchip_alloc ...). (Some inline functions depend on some of these, so
some might have to stay in pwm.h)

I can address that in parallel, don't add this quest to your series. So
yes, move WFHWSIZE to include/linux/pwm.h (and rename it to PWM_WFHWSIZE
to not pollute the global namespace).
 
> > Please don't expose these non-atomic callbacks. pwm_disable() would be
> > fine.
> > 
> > Otherwise I'd prefer if pwm_set_waveform_might_sleep() is the API
> > exposed to/by Rust.
> 
> 
> OK, I'll remove all the setters from the State, while will keep the
> getters, as they would be useful in apply callbacks.

How so? They might be useful for consumers, but my preferred idiom for
them is that they know at each point in time what they want completely
and don't make that dependant on the previous setting.

> Will implement additional functions for Device i.e set_waveform,
> round_waveform and get_waveform, and the new enum to expose the result
> of the round_waveform more idiomatically.
> 
> /// Describes the outcome of a `round_waveform` operation.
> #[derive(Debug, Clone, Copy, PartialEq, Eq)]
> pub enum RoundingOutcome {
>     /// The requested waveform was achievable exactly or by rounding values down.
>     ExactOrRoundedDown,
> 
>     /// The requested waveform could only be achieved by rounding up.
>     RoundedUp,
> }

Sounds good. Hoever I have some doubts about the C semantic here, too.
Is it really helpful to provide that info? A user of that return value
has to check anyhow which parameter got rounded up. If you have an
opinion here, please share.
 
Best regards
Uwe

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