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

[Suzuki K Poulose]

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
> > >