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

[Jonathan Cameron]

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

>  	/* 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);

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

Doesn't really matter

-- 
Jonathan Cameron <jonathan.cameron@xxxxxxxxxxxxxxxx>