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

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

On Wed Nov 11, 2026 at 9:45 PM CET, Beata Michalska wrote:
> On Thu, Nov 05, 2026 at 05:47:28PM +0100, Danilo Krummrich wrote:
>> On Mon Oct 26, 2026 at 1:05 PM CET, Beata Michalska wrote:
>> > +#[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.
>> 
> The `'static' lifetime is definitely a shortsighted idea on my side.
> Splitting the data does seem reasonable, though I'm not sure how that would allow
> us to get rid of moving around the `PMPayload`, as this is what I assume you
> are referring to by 'magic'. The pm framework still needs to keep the callback
> data between the transitions. Also I am not convinced the pm should distinguish
> between suspend and resume data - my thinking was it should be handled by the
> driver. We just need 'an' object to move back and forth.

For data that does outlive all the suspend/resume cycles (i.e. data that lives
as long as the pm::Registration), it should go into pm::Ops::RegistrationData.

For Tyr this would e.g. be:

	let runtime_payload = TyrPmRegistrationData {
	    clks: Clocks {
	        core: core_clk,
	        stacks: stacks_clk,
	        coregroup: coregroup_clk,
	    },
	    _regulators: Regulators {
	        _mali: mali_regulator,
	        _sram: sram_regulator,
	    },
	};

pm::Ops::SuspendData and pm::Ops::ResumeData (if needed) should only live
throughout suspend and resume, so they can carry e.g. reverse guard types such
as pci::DeviceDisableGuard, which should only live during a a suspend (it can't
live in resume data for obvious reasons, so a single type is not sufficient).

If we have both, pm::Ops::SuspendData and pm::Ops::ResumeData, nothing prevents
drivers to make those types share fields or even be identical, so you can still
cycle things around. Although I don't really see what would be the use case of
that.

I assume that my below function signature caused some confusion, because I
messed it up. What I actually meant is:

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

>> > +
>> > +    /// 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)>;
>>