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

[Beata Michalska <beata.michalska@arm.com>]

Hi,

First of all apologies for such a late response.
I'm currently on leave and other things have kept me busy enough.

On Sun, May 17, 2026 at 02:08:07AM +0200, Danilo Krummrich wrote:
> (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.
>
For the latter, indeed that makes little sens, agreed.
For the first one, PM core already makes sure that no PM transition request
is being carried out for devices without active binding.
This is being achieved via the disable_depth which is updated on
__pm_runtime_disable.
Now I do understand you would like to enforce the contact on the Rust side as
well.
> > +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.
> 
Point taken. 
> 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.
 I appreciate your comments.
 Will try to amend the current version and add along necessary changes,
 though there were no comments on the overall idea yet.

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