Message-ID: <9394dd00-a8a2-4ffd-98bd-a4d512e80591@samsung.com>
Date: Sun, 30 Nov 2025 23:13:21 +0100
From: Michal Wilczynski <m.wilczynski@...sung.com>
To: Markus Probst <markus.probst@...teo.de>
Cc: linux-riscv@...ts.infradead.org, linux-pwm@...r.kernel.org,
	linux-kernel@...r.kernel.org, rust-for-linux@...r.kernel.org, Drew Fustini
	<fustini@...nel.org>, Guo Ren <guoren@...nel.org>, Fu Wei <wefu@...hat.com>,
	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>, Benno Lossin
	<lossin@...nel.org>, Andreas Hindborg <a.hindborg@...nel.org>, Alice Ryhl
	<aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>, Danilo Krummrich
	<dakr@...nel.org>
Subject: Re: [PATCH] Move pwm registration into pwm::Chip::new


Hi Markus,
On 11/27/25 18:15, Markus Probst wrote:
> The `pwm::Registration::register` function provides no guarantee that the
> function isn't called twice with the same pwm chip, which is considered
> unsafe.
> 
> Add the code responsible for the registration into `pwm::Chip::new`. The
> registration will happen before the driver gets access to the refcounted
> pwm chip and can therefore guarantee that the registration isn't called
> twice on the same pwm chip.

I agree that the double registration safety issue needs to be fixed.

However, merging the allocation and registration steps into Chip::new
restricts the API significantly.

There is a specific reason the C API separates pwmchip_alloc and
pwmchip_add. Drivers might need to configure the chip e.g. setting the
atomic flag, or acquire resources that require a pointer to the chip
(like requesting an IRQ) after allocation but before the device is
registered and visible to the subsystem.

I suggest we solve this using the Builder Pattern instead. This would
allow us to allocate, configure, and finally register the chip in
distinct safe steps.

Best regards,
-- 
Michal Wilczynski <m.wilczynski@...sung.com>

