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

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

On 19/09/2026 02:27, Jonathan Cameron wrote:
>> 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>
> 
> With the declaration moved as per standard way of dealing with __free
> magic (there are docs in cleanup.h on this)
> 
> Reviewed-by: Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>
> 
>> ---
>> Changes since v17:
>>   * Inline RMM_ACTIVATE command and remove the definitions from arm-rmi-cmds.h
>>   * Use scope-based cleanup to free sro object
>> Changes since v16:
>>   * Split into a new patch
>> ---
>>   drivers/firmware/arm_rmm/rmi.c | 14 +++++++++++++-
>>   1 file changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/firmware/arm_rmm/rmi.c b/drivers/firmware/arm_rmm/rmi.c
>> index 4f9898ece754..ecc89e91d264 100644
>> --- a/drivers/firmware/arm_rmm/rmi.c
>> +++ b/drivers/firmware/arm_rmm/rmi.c
>> @@ -762,6 +762,7 @@ static int rmi_configure(void)
>>   static int __init arm64_init_rmi(void)
>>   {
>>   	int ret;
>> +	struct rmi_sro_state *sro __free(kfree) = NULL;
>>
> Read the guidance in cleanup.h
> 
> This is a path to a potentially grumpy Linus.  He's made the sensible
> point a number of times that putting the declaration of the destructor
> anywhere that isn't right next to the contstructor is a recipe for
> hard to read code and odd bugs (this one isn't buggy,
> but the principle is more general and the readability thing does hold!)

Ack

> 
>>   	/* Continue without realm support if we can't agree on a version */
>>   	ret = rmi_check_version();
>> @@ -776,7 +777,18 @@ static int __init arm64_init_rmi(void)
>>   	if (ret)
>>   		return ret;
>>   
>> -	return 0;
>> +	/* Activate the RMM */
>> +	sro = kmalloc_obj(*sro);
> 
> struct rmi_sro_state *sro __free(kfree) = kmalloc_obj(*sro);

Thanks

> 
>> +	if (!sro)
>> +		return -ENOMEM;
>> +
>> +	ret = rmi_sro_memxfer_cmd(sro, GFP_KERNEL, SMC_RMI_RMM_ACTIVATE);
>> +	if (ret) {
>> +		pr_err("RMM activate failed\n");
>> +		ret = ret < 0 ? ret : -ENXIO;
>    		return ret < 0 ? ...
>> +	}
> 
> return 0;
> Perhaps to make it more obvious that any non 0 return means error?

We want to differentiate between the linux errnos vs RMM errors 
(positive). I will rework this to address the RMI_BLOCKED comment
from Sudeep.

Cheers
Suzuki


> 
> Doesn't really matter
>