Re: [PATCH v3 1/3] rust: add runtime PM support

[Sami Tolvanen <samitolvanen@google.com>]

Hi Beata,

On Wed, Aug 26, 2026 at 03:10:55PM +0200, Beata Michalska wrote:
> diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
> index a56ba6309594..43bbf4bce993 100644
> --- a/rust/kernel/error.rs
> +++ b/rust/kernel/error.rs
> @@ -67,6 +67,7 @@ macro_rules! declare_err {
>      declare_err!(EOVERFLOW, "Value too large for defined data type.");
>      declare_err!(EMSGSIZE, "Message too long.");
>      declare_err!(ETIMEDOUT, "Connection timed out.");
> +    declare_err!(EINPROGRESS, "Operation now in progress.");

Looks like this is already upstream since commit b93fb6e76ec1.

> +/// Device's runtime power management status
> +#[repr(i32)]
> +pub enum RuntimePMState {
> +    /// Runtime PM has not been initialized for this device yet.
> +    UNKNOWN = bindings::rpm_status_RPM_INVALID,
> +    /// The device is expected to be runtime active and in it's normal operating state

Nit: it's -> its.

> +    RESUMED = bindings::rpm_status_RPM_ACTIVE,
> +    /// The device is expected to be suspended, unavailable for normal operations
> +    SUSPENDED = bindings::rpm_status_RPM_SUSPENDED,

Should the enum variant names use CamelCase?

> +impl<'a> ResumeScope<'a> {
> +    fn new(dev: &'a device::Device<device::Bound>, mode: Mode) -> Result<Self> {
> +        if mode.contains(ModeFlag::Acquire) {
> +            // ModeFlag::Acquire is intended to be used with Awake scope
> +            // Avoid mixing the modes.
> +            return Err(EINVAL);
> +        }
> +
> +        // ModeFlag::Idle is internal so strip it of before passing further

Nit: of -> off. Also in the identical comment below.

> +impl<'a> AwakeScope<'a> {
> +    fn new(dev: &'a device::Device<device::Bound>, mode: Mode) -> Result<Self> {
> +        if !mode.contains(ModeFlag::Acquire) {
> +            return Err(EINVAL);
> +        }
> +        // ModeFlag::Idle is internal so strip it of before passing further
> +        match Request::resume(dev, mode & !ModeFlag::Idle) {
> +            Ok(()) => {}
> +            // For async/nowait requests, `EINPROGRESS` means the resume is in
> +            // flight and the usage reference already keeps the device active.
> +            Err(e) if e == EINPROGRESS && mode.contains_any(ModeFlag::Async | ModeFlag::Nowait) => {
> +            }
> +            Err(e) => {
> +                Request::put_noidle(dev);
> +                return Err(e);
> +            }
> +        }
> +
> +        Ok(Self(Scope::<Awake> {
> +            dev,
> +            mode,
> +            _tag: PhantomData,
> +        }))
> +    }
> +
> +    fn release_inner(&self) -> Result {
> +        let scope_mode = self.0.mode & !ModeFlag::Idle;
> +        match self.0.mode {
> +            mode if mode.contains(ModeFlag::Idle) => Request::idle(self.0.dev, scope_mode),
> +            mode if mode.contains(ModeFlag::Auto) => {
> +                Request::mark_last_busy(self.0.dev);
> +                Request::suspend(self.0.dev, scope_mode)
> +            }
> +            _ => Request::suspend(self.0.dev, scope_mode),

In v2 you had Request::idle in the default arm. I didn't see a note
about this in the changelog. Was the change in behavior intentional?

> +impl<'a> RetainScope<'a> {
> +    fn new(dev: &'a device::Device<device::Bound>) -> Result<Self> {
> +        Request::get_noresume(dev);
> +        Ok(Self(Scope::<Retain> {
> +            dev,
> +            mode: Mode(ModeFlag::Sync as u32),
> +            _tag: PhantomData,
> +        }))
> +    }
> +
> +    fn try_new(dev: &'a device::Device<device::Bound>) -> Result<Self> {
> +        Request::get_if_active(dev)?;
> +        Ok(Self(Scope::<Retain> {
> +            dev,
> +            mode: Mode(ModeFlag::Sync as u32),
> +            _tag: PhantomData,
> +        }))
> +    }
> +
> +    fn release_inner(&self) {
> +        Request::put_noidle(self.0.dev);

What's the reason for using put_noidle here? It doesn't queue
autosuspend, so wouldn't the try_hold_active pattern (i.e. grab if
active, do something, drop) leave the device active until something
else triggers a suspend?

> +/// SAFETY:
> +/// bindings::dev_pm_ops is #[repr(C)], implements Default
> +/// and the struct itself is all nullable function pointers.
> +/// There is no padding and all zero bit-pattern is valid
> +///
> +pub const PMOPS_NONE: bindings::dev_pm_ops =
> +    unsafe { core::mem::MaybeUninit::<bindings::dev_pm_ops>::zeroed().assume_init() };

The safety comment shouldn't be a doc comment.

> +/// Runtime PM context tied to a device.
> +pub struct PMContext<'a, D: driver::DriverLayout, T: PMOps<D>> {
> +    // Preferably, PMContext could be shared via borrowed reference over
> +    // a pm Registration's lifetime but that bares complications on its own
> +    // when the context needs to be shared across different Registration types.

Nit: bares -> bears. Also in the identical comment below.

> +impl<'a, D: driver::DriverLayout, T: PMOps<D>> PMContext<'a, D, T> {
> +    /// Driver-provided runtime PM operations.
> +    ///
> +    /// A driver implements this trait to handle runtime PM
> +    /// transitions for its device type.
> +    ///
> +    /// Each callback receives the device and the current payload.
> +    /// On success, it returns the payload to keep for the next
> +    /// transition. On failure, it returns the payload together
> +    /// with the error so the previous, or otherwise sane state
> +    /// can be preserved.
> +    pub const PM_OPS: bindings::dev_pm_ops = bindings::dev_pm_ops {
> +        runtime_resume: if T::HAS_RUNTIME_RESUME {
> +            Some(runtime_resume_callback::<D, T>)
> +        } else {
> +            None
> +        },
> +        runtime_suspend: if T::HAS_RUNTIME_SUSPEND {
> +            Some(runtime_suspend_callback::<D, T>)
> +        } else {
> +            None
> +        },
> +        ..PMOPS_NONE
> +    };
> +
> +    /// Enable runtime PM
> +    pub fn enable(&self, state: RuntimePMState) -> Result {
> +        if self.inner.enabled.cmpxchg(false, true, ordering::Full).is_err() {
> +            return Err(EBUSY);
> +        }
> +        Self::apply_config(self.inner.dev, &self.inner.configs);
> +        match state {
> +            RuntimePMState::RESUMED => Request::mark_active(self.inner.dev),
> +            RuntimePMState::SUSPENDED => Request::mark_suspended(self.inner.dev),
> +            _ => Err(EINVAL),
> +        }.inspect_err(|_| self.inner.enabled.store(false, ordering::Release))?;

This always applies the config even if state is invalid. Should we
validate the state before making changes?

> +    /// Runs a closure while holding an `AwakeScope`.
> +    pub fn with_get<R>(&self, profile: PMProfile, f: impl FnOnce() -> Result<R>) -> Result<R> {
> +        if profile.0.contains(ModeFlag::Async) {
> +            return Err(EINVAL);
> +        }
> +        let _scope = self.get(profile)?;
> +        f()
> +    }

Shouldn't this reject NoWait too to make sure the device will actually
be powered when the closure is executed?

Sami