Introduce initial Rust abstractions for the Linux PWM subsystem. These
abstractions provide safe wrappers around the core C data structures and
functions, enabling the development of PWM chip drivers in Rust.
The main components added are:
- A Kconfig option RUST_PWM_ABSTRACTIONS
- C helper functions in rust/helpers/pwm.c to provide stable callable
interfaces for Rust, for pwmchip_parent, pwmchip_get_drvdata, and
pwmchip_set_drvdata
- A new Rust module rust/kernel/pwm.rs containing:
- Safe wrappers for struct pwm_chip, struct pwm_device,
struct pwm_state, and struct pwm_args
- An enum Polarity for type safe polarity handling
- Functions devm_chip_alloc and devm_chip_add which wrap the
kernel's device-managed APIs for PWM chip allocation and
registration.
- A PwmOps trait and create_pwm_ops function to allow Rust
drivers to define their PWM operations, initially supporting the
.apply callback.
This foundational layer will be used by subsequent patches to implement
a specific PWM chip driver in Rust. It focuses on the pwm_chip provider
APIs necessary for such a driver.
Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
---
MAINTAINERS | 6 +
drivers/pwm/Kconfig | 8 +
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 | 376 ++++++++++++++++++++++++++++++++++++++++
7 files changed, 414 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index b6126bccf17c899cbbf5b729e1f426ff38d04a8e..2b080e8f3d873b1e401b3a2fe1207c224c4591fc 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19678,6 +19678,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@samsung.com>
+S: Maintained
+F: rust/helpers/pwm.c
+F: rust/kernel/pwm.rs
+
PXA GPIO DRIVER
M: Robert Jarzmik <robert.jarzmik@free.fr>
L: linux-gpio@vger.kernel.org
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 4731d5b90d7edcc61138e4a5bf7e98906953ece4..b5bd5c13b3a5e5a575a0fbfb2e285f5665b7a671 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -755,4 +755,12 @@ 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
+ help
+ Adds support needed for PWM drivers written in Rust. It provides a
+ wrapper around the C pwm core.
+
endif
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 43aabecc1d6161398acf3bb402d1f67b48bfcd6f..24a2498f17a2fc56f5dc012a20d621007be28b5e 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -32,6 +32,7 @@
#include <linux/platform_device.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 34f040e5f260b892fc421a6e55de32cbf90c8c22..7dda45d1794c957d83227362d9483eb90543f053 100644
--- a/rust/helpers/helpers.c
+++ b/rust/helpers/helpers.c
@@ -26,6 +26,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@samsung.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 1b0b6790a7f33c30af16a056d6afcca3d15a2a0d..72b60afe5f51d8a62b3c95b1b320a48386d14e61 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -92,6 +92,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..357fda46faa99c4462149658951ec53bf9cc2d1e
--- /dev/null
+++ b/rust/kernel/pwm.rs
@@ -0,0 +1,376 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2025 Samsung Electronics Co., Ltd.
+// Author: Michal Wilczynski <m.wilczynski@samsung.com>
+
+//! PWM (Pulse Width Modulator) abstractions.
+//!
+//! This module provides safe Rust abstractions for working with the Linux
+//! kernel's PWM subsystem, leveraging types generated by `bindgen`
+//! from `<linux/pwm.h>` and `drivers/pwm/core.c`.
+
+use crate::{
+ bindings,
+ device::Device as CoreDevice,
+ error::*,
+ prelude::*,
+ str::CStr,
+ types::{ForeignOwnable, Opaque},
+};
+use core::marker::PhantomData;
+
+/// PWM polarity. Mirrors `enum pwm_polarity`.
+#[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 From<bindings::pwm_polarity> for Polarity {
+ fn from(polarity: bindings::pwm_polarity) -> Self {
+ match polarity {
+ bindings::pwm_polarity_PWM_POLARITY_NORMAL => Polarity::Normal,
+ bindings::pwm_polarity_PWM_POLARITY_INVERSED => Polarity::Inversed,
+ _ => {
+ pr_warn!(
+ "Unknown pwm_polarity value {}, defaulting to Normal\n",
+ polarity
+ );
+ Polarity::Normal
+ }
+ }
+ }
+}
+
+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,
+ }
+ }
+}
+
+/// Wrapper for board-dependent PWM arguments (`struct pwm_args`).
+#[repr(transparent)]
+pub struct Args(Opaque<bindings::pwm_args>);
+
+impl Args {
+ /// Creates an `Args` wrapper from the C struct reference.
+ fn from_c_ref(c_args: &bindings::pwm_args) -> Self {
+ // SAFETY: Pointer is valid, construct Opaque wrapper. We copy the data.
+ Args(Opaque::new(*c_args))
+ }
+
+ /// Returns the period of the PWM signal in nanoseconds.
+ pub fn period(&self) -> u64 {
+ // SAFETY: Reading from the valid pointer obtained by `get()`.
+ unsafe { (*self.0.get()).period }
+ }
+
+ /// Returns the polarity of the PWM signal.
+ pub fn polarity(&self) -> Polarity {
+ // SAFETY: Reading from the valid pointer obtained by `get()`.
+ Polarity::from(unsafe { (*self.0.get()).polarity })
+ }
+}
+
+/// Wrapper for PWM state (`struct pwm_state`).
+#[repr(transparent)]
+pub struct State(Opaque<bindings::pwm_state>);
+
+impl State {
+ /// Creates a new zeroed `State`.
+ pub fn new() -> Self {
+ State(Opaque::new(bindings::pwm_state::default()))
+ }
+
+ /// Creates a `State` wrapper around a copy of a C `pwm_state`.
+ pub(crate) fn from_c(c_state: bindings::pwm_state) -> Self {
+ State(Opaque::new(c_state))
+ }
+
+ /// Creates a `State` wrapper around a reference to a C `pwm_state`.
+ fn from_c_ref(c_state: &bindings::pwm_state) -> &Self {
+ // SAFETY: Pointer is valid, lifetime tied to input ref. Cast pointer type.
+ unsafe { &*(c_state as *const bindings::pwm_state as *const Self) }
+ }
+
+ /// Gets the period of the PWM signal in nanoseconds.
+ pub fn period(&self) -> u64 {
+ unsafe { (*self.0.get()).period }
+ }
+
+ /// Sets the period of the PWM signal in nanoseconds.
+ pub fn set_period(&mut self, period_ns: u64) {
+ unsafe {
+ (*self.0.get()).period = period_ns;
+ }
+ }
+
+ /// Gets the duty cycle of the PWM signal in nanoseconds.
+ pub fn duty_cycle(&self) -> u64 {
+ unsafe { (*self.0.get()).duty_cycle }
+ }
+
+ /// Sets the duty cycle of the PWM signal in nanoseconds.
+ pub fn set_duty_cycle(&mut self, duty_ns: u64) {
+ unsafe {
+ (*self.0.get()).duty_cycle = duty_ns;
+ }
+ }
+
+ /// Returns `true` if the PWM signal is enabled.
+ pub fn enabled(&self) -> bool {
+ unsafe { (*self.0.get()).enabled }
+ }
+
+ /// Sets the enabled state of the PWM signal.
+ pub fn set_enabled(&mut self, enabled: bool) {
+ unsafe {
+ (*self.0.get()).enabled = enabled;
+ }
+ }
+
+ /// Gets the polarity of the PWM signal.
+ pub fn polarity(&self) -> Polarity {
+ Polarity::from(unsafe { (*self.0.get()).polarity })
+ }
+
+ /// Sets the polarity of the PWM signal.
+ pub fn set_polarity(&mut self, polarity: Polarity) {
+ unsafe {
+ (*self.0.get()).polarity = polarity.into();
+ }
+ }
+
+ /// Returns `true` if the PWM signal is configured for power usage hint.
+ pub fn usage_power(&self) -> bool {
+ unsafe { (*self.0.get()).usage_power }
+ }
+
+ /// Sets the power usage hint for the PWM signal.
+ pub fn set_usage_power(&mut self, usage_power: bool) {
+ unsafe {
+ (*self.0.get()).usage_power = usage_power;
+ }
+ }
+}
+
+/// Wrapper for a PWM device/channel (`struct pwm_device`).
+#[repr(transparent)]
+pub struct Device(Opaque<bindings::pwm_device>);
+
+impl Device {
+ pub(crate) unsafe fn from_ptr<'a>(ptr: *mut bindings::pwm_device) -> &'a mut Self {
+ unsafe { &mut *ptr.cast::<Self>() }
+ }
+
+ fn as_ptr(&self) -> *mut bindings::pwm_device {
+ self.0.get()
+ }
+
+ /// Gets the hardware PWM index for this device within its chip.
+ pub fn hwpwm(&self) -> u32 {
+ unsafe { (*self.as_ptr()).hwpwm }
+ }
+
+ /// Gets a reference to the parent `Chip` that this device belongs to.
+ pub fn chip(&self) -> &Chip {
+ unsafe { Chip::from_ptr((*self.as_ptr()).chip) }
+ }
+
+ /// Gets the label for this PWM device, if any.
+ pub fn label(&self) -> Option<&CStr> {
+ let label_ptr = unsafe { (*self.as_ptr()).label };
+ if label_ptr.is_null() {
+ None
+ } else {
+ Some(unsafe { CStr::from_char_ptr(label_ptr) })
+ }
+ }
+
+ /// Gets a copy of the board-dependent arguments for this PWM device.
+ pub fn args(&self) -> Args {
+ Args::from_c_ref(unsafe { &(*self.as_ptr()).args })
+ }
+
+ /// Gets a copy of the current state of this PWM device.
+ pub fn state(&self) -> State {
+ State::from_c(unsafe { (*self.as_ptr()).state })
+ }
+
+ /// Returns `true` if the PWM signal is currently enabled based on its state.
+ pub fn is_enabled(&self) -> bool {
+ self.state().enabled()
+ }
+}
+
+/// Wrapper for a PWM chip/controller (`struct pwm_chip`).
+#[repr(transparent)]
+pub struct Chip(Opaque<bindings::pwm_chip>);
+
+impl Chip {
+ /// Creates a `Chip` reference from a raw pointer. (Safety notes apply)
+ pub(crate) unsafe fn from_ptr<'a>(ptr: *mut bindings::pwm_chip) -> &'a mut Self {
+ unsafe { &mut *ptr.cast::<Self>() }
+ }
+
+ /// Returns a raw pointer to the underlying `pwm_chip`.
+ pub(crate) fn as_ptr(&self) -> *mut bindings::pwm_chip {
+ self.0.get()
+ }
+
+ /// Gets the number of PWM channels (hardware PWMs) on this chip.
+ pub fn npwm(&self) -> u32 {
+ unsafe { (*self.as_ptr()).npwm }
+ }
+
+ /// Returns `true` if the chip supports atomic operations for configuration.
+ pub fn is_atomic(&self) -> bool {
+ unsafe { (*self.as_ptr()).atomic }
+ }
+
+ /// Returns a reference to the embedded `struct device` abstraction (`CoreDevice`).
+ pub fn device(&self) -> &CoreDevice {
+ // SAFETY: `dev` field exists and points to the embedded device.
+ let dev_ptr = unsafe { &(*self.as_ptr()).dev as *const _ as *mut bindings::device };
+ unsafe { &*(dev_ptr as *mut CoreDevice) }
+ }
+
+ /// Returns a reference to the parent device (`struct device`) of this PWM chip's device.
+ pub fn parent_device(&self) -> Option<&CoreDevice> {
+ // SAFETY: Accessing fields via assumed-valid pointer and bindgen layout.
+ let parent_ptr = unsafe { bindings::pwmchip_parent(self.as_ptr()) };
+ if parent_ptr.is_null() {
+ None
+ } else {
+ // SAFETY: Pointer is non-null, assume valid device managed by kernel.
+ Some(unsafe { &*(parent_ptr as *mut CoreDevice) })
+ }
+ }
+
+ /// Gets the *typed* driver-specific data associated with this chip's embedded device.
+ pub fn get_drvdata<T: 'static>(&self) -> Option<&T> {
+ let ptr = unsafe { bindings::pwmchip_get_drvdata(self.as_ptr()) };
+ if ptr.is_null() {
+ None
+ } else {
+ unsafe { Some(&*(ptr as *const T)) }
+ }
+ }
+
+ /// Sets the *typed* driver-specific data associated with this chip's embedded device.
+ pub fn set_drvdata<T: 'static + ForeignOwnable>(&mut self, data: T) {
+ unsafe { bindings::pwmchip_set_drvdata(self.as_ptr(), data.into_foreign() as _) }
+ }
+}
+
+/// Allocates a PWM chip structure using device resource management. Mirrors `devm_pwmchip_alloc`.
+pub fn devm_chip_alloc<'a>(
+ parent: &'a CoreDevice,
+ npwm: u32,
+ sizeof_priv: usize,
+) -> Result<&'a mut Chip> {
+ // SAFETY: `devm_pwmchip_alloc` called with valid args. Returns valid ptr or ERR_PTR.
+ let parent_ptr = parent as *const CoreDevice as *mut bindings::device;
+ let chip_ptr = unsafe { bindings::devm_pwmchip_alloc(parent_ptr, npwm, sizeof_priv) };
+ if unsafe { bindings::IS_ERR(chip_ptr as *const core::ffi::c_void) } {
+ let err = unsafe { bindings::PTR_ERR(chip_ptr as *const core::ffi::c_void) };
+ pr_err!("devm_pwmchip_alloc failed: {}\n", err);
+ Err(Error::from_errno(err as i32))
+ } else {
+ // SAFETY: `chip_ptr` valid, lifetime managed by `devm` tied to `parent`.
+ Ok(unsafe { &mut *(chip_ptr as *mut Chip) })
+ }
+}
+
+/// Registers a PWM chip with the PWM subsystem. Mirrors `__pwmchip_add`.
+pub fn chip_add(chip: &mut Chip, ops: &'static PwmOpsVTable) -> Result {
+ // SAFETY: Pointers are valid. `__pwmchip_add` requires ops to be set.
+ unsafe {
+ let chip_ptr = chip.as_ptr();
+ // Assign the ops pointer directly to the C struct field
+ (*chip_ptr).ops = ops.as_ptr();
+ to_result(bindings::__pwmchip_add(
+ chip_ptr,
+ core::ptr::null_mut()
+ ))
+ }
+}
+
+/// Registers a PWM chip using device resource management. Mirrors `__devm_pwmchip_add`.
+pub fn devm_chip_add(parent: &CoreDevice, chip: &mut Chip, ops: &'static PwmOpsVTable) -> Result {
+ // SAFETY: Pointers are valid. `__devm_pwmchip_add` requires ops to be set.
+ unsafe {
+ let chip_ptr = chip.as_ptr();
+ // Assign the ops pointer directly to the C struct field
+ (*chip_ptr).ops = ops.as_ptr();
+ let parent_ptr = parent as *const CoreDevice as *mut bindings::device;
+ to_result(bindings::__devm_pwmchip_add(
+ parent_ptr,
+ chip_ptr,
+ core::ptr::null_mut()
+ ))
+ }
+}
+
+/// Trait defining the operations for a PWM driver. Mirrors relevant parts of `struct pwm_ops`.
+pub trait PwmOps: 'static {
+ /// Atomically apply a new state to the PWM device. Mirrors `pwm_ops->apply`.
+ fn apply(chip: &mut Chip, pwm: &mut Device, state: &State) -> Result;
+
+ // TODO: Add other ops like request, free, capture, waveform ops if needed.
+}
+
+/// Holds the vtable for PwmOps implementations.
+struct Adapter<T: PwmOps> {
+ _p: PhantomData<T>,
+}
+
+impl<T: PwmOps> Adapter<T> {
+ // Trampoline for `apply`.
+ unsafe extern "C" fn apply_callback(
+ chip: *mut bindings::pwm_chip,
+ pwm: *mut bindings::pwm_device,
+ state: *const bindings::pwm_state, // Input state is const
+ ) -> core::ffi::c_int {
+ // SAFETY: Pointers from core are valid. Create temporary wrappers.
+ let chip_ref = unsafe { Chip::from_ptr(chip) };
+ let pwm_ref = unsafe { Device::from_ptr(pwm) };
+ // Use the reference wrapper for the const C state
+ let state_ref = State::from_c_ref(unsafe { &*state });
+
+ match T::apply(chip_ref, pwm_ref, state_ref) {
+ Ok(()) => 0,
+ Err(e) => e.to_errno(),
+ }
+ }
+}
+
+/// VTable structure wrapper for PWM operations. Mirrors `struct pwm_ops`.
+#[repr(transparent)]
+pub struct PwmOpsVTable(Opaque<bindings::pwm_ops>);
+
+// SAFETY: Holds function pointers, no accessible mutable state via &self.
+unsafe impl Sync for PwmOpsVTable {}
+
+impl PwmOpsVTable {
+ /// Returns a raw pointer to the underlying `pwm_ops` struct.
+ pub(crate) fn as_ptr(&self) -> *const bindings::pwm_ops {
+ self.0.get()
+ }
+}
+
+/// Creates a PWM operations vtable for a type `T` that implements `PwmOps`.
+///
+/// This is used to bridge Rust trait implementations to the C `struct pwm_ops`
+/// expected by the kernel.
+pub const fn create_pwm_ops<T: PwmOps>() -> PwmOpsVTable {
+ let mut ops: bindings::pwm_ops = unsafe { core::mem::zeroed() };
+
+ ops.apply = Some(Adapter::<T>::apply_callback);
+
+ PwmOpsVTable(Opaque::new(ops))
+}
--
2.34.1Hello Michal, On Sat, May 24, 2025 at 11:14:55PM +0200, Michal Wilczynski wrote: > Introduce initial Rust abstractions for the Linux PWM subsystem. These > abstractions provide safe wrappers around the core C data structures and > functions, enabling the development of PWM chip drivers in Rust. Oh wow, thanks for rustifying PWM. That might be my chance to actually learn Rust. I don't know when I will find the time to look into that in detail but one thing I'd like to have for Rust support is that the drivers use the new abstraction (i.e. .round_waveform_tohw() + .round_waveform_fromhw() + .read_waveform() + .write_waveform()) instead of the older .apply() callback. Best regards Uwe
On 5/26/25 09:53, Uwe Kleine-König wrote: > Hello Michal, > > On Sat, May 24, 2025 at 11:14:55PM +0200, Michal Wilczynski wrote: >> Introduce initial Rust abstractions for the Linux PWM subsystem. These >> abstractions provide safe wrappers around the core C data structures and >> functions, enabling the development of PWM chip drivers in Rust. > > Oh wow, thanks for rustifying PWM. That might be my chance to actually > learn Rust. > > I don't know when I will find the time to look into that in detail but > one thing I'd like to have for Rust support is that the drivers use the > new abstraction (i.e. .round_waveform_tohw() + .round_waveform_fromhw() > + .read_waveform() + .write_waveform()) instead of the older .apply() > callback. Hi Uwe, Thanks for the valuable feedback. You're right, building on the newer waveform abstractions is a correct approach. I'll rework the patches to use .round_waveform_tohw(), .round_waveform_fromhw(), .read_waveform(), and .write_waveform() as you suggested, instead of the .apply() callback. I appreciate you steering me in the right direction. > > Best regards > Uwe Best regards, -- Michal Wilczynski <m.wilczynski@samsung.com>
Hi Michal,
On Sat, May 24, 2025 at 11:14:55PM +0200, Michal Wilczynski wrote:
> diff --git a/rust/kernel/pwm.rs b/rust/kernel/pwm.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..357fda46faa99c4462149658951ec53bf9cc2d1e
> --- /dev/null
> +++ b/rust/kernel/pwm.rs
> @@ -0,0 +1,376 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2025 Samsung Electronics Co., Ltd.
> +// Author: Michal Wilczynski <m.wilczynski@samsung.com>
> +
> +//! PWM (Pulse Width Modulator) abstractions.
> +//!
> +//! This module provides safe Rust abstractions for working with the Linux
> +//! kernel's PWM subsystem, leveraging types generated by `bindgen`
> +//! from `<linux/pwm.h>` and `drivers/pwm/core.c`.
> +
> +use crate::{
> + bindings,
> + device::Device as CoreDevice,
I recommend referring the the base device as just device::Device, like we do it
everywhere else where we have this name conflict.
I'm not a huge fan of such aliases, since it's confusing when looking at patches
that do not have the alias as context later on.
> + error::*,
> + prelude::*,
> + str::CStr,
> + types::{ForeignOwnable, Opaque},
> +};
> +use core::marker::PhantomData;
> +
> +/// PWM polarity. Mirrors `enum pwm_polarity`.
> +#[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 From<bindings::pwm_polarity> for Polarity {
> + fn from(polarity: bindings::pwm_polarity) -> Self {
> + match polarity {
> + bindings::pwm_polarity_PWM_POLARITY_NORMAL => Polarity::Normal,
> + bindings::pwm_polarity_PWM_POLARITY_INVERSED => Polarity::Inversed,
> + _ => {
> + pr_warn!(
> + "Unknown pwm_polarity value {}, defaulting to Normal\n",
> + polarity
> + );
> + Polarity::Normal
Either Polarity::Normal is the correct thing to return in such a case, but then
we do not need the pr_warn!(), or it is not, but then this should be a TryFrom
impl instead.
> + }
> + }
> + }
> +}
> +
> +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,
> + }
> + }
> +}
> +
> +/// Wrapper for board-dependent PWM arguments (`struct pwm_args`).
> +#[repr(transparent)]
> +pub struct Args(Opaque<bindings::pwm_args>);
> +
> +impl Args {
> + /// Creates an `Args` wrapper from the C struct reference.
> + fn from_c_ref(c_args: &bindings::pwm_args) -> Self {
I'd make this a pointer type instead, rather than having conversion to a
reference at the caller's side.
> + // SAFETY: Pointer is valid, construct Opaque wrapper. We copy the data.
> + Args(Opaque::new(*c_args))
> + }
> +
> + /// Returns the period of the PWM signal in nanoseconds.
> + pub fn period(&self) -> u64 {
> + // SAFETY: Reading from the valid pointer obtained by `get()`.
Here and below, you should explain why this pointer is guaranteed to be valid
instead.
> + unsafe { (*self.0.get()).period }
> + }
> +
> + /// Returns the polarity of the PWM signal.
> + pub fn polarity(&self) -> Polarity {
> + // SAFETY: Reading from the valid pointer obtained by `get()`.
> + Polarity::from(unsafe { (*self.0.get()).polarity })
> + }
> +}
> +
> +/// Wrapper for PWM state (`struct pwm_state`).
> +#[repr(transparent)]
> +pub struct State(Opaque<bindings::pwm_state>);
> +
> +impl State {
> + /// Creates a new zeroed `State`.
> + pub fn new() -> Self {
> + State(Opaque::new(bindings::pwm_state::default()))
> + }
> +
> + /// Creates a `State` wrapper around a copy of a C `pwm_state`.
> + pub(crate) fn from_c(c_state: bindings::pwm_state) -> Self {
> + State(Opaque::new(c_state))
> + }
You probably don't need Opaque here, given that you have instances of
bindings::pwm_state already.
> +
> + /// Creates a `State` wrapper around a reference to a C `pwm_state`.
> + fn from_c_ref(c_state: &bindings::pwm_state) -> &Self {
> + // SAFETY: Pointer is valid, lifetime tied to input ref. Cast pointer type.
> + unsafe { &*(c_state as *const bindings::pwm_state as *const Self) }
> + }
> +
> + /// Gets the period of the PWM signal in nanoseconds.
> + pub fn period(&self) -> u64 {
> + unsafe { (*self.0.get()).period }
This and all the methods below lack SAFETY comments, did you compile with
CLIPPY=1? You should get lots of warnings reminding you to add them.
<snip>
> +
> +/// Wrapper for a PWM device/channel (`struct pwm_device`).
> +#[repr(transparent)]
> +pub struct Device(Opaque<bindings::pwm_device>);
> +
> +impl Device {
> + pub(crate) unsafe fn from_ptr<'a>(ptr: *mut bindings::pwm_device) -> &'a mut Self {
We usually call this kind of function as_ref(), it'd be nice to stick to that
for consistency.
> + unsafe { &mut *ptr.cast::<Self>() }
A mutable reference -- why?
<snip>
> +/// Wrapper for a PWM chip/controller (`struct pwm_chip`).
> +#[repr(transparent)]
> +pub struct Chip(Opaque<bindings::pwm_chip>);
> +
> +impl Chip {
> + /// Creates a `Chip` reference from a raw pointer. (Safety notes apply)
> + pub(crate) unsafe fn from_ptr<'a>(ptr: *mut bindings::pwm_chip) -> &'a mut Self {
Same here, for the name...
> + unsafe { &mut *ptr.cast::<Self>() }
and the mutability.
> + }
> +
> + /// Returns a raw pointer to the underlying `pwm_chip`.
> + pub(crate) fn as_ptr(&self) -> *mut bindings::pwm_chip {
> + self.0.get()
> + }
> +
> + /// Gets the number of PWM channels (hardware PWMs) on this chip.
> + pub fn npwm(&self) -> u32 {
> + unsafe { (*self.as_ptr()).npwm }
> + }
> +
> + /// Returns `true` if the chip supports atomic operations for configuration.
> + pub fn is_atomic(&self) -> bool {
> + unsafe { (*self.as_ptr()).atomic }
> + }
> +
> + /// Returns a reference to the embedded `struct device` abstraction (`CoreDevice`).
> + pub fn device(&self) -> &CoreDevice {
> + // SAFETY: `dev` field exists and points to the embedded device.
> + let dev_ptr = unsafe { &(*self.as_ptr()).dev as *const _ as *mut bindings::device };
> + unsafe { &*(dev_ptr as *mut CoreDevice) }
Missing SAFETY comment, also please use cast() and cast_{const,mut}() instead.
> + }
> +
> + /// Returns a reference to the parent device (`struct device`) of this PWM chip's device.
> + pub fn parent_device(&self) -> Option<&CoreDevice> {
> + // SAFETY: Accessing fields via assumed-valid pointer and bindgen layout.
> + let parent_ptr = unsafe { bindings::pwmchip_parent(self.as_ptr()) };
> + if parent_ptr.is_null() {
> + None
> + } else {
> + // SAFETY: Pointer is non-null, assume valid device managed by kernel.
> + Some(unsafe { &*(parent_ptr as *mut CoreDevice) })
> + }
This can just be
self.device().parent() // [1]
which lands through the DRM tree in the upcoming merge window.
[1] https://gitlab.freedesktop.org/drm/kernel/-/blob/drm-next/rust/kernel/device.rs?ref_type=heads#L72
> + /// Gets the *typed* driver-specific data associated with this chip's embedded device.
> + pub fn get_drvdata<T: 'static>(&self) -> Option<&T> {
I will soon send a patch series that adds drvdata accessors to the generic
Device abstraction.
Anyways, no need for you to wait for this.
> + let ptr = unsafe { bindings::pwmchip_get_drvdata(self.as_ptr()) };
Obviously, the C side calls this pwmchip_get_drvdata() and dev_get_drvdata(),
however, I suggest not to use get_drvdata() as a method name, since 'get' is
used in the context of reference counting (get/put) instead, and hence may be
confusing. Let's just name this drvdata().
> + if ptr.is_null() {
> + None
> + } else {
> + unsafe { Some(&*(ptr as *const T)) }
> + }
> + }
> +
> + /// Sets the *typed* driver-specific data associated with this chip's embedded device.
> + pub fn set_drvdata<T: 'static + ForeignOwnable>(&mut self, data: T) {
> + unsafe { bindings::pwmchip_set_drvdata(self.as_ptr(), data.into_foreign() as _) }
> + }
> +}
> +
> +/// Allocates a PWM chip structure using device resource management. Mirrors `devm_pwmchip_alloc`.
> +pub fn devm_chip_alloc<'a>(
This should be a function of pwm::Chip rather than standalone.
> + parent: &'a CoreDevice,
Since you're using devres, this must be a bound device, i.e. the parameter must
be &'a device::Device<device::Bound>, see also [2], which lands in the upcoming
merge window through the driver-core tree.
But I think this shouldn't use devm_pwmchip_alloc() anyways, see below.
[2] https://git.kernel.org/pub/scm/linux/kernel/git/driver-core/driver-core.git/tree/rust/kernel/device.rs?h=driver-core-next#n237
> + npwm: u32,
> + sizeof_priv: usize,
> +) -> Result<&'a mut Chip> {
Besides the above, this solution seems a bit half-baked, since it means that you
can only ever access the pwm::Chip as long as you have the &Device<Bound>,
which, given that you call this function typically from probe(), not beyond the
scope of probe().
This is because you return a reference which you can't save in the driver's
private data.
Instead you should use pwmchip_alloc() instead and make this return a real
instance of pwm::Chip and implement AlwaysRefCounted [3] for pwm::Chip.
You can then store the pwm::Chip instance in the Driver's private data, which
will only live until the driver is unbound, so it gives the same guarantees.
[3] https://rust.docs.kernel.org/kernel/types/trait.AlwaysRefCounted.html
> + // SAFETY: `devm_pwmchip_alloc` called with valid args. Returns valid ptr or ERR_PTR.
> + let parent_ptr = parent as *const CoreDevice as *mut bindings::device;
> + let chip_ptr = unsafe { bindings::devm_pwmchip_alloc(parent_ptr, npwm, sizeof_priv) };
> + if unsafe { bindings::IS_ERR(chip_ptr as *const core::ffi::c_void) } {
> + let err = unsafe { bindings::PTR_ERR(chip_ptr as *const core::ffi::c_void) };
> + pr_err!("devm_pwmchip_alloc failed: {}\n", err);
You have the parent device, please use dev_err!(). But I don't think this needs
to error print at all.
> + Err(Error::from_errno(err as i32))
> + } else {
> + // SAFETY: `chip_ptr` valid, lifetime managed by `devm` tied to `parent`.
> + Ok(unsafe { &mut *(chip_ptr as *mut Chip) })
> + }
> +}
> +
> +/// Registers a PWM chip with the PWM subsystem. Mirrors `__pwmchip_add`.
> +pub fn chip_add(chip: &mut Chip, ops: &'static PwmOpsVTable) -> Result {
> + // SAFETY: Pointers are valid. `__pwmchip_add` requires ops to be set.
> + unsafe {
> + let chip_ptr = chip.as_ptr();
> + // Assign the ops pointer directly to the C struct field
> + (*chip_ptr).ops = ops.as_ptr();
> + to_result(bindings::__pwmchip_add(
> + chip_ptr,
> + core::ptr::null_mut()
> + ))
> + }
> +}
How do you ensure to unregister the chip, once it was registered through this
function? I think this can cause UAF bugs. Instead you should wrap this in a
'Registration' structure, like we do everywhere else, see for example [4].
The structure should look like this:
pub struct Registration(ARef<Chip>);
Registration::new() should register the chip and Registration::drop() should
unregister the chip.
[4] https://gitlab.freedesktop.org/drm/kernel/-/blob/drm-next/rust/kernel/drm/driver.rs?ref_type=heads#L121
> +/// Registers a PWM chip using device resource management. Mirrors `__devm_pwmchip_add`.
> +pub fn devm_chip_add(parent: &CoreDevice, chip: &mut Chip, ops: &'static PwmOpsVTable) -> Result {
> + // SAFETY: Pointers are valid. `__devm_pwmchip_add` requires ops to be set.
> + unsafe {
> + let chip_ptr = chip.as_ptr();
> + // Assign the ops pointer directly to the C struct field
> + (*chip_ptr).ops = ops.as_ptr();
> + let parent_ptr = parent as *const CoreDevice as *mut bindings::device;
> + to_result(bindings::__devm_pwmchip_add(
> + parent_ptr,
> + chip_ptr,
> + core::ptr::null_mut()
> + ))
> + }
> +}
This won't work anymore when creating a real pwm::Chip instance, since you can't
guarantee that the pwm::Chip still exists when devres will clean this up.
If you want devres to clean this up, you should make Registration::new() return
a Result<Devres<Registration>> instance.
This way the Registration keeps a reference to the pwm::Chip (giving the
guarantee of no potential UAF), and the Devres container ensures to drop the
Registration when the parent device is unbound internally.
If you want the exact same semantics as your current devm_chip_add(), but
without potential UAF, there is also Devres::new_foreign_owned() [5]. This way
Registration::new() will only return a Result.
[5] https://rust.docs.kernel.org/kernel/devres/struct.Devres.html#method.new_foreign_owned
W dniu 25.05.2025 o 13:49, Danilo Krummrich pisze:
> Hi Michal,
>
> On Sat, May 24, 2025 at 11:14:55PM +0200, Michal Wilczynski wrote:
>> diff --git a/rust/kernel/pwm.rs b/rust/kernel/pwm.rs
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..357fda46faa99c4462149658951ec53bf9cc2d1e
>> --- /dev/null
>> +++ b/rust/kernel/pwm.rs
>> @@ -0,0 +1,376 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +// Copyright (c) 2025 Samsung Electronics Co., Ltd.
>> +// Author: Michal Wilczynski <m.wilczynski@samsung.com>
>> +
>> +//! PWM (Pulse Width Modulator) abstractions.
>> +//!
>> +//! This module provides safe Rust abstractions for working with the Linux
>> +//! kernel's PWM subsystem, leveraging types generated by `bindgen`
>> +//! from `<linux/pwm.h>` and `drivers/pwm/core.c`.
>> +
>> +use crate::{
>> + bindings,
>> + device::Device as CoreDevice,
> I recommend referring the the base device as just device::Device, like we do it
> everywhere else where we have this name conflict.
>
> I'm not a huge fan of such aliases, since it's confusing when looking at patches
> that do not have the alias as context later on.
>
>> + error::*,
>> + prelude::*,
>> + str::CStr,
>> + types::{ForeignOwnable, Opaque},
>> +};
>> +use core::marker::PhantomData;
>> +
>> +/// PWM polarity. Mirrors `enum pwm_polarity`.
>> +#[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 From<bindings::pwm_polarity> for Polarity {
>> + fn from(polarity: bindings::pwm_polarity) -> Self {
>> + match polarity {
>> + bindings::pwm_polarity_PWM_POLARITY_NORMAL => Polarity::Normal,
>> + bindings::pwm_polarity_PWM_POLARITY_INVERSED => Polarity::Inversed,
>> + _ => {
>> + pr_warn!(
>> + "Unknown pwm_polarity value {}, defaulting to Normal\n",
>> + polarity
>> + );
>> + Polarity::Normal
> Either Polarity::Normal is the correct thing to return in such a case, but then
> we do not need the pr_warn!(), or it is not, but then this should be a TryFrom
> impl instead.
>
>> + }
>> + }
>> + }
>> +}
>> +
>> +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,
>> + }
>> + }
>> +}
>> +
>> +/// Wrapper for board-dependent PWM arguments (`struct pwm_args`).
>> +#[repr(transparent)]
>> +pub struct Args(Opaque<bindings::pwm_args>);
>> +
>> +impl Args {
>> + /// Creates an `Args` wrapper from the C struct reference.
>> + fn from_c_ref(c_args: &bindings::pwm_args) -> Self {
> I'd make this a pointer type instead, rather than having conversion to a
> reference at the caller's side.
>
>> + // SAFETY: Pointer is valid, construct Opaque wrapper. We copy the data.
>> + Args(Opaque::new(*c_args))
>> + }
>> +
>> + /// Returns the period of the PWM signal in nanoseconds.
>> + pub fn period(&self) -> u64 {
>> + // SAFETY: Reading from the valid pointer obtained by `get()`.
> Here and below, you should explain why this pointer is guaranteed to be valid
> instead.
>
>> + unsafe { (*self.0.get()).period }
>> + }
>> +
>> + /// Returns the polarity of the PWM signal.
>> + pub fn polarity(&self) -> Polarity {
>> + // SAFETY: Reading from the valid pointer obtained by `get()`.
>> + Polarity::from(unsafe { (*self.0.get()).polarity })
>> + }
>> +}
>> +
>> +/// Wrapper for PWM state (`struct pwm_state`).
>> +#[repr(transparent)]
>> +pub struct State(Opaque<bindings::pwm_state>);
>> +
>> +impl State {
>> + /// Creates a new zeroed `State`.
>> + pub fn new() -> Self {
>> + State(Opaque::new(bindings::pwm_state::default()))
>> + }
>> +
>> + /// Creates a `State` wrapper around a copy of a C `pwm_state`.
>> + pub(crate) fn from_c(c_state: bindings::pwm_state) -> Self {
>> + State(Opaque::new(c_state))
>> + }
> You probably don't need Opaque here, given that you have instances of
> bindings::pwm_state already.
>
>> +
>> + /// Creates a `State` wrapper around a reference to a C `pwm_state`.
>> + fn from_c_ref(c_state: &bindings::pwm_state) -> &Self {
>> + // SAFETY: Pointer is valid, lifetime tied to input ref. Cast pointer type.
>> + unsafe { &*(c_state as *const bindings::pwm_state as *const Self) }
>> + }
>> +
>> + /// Gets the period of the PWM signal in nanoseconds.
>> + pub fn period(&self) -> u64 {
>> + unsafe { (*self.0.get()).period }
> This and all the methods below lack SAFETY comments, did you compile with
> CLIPPY=1? You should get lots of warnings reminding you to add them.
>
> <snip>
>
>> +
>> +/// Wrapper for a PWM device/channel (`struct pwm_device`).
>> +#[repr(transparent)]
>> +pub struct Device(Opaque<bindings::pwm_device>);
>> +
>> +impl Device {
>> + pub(crate) unsafe fn from_ptr<'a>(ptr: *mut bindings::pwm_device) -> &'a mut Self {
> We usually call this kind of function as_ref(), it'd be nice to stick to that
> for consistency.
>
>> + unsafe { &mut *ptr.cast::<Self>() }
> A mutable reference -- why?
>
> <snip>
>
>> +/// Wrapper for a PWM chip/controller (`struct pwm_chip`).
>> +#[repr(transparent)]
>> +pub struct Chip(Opaque<bindings::pwm_chip>);
>> +
>> +impl Chip {
>> + /// Creates a `Chip` reference from a raw pointer. (Safety notes apply)
>> + pub(crate) unsafe fn from_ptr<'a>(ptr: *mut bindings::pwm_chip) -> &'a mut Self {
> Same here, for the name...
>
>> + unsafe { &mut *ptr.cast::<Self>() }
> and the mutability.
>
>> + }
>> +
>> + /// Returns a raw pointer to the underlying `pwm_chip`.
>> + pub(crate) fn as_ptr(&self) -> *mut bindings::pwm_chip {
>> + self.0.get()
>> + }
>> +
>> + /// Gets the number of PWM channels (hardware PWMs) on this chip.
>> + pub fn npwm(&self) -> u32 {
>> + unsafe { (*self.as_ptr()).npwm }
>> + }
>> +
>> + /// Returns `true` if the chip supports atomic operations for configuration.
>> + pub fn is_atomic(&self) -> bool {
>> + unsafe { (*self.as_ptr()).atomic }
>> + }
>> +
>> + /// Returns a reference to the embedded `struct device` abstraction (`CoreDevice`).
>> + pub fn device(&self) -> &CoreDevice {
>> + // SAFETY: `dev` field exists and points to the embedded device.
>> + let dev_ptr = unsafe { &(*self.as_ptr()).dev as *const _ as *mut bindings::device };
>> + unsafe { &*(dev_ptr as *mut CoreDevice) }
> Missing SAFETY comment, also please use cast() and cast_{const,mut}() instead.
>
>> + }
>> +
>> + /// Returns a reference to the parent device (`struct device`) of this PWM chip's device.
>> + pub fn parent_device(&self) -> Option<&CoreDevice> {
>> + // SAFETY: Accessing fields via assumed-valid pointer and bindgen layout.
>> + let parent_ptr = unsafe { bindings::pwmchip_parent(self.as_ptr()) };
>> + if parent_ptr.is_null() {
>> + None
>> + } else {
>> + // SAFETY: Pointer is non-null, assume valid device managed by kernel.
>> + Some(unsafe { &*(parent_ptr as *mut CoreDevice) })
>> + }
> This can just be
>
> self.device().parent() // [1]
>
> which lands through the DRM tree in the upcoming merge window.
>
> [1] https://gitlab.freedesktop.org/drm/kernel/-/blob/drm-next/rust/kernel/device.rs?ref_type=heads#L72
>
>> + /// Gets the *typed* driver-specific data associated with this chip's embedded device.
>> + pub fn get_drvdata<T: 'static>(&self) -> Option<&T> {
> I will soon send a patch series that adds drvdata accessors to the generic
> Device abstraction.
>
> Anyways, no need for you to wait for this.
>
>> + let ptr = unsafe { bindings::pwmchip_get_drvdata(self.as_ptr()) };
> Obviously, the C side calls this pwmchip_get_drvdata() and dev_get_drvdata(),
> however, I suggest not to use get_drvdata() as a method name, since 'get' is
> used in the context of reference counting (get/put) instead, and hence may be
> confusing. Let's just name this drvdata().
>
>> + if ptr.is_null() {
>> + None
>> + } else {
>> + unsafe { Some(&*(ptr as *const T)) }
>> + }
>> + }
>> +
>> + /// Sets the *typed* driver-specific data associated with this chip's embedded device.
>> + pub fn set_drvdata<T: 'static + ForeignOwnable>(&mut self, data: T) {
>> + unsafe { bindings::pwmchip_set_drvdata(self.as_ptr(), data.into_foreign() as _) }
>> + }
>> +}
>> +
>> +/// Allocates a PWM chip structure using device resource management. Mirrors `devm_pwmchip_alloc`.
>> +pub fn devm_chip_alloc<'a>(
> This should be a function of pwm::Chip rather than standalone.
>
>> + parent: &'a CoreDevice,
> Since you're using devres, this must be a bound device, i.e. the parameter must
> be &'a device::Device<device::Bound>, see also [2], which lands in the upcoming
> merge window through the driver-core tree.
>
> But I think this shouldn't use devm_pwmchip_alloc() anyways, see below.
>
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/driver-core/driver-core.git/tree/rust/kernel/device.rs?h=driver-core-next#n237
>
>> + npwm: u32,
>> + sizeof_priv: usize,
>> +) -> Result<&'a mut Chip> {
> Besides the above, this solution seems a bit half-baked, since it means that you
> can only ever access the pwm::Chip as long as you have the &Device<Bound>,
> which, given that you call this function typically from probe(), not beyond the
> scope of probe().
>
> This is because you return a reference which you can't save in the driver's
> private data.
>
> Instead you should use pwmchip_alloc() instead and make this return a real
> instance of pwm::Chip and implement AlwaysRefCounted [3] for pwm::Chip.
>
> You can then store the pwm::Chip instance in the Driver's private data, which
> will only live until the driver is unbound, so it gives the same guarantees.
>
> [3] https://rust.docs.kernel.org/kernel/types/trait.AlwaysRefCounted.html
>
>> + // SAFETY: `devm_pwmchip_alloc` called with valid args. Returns valid ptr or ERR_PTR.
>> + let parent_ptr = parent as *const CoreDevice as *mut bindings::device;
>> + let chip_ptr = unsafe { bindings::devm_pwmchip_alloc(parent_ptr, npwm, sizeof_priv) };
>> + if unsafe { bindings::IS_ERR(chip_ptr as *const core::ffi::c_void) } {
>> + let err = unsafe { bindings::PTR_ERR(chip_ptr as *const core::ffi::c_void) };
>> + pr_err!("devm_pwmchip_alloc failed: {}\n", err);
> You have the parent device, please use dev_err!(). But I don't think this needs
> to error print at all.
>
>> + Err(Error::from_errno(err as i32))
>> + } else {
>> + // SAFETY: `chip_ptr` valid, lifetime managed by `devm` tied to `parent`.
>> + Ok(unsafe { &mut *(chip_ptr as *mut Chip) })
>> + }
>> +}
>> +
>> +/// Registers a PWM chip with the PWM subsystem. Mirrors `__pwmchip_add`.
>> +pub fn chip_add(chip: &mut Chip, ops: &'static PwmOpsVTable) -> Result {
>> + // SAFETY: Pointers are valid. `__pwmchip_add` requires ops to be set.
>> + unsafe {
>> + let chip_ptr = chip.as_ptr();
>> + // Assign the ops pointer directly to the C struct field
>> + (*chip_ptr).ops = ops.as_ptr();
>> + to_result(bindings::__pwmchip_add(
>> + chip_ptr,
>> + core::ptr::null_mut()
>> + ))
>> + }
>> +}
> How do you ensure to unregister the chip, once it was registered through this
> function? I think this can cause UAF bugs. Instead you should wrap this in a
> 'Registration' structure, like we do everywhere else, see for example [4].
>
> The structure should look like this:
>
> pub struct Registration(ARef<Chip>);
>
> Registration::new() should register the chip and Registration::drop() should
> unregister the chip.
>
> [4] https://gitlab.freedesktop.org/drm/kernel/-/blob/drm-next/rust/kernel/drm/driver.rs?ref_type=heads#L121
>
>> +/// Registers a PWM chip using device resource management. Mirrors `__devm_pwmchip_add`.
>> +pub fn devm_chip_add(parent: &CoreDevice, chip: &mut Chip, ops: &'static PwmOpsVTable) -> Result {
>> + // SAFETY: Pointers are valid. `__devm_pwmchip_add` requires ops to be set.
>> + unsafe {
>> + let chip_ptr = chip.as_ptr();
>> + // Assign the ops pointer directly to the C struct field
>> + (*chip_ptr).ops = ops.as_ptr();
>> + let parent_ptr = parent as *const CoreDevice as *mut bindings::device;
>> + to_result(bindings::__devm_pwmchip_add(
>> + parent_ptr,
>> + chip_ptr,
>> + core::ptr::null_mut()
>> + ))
>> + }
>> +}
> This won't work anymore when creating a real pwm::Chip instance, since you can't
> guarantee that the pwm::Chip still exists when devres will clean this up.
>
> If you want devres to clean this up, you should make Registration::new() return
> a Result<Devres<Registration>> instance.
>
> This way the Registration keeps a reference to the pwm::Chip (giving the
> guarantee of no potential UAF), and the Devres container ensures to drop the
> Registration when the parent device is unbound internally.
>
> If you want the exact same semantics as your current devm_chip_add(), but
> without potential UAF, there is also Devres::new_foreign_owned() [5]. This way
> Registration::new() will only return a Result.
>
> [5] https://rust.docs.kernel.org/kernel/devres/struct.Devres.html#method.new_foreign_owned
Hi Danilo,
Thank you very much for the detailed review. I took some time to digest
it and everything you wrote
makes perfect sense. Will also make sure to compile with CLIPPY=1 next time.
Thanks !
Michał
>
© 2016 - 2025 Red Hat, Inc.