Re: [PATCH RFC 1/6] rust: Add basic PWM abstractions

["Michal Wilczynski/Kernel (PLT) /SRPOL/Engineer/Samsung Electronics" <m.wilczynski@samsung.com>]

W dniu 25.05.2025 o 13:49, Danilo Krummrich pisze:
> Hi Michal,
>
> On Sat, May 24, 2025 at 11:14:55PM +0200, Michal Wilczynski wrote:
>> diff --git a/rust/kernel/pwm.rs b/rust/kernel/pwm.rs
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..357fda46faa99c4462149658951ec53bf9cc2d1e
>> --- /dev/null
>> +++ b/rust/kernel/pwm.rs
>> @@ -0,0 +1,376 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +// Copyright (c) 2025 Samsung Electronics Co., Ltd.
>> +// Author: Michal Wilczynski <m.wilczynski@samsung.com>
>> +
>> +//! PWM (Pulse Width Modulator) abstractions.
>> +//!
>> +//! This module provides safe Rust abstractions for working with the Linux
>> +//! kernel's PWM subsystem, leveraging types generated by `bindgen`
>> +//! from `<linux/pwm.h>` and `drivers/pwm/core.c`.
>> +
>> +use crate::{
>> +    bindings,
>> +    device::Device as CoreDevice,
> I recommend referring the the base device as just device::Device, like we do it
> everywhere else where we have this name conflict.
>
> I'm not a huge fan of such aliases, since it's confusing when looking at patches
> that do not have the alias as context later on.
>
>> +    error::*,
>> +    prelude::*,
>> +    str::CStr,
>> +    types::{ForeignOwnable, Opaque},
>> +};
>> +use core::marker::PhantomData;
>> +
>> +/// PWM polarity. Mirrors `enum pwm_polarity`.
>> +#[derive(Copy, Clone, Debug, PartialEq, Eq)]
>> +pub enum Polarity {
>> +    /// Normal polarity (duty cycle defines the high period of the signal)
>> +    Normal,
>> +    /// Inversed polarity (duty cycle defines the low period of the signal)
>> +    Inversed,
>> +}
>> +
>> +impl From<bindings::pwm_polarity> for Polarity {
>> +    fn from(polarity: bindings::pwm_polarity) -> Self {
>> +        match polarity {
>> +            bindings::pwm_polarity_PWM_POLARITY_NORMAL => Polarity::Normal,
>> +            bindings::pwm_polarity_PWM_POLARITY_INVERSED => Polarity::Inversed,
>> +            _ => {
>> +                pr_warn!(
>> +                    "Unknown pwm_polarity value {}, defaulting to Normal\n",
>> +                    polarity
>> +                );
>> +                Polarity::Normal
> Either Polarity::Normal is the correct thing to return in such a case, but then
> we do not need the pr_warn!(), or it is not, but then this should be a TryFrom
> impl instead.
>
>> +            }
>> +        }
>> +    }
>> +}
>> +
>> +impl From<Polarity> for bindings::pwm_polarity {
>> +    fn from(polarity: Polarity) -> Self {
>> +        match polarity {
>> +            Polarity::Normal => bindings::pwm_polarity_PWM_POLARITY_NORMAL,
>> +            Polarity::Inversed => bindings::pwm_polarity_PWM_POLARITY_INVERSED,
>> +        }
>> +    }
>> +}
>> +
>> +/// Wrapper for board-dependent PWM arguments (`struct pwm_args`).
>> +#[repr(transparent)]
>> +pub struct Args(Opaque<bindings::pwm_args>);
>> +
>> +impl Args {
>> +    /// Creates an `Args` wrapper from the C struct reference.
>> +    fn from_c_ref(c_args: &bindings::pwm_args) -> Self {
> I'd make this a pointer type instead, rather than having conversion to a
> reference at the caller's side.
>
>> +        // SAFETY: Pointer is valid, construct Opaque wrapper. We copy the data.
>> +        Args(Opaque::new(*c_args))
>> +    }
>> +
>> +    /// Returns the period of the PWM signal in nanoseconds.
>> +    pub fn period(&self) -> u64 {
>> +        // SAFETY: Reading from the valid pointer obtained by `get()`.
> Here and below, you should explain why this pointer is guaranteed to be valid
> instead.
>
>> +        unsafe { (*self.0.get()).period }
>> +    }
>> +
>> +    /// Returns the polarity of the PWM signal.
>> +    pub fn polarity(&self) -> Polarity {
>> +        // SAFETY: Reading from the valid pointer obtained by `get()`.
>> +        Polarity::from(unsafe { (*self.0.get()).polarity })
>> +    }
>> +}
>> +
>> +/// Wrapper for PWM state (`struct pwm_state`).
>> +#[repr(transparent)]
>> +pub struct State(Opaque<bindings::pwm_state>);
>> +
>> +impl State {
>> +    /// Creates a new zeroed `State`.
>> +    pub fn new() -> Self {
>> +        State(Opaque::new(bindings::pwm_state::default()))
>> +    }
>> +
>> +    /// Creates a `State` wrapper around a copy of a C `pwm_state`.
>> +    pub(crate) fn from_c(c_state: bindings::pwm_state) -> Self {
>> +        State(Opaque::new(c_state))
>> +    }
> You probably don't need Opaque here, given that you have instances of
> bindings::pwm_state already.
>
>> +
>> +    /// Creates a `State` wrapper around a reference to a C `pwm_state`.
>> +    fn from_c_ref(c_state: &bindings::pwm_state) -> &Self {
>> +        // SAFETY: Pointer is valid, lifetime tied to input ref. Cast pointer type.
>> +        unsafe { &*(c_state as *const bindings::pwm_state as *const Self) }
>> +    }
>> +
>> +    /// Gets the period of the PWM signal in nanoseconds.
>> +    pub fn period(&self) -> u64 {
>> +        unsafe { (*self.0.get()).period }
> This and all the methods below lack SAFETY comments, did you compile with
> CLIPPY=1? You should get lots of warnings reminding you to add them.
>
> <snip>
>
>> +
>> +/// Wrapper for a PWM device/channel (`struct pwm_device`).
>> +#[repr(transparent)]
>> +pub struct Device(Opaque<bindings::pwm_device>);
>> +
>> +impl Device {
>> +    pub(crate) unsafe fn from_ptr<'a>(ptr: *mut bindings::pwm_device) -> &'a mut Self {
> We usually call this kind of function as_ref(), it'd be nice to stick to that
> for consistency.
>
>> +        unsafe { &mut *ptr.cast::<Self>() }
> A mutable reference -- why?
>
> <snip>
>
>> +/// Wrapper for a PWM chip/controller (`struct pwm_chip`).
>> +#[repr(transparent)]
>> +pub struct Chip(Opaque<bindings::pwm_chip>);
>> +
>> +impl Chip {
>> +    /// Creates a `Chip` reference from a raw pointer. (Safety notes apply)
>> +    pub(crate) unsafe fn from_ptr<'a>(ptr: *mut bindings::pwm_chip) -> &'a mut Self {
> Same here, for the name...
>
>> +        unsafe { &mut *ptr.cast::<Self>() }
> and the mutability.
>
>> +    }
>> +
>> +    /// Returns a raw pointer to the underlying `pwm_chip`.
>> +    pub(crate) fn as_ptr(&self) -> *mut bindings::pwm_chip {
>> +        self.0.get()
>> +    }
>> +
>> +    /// Gets the number of PWM channels (hardware PWMs) on this chip.
>> +    pub fn npwm(&self) -> u32 {
>> +        unsafe { (*self.as_ptr()).npwm }
>> +    }
>> +
>> +    /// Returns `true` if the chip supports atomic operations for configuration.
>> +    pub fn is_atomic(&self) -> bool {
>> +        unsafe { (*self.as_ptr()).atomic }
>> +    }
>> +
>> +    /// Returns a reference to the embedded `struct device` abstraction (`CoreDevice`).
>> +    pub fn device(&self) -> &CoreDevice {
>> +        // SAFETY: `dev` field exists and points to the embedded device.
>> +        let dev_ptr = unsafe { &(*self.as_ptr()).dev as *const _ as *mut bindings::device };
>> +        unsafe { &*(dev_ptr as *mut CoreDevice) }
> Missing SAFETY comment, also please use cast() and cast_{const,mut}() instead.
>
>> +    }
>> +
>> +    /// Returns a reference to the parent device (`struct device`) of this PWM chip's device.
>> +    pub fn parent_device(&self) -> Option<&CoreDevice> {
>> +        // SAFETY: Accessing fields via assumed-valid pointer and bindgen layout.
>> +        let parent_ptr = unsafe { bindings::pwmchip_parent(self.as_ptr()) };
>> +        if parent_ptr.is_null() {
>> +            None
>> +        } else {
>> +            // SAFETY: Pointer is non-null, assume valid device managed by kernel.
>> +            Some(unsafe { &*(parent_ptr as *mut CoreDevice) })
>> +        }
> This can just be
>
> 	self.device().parent() // [1]
>
> which lands through the DRM tree in the upcoming merge window.
>
> [1] https://gitlab.freedesktop.org/drm/kernel/-/blob/drm-next/rust/kernel/device.rs?ref_type=heads#L72
>
>> +    /// Gets the *typed* driver-specific data associated with this chip's embedded device.
>> +    pub fn get_drvdata<T: 'static>(&self) -> Option<&T> {
> I will soon send a patch series that adds drvdata accessors to the generic
> Device abstraction.
>
> Anyways, no need for you to wait for this.
>
>> +        let ptr = unsafe { bindings::pwmchip_get_drvdata(self.as_ptr()) };
> Obviously, the C side calls this pwmchip_get_drvdata() and dev_get_drvdata(),
> however, I suggest not to use get_drvdata() as a method name, since 'get' is
> used in the context of reference counting (get/put) instead, and hence may be
> confusing. Let's just name this drvdata().
>
>> +        if ptr.is_null() {
>> +            None
>> +        } else {
>> +            unsafe { Some(&*(ptr as *const T)) }
>> +        }
>> +    }
>> +
>> +    /// Sets the *typed* driver-specific data associated with this chip's embedded device.
>> +    pub fn set_drvdata<T: 'static + ForeignOwnable>(&mut self, data: T) {
>> +        unsafe { bindings::pwmchip_set_drvdata(self.as_ptr(), data.into_foreign() as _) }
>> +    }
>> +}
>> +
>> +/// Allocates a PWM chip structure using device resource management. Mirrors `devm_pwmchip_alloc`.
>> +pub fn devm_chip_alloc<'a>(
> This should be a function of pwm::Chip rather than standalone.
>
>> +    parent: &'a CoreDevice,
> Since you're using devres, this must be a bound device, i.e. the parameter must
> be &'a device::Device<device::Bound>, see also [2], which lands in the upcoming
> merge window through the driver-core tree.
>
> But I think this shouldn't use devm_pwmchip_alloc() anyways, see below.
>
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/driver-core/driver-core.git/tree/rust/kernel/device.rs?h=driver-core-next#n237
>
>> +    npwm: u32,
>> +    sizeof_priv: usize,
>> +) -> Result<&'a mut Chip> {
> Besides the above, this solution seems a bit half-baked, since it means that you
> can only ever access the pwm::Chip as long as you have the &Device<Bound>,
> which, given that you call this function typically from probe(), not beyond the
> scope of probe().
>
> This is because you return a reference which you can't save in the driver's
> private data.
>
> Instead you should use pwmchip_alloc() instead and make this return a real
> instance of pwm::Chip and implement AlwaysRefCounted [3] for pwm::Chip.
>
> You can then store the pwm::Chip instance in the Driver's private data, which
> will only live until the driver is unbound, so it gives the same guarantees.
>
> [3] https://rust.docs.kernel.org/kernel/types/trait.AlwaysRefCounted.html
>
>> +    // SAFETY: `devm_pwmchip_alloc` called with valid args. Returns valid ptr or ERR_PTR.
>> +    let parent_ptr = parent as *const CoreDevice as *mut bindings::device;
>> +    let chip_ptr = unsafe { bindings::devm_pwmchip_alloc(parent_ptr, npwm, sizeof_priv) };
>> +    if unsafe { bindings::IS_ERR(chip_ptr as *const core::ffi::c_void) } {
>> +        let err = unsafe { bindings::PTR_ERR(chip_ptr as *const core::ffi::c_void) };
>> +        pr_err!("devm_pwmchip_alloc failed: {}\n", err);
> You have the parent device, please use dev_err!(). But I don't think this needs
> to error print at all.
>
>> +        Err(Error::from_errno(err as i32))
>> +    } else {
>> +        // SAFETY: `chip_ptr` valid, lifetime managed by `devm` tied to `parent`.
>> +        Ok(unsafe { &mut *(chip_ptr as *mut Chip) })
>> +    }
>> +}
>> +
>> +/// Registers a PWM chip with the PWM subsystem. Mirrors `__pwmchip_add`.
>> +pub fn chip_add(chip: &mut Chip, ops: &'static PwmOpsVTable) -> Result {
>> +    // SAFETY: Pointers are valid. `__pwmchip_add` requires ops to be set.
>> +    unsafe {
>> +        let chip_ptr = chip.as_ptr();
>> +        // Assign the ops pointer directly to the C struct field
>> +        (*chip_ptr).ops = ops.as_ptr();
>> +        to_result(bindings::__pwmchip_add(
>> +            chip_ptr,
>> +            core::ptr::null_mut()
>> +        ))
>> +    }
>> +}
> How do you ensure to unregister the chip, once it was registered through this
> function? I think this can cause UAF bugs. Instead you should wrap this in a
> 'Registration' structure, like we do everywhere else, see for example [4].
>
> The structure should look like this:
>
> 	pub struct Registration(ARef<Chip>);
>
> Registration::new() should register the chip and Registration::drop() should
> unregister the chip.
>
> [4] https://gitlab.freedesktop.org/drm/kernel/-/blob/drm-next/rust/kernel/drm/driver.rs?ref_type=heads#L121
>
>> +/// Registers a PWM chip using device resource management. Mirrors `__devm_pwmchip_add`.
>> +pub fn devm_chip_add(parent: &CoreDevice, chip: &mut Chip, ops: &'static PwmOpsVTable) -> Result {
>> +    // SAFETY: Pointers are valid. `__devm_pwmchip_add` requires ops to be set.
>> +    unsafe {
>> +        let chip_ptr = chip.as_ptr();
>> +        // Assign the ops pointer directly to the C struct field
>> +        (*chip_ptr).ops = ops.as_ptr();
>> +        let parent_ptr = parent as *const CoreDevice as *mut bindings::device;
>> +        to_result(bindings::__devm_pwmchip_add(
>> +            parent_ptr,
>> +            chip_ptr,
>> +            core::ptr::null_mut()
>> +        ))
>> +    }
>> +}
> This won't work anymore when creating a real pwm::Chip instance, since you can't
> guarantee that the pwm::Chip still exists when devres will clean this up.
>
> If you want devres to clean this up, you should make Registration::new() return
> a Result<Devres<Registration>> instance.
>
> This way the Registration keeps a reference to the pwm::Chip (giving the
> guarantee of no potential UAF), and the Devres container ensures to drop the
> Registration when the parent device is unbound internally.
>
> If you want the exact same semantics as your current devm_chip_add(), but
> without potential UAF, there is also Devres::new_foreign_owned() [5]. This way
> Registration::new() will only return a Result.
>
> [5] https://rust.docs.kernel.org/kernel/devres/struct.Devres.html#method.new_foreign_owned

Hi Danilo,
Thank you very much for the detailed review. I took some time to digest 
it and everything you wrote
makes perfect sense. Will also make sure to compile with CLIPPY=1 next time.

Thanks !
Michał

>