linux-kernel - Re: [PATCH v12 2/3] 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: <437DA583-95FF-4ADF-9947-1F39D242E157@collabora.com>
Date: Fri, 25 Jul 2025 12:08:37 -0300
From: Daniel Almeida <daniel.almeida@...labora.com>
To: Michal Wilczynski <m.wilczynski@...sung.com>
Cc: Uwe Kleine-König <ukleinek@...nel.org>,
 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>,
 Drew Fustini <fustini@...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
Subject: Re: [PATCH v12 2/3] rust: pwm: Add Kconfig and basic data structures

Hi Michal,


Overall looks good, a few minor comments:

[…]

> +
> +/// Wrapper for board-dependent PWM arguments [`struct pwm_args`](srctree/include/linux/pwm.h).
> +#[repr(transparent)]
> +pub struct Args(Opaque<bindings::pwm_args>);
> +
> +impl Args {
> +    /// Creates an `Args` wrapper from a C struct pointer.
> +    ///
> +    /// # Safety
> +    ///
> +    /// The caller must ensure that `c_args_ptr` is a valid, non-null pointer
> +    /// to `bindings::pwm_args` and that the pointed-to data is valid
> +    /// for the duration of this function call (as data is copied).
> +    unsafe fn from_c_ptr(c_args_ptr: *const bindings::pwm_args) -> Self {
> +        // SAFETY: Caller guarantees `c_args_ptr` is valid. We dereference it to copy.
> +        Args(Opaque::new(unsafe { *c_args_ptr }))
> +    }

from_raw()


> +
> +    /// Returns the period of the PWM signal in nanoseconds.
> +    pub fn period(&self) -> u64 {
> +        // SAFETY: `self.0.get()` returns a pointer to the `bindings::pwm_args`
> +        // managed by the `Opaque` wrapper. This pointer is guaranteed to be
> +        // valid and aligned for the lifetime of `self` because `Opaque` owns a copy.
> +        unsafe { (*self.0.get()).period }
> +    }
> +
> +    /// Returns the polarity of the PWM signal.
> +    pub fn polarity(&self) -> Result<Polarity, Error> {
> +        // SAFETY: `self.0.get()` returns a pointer to the `bindings::pwm_args`
> +        // managed by the `Opaque` wrapper. This pointer is guaranteed to be
> +        // valid and aligned for the lifetime of `self`.
> +        let raw_polarity = unsafe { (*self.0.get()).polarity };
> +        Polarity::try_from(raw_polarity)
> +    }
> +}
> +
> +/// Wrapper for PWM state [`struct pwm_state`](srctree/include/linux/pwm.h).
> +#[repr(transparent)]
> +pub struct State(bindings::pwm_state);

No Opaque<T>?

> +
> +impl State {
> +    /// Creates a `State` wrapper by taking ownership of a C `pwm_state` value.
> +    pub(crate) fn from_c(c_state: bindings::pwm_state) -> Self {
> +        State(c_state)
> +    }
> +
> +    /// Returns `true` if the PWM signal is enabled.
> +    pub fn enabled(&self) -> bool {
> +        self.0.enabled
> +    }
> +}
> 
> -- 
> 2.34.1
> 
> 

If the lack of Opaque<T> is not a problem for whatever reason:

Reviewed-by: Daniel Almeida <daniel.almeida@...labora.com>

— Daniel