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

[Suzuki K Poulose <suzuki.poulose@arm.com>]

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@arm.com>
>>
>> Activate the RMM after the basic configuration. This is a memory 
>> transferring,
>> stateful operation.
>>
>> Signed-off-by: Steven Price <steven.price@arm.com>
>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>> ---
>> 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