> 
> Signed-off-by: Markus Probst <markus.probst@...teo.de>
> ---
> This patch provides the additional guarantee that the pwm chip doesn't
> get registered twice.
> 
> The following changes were made:
> - change the visibility of `pwm::Registration` to private
> - remove the `pwm::Registration::register` function
> - add code for registering the pwm chip in `pwm::Chip::new`
> - add Send + Sync bounds to `PwmOps`
> 
> Note that I wasn't able to test this patch, due to the lack of hardware.
> ---
>  drivers/pwm/pwm_th1520.rs |  4 +---
>  rust/kernel/pwm.rs        | 53 ++++++++++++++++++-----------------------------
>  2 files changed, 21 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/pwm/pwm_th1520.rs b/drivers/pwm/pwm_th1520.rs
> index 955c359b07fb..1919b5a1f69e 100644
> --- a/drivers/pwm/pwm_th1520.rs
> +++ b/drivers/pwm/pwm_th1520.rs
> @@ -363,7 +363,7 @@ fn probe(
>              return Err(EINVAL);
>          }
>  
> -        let chip = pwm::Chip::new(
> +        pwm::Chip::new(
>              dev,
>              TH1520_MAX_PWM_NUM,
>              try_pin_init!(Th1520PwmDriverData {
> @@ -372,8 +372,6 @@ fn probe(
>              }),
>          )?;
>  
> -        pwm::Registration::register(dev, chip)?;
> -
>          Ok(KBox::new(Th1520PwmPlatformDriver, GFP_KERNEL)?.into())
>      }
>  }
> diff --git a/rust/kernel/pwm.rs b/rust/kernel/pwm.rs
> index cb00f8a8765c..c5d03ee8bc95 100644
> --- a/rust/kernel/pwm.rs
> +++ b/rust/kernel/pwm.rs
> @@ -173,7 +173,7 @@ pub struct RoundedWaveform<WfHw> {
>  }
>  
>  /// Trait defining the operations for a PWM driver.
> -pub trait PwmOps: 'static + Sized {
> +pub trait PwmOps: 'static + Send + Sync + Sized {
>      /// The driver-specific hardware representation of a waveform.
>      ///
>      /// This type must be [`Copy`], [`Default`], and fit within `PWM_WFHWSIZE`.
> @@ -585,7 +585,7 @@ unsafe fn bound_parent_device(&self) -> &device::Device<Bound> {
>      /// Returns an [`ARef<Chip>`] managing the chip's lifetime via refcounting
>      /// on its embedded `struct device`.
>      pub fn new(
> -        parent_dev: &device::Device,
> +        parent_dev: &device::Device<Bound>,
>          num_channels: u32,
>          data: impl pin_init::PinInit<T, Error>,
>      ) -> Result<ARef<Self>> {
> @@ -623,7 +623,21 @@ pub fn new(
>          // SAFETY: `chip_ptr_as_self` points to a valid `Chip` (layout-compatible with
>          // `bindings::pwm_chip`) whose embedded device has refcount 1.
>          // `ARef::from_raw` takes this pointer and manages it via `AlwaysRefCounted`.
> -        Ok(unsafe { ARef::from_raw(NonNull::new_unchecked(chip_ptr_as_self)) })
> +        let chip = unsafe { ARef::from_raw(NonNull::new_unchecked(chip_ptr_as_self)) };
> +
> +        // SAFETY: `c_chip_ptr` points to a valid chip with its ops initialized.
> +        // `__pwmchip_add` is the C function to register the chip with the PWM core.
> +        unsafe {
> +            to_result(bindings::__pwmchip_add(c_chip_ptr, core::ptr::null_mut()))?;
> +        }
> +
> +        let registration = Registration {
> +            chip: ARef::clone(&chip),
> +        };
> +
> +        devres::register(parent_dev, registration, GFP_KERNEL)?;
> +
> +        Ok(chip)
>      }
>  }
>  
> @@ -654,50 +668,23 @@ unsafe fn dec_ref(obj: NonNull<Chip<T>>) {
>  // structure's state is managed and synchronized by the kernel's device model
>  // and PWM core locking mechanisms. Therefore, it is safe to move the `Chip`
>  // wrapper (and the pointer it contains) across threads.
> -unsafe impl<T: PwmOps + Send> Send for Chip<T> {}
> +unsafe impl<T: PwmOps> Send for Chip<T> {}
>  
>  // SAFETY: It is safe for multiple threads to have shared access (`&Chip`) because
>  // the `Chip` data is immutable from the Rust side without holding the appropriate
>  // kernel locks, which the C core is responsible for. Any interior mutability is
>  // handled and synchronized by the C kernel code.
> -unsafe impl<T: PwmOps + Sync> Sync for Chip<T> {}
> +unsafe impl<T: PwmOps> Sync for Chip<T> {}
>  
>  /// A resource guard that ensures `pwmchip_remove` is called on drop.
>  ///
>  /// This struct is intended to be managed by the `devres` framework by transferring its ownership
>  /// via [`devres::register`]. This ties the lifetime of the PWM chip registration
>  /// to the lifetime of the underlying device.
> -pub struct Registration<T: PwmOps> {
> +struct Registration<T: PwmOps> {
>      chip: ARef<Chip<T>>,
>  }
>  
> -impl<T: 'static + PwmOps + Send + Sync> Registration<T> {
> -    /// Registers a PWM chip with the PWM subsystem.
> -    ///
> -    /// Transfers its ownership to the `devres` framework, which ties its lifetime
> -    /// to the parent device.
> -    /// On unbind of the parent device, the `devres` entry will be dropped, automatically
> -    /// calling `pwmchip_remove`. This function should be called from the driver's `probe`.
> -    pub fn register(dev: &device::Device<Bound>, chip: ARef<Chip<T>>) -> Result {
> -        let chip_parent = chip.device().parent().ok_or(EINVAL)?;
> -        if dev.as_raw() != chip_parent.as_raw() {
> -            return Err(EINVAL);
> -        }
> -
> -        let c_chip_ptr = chip.as_raw();
> -
> -        // SAFETY: `c_chip_ptr` points to a valid chip with its ops initialized.
> -        // `__pwmchip_add` is the C function to register the chip with the PWM core.
> -        unsafe {
> -            to_result(bindings::__pwmchip_add(c_chip_ptr, core::ptr::null_mut()))?;
> -        }
> -
> -        let registration = Registration { chip };
> -
> -        devres::register(dev, registration, GFP_KERNEL)
> -    }
> -}
> -
>  impl<T: PwmOps> Drop for Registration<T> {
>      fn drop(&mut self) {
>          let chip_raw = self.chip.as_raw();
> 
> ---
> base-commit: fae00ea9f00367771003ace78f29549dead58fc7
> change-id: 20251127-pwm_safe_register-e49b0a261101
> 
> 

Powered by blists - more mailing lists