Message-Id: <20251127-pwm_safe_register-v1-1-d22d0ed068ac@posteo.de>
Date: Thu, 27 Nov 2025 17:15:06 +0000
From: Markus Probst <markus.probst@...teo.de>
To: Drew Fustini <fustini@...nel.org>, Guo Ren <guoren@...nel.org>, 
 Fu Wei <wefu@...hat.com>, 
 Uwe Kleine-König <ukleinek@...nel.org>, 
 Michal Wilczynski <m.wilczynski@...sung.com>, 
 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>
Cc: linux-riscv@...ts.infradead.org, linux-pwm@...r.kernel.org, 
 linux-kernel@...r.kernel.org, rust-for-linux@...r.kernel.org, 
 Markus Probst <markus.probst@...teo.de>
Subject: [PATCH] Move pwm registration into pwm::Chip::new

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.

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