Re: [PATCH v7 3/8] rust: pwm: Add core 'Device' and 'Chip' object wrappers

Danilo Krummrich
2025-07-02T15:13:42Z

On Wed, Jul 02, 2025 at 03:45:31PM +0200, Michal Wilczynski wrote:
> Building on the basic data types, this commit introduces the central
> object abstractions for the PWM subsystem: Device and Chip. It also
> includes the core trait implementations that make the Chip wrapper a
> complete, safe, and managed object.
>
> The main components of this change are:
> - Device and Chip Structs: These structs wrap the underlying struct
> pwm_device and struct pwm_chip C objects, providing safe, idiomatic
> methods to access their fields.
>
> - High-Level `Device` API: Exposes safe wrappers for the modern
> `waveform` API, allowing consumers to apply, read, and pre-validate
> hardware configurations.
>
> - Core Trait Implementations for Chip:
> - AlwaysRefCounted: Links the Chip's lifetime to its embedded
> struct device reference counter. This enables automatic lifetime
> management via ARef.
> - Send and Sync: Marks the Chip wrapper as safe for use across
> threads. This is sound because the C core handles all necessary
> locking for the underlying object's state.
>
> These wrappers and traits form a robust foundation for building PWM
> drivers in Rust.
>
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>

Few more comments below, with those fixed:

Reviewed-by: Danilo Krummrich <dakr@kernel.org>

> +/// Wrapper for a PWM device [`struct pwm_device`](srctree/include/linux/pwm.h).
> +#[repr(transparent)]
> +pub struct Device(Opaque<bindings::pwm_device>);
> +
> +impl Device {

<snip>

> + /// Gets a reference to the parent `Chip` that this device belongs to.
> + pub fn chip(&self) -> &Chip {
> + // SAFETY: `self.as_raw()` provides a valid pointer. (*self.as_raw()).chip
> + // is assumed to be a valid pointer to `pwm_chip` managed by the kernel.
> + // Chip::as_ref's safety conditions must be met.
> + unsafe { Chip::as_ref((*self.as_raw()).chip) }

I assume the C API does guarantee that a struct pwm_device *always* holds a
valid pointer to a struct pwm_chip?

> +
> +/// Wrapper for a PWM chip/controller ([`struct pwm_chip`](srctree/include/linux/pwm.h)).
> +#[repr(transparent)]
> +pub struct Chip(Opaque<bindings::pwm_chip>);
> +
> +impl Chip {
> + /// Creates a reference to a [`Chip`] from a valid pointer.
> + ///
> + /// # Safety
> + ///
> + /// The caller must ensure that `ptr` is valid and remains valid for the lifetime of the
> + /// returned [`Chip`] reference.
> + pub(crate) unsafe fn as_ref<'a>(ptr: *mut bindings::pwm_chip) -> &'a Self {
> + // SAFETY: The safety requirements guarantee the validity of the dereference, while the
> + // `Chip` type being transparent makes the cast ok.
> + unsafe { &*ptr.cast::<Self>() }
> + }
> +
> + /// Returns a raw pointer to the underlying `pwm_chip`.
> + pub(crate) fn as_raw(&self) -> *mut bindings::pwm_chip {
> + self.0.get()
> + }
> +
> + /// Gets the number of PWM channels (hardware PWMs) on this chip.
> + pub fn npwm(&self) -> u32 {
> + // SAFETY: `self.as_raw()` provides a valid pointer for `self`'s lifetime.
> + unsafe { (*self.as_raw()).npwm }
> + }
> +
> + /// Returns `true` if the chip supports atomic operations for configuration.
> + pub fn is_atomic(&self) -> bool {
> + // SAFETY: `self.as_raw()` provides a valid pointer for `self`'s lifetime.
> + unsafe { (*self.as_raw()).atomic }
> + }
> +
> + /// Returns a reference to the embedded `struct device` abstraction.
> + pub fn device(&self) -> &device::Device {
> + // SAFETY: `self.as_raw()` provides a valid pointer to `bindings::pwm_chip`.
> + // The `dev` field is an instance of `bindings::device` embedded within `pwm_chip`.
> + // Taking a pointer to this embedded field is valid.
> + // `device::Device` is `#[repr(transparent)]`.
> + // The lifetime of the returned reference is tied to `self`.
> + let dev_field_ptr = unsafe { core::ptr::addr_of!((*self.as_raw()).dev) };

I think you can use `&raw` instead.

> + // SAFETY: `dev_field_ptr` is a valid pointer to `bindings::device`.
> + // Casting and dereferencing is safe due to `repr(transparent)` and lifetime.
> + unsafe { &*(dev_field_ptr.cast::<device::Device>()) }

Please use Device::as_ref() instead.

> + }
> +
> + /// Gets the *typed* driver-specific data associated with this chip's embedded device.
> + pub fn drvdata<T: 'static>(&self) -> &T {

You need to make the whole Chip structure generic over T, i.e.
Chip<T: ForeignOwnable>.

Otherwise the API is unsafe, since the caller can pass in any T when calling
`chip.drvdata()` regardless of whether you actually stored as private data
through Chip::new().

Also, given that `T: ForeignOwnable`, you should return `T::Borrowed`.

> + // SAFETY: `self.as_raw()` gives a valid pwm_chip pointer.
> + // `bindings::pwmchip_get_drvdata` is the C function to retrieve driver data.
> + let ptr = unsafe { bindings::pwmchip_get_drvdata(self.as_raw()) };
> +
> + // SAFETY: The only way to create a chip is through Chip::new, which initializes
> + // this pointer.
> + unsafe { &*ptr.cast::<T>() }
> + }
> +
> + /// Allocates and wraps a PWM chip using `bindings::pwmchip_alloc`.
> + ///
> + /// Returns an [`ARef<Chip>`] managing the chip's lifetime via refcounting
> + /// on its embedded `struct device`.
> + pub fn new<T: 'static + ForeignOwnable>(
> + parent_dev: &device::Device,
> + npwm: u32,
> + sizeof_priv: usize,
> + drvdata: T,

As mentioned above, the whole Chip structure needs to be generic over T,
otherwise you can't guarantee that this T is the same T as the one in drvdata().

> +// SAFETY: Implements refcounting for `Chip` using the embedded `struct device`.
> +unsafe impl AlwaysRefCounted for Chip {
> + #[inline]
> + fn inc_ref(&self) {
> + // SAFETY: `self.0.get()` points to a valid `pwm_chip` because `self` exists.
> + // The embedded `dev` is valid. `get_device` increments its refcount.
> + unsafe {
> + bindings::get_device(core::ptr::addr_of_mut!((*self.0.get()).dev));

I think you can use `&raw mut` instead.

Also, if you move the semicolon at the end of the unsafe block, this goes in one
line.

> + }
> + }
> +
> + #[inline]
> + unsafe fn dec_ref(obj: NonNull<Chip>) {
> + let c_chip_ptr = obj.cast::<bindings::pwm_chip>().as_ptr();
> +
> + // SAFETY: `obj` is a valid pointer to a `Chip` (and thus `bindings::pwm_chip`)
> + // with a non-zero refcount. `put_device` handles decrement and final release.
> + unsafe {
> + bindings::put_device(core::ptr::addr_of_mut!((*c_chip_ptr).dev));
> + }

Same here.

> + }
> +}