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

[Michal Wilczynski <m.wilczynski@samsung.com>]

On 7/6/25 14:23, Danilo Krummrich wrote:
> 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.

Thanks, a pwmchip_release() is static though, so it's either expose the
pwmchip_release in the header, or call kfree() here directly on pwmchip.

> 
> <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
> 

Best regards,
-- 
Michal Wilczynski <m.wilczynski@samsung.com>