Message-ID: <aGVMnmoepIVSS0yK@pollux>
Date: Wed, 2 Jul 2025 17:13:34 +0200
From: Danilo Krummrich <dakr@...nel.org>
To: Michal Wilczynski <m.wilczynski@...sung.com>
Cc: 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>,
	Andreas Hindborg <a.hindborg@...nel.org>,
	Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>,
	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>,
	Drew Fustini <fustini@...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
Subject: Re: [PATCH v7 3/8] rust: pwm: Add core 'Device' and 'Chip' object
 wrappers

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@...sung.com>

Few more comments below, with those fixed:

	Reviewed-by: Danilo Krummrich <dakr@...nel.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.

> +    }
> +}

Powered by blists - more mailing lists