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

[Suzuki K Poulose]

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@xxxxxxx>
> > Signed-off-by: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
>
> 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@xxxxxxxxxxxxxxxx>
>
> > ---
> > 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