Re: [PATCH v9 2/6] rust: pwm: Add complete abstraction layer

[Danilo Krummrich <dakr@kernel.org>]

On Sun, Jul 06, 2025 at 01:45:13PM +0200, Michal Wilczynski wrote:
> +/// Trait defining the operations for a PWM driver.
> +pub trait PwmOps: 'static + Sized {
> +    /// The type of the owned driver data (e.g., `Pin<KBox<...>>`).
> +    type DrvData: 'static + ForeignOwnable;
> +    /// The driver-specific hardware representation of a waveform.
> +    ///
> +    /// This type must be [`Copy`], [`Default`], and fit within `PWM_WFHWSIZE`.
> +    type WfHw: Copy + Default;
> +
> +    /// Optional hook for when a PWM device is requested.
> +    fn request(
> +        _chip: &Chip<Self::DrvData>,
> +        _pwm: &Device,
> +        _parent_dev: &device::Device<Bound>,
> +    ) -> Result {
> +        Ok(())
> +    }
> +
> +    /// Optional hook for when a PWM device is freed.
> +    fn free(_chip: &Chip<Self::DrvData>, _pwm: &Device, _parent_dev: &device::Device<Bound>) {}

NIT: I can't think of a case providing this callback in Rust is useful. Do you
have a clear use-case in mind? Otherwise, I'd not provide this callback until
you have one. Should be trivial to add later on.

> +impl<T: PwmOps> Adapter<T> {

<snip>

> +    /// # Safety
> +    ///
> +    /// `dev` must be a valid pointer to a `bindings::device` embedded within a
> +    /// `bindings::pwm_chip`. This function is called by the device core when the
> +    /// last reference to the device is dropped.
> +    unsafe extern "C" fn release_callback(dev: *mut bindings::device) {
> +        // SAFETY: The function's contract guarantees that `dev` points to a `device`
> +        // field embedded within a valid `pwm_chip`. `container_of!` can therefore
> +        // safely calculate the address of the containing struct.
> +        let c_chip_ptr = unsafe { container_of!(dev, bindings::pwm_chip, dev) };
> +
> +        // SAFETY: `c_chip_ptr` is a valid pointer to a `pwm_chip` as established
> +        // above. Calling this FFI function is safe.
> +        let drvdata_ptr = unsafe { bindings::pwmchip_get_drvdata(c_chip_ptr) };
> +
> +        if !drvdata_ptr.is_null() {

Is this check needed? I think one can't create a pwm::Chip instance without
providing a T, so this pointer can't be NULL I think.

> +            // SAFETY: `drvdata_ptr` was stored by `Chip::new` from an owned `T::DrvData`
> +            // and is guaranteed to be valid if non-null. `from_foreign` can safely
> +            // reclaim ownership to allow Rust to drop and free the data.
> +            let _owned_drvdata = unsafe { T::DrvData::from_foreign(drvdata_ptr.cast()) };
> +        }
> +    }

If you overwrite this callback (as you do below) you're leaking the memory
allocated by pwmchip_alloc().

The simple way to solve this would be to call pwmchip_release() from here.

<snip>

> +impl<T: 'static + ForeignOwnable> Chip<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<O: PwmOps<DrvData = T>>(
> +        parent_dev: &device::Device,
> +        npwm: u32,
> +        sizeof_priv: usize,
> +        drvdata: T,
> +    ) -> Result<ARef<Self>> {
> +        // SAFETY: `parent_device_for_dev_field.as_raw()` is valid.
> +        // `bindings::pwmchip_alloc` returns a valid `*mut bindings::pwm_chip` (refcount 1)
> +        // or an ERR_PTR.
> +        let c_chip_ptr_raw =
> +            unsafe { bindings::pwmchip_alloc(parent_dev.as_raw(), npwm, sizeof_priv) };
> +
> +        let c_chip_ptr: *mut bindings::pwm_chip = error::from_err_ptr(c_chip_ptr_raw)?;
> +
> +        // Set the custom release function on the embedded device. This is the crucial step
> +        // to ensure `drvdata` is freed when the chip's refcount reaches zero, regardless
> +        // of whether `Registration::register` was called.
> +        // SAFETY: `c_chip_ptr` points to a valid chip.
> +        unsafe { (*c_chip_ptr).dev.release = Some(Adapter::<O>::release_callback); }

This overwrites [1].

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pwm/core.c?h=v6.16-rc4#n1601