Re: [PATCH v2 2/3] rust: platform: wire runtime PM callbacks

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

On Tue, Aug 04, 2026 at 09:16:11AM +0000, Alice Ryhl wrote:
> On Tue, Jul 21, 2026 at 05:34:03PM +0200, Beata Michalska wrote:
> > Allow Rust platform drivers to expose runtime PM callbacks to the driver core.
> > 
> > The runtime PM abstraction builds a dev_pm_ops table for the concrete driver
> > implementation, but the platform bus still needs to receive that table through
> > struct platform_driver. Add an optional PM_OPS associated constant to
> > platform::Driver and initialize the coresponding C device_driver struct
> > accordingly during registration. The platform glue only wires the callback
> > table into the C driver model; ownership of the callback payload and
> > runtime PM teardown remain with the pm module.
> > 
> > Signed-off-by: Beata Michalska <beata.michalska@arm.com>
> > ---
> >  rust/kernel/platform.rs | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/rust/kernel/platform.rs b/rust/kernel/platform.rs
> > index d8d48f60b0b9..b7e422388634 100644
> > --- a/rust/kernel/platform.rs
> > +++ b/rust/kernel/platform.rs
> > @@ -72,6 +72,11 @@ unsafe fn register(
> >              None => core::ptr::null(),
> >          };
> >  
> > +        let pm_ops = match T::PM_OPS {
> > +            Some(ops) => ops,
> > +            None => core::ptr::null(),
> > +        };
> > +
> >          // SAFETY: It's safe to set the fields of `struct platform_driver` on initialization.
> >          unsafe {
> >              (*pdrv.get()).driver.name = name.as_char_ptr();
> > @@ -79,6 +84,7 @@ unsafe fn register(
> >              (*pdrv.get()).remove = Some(Self::remove_callback);
> >              (*pdrv.get()).driver.of_match_table = of_table;
> >              (*pdrv.get()).driver.acpi_match_table = acpi_table;
> > +            (*pdrv.get()).driver.pm = pm_ops;
> >          }
> >  
> >          // SAFETY: `pdrv` is guaranteed to be a valid `DriverType`.
> > @@ -222,6 +228,9 @@ pub trait Driver {
> >      /// The table of ACPI device ids supported by the driver.
> >      const ACPI_ID_TABLE: Option<acpi::IdTable<Self::IdInfo>> = None;
> >  
> > +    /// Runtime PM callbacks
> > +    const PM_OPS: Option<&'static bindings::dev_pm_ops> = None;
> > +
> >      /// Platform driver probe.
> >      ///
> >      /// Called when a new platform device is added or discovered.
> 
> I've been thinking more about this, and I can't help but wonder whether
> we could significantly simplify it. Why not just do this:
> 
> 1. Update rust/kernel/platform.rs Driver trait to include pm ops
> directly in the trait:
> 
> 	#[vtable]
> 	pub trait Driver {
> 	    type IdInfo: 'static;
> 	    type Data<'bound>: Send + 'bound;
> 	    const OF_ID_TABLE: Option<of::IdTable<Self::IdInfo>> = None;
> 	    const ACPI_ID_TABLE: Option<acpi::IdTable<Self::IdInfo>> = None;
> 	
> 	    fn probe<'bound>(
> 	        dev: &'bound Device<device::Core<'_>>,
> 	        id_info: Option<&'bound Self::IdInfo>,
> 	    ) -> impl PinInit<Self::Data<'bound>, Error> + 'bound;
> 	
> 	    fn unbind<'bound>(dev: &'bound Device<device::Core<'_>>, this: Pin<&Self::Data<'bound>>) {
> 	        let _ = (dev, this);
> 	    }
> 	
> 	    // Add these methods.
> 	    fn runtime_suspend<'bound>(
> 	        dev: &'bound Device<device::Bound>,
> 	        data: &Self::Data<'bound>,
> 	    ) -> Result
> 	    {
> 	        build_error!(VTABLE_DEFAULT_ERROR)
> 	    }
> 	
> 	    fn runtime_resume<'bound>(
> 	        dev: &'bound Device<device::Bound>,
> 	        data: &Self::Data<'bound>,
> 	    ) -> Result
> 	    {
> 	        build_error!(VTABLE_DEFAULT_ERROR)
> 	    }
> 	}
> 
> By marking the trait with #[vtable], we know whether the user has
> overridden runtime_suspend() and runtime_resume() and then the platform
> abstraction can enable PM in that scenario:
> 
>  - If `T::HAS_RUNTIME_SUSPEND && T::HAS_RUNTIME_RESUME` then set
>    `(*pdrv.get()).driver.pm` to a table using those methods in
>    register().
>  - If `T::HAS_RUNTIME_SUSPEND && T::HAS_RUNTIME_RESUME` then invoke
>    `pm_runtime_enable()` from `probe_callback()` after the
>    `set_callback()` line. Note that this call is infallible.
>  - Do the same from unplug to disable PM.
> 
> And then you automatically PM whenever you implement those two methods
> in the `platform::Driver` trait, and that's all you need to do. Since we
> invoke `pm_runtime_enable()` in `probe_callback()` after setting the
> private data, there's no issue with passing the device private data to
> the callbacks.
> 
> Note that we can trigger a const eval panic on `T::HAS_RUNTIME_SUSPEND
> != T::HAS_RUNTIME_RESUME` to enforce that you must implement both
> methods if you implement either one.
> 
> Thoughts? I know we have gone down this route before, but I just think
> it would be so so much simpler than the current approach. I know that
> this diverges from IRQ and such by not having a Registration, but I
> actually think that's ok. PM is already different from IRQ callbacks in
> the sense that the platform abstractions need PM-specific code *anyway*
> to properly set pm_ops in the `struct platform_device`.
> 

This is a sound idea, though there are few caveats.
This patchset only introduces runtime_resume and runtime_suspend, though the
range of pm ops is wider than that. So if we go the way you suggest, we need to
accept that all of those would finally end up in that trait's implementation.
Further more this would need to be replicated for other bus devices.
Which makes the implementation pretty noisy.
Offloading enabling runtime PM to bus driver might not be suitable for all
potential users of runtime PM either. Some drivers might want to enable it
while in probe, call device resume and do some early setup. With this approach,
using drvdata is still an issue.
The sample implementation for Tyr is just one way of doing things.

---
BR
Beata
> Alice