Re: [RFC PATCH 1/3] rust: Add runtime PM support

[Danilo Krummrich]

(By far not a full review; but please find a few comments below.)

On Thu May 14, 2026 at 5:09 PM CEST, Beata Michalska wrote:
> +#[cfg(CONFIG_PM)]
> +impl Request {
> +    #[inline(always)]
> +    fn resume(dev: &ARef<device::Device>, mode: Mode) -> Result {

Those functions all require a bound device, so this has to be &Device<Bound>.

Also, there is usually no advantage to write &ARef<T> instead of &T.

> +macro_rules! define_pm_callback {
> +    // Bare callback with no associate transition nor fallible descriptor
> +    (@parse_desc $name:ident) => { define_pm_callback!(@default $name); };
> +    // Callback with associated state transition
> +    (@parse_desc $name:ident ($from:ident, $pre:ident, $post:ident)) => {
> +        define_pm_callback!(@default $name, $from, $pre, $post);
> +    };
> +
> +    (@default $name:ident $(, $from:ident, $pre:ident, $post:ident)? ) => {
> +        paste!(
> +            /// # Safety
> +            ///
> +            /// `dev` must be a valid `struct device *` provided by the PM core.
> +            unsafe extern "C" fn [<$name _callback>]<T:PMOps>(
> +                dev: *mut bindings::device
> +            ) -> core::ffi::c_int {
> +
> +                let dev: &device::Device<device::CoreInternal> =
> +                     // SAFETY: `dev` is provided by the PM core to a runtime PM callback and
> +                     // is valid for the duration of the callback.

This doesn't justify the CoreInternal type state; in fact, we can't justify it
here as those callbacks are not called with the device lock held.

It is guaranteed that the device remains bound during the callbacks though, so
the Bound type state is valid.

> +                    unsafe { device::Device::from_raw(dev) };
> +
> +                // SAFETY: PM callbacks are only invoked while the device is bound
> +                // See [`pm_runtime_remove`](srctree/drivers/base/power/runtime.c)
> +                let bound_dev = unsafe { dev.as_bound() };
> +
> +                bound_dev.drvdata::<T::DriverDataType>()

This function does not exist anymore, please use drvdata_borrow() instead. I
understand you want the type check this accessor does for you, but it shouldn't
be necessary. It should be possible to design the trait in a way that the type
system ensures that a driver cannot pass in the wrong type at compile time.

In order to call drvdata_borrow() we currently need the CoreInternal type state
(which we can't justify).

To support this, we should add a BoundInternal type state, make CoreInternal
deref to BoundInternal and implement drvdata_borrow() for BoundInternal.

> +                    .and_then(|data| {
> +                        let pm_ctx = T::get_pmcontext(data.get_ref())?;
> +                        let pm_dev = match <T as PMOps>::DeviceType::try_from(dev) {

Please don't use a fallible conversion here, take advantage of the type system
instead, e.g. with the AsBusDevice trait, see [1] for reference.

However, as mentioned above, I think it should be possible to tie bus specific
driver traits and this together so we can check types at compile time. So, maybe
AsBusDevice isn't even necessary.

[1] https://lore.kernel.org/driver-core/20260506221027.858481-2-dakr@xxxxxxxxxx/