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

[Danilo Krummrich <dakr@kernel.org>]

On Thu, Jul 03, 2025 at 01:37:43PM +0200, Michal Wilczynski wrote:
> 
> 
> On 7/2/25 17:13, Danilo Krummrich wrote:
> > 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().
> 
> You were right that the original drvdata<T>() method was unsafe. The
> most direct fix, making Chip generic to Chip<T>, unfortunately creates a
> significant cascade effect:
> 
> - If Chip becomes Chip<T>, then anything holding it, like ARef, must
>   become ARef<Chip<T>>.
> 
> - This in turn forces container structs like Registration to become
>   generic (Registration<T>).
> 
> - Finally, the PwmOps trait itself needs to be aware of T, which
>   complicates the trait and all driver implementations.
> 
> This chain reaction adds a lot of complexity. To avoid it, I've
> figured an alternative:
> 
> The new idea keeps Chip simple and non generic but ensures type safety
> through two main improvements to the abstraction layer:
> 
> 1. A Thread Safe DriverData Wrapper
> 
> The pwm.rs module now provides a generic pwm::DriverData<T> struct. Its
> only job is to wrap the driver's private data and provide the necessary
> unsafe impl Send + Sync.
> 
> // In `rust/kernel/pwm.rs`
> // SAFETY: The contained data is guaranteed by the kernel to have
> // synchronized access during callbacks.
> pub struct DriverData<T>(T);
> unsafe impl<T: Send> Send for DriverData<T> {}
> unsafe impl<T: Sync> Sync for DriverData<T> {}

I think you don't need to implement them explicitly, it's automatically derived
from T.

> 
> // In the driver's `probe` function
> let safe_data = pwm::DriverData::new(Th1520PwmDriverData{ });
> 
> 2. A More Ergonomic PwmOps Trait
> 
> The PwmOps trait methods now receive the driver's data directly as
> &self, which is much more intuitive. We achieve this by providing a
> default associated type for the data owner, which removes boilerplate
> from the driver.
> 
> // In `rust/kernel/pwm.rs`
> pub trait PwmOps: 'static + Sized {
>     type Owner: Deref<Target = DriverData<Self>> + ForeignOwnable =
>         Pin<KBox<DriverData<Self>>>;
>     /// For now I'm getting compiler error here: `associated type defaults are unstable`
>     /// So the driver would need to specify this for now, until this feature
>     /// is stable
> 
> 
>     // Methods now receive `&self`, making them much cleaner to implement.
>     fn round_waveform_tohw(&self, chip: &Chip, pwm: &Device, wf: &Waveform) -> Result<...>;
> }
> 
> // In the driver
> impl pwm::PwmOps for Th1520PwmDriverData {
>     type WfHw = Th1520WfHw;

Shouldn't this be:

	type Owner = Pin<KBox<DriverData<Self>>>;

> 
>     fn round_waveform_tohw(&self, chip: &pwm::Chip, ...) -> Result<...> {

If you accept any ForeignOwnable, I think this has to be Owner::Borrowed.

>         // no drvdata() call here :-)
>         let rate_hz = self.clk.rate().as_hz();
>         // ...
>     }
> }
> 
> This solution seem to address to issue you've pointed (as the user of
> the API never deals with drvdata directly at this point), while making
> it easier to develop PWM drivers in Rust.
> 
> Please let me know what you think.

In DRM [1][2] we used the approach I proposed, but at a first glance what you
propose seems like it should work as well.

Drivers having to set the Owner type seems a bit unfortunate, but otherwise it
seems like a matter of personal preference.

Although, I just notice, isn't this broken if a driver sets Owner to something
else than Self?

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/rust/kernel/drm/device.rs
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/rust/kernel/drm/driver.rs