Re: [PATCH v18 6/7] firmware: arm_rmm: Ensure the RMM has GPT entries for memory

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

On 21/09/2026 22:58, Jonathan Cameron wrote:
> 
>>>>>    static int __init arm64_init_rmi(void)
>>>>>    {
>>>>>        int ret;
>>>>> @@ -786,8 +970,24 @@ static int __init arm64_init_rmi(void)
>>>>>        if (ret) {
>>>>>            pr_err("RMM activate failed\n");
>>>>>            ret = ret < 0 ? ret : -ENXIO;
>>>>> +        return ret;
>>>>
>>>> Why did this change?
>>>
>>> Rebase messed up. I will restore it.
>>
>> Actually this is not. We dont have to check the metadata if
>> we couldn't activate the RMM. Also, the failure path at the
>> bottom has "deactivate", which again is not needed. So
>> it is the right thing to do.
> 
> Only after this patch? Not from the previous patch?
> 
>>
>>>    
>>>>   
>>>>>        }
>>>>> +    ret = rmi_init_metadata();
>>>>> +    if (ret)
>>>>
>>>> And this is hitting another bit of guidance in cleanup.h.
>>>> Functions shouldn't be mixing __free and friends with
>>>> gotos.  Again, not a bug here but there are large ugly
>>>> monsters around this stuff, hence the blanket guidance.
>>>> I haven't thought that hard on how you avoid it here, but
>>>> usually it's a combination of suitable helpers and wrappers
>>>> and resulting code is often more readable as a result.
>>
>> I could change the hunk to something like, but that looks ugly.
>>
>> @@ -1010,20 +1010,12 @@ static int __init arm64_init_rmi(void)
>>                   return ret;
>>           }
>>
>> -       ret = rmi_init_metadata();
>> -       if (ret)
>> -               goto out_deactivate;
>> +       if (!rmi_init_metadata() &&
>> !register_memory_notifier(&rmi_memory_nb)) {
>> +               arm64_rmi_is_available = true;
>> +               pr_info("RMI configured\n");
>> +               return 0;
>> +       }
>>
>> -       ret = register_memory_notifier(&rmi_memory_nb);
>> -       if (ret)
>> -               goto out_deactivate;
>> -
>> -       arm64_rmi_is_available = true;
>> -       pr_info("RMI configured\n");
>> -
>> -       return 0;
>> -
>> -out_deactivate:
>>           WARN_ON(rmi_sro_memxfer_cmd(sro, GFP_KERNEL,
>> SMC_RMI_RMM_DEACTIVATE));
>>           return ret;
>>    }
>>
>>
>> Either ways, we have to cleanup the object on return, no matter
>> the route we take. So the original form is much more readable
>> for me.
> 
> Agree to more readable, but that fragility of mixing __free() and
> goto is a real problem that has tripped many folk up - hence
> the perhaps overly strict guidance.  Rather than avoiding the goto, I'd just
> not use __free()  - go old school and have two labels for errors
> and an extra manual free in the good path.
> 
> 	pr_info("RMI configured\n:);
> 	kfree(sro);
> 
> 	return 0;
> 
> out_deactivate:
> 	WARN_ON(rmi_sro_memxfer_cmd(sro, GFP_KERNEL, SMC_RMI_RMM_DEACTIVATE));
> out_free_sro:
> 	kfree(sro);
> 	return ret;
> }
> 
> Sometime the new toys aren't the right answer.

I have the following hunk on top of this patch, that could do the trick.

diff --git a/drivers/firmware/arm_rmm/rmi.c b/drivers/firmware/arm_rmm/rmi.c
index bcdbed26cf08d..4385f49068965 100644
--- a/drivers/firmware/arm_rmm/rmi.c
+++ b/drivers/firmware/arm_rmm/rmi.c
@@ -1033,20 +1033,19 @@ static int __init arm64_init_rmi(void)
                 return ret;
         }

-       ret = rmi_init_metadata();
-       if (ret)
-               goto out_deactivate;
-
-       ret = register_memory_notifier(&rmi_memory_nb);
-       if (ret)
-               goto out_deactivate;
-
-       arm64_rmi_is_available = true;
-       pr_info("RMI configured\n");
-
-       return 0;
-
-out_deactivate:
+       do {
+               ret = rmi_init_metadata();
+               if (ret)
+                       break;
+               ret = register_memory_notifier(&rmi_memory_nb);
+               if (ret)
+                       break;
+               arm64_rmi_is_available = true;
+               pr_info("RMI configured\n");
+               return 0;
+       } while (0);
+
+       /* De-activate the RMM and reclaim any donated memory */
         WARN_ON(rmi_sro_memxfer_cmd(sro, GFP_KERNEL, 
SMC_RMI_RMM_DEACTIVATE));
         return ret;

Cheers
Suzuki

> 
> Jonathan
> 
> 
> 
> 
>>
>> Cheers
>> Suzuki
>>
>>>>   
>>>
>>> I will see if I can improve it.
>>>
>>> Cheers
>>> Suzuki
>>>    
>>
>