Re: [PATCH v18 4/7] firmware: arm_rmm: Add support for SRO

[Gavin Shan]

Hi Suzuki,

On 9/14/26 4:22 PM, Suzuki K Poulose wrote: 
> On 14/09/2026 06:04, Gavin Shan wrote:
> > On 9/12/26 6:36 PM, Suzuki K Poulose wrote:
> > > RMM v2.0 introduces the concept of "Stateful RMI Operations" (SRO). This
> > > means that an SMC can return with an operation still in progress. The
> > > host is expected to continue the operation until it reaches a conclusion
> > > (either success or failure). During this process the RMM can request
> > > additional memory ('donate') or hand memory back to the host
> > > ('reclaim'). The host can request an in progress operation is cancelled,
> > > but still continue the operation until it has completed (otherwise the
> > > incomplete operation may cause future RMM operations to fail).
> > >
> > > The SRO is tracked using a struct rmi_sro_state object which keeps track
> > > of any memory which has been allocated but not yet consumed by the RMM
> > > or reclaimed from the RMM. This allows the memory to be reused in a
> > > future request within the same operation. It will also permit an
> > > operation to be done in a context where memory allocation may be
> > > difficult (e.g. atomic context) with the option to abort the operation
> > > and retry the memory allocation outside of the atomic context. The
> > > memory stored in the struct rmi_sro_state object can then be reused on
> > > the subsequent attempt.
> > >
> > > Wrappers for SRO RMI commands are also provided here because they depend
> > > on the rmi_sro_execute() implementation added by this patch.
> > > Delegate/undelegate handles are also added here because they now use the
> > > SRO/stateful command infrastructure and are also used for the memory
> > > DONATE/RECLAIM flows.
> > >
> > > Signed-off-by: Steven Price <steven.price@xxxxxxx>
> > > Co-Developed-by: Suzuki K Poulose <suzuki.poulose@xxxxxxx>

scripts/checkpatch.pl recommends s/Co-Developed-by/Co-developed-by, the same
format issue exists in other patches and please double check.

> > > Signed-off-by: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
>
>
>
> > > ---
> > >   drivers/firmware/arm_rmm/rmi.c | 586 +++++++++++++++++++++++++++++++++
> > >   include/linux/arm-rmi-cmds.h   |  41 +++
> > >   2 files changed, 627 insertions(+)
> > >
> > > diff --git a/drivers/firmware/arm_rmm/rmi.c b/drivers/firmware/ arm_rmm/rmi.c
> > > index 5b0e342ce3d58..4f9898ece7547 100644
> > > --- a/drivers/firmware/arm_rmm/rmi.c
> > > +++ b/drivers/firmware/arm_rmm/rmi.c

[...]

> > > +
> > > +static void rmi_op_continue(unsigned long sro_handle, unsigned long flags,
> > > +                struct arm_smccc_1_2_regs *out_regs)
> > > +{
> > > +    *out_regs = (struct arm_smccc_1_2_regs) {
> > > +        SMC_RMI_OP_CONTINUE, sro_handle, flags
> > > +    };
> > > +
> > > +    rmi_smccc_invoke(out_regs);
> > > +}
> > > +
> >
> > The pattern 'regs' is used in some of the 'struct arm_smccc_1_2_regs' arguments
> > or variables in this series, which is incosistent to the existing patterns which
> > is either 'args' or 'res' by searching the source files using 'git grep arm_smccc_1_2_regs'.
> > So I would suggest we have the fixed the pattern 'args' :-)
>
>
> I would prefer to keep it "regs" as, unlike the smccc_1_1 calls, we
> pass "arm_smccc_1_2_regs" for both arguments and results. In this case
> we are using a single structure, so, to avoid the confusion, I
> intentionally used regs

It's fine to keep "regs" pattern, then the only place using "args" is
rmi_smccc_invoke(). I think the variable or argument names in rmi_smccc_invoke()
can be improved there to use "regs" pattern. With this, we have the unified
pattern "regs".

static inline void rmi_smccc_invoke(struct arm_smccc_1_2_regs *pregs)
{
        struct arm_smccc_1_2_regs regs = *pregs;
		:
}

Thanks,
Gavin