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

["Danilo Krummrich" <dakr@kernel.org>]

On Mon Oct 26, 2026 at 1:05 PM CET, Beata Michalska wrote:
>  drivers/base/base.h             |    4 +
>  rust/bindings/bindings_helper.h |    1 +
>  rust/helpers/helpers.c          |    1 +
>  rust/helpers/pm_runtime.c       |   89 +++
>  rust/kernel/lib.rs              |    1 +
>  rust/kernel/pm.rs               | 1106 +++++++++++++++++++++++++++++++

I think this fits better under rust/kernel/device/pm.rs. This would also avoid
naming like DevPMOps, which can then just be device::PMOps.

We should also add the file to POWER MANAGEMENT CORE in MAINTAINERS.

> diff --git a/drivers/base/base.h b/drivers/base/base.h
> index a5b7abc10ff0..e79108bc754a 100644
> --- a/drivers/base/base.h
> +++ b/drivers/base/base.h
> @@ -103,6 +103,7 @@ struct driver_private {
>   *			   dev_err_probe() for later retrieval via debugfs
>   * @device: pointer back to the struct device that this structure is
>   *	    associated with.
> + * @rust_private: private data used exclusively by the Rust driver core
>   * @dead: This device is currently either in the process of or has been
>   *	  removed from the system. Any asynchronous events scheduled for this
>   *	  device should exit without taking any action.
> @@ -119,6 +120,9 @@ struct device_private {
>  	const struct device_driver *async_driver;
>  	char *deferred_probe_reason;
>  	struct device *device;
> +#ifdef CONFIG_RUST
> +	void *rust_private;

Let's call this pointer pm_registration_data_rust. Or even just add

	struct {
		void *pm_registration_data;
	} rust;
	
> +use core::{cell::UnsafeCell, marker::PhantomData, mem::ManuallyDrop};

NIT: Formatting.

> +impl From<Mode> for core::ffi::c_int {
> +    #[inline]
> +    fn from(mode: Mode) -> core::ffi::c_int {
> +        mode.0 as core::ffi::c_int
> +    }
> +}

You can just use c_int, as it comes from the prelude. Also note that the prelude
takes it from ::ffi, rather than ::core.

> +    /// Explicitly release the scope
> +    /// This should be used in favor of regular drop
> +    /// when error handling is required.

NIT:

	// Explicitly release the scope.
	//
	// This should be used in favor of regular drop when error handling is
	// required.

> +    pub fn release(self) -> Result {
> +        self.release_inner();
> +        core::mem::forget(self);
> +        Ok(())
> +    }
> +}
> +
> +impl<'a> Drop for RetainScope<'a> {
> +    fn drop(&mut self) {
> +        self.release_inner();
> +    }
> +}
> +
> +/// Runtime PM helpers - wrappers around C runtime PM interface.
> +/// All methods require a reference to a bound device.

I'd rather say a few words about what those helpers are. The fact they require a
&Device<Bound> is rather obvious from the signature; if at all I'd rather
mention why that is.

> +struct Request;
> +
> +impl Request {
> +    #[inline]
> +    fn active(dev: &device::Device<device::Bound>) -> bool {
> +        // SAFETY: `dev.as_raw()` must provide a valid pointer to
> +        // `struct device` for the duration of the call.

This reads like a requirement, but it rather is the justifiation for this
requirement coming from pm_runtime_active().

> +        // The `Device<Bound>` reference provides that guarantee.

Plus that we are in a scope where the device is guaranteed that the device
remains bound to the driver, which is a requirement as well.

> +/// Common runtime PM callback entry point
> +///
> +/// The generated extern "C" callbacks call into this helper with the raw
> +/// `struct device *` provided by the PM core. It rebuilds the Rust device
> +/// reference, retrieves the device's PM registration data and performs
> +/// handoff to corresponding driver callback.
> +#[cfg(CONFIG_PM)]
> +fn runtime_pm_callback<T, F>(dev: *mut bindings::device, cb: F) -> Result

This function has to be unsafe, given that it takes a raw pointer.

> +where
> +    T: PMOps,
> +    F: FnOnce(
> +        &<T as PMOps>::DeviceType,
> +        <T as PMOps>::RuntimePayloadType,
> +    ) -> PMCallbackResult<<T as PMOps>::RuntimePayloadType>,
> +{
> +    let dev: &device::Device<device::Bound>  =
> +             // SAFETY: `dev` is provided by the PM core and remains
> +             // valid for the duration of the callback.
> +            unsafe { device::Device::from_raw(dev) };

Please prefer Device::from_raw() plus Device::as_bound().

> +
> +    // SAFETY: `dev` is provided by the PM core and remains
> +    // valid for the duration of the callback.
> +    let ptr = unsafe { (*(*dev.as_raw()).p).rust_private };
> +
> +    if ptr.is_null() {
> +        return Err(ENODEV);
> +    }

How can we ever reach this?

> +/// Typed reference to a driver's runtime PM ops.
> +///
> +/// Instances of this type are generated from static [`DevPMOps`]
> +/// and are used by bus registration code to install the matching
> +/// C `dev_pm_ops` pointer into the driver object.
> +/// The `A` parameter identifies the bus adapter that is allowed
> +/// to install the table.
> +pub struct DevPMOpsRef<'a, A> {
> +    #[cfg_attr(not(CONFIG_PM), allow(unused))]
> +    ops: &'a bindings::dev_pm_ops,
> +    _p: PhantomData<fn() -> *const A>,
> +}

I'd probably rename this to pm::AdapterOpsRef<'a, A> and add a type alias:

	pub type OpsRef<T> = AdapterOpsRef<'static, <T as PMOps>::Adapter>;

so it looks cleaner in drivers:

	 const DEV_PM_OPS: Option<device::pm::OpsRef<Self>> = Some(TYR_PM_OPS.pm_ops());

(I'd probably also rename DevPMOps to device::pm::Ops, as I think that reads
cleaner.)

> +/// Runtime PM callbacks implemented by a driver.
> +///
> +/// Defines the [`PMOps`] trait and its corresponding [`bindings::dev_pm_ops`].
> +///
> +/// The Adapter associated type identifies the bus adapter that installs
> +/// the generated PM ops.
> +/// Implementations should normally be enabled through a
> +/// bus-specific constructor for [`DevPMOps`], which constrains
> +/// [`Self::DeviceType`] to the correct bound device type for that bus.
> +///
> +/// Each C callback recovers the Rust bus device from the raw
> +/// `struct device *`, borrows the registered runtime PM payload, and delegates
> +/// to the matching [`PMOps`] trait method.

This kind of implementation detail doesn't belong in this doc-comment.

> +#[vtable]
> +pub trait PMOps: Sized {
> +    /// The bus adapter that installs the generated PM ops.
> +    type Adapter: driver::DriverLayout + 'static;
> +
> +    /// Type of the bound bus device passed to runtime PM callbacks.
> +    type DeviceType: AsBusDevice<device::Bound>;
> +
> +    /// Type of the payload moved through runtime PM transitions.
> +    type RuntimePayloadType: Send + 'static;

I got back to this type after writing the below, because at first I thought this
is purely the data type that is cycled through suspend() and resume().

However, reading further this is currently the Registration's private data. This
won't work. The Registration's private data must not require 'static as we need
device resources in this data.

So, there should be at least two separate types:

  (1) The private data of pm::Registration.

  (2) An object that is created and returned by suspend() and passed by value to
      resume(), so it can store e.g. reverse guard types (see below).

This should also get us rid of the PMPayload magic.

> +
> +    /// Runtime resume callback.

It would be good to expand this doc-comment explaining the semantics of
RuntimePayloadType.

> +    fn runtime_resume<'a>(
> +        _dev: &'a Self::DeviceType,
> +        _payload: Self::RuntimePayloadType,
> +    ) -> PMCallbackResult<Self::RuntimePayloadType> {
> +        build_error!(VTABLE_DEFAULT_ERROR)
> +    }
> +
> +    /// Runtime suspend callback.
> +    fn runtime_suspend<'a>(
> +        _dev: &'a Self::DeviceType,
> +        _payload: Self::RuntimePayloadType,
> +    ) -> PMCallbackResult<Self::RuntimePayloadType> {
> +        build_error!(VTABLE_DEFAULT_ERROR)
> +    }

I think for Tyr it so far is sufficient to have a single RuntimePayloadType that
keeps cycling through the callbacks, but there will be use-cases where the types
differ.

For instance, for a PCI driver you will have DeviceDisableGuard stored in
suspend() and then you'll want to drop it in resume() so the device gets enabled
again.

We should also avoid requiring 'static, as reverse guards will borrow the
regular guard, e.g. pci::DeviceDisableGuard will borrow from
pci::DeviceEnableGuard.

For runtime_resume() I think of something like:

	fn runtime_resume<'a>(
	    dev: &Self::DeviceType,
	    suspended: Self::SuspendData<'a>,
	) -> Result<(), (Self::SuspendData<'a>, Error)>;


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

Since we now have self-referencial pin-init, can we please avoid this Arc?

> +impl<'a, T: PMOps> Registration<'a, T> {
> +    /// Creates a runtime PM registration for `dev`.
> +    ///
> +    /// The provided profiles and configuration are stored in the associated
> +    /// [`PMContext`]. The `payload` is stored as a Registration data
> +    /// and is used to service PM transitions.
> +    ///
> +    /// The device must use the callback represented by `ops`, generated
> +    /// for the same bus adapter and driver pair `(D, T)`.
> +    ///
> +    /// # Safety
> +    ///
> +    /// The returned [`Registration`] object cannot be mem::forgotten,
> +    /// or it's [`Drop`] implementation cannot be skipped in any way.
> +    ///
> +    pub unsafe fn new(
> +        dev: &'a device::Device<device::Core<'_>>,
> +        ops: &'static DevPMOps<T>,
> +        profiles: Option<KVec<PMProfile>>,
> +        configs: Option<KVec<PMConfig>>,
> +        payload: T::RuntimePayloadType,

This should be impl PinInit<T::PMRegistrationData<'a>, E> + 'a.

> +    ) -> Result<Self> {

This should be impl PinInit<Self, Error> + 'a.

> +        // SAFETY: For the duration of this call, `dev` is a valid `Device<Core>`,
> +        // and so is its raw `struct device` pointer.
> +        unsafe {

Please use separate unsafe blocks per unsafe operation.

> +            let drv = (*dev.as_raw()).driver;
> +            if drv.is_null() || (*drv).pm != ops.as_raw() {

drv.is_null() can't happen as we require Device<Core>.

> +                return Err(EINVAL);
> +            }
> +        }
> +
> +        let payload = KBox::new(
> +            RegistrationData::<T> {
> +                data: PMPayload {
> +                    in_flight: AtomicFlag::new(false),
> +                    inner: UnsafeCell::new(ManuallyDrop::new(payload)),
> +                },
> +                _marker: PhantomData,
> +            },
> +            GFP_KERNEL,
> +        )?;
> +
> +        let inner_ctx = Arc::new(
> +            PMContextInner {
> +                dev,
> +                state: Atomic::new(PMContextState::Disabled),
> +                profiles: profiles.unwrap_or_default(),
> +                configs: configs.unwrap_or_default(),
> +                _marker: PhantomData,
> +            },
> +            GFP_KERNEL,
> +        )?;
> +
> +        // SAFETY: For the duration of this call, `dev` is a valid `Device<Core>`,
> +        // and so is its raw `struct device` pointer.
> +        // The payload allocation is converted into a foreign pointer
> +        // and owned by this `Registration` until `Drop` clears
> +        // `rust_private` and reconstructs the `KBox`.
> +        unsafe {

Same as above.

> +            let ptr = (*(*dev.as_raw()).p).rust_private;

This is accessed a couple of time, let's add a helper for this.

> +            if !ptr.is_null() {
> +                return Err(EBUSY);
> +            }
> +            (*(*dev.as_raw()).p).rust_private = payload.into_foreign();
> +        }
> +
> +        Ok(Self {
> +            ctx: PMContext { inner: inner_ctx },
> +        })
> +    }