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

Danilo Krummrich
2025-07-03T21:42:08Z

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