Re: [PATCH v17 5/7] firmware: arm_rmm: Activate the RMM

[Suzuki K Poulose]

On 09/09/2026 05:29, Gavin Shan wrote:
> Hi Suzuki,
>
> On 9/7/26 7:59 PM, Suzuki K Poulose wrote:
> > From: Steven Price <steven.price@xxxxxxx>
> >
> > Activate the RMM after the basic configuration. This is a memory 
> > transferring,
> > stateful operation.
> >
> > Signed-off-by: Steven Price <steven.price@xxxxxxx>
> > Signed-off-by: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
> > ---
> > Changes since v16:
> >   * Split into a new patch
> > ---
> >   drivers/firmware/arm_rmm/rmi.c | 17 +++++++++++++++--
> >   include/linux/arm-rmi-cmds.h   | 11 +++++++++++
> >   2 files changed, 26 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/firmware/arm_rmm/rmi.c b/drivers/firmware/ 
> > arm_rmm/rmi.c
> > index 42c973c3a98bb..d969c8738efde 100644
> > --- a/drivers/firmware/arm_rmm/rmi.c
> > +++ b/drivers/firmware/arm_rmm/rmi.c
> > @@ -641,7 +641,8 @@ static int rmi_configure(void)
> >   static int __init arm64_init_rmi(void)
> >   {
> > -    int ret;
> > +    int ret = 0;
> > +    struct rmi_sro_state *sro = NULL;
>
> Why we need to initialize those local variables ('ret' and 'sro')? :-)
> Also, we can use the scope-based cleanup function supported by cleanup.h
> for @sro.
>
>      struct smi_sro_state *sro __free(kfree) = NULL;
>
> With the scope-based cleanup in place, we needn't explict 'kfree(sro)' at
> end of this function.

Ack.

> >       /* Continue without realm support if we can't agree on a version */
> >       ret = rmi_check_version();
> > @@ -656,7 +657,19 @@ static int __init arm64_init_rmi(void)
> >       if (ret)
> >           return ret;
> > -    return 0;
> > +    /* Activate the RMM */
> > +    sro = kmalloc_obj(*sro);
> > +    if (!sro)
> > +        return -ENOMEM;
>
>      if (!sro) {
>          pr_err("Unable to alloc SRO object for RMM activation\n");
>          return -ENOMEM;
>      }

Ack

> > +
> > +    ret = rmi_rmm_activate(sro);
> > +    if (ret) {
> > +        pr_err("RMM activate failed\n");
> > +        ret = ret < 0 ? ret : -ENXIO;
> > +    }
> > +
> > +    kfree(sro);
> > +    return ret;
> >   }
> >   /*
> > diff --git a/include/linux/arm-rmi-cmds.h b/include/linux/arm-rmi-cmds.h
> > index fed0f3421895d..dea7c7004d35f 100644
> > --- a/include/linux/arm-rmi-cmds.h
> > +++ b/include/linux/arm-rmi-cmds.h
> > @@ -64,6 +64,17 @@ static inline int rmi_rmm_config_set(unsigned long 
> > cfg_ptr)
> >       return res.a0;
> >   }
> > +/**
> > + * rmi_rmm_activate() - Activate the RMM
> > + * @sro: Preallocated SRO context to be used
> > + *
> > + * Return: 0 on success, positive RMI result code or negative Linux 
> > error code
> > + */
> > +static inline long rmi_rmm_activate(struct rmi_sro_state *sro)
> > +{
> > +    return rmi_sro_memxfer_cmd(sro, GFP_KERNEL, SMC_RMI_RMM_ACTIVATE);
> > +}
> > +
>
> rmi_rmm_acvivate() would be used for once by rmi.c::arm64_init_rmi(). Lets
> not expose the function by combining the logic to rmi.c::arm64_init_rmi().

Have done that already in my local tree.


Thanks for the review.

Cheers
Suzuki