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: <n5zfbzu3hn7kqdf3xc7orpeovvdprc2xlf7w3f62uoohkxdk5c@cc24urt5xf36>
Date: Sun, 29 Jun 2025 12:29:13 +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

On Sat, Jun 28, 2025 at 09:47:19PM +0200, Michal Wilczynski wrote:
> 
> 
> On 6/28/25 16:38, 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:
> >>> Introduce the foundational support for PWM abstractions in Rust.
> >>>
> >>> This commit adds the `RUST_PWM_ABSTRACTIONS` Kconfig option to enable
> >>> the feature, along with the necessary build-system support and C
> >>> helpers.
> >>>
> >>> It also introduces the first set of safe wrappers for the PWM
> >>> subsystem, covering the basic data carrying C structs and enums:
> >>> - `Polarity`: A safe wrapper for `enum pwm_polarity`.
> >>> - `Waveform`: A wrapper for `struct pwm_waveform`.
> >>> - `Args`: A wrapper for `struct pwm_args`.
> >>> - `State`: A wrapper for `struct pwm_state`.
> >>>
> >>> These types provide memory safe, idiomatic Rust representations of the
> >>> core PWM data structures and form the building blocks for the
> >>> abstractions that will follow.
> >>>
> >>> Signed-off-by: Michal Wilczynski <m.wilczynski@...sung.com>
> >>> ---
> >>>  MAINTAINERS                     |   6 ++
> >>>  drivers/pwm/Kconfig             |  13 +++
> >>>  rust/bindings/bindings_helper.h |   1 +
> >>>  rust/helpers/helpers.c          |   1 +
> >>>  rust/helpers/pwm.c              |  20 ++++
> >>>  rust/kernel/lib.rs              |   2 +
> >>>  rust/kernel/pwm.rs              | 198 ++++++++++++++++++++++++++++++++++++++++
> >>>  7 files changed, 241 insertions(+)
> >>>
> >>> diff --git a/MAINTAINERS b/MAINTAINERS
> >>> index 0c1d245bf7b84f8a78b811e0c9c5a3edc09edc22..a575622454a2ef57ce055c8a8c4765fa4fddc490 100644
> >>> --- a/MAINTAINERS
> >>> +++ b/MAINTAINERS
> >>> @@ -20073,6 +20073,12 @@ F:	include/linux/pwm.h
> >>>  F:	include/linux/pwm_backlight.h
> >>>  K:	pwm_(config|apply_might_sleep|apply_atomic|ops)
> >>>  
> >>> +PWM SUBSYSTEM BINDINGS [RUST]
> >>> +M:	Michal Wilczynski <m.wilczynski@...sung.com>
> >>> +S:	Maintained
> >>> +F:	rust/helpers/pwm.c
> >>> +F:	rust/kernel/pwm.rs
> >>> +
> >>>  PXA GPIO DRIVER
> >>>  M:	Robert Jarzmik <robert.jarzmik@...e.fr>
> >>>  L:	linux-gpio@...r.kernel.org
> >>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> >>> index d9bcd1e8413eaed1602d6686873e263767c58f5f..cfddeae0eab3523f04f361fb41ccd1345c0c937b 100644
> >>> --- a/drivers/pwm/Kconfig
> >>> +++ b/drivers/pwm/Kconfig
> >>> @@ -790,4 +790,17 @@ config PWM_XILINX
> >>>  	  To compile this driver as a module, choose M here: the module
> >>>  	  will be called pwm-xilinx.
> >>>  
> >>> + config RUST_PWM_ABSTRACTIONS
> >>> +	bool "Rust PWM abstractions support"
> >>> +	depends on RUST
> >>> +	depends on PWM=y
> >>
> >> Currently CONFIG_PWM is a bool, so it cannot be =m. But I considered
> >> making PWM a tristate variable. How would that interfere with Rust
> >> support?
> >>
> >>> +	help
> >>> +	  This option enables the safe Rust abstraction layer for the PWM
> >>> +	  subsystem. It provides idiomatic wrappers and traits necessary for
> >>> +	  writing PWM controller drivers in Rust.
> >>> +
> >>> +	  The abstractions handle resource management (like memory and reference
> >>> +	  counting) and provide safe interfaces to the underlying C core,
> >>> +	  allowing driver logic to be written in safe Rust.
> >>> +
> >>>  endif
> >>> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> >>> index 693cdd01f9290fa01375cf78cac0e5a90df74c6c..6fe7dd529577952bf7adb4fe0526b0d5fbd6f3bd 100644
> >>> --- a/rust/bindings/bindings_helper.h
> >>> +++ b/rust/bindings/bindings_helper.h
> >>> @@ -64,6 +64,7 @@
> >>>  #include <linux/pm_opp.h>
> >>>  #include <linux/poll.h>
> >>>  #include <linux/property.h>
> >>> +#include <linux/pwm.h>
> >>>  #include <linux/refcount.h>
> >>>  #include <linux/sched.h>
> >>>  #include <linux/security.h>
> >>> diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
> >>> index 16fa9bca5949b85e8d4cdcfe8e6886124f72d8d8..60879e6d794ce0f87e39caafc5495bf5e8acf8f0 100644
> >>> --- a/rust/helpers/helpers.c
> >>> +++ b/rust/helpers/helpers.c
> >>> @@ -31,6 +31,7 @@
> >>>  #include "platform.c"
> >>>  #include "pci.c"
> >>>  #include "pid_namespace.c"
> >>> +#include "pwm.c"
> >>>  #include "rbtree.c"
> >>>  #include "rcu.c"
> >>>  #include "refcount.c"
> >>> diff --git a/rust/helpers/pwm.c b/rust/helpers/pwm.c
> >>> new file mode 100644
> >>> index 0000000000000000000000000000000000000000..d75c588863685d3990b525bb1b84aa4bc35ac397
> >>> --- /dev/null
> >>> +++ b/rust/helpers/pwm.c
> >>> @@ -0,0 +1,20 @@
> >>> +// SPDX-License-Identifier: GPL-2.0
> >>> +// Copyright (c) 2025 Samsung Electronics Co., Ltd.
> >>> +// Author: Michal Wilczynski <m.wilczynski@...sung.com>
> >>> +
> >>> +#include <linux/pwm.h>
> >>> +
> >>> +struct device *rust_helper_pwmchip_parent(const struct pwm_chip *chip)
> >>> +{
> >>> +	return pwmchip_parent(chip);
> >>> +}
> >>> +
> >>> +void *rust_helper_pwmchip_get_drvdata(struct pwm_chip *chip)
> >>> +{
> >>> +	return pwmchip_get_drvdata(chip);
> >>> +}
> >>> +
> >>> +void rust_helper_pwmchip_set_drvdata(struct pwm_chip *chip, void *data)
> >>> +{
> >>> +	pwmchip_set_drvdata(chip, data);
> >>> +}
> >>> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> >>> index 6b4774b2b1c37f4da1866e993be6230bc6715841..ce1d08b14e456905dbe7b625bbb8ca8b08deae2a 100644
> >>> --- a/rust/kernel/lib.rs
> >>> +++ b/rust/kernel/lib.rs
> >>> @@ -105,6 +105,8 @@
> >>>  pub mod seq_file;
> >>>  pub mod sizes;
> >>>  mod static_assert;
> >>> +#[cfg(CONFIG_RUST_PWM_ABSTRACTIONS)]
> >>> +pub mod pwm;
> >>>  #[doc(hidden)]
> >>>  pub mod std_vendor;
> >>>  pub mod str;
> >>> diff --git a/rust/kernel/pwm.rs b/rust/kernel/pwm.rs
> >>> new file mode 100644
> >>> index 0000000000000000000000000000000000000000..ed681b228c414e7ae8bf80ca649ad497c9dc4ec3
> >>> --- /dev/null
> >>> +++ b/rust/kernel/pwm.rs
> >>> @@ -0,0 +1,198 @@
> >>> +// SPDX-License-Identifier: GPL-2.0
> >>> +// Copyright (c) 2025 Samsung Electronics Co., Ltd.
> >>> +// Author: Michal Wilczynski <m.wilczynski@...sung.com>
> >>> +
> >>> +//! PWM subsystem abstractions.
> >>> +//!
> >>> +//! C header: [`include/linux/pwm.h`](srctree/include/linux/pwm.h).
> >>> +
> >>> +use crate::{
> >>> +    bindings,
> >>> +    prelude::*,
> >>> +    types::Opaque,
> >>> +};
> >>> +use core::convert::TryFrom;
> >>> +
> >>> +/// Maximum size for the hardware-specific waveform representation buffer.
> >>> +///
> >>> +/// 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 ?
> > 
> >>
> >>> +/// PWM polarity. Mirrors [`enum pwm_polarity`](srctree/include/linux/pwm.h).
> >>> +#[derive(Copy, Clone, Debug, PartialEq, Eq)]
> >>> +pub enum Polarity {
> >>> +    /// Normal polarity (duty cycle defines the high period of the signal).
> >>> +    Normal,
> >>> +
> >>> +    /// Inversed polarity (duty cycle defines the low period of the signal).
> >>> +    Inversed,
> >>> +}
> >>> +
> >>> +impl TryFrom<bindings::pwm_polarity> for Polarity {
> >>> +    type Error = Error;
> >>> +
> >>> +    fn try_from(polarity: bindings::pwm_polarity) -> Result<Self, Error> {
> >>> +        match polarity {
> >>> +            bindings::pwm_polarity_PWM_POLARITY_NORMAL => Ok(Polarity::Normal),
> >>> +            bindings::pwm_polarity_PWM_POLARITY_INVERSED => Ok(Polarity::Inversed),
> >>> +            _ => Err(EINVAL),
> >>> +        }
> >>> +    }
> >>> +}
> >>> +
> >>> +impl From<Polarity> for bindings::pwm_polarity {
> >>> +    fn from(polarity: Polarity) -> Self {
> >>> +        match polarity {
> >>> +            Polarity::Normal => bindings::pwm_polarity_PWM_POLARITY_NORMAL,
> >>> +            Polarity::Inversed => bindings::pwm_polarity_PWM_POLARITY_INVERSED,
> >>> +        }
> >>> +    }
> >>> +}
> >>> +
> >>> +/// Represents a PWM waveform configuration.
> >>> +/// Mirrors struct [`struct pwm_waveform`](srctree/include/linux/pwm.h).
> >>> +#[derive(Copy, Clone, Debug, Default, PartialEq, Eq)]
> >>> +pub struct Waveform {
> >>> +    /// Total duration of one complete PWM cycle, in nanoseconds.
> >>> +    pub period_length_ns: u64,
> >>> +
> >>> +    /// Duty-cycle active time, in nanoseconds.
> >>> +    ///
> >>> +    /// For a typical normal polarity configuration (active-high) this is the
> >>> +    /// high time of the signal.
> >>> +    pub duty_length_ns: u64,
> >>> +
> >>> +    /// Duty-cycle start offset, in nanoseconds.
> >>> +    ///
> >>> +    /// Delay from the beginning of the period to the first active edge.
> >>> +    /// In most simple PWM setups this is `0`, so the duty cycle starts
> >>> +    /// immediately at each period’s start.
> >>> +    pub duty_offset_ns: u64,
> >>> +}
> >>> +
> >>> +impl From<bindings::pwm_waveform> for Waveform {
> >>> +    fn from(wf: bindings::pwm_waveform) -> Self {
> >>> +        Waveform {
> >>> +            period_length_ns: wf.period_length_ns,
> >>> +            duty_length_ns: wf.duty_length_ns,
> >>> +            duty_offset_ns: wf.duty_offset_ns,
> >>> +        }
> >>> +    }
> >>> +}
> >>> +
> >>> +impl From<Waveform> for bindings::pwm_waveform {
> >>> +    fn from(wf: Waveform) -> Self {
> >>> +        bindings::pwm_waveform {
> >>> +            period_length_ns: wf.period_length_ns,
> >>> +            duty_length_ns: wf.duty_length_ns,
> >>> +            duty_offset_ns: wf.duty_offset_ns,
> >>> +        }
> >>> +    }
> >>> +}
> >>> +
> >>> +/// 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 }))
> >>> +    }
> >>> +
> >>> +    /// 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);
> >>> +
> >>> +impl Default for State {
> >>> +    fn default() -> Self {
> >>> +        Self::new()
> >>> +    }
> >>> +}
> >>> +
> >>> +impl State {
> >>> +    /// Creates a new zeroed `State`.
> >>> +    pub fn new() -> Self {
> >>> +        State(bindings::pwm_state::default())
> >>> +    }
> >>> +
> >>> +    /// 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)
> >>> +    }
> >>> +
> >>> +    /// Gets the period of the PWM signal in nanoseconds.
> >>> +    pub fn period(&self) -> u64 {
> >>> +        self.0.period
> >>> +    }
> >>> +
> >>> +    /// Sets the period of the PWM signal in nanoseconds.
> >>> +    pub fn set_period(&mut self, period_ns: u64) {
> >>> +        self.0.period = period_ns;
> >>> +    }
> >>> +
> >>> +    /// Gets the duty cycle of the PWM signal in nanoseconds.
> >>> +    pub fn duty_cycle(&self) -> u64 {
> >>> +        self.0.duty_cycle
> >>> +    }
> >>> +
> >>> +    /// Sets the duty cycle of the PWM signal in nanoseconds.
> >>> +    pub fn set_duty_cycle(&mut self, duty_ns: u64) {
> >>> +        self.0.duty_cycle = duty_ns;
> >>> +    }
> >>> +
> >>> +    /// Returns `true` if the PWM signal is enabled.
> >>> +    pub fn enabled(&self) -> bool {
> >>> +        self.0.enabled
> >>> +    }
> >>> +
> >>> +    /// Sets the enabled state of the PWM signal.
> >>> +    pub fn set_enabled(&mut self, enabled: bool) {
> >>> +        self.0.enabled = enabled;
> >>> +    }
> >>> +
> >>> +    /// Gets the polarity of the PWM signal.
> >>> +    pub fn polarity(&self) -> Result<Polarity, Error> {
> >>> +        Polarity::try_from(self.0.polarity)
> >>> +    }
> >>> +
> >>> +    /// Sets the polarity of the PWM signal.
> >>> +    pub fn set_polarity(&mut self, polarity: Polarity) {
> >>> +        self.0.polarity = polarity.into();
> >>> +    }
> >>
> >> Please don't expose these non-atomic callbacks. pwm_disable() would be
> >> fine.
> 
> Hmm, I've just realized that without those setters it would most likely
> impossible to correctly implement the get_state callback.

You shouldn't implement the get_state callback for a waveform driver.

Best regards
Uwe

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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