Re: [PATCH 10/14] powerpc/rtas: replace rtas_call_unlocked with raw_rtas_call

[Laurent Dufour <ldufour@linux.ibm.com>]

On 08/03/2022, 14:50:43, Nicholas Piggin wrote:
> Use the same calling and rets return convention with the raw rtas
> call rather than requiring callers to load and byteswap return
> values out of rtas_args.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>

Despite a minor comment below
Reviewed-by: Laurent Dufour <ldufour@linux.ibm.com>

> ---
>  arch/powerpc/include/asm/rtas.h              |  4 +-
>  arch/powerpc/kernel/rtas.c                   | 53 +++++++++++---------
>  arch/powerpc/platforms/pseries/hotplug-cpu.c |  2 +-
>  arch/powerpc/platforms/pseries/ras.c         |  7 +--
>  arch/powerpc/xmon/xmon.c                     |  2 +-
>  5 files changed, 33 insertions(+), 35 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/rtas.h b/arch/powerpc/include/asm/rtas.h
> index 82e5b055fa2a..1014ff140cf8 100644
> --- a/arch/powerpc/include/asm/rtas.h
> +++ b/arch/powerpc/include/asm/rtas.h
> @@ -241,8 +241,8 @@ extern int rtas_token(const char *service);
>  extern int rtas_service_present(const char *service);
>  extern int rtas_call(int token, int, int, int *, ...);
>  int rtas_call_reentrant(int token, int nargs, int nret, int *outputs, ...);
> -void rtas_call_unlocked(struct rtas_args *args, int token, int nargs,
> -			int nret, ...);
> +int raw_rtas_call(struct rtas_args *args, int token,
> +			int nargs, int nret, int *outputs, ...);

Minor, would it be better to keep the "unlocked" suffix to advise that the
rtas lock is not held here?

>  extern void __noreturn rtas_restart(char *cmd);
>  extern void rtas_power_off(void);
>  extern void __noreturn rtas_halt(void);
> diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
> index fece066115f0..751a20669669 100644
> --- a/arch/powerpc/kernel/rtas.c
> +++ b/arch/powerpc/kernel/rtas.c
> @@ -123,7 +123,7 @@ static void call_rtas_display_status(unsigned char c)
>  		return;
>  
>  	s = lock_rtas();
> -	rtas_call_unlocked(&rtas.args, 10, 1, 1, NULL, c);
> +	raw_rtas_call(&rtas.args, 10, 1, 1, NULL, c);
>  	unlock_rtas(s);
>  }
>  
> @@ -434,10 +434,9 @@ static char *__fetch_rtas_last_error(char *altbuf)
>  #define get_errorlog_buffer()		NULL
>  #endif
>  
> -
> -static void
> -va_rtas_call_unlocked(struct rtas_args *args, int token, int nargs, int nret,
> -		      va_list list)
> +static int notrace va_raw_rtas_call(struct rtas_args *args, int token,
> +				int nargs, int nret, int *outputs,
> +				va_list list)
>  {
>  	int i;
>  
> @@ -453,21 +452,37 @@ va_rtas_call_unlocked(struct rtas_args *args, int token, int nargs, int nret,
>  		args->rets[i] = 0;
>  
>  	do_enter_rtas(__pa(args));
> +
> +	if (nret > 1 && outputs != NULL) {
> +		for (i = 0; i < nret-1; ++i)
> +			outputs[i] = be32_to_cpu(args->rets[i+1]);
> +	}
> +
> +	return (nret > 0) ? be32_to_cpu(args->rets[0]) : 0;
>  }
>  
> -void rtas_call_unlocked(struct rtas_args *args, int token, int nargs, int nret, ...)
> +/*
> + * Like rtas_call but no kmalloc or printk etc in error handling, so
> + * error won't go through log_error. No tracing, may be called in real mode.
> + * rtas_args must be supplied, and appropriate synchronization for the rtas
> + * call being made has to be performed by the caller.
> + */
> +int notrace raw_rtas_call(struct rtas_args *args, int token,
> +			int nargs, int nret, int *outputs, ...)
>  {
>  	va_list list;
> +	int ret;
>  
> -	va_start(list, nret);
> -	va_rtas_call_unlocked(args, token, nargs, nret, list);
> +	va_start(list, outputs);
> +	ret = va_raw_rtas_call(args, token, nargs, nret, outputs, list);
>  	va_end(list);
> +
> +	return ret;
>  }
>  
>  int rtas_call(int token, int nargs, int nret, int *outputs, ...)
>  {
>  	va_list list;
> -	int i;
>  	unsigned long s;
>  	struct rtas_args *rtas_args;
>  	char *buff_copy = NULL;
> @@ -482,19 +497,14 @@ int rtas_call(int token, int nargs, int nret, int *outputs, ...)
>  	rtas_args = &rtas.args;
>  
>  	va_start(list, outputs);
> -	va_rtas_call_unlocked(rtas_args, token, nargs, nret, list);
> +	ret = va_raw_rtas_call(rtas_args, token, nargs, nret, outputs, list);
>  	va_end(list);
>  
>  	/* A -1 return code indicates that the last command couldn't
>  	   be completed due to a hardware error. */
> -	if (be32_to_cpu(rtas_args->rets[0]) == -1)
> +	if (ret == -1)
>  		buff_copy = __fetch_rtas_last_error(NULL);
>  
> -	if (nret > 1 && outputs != NULL)
> -		for (i = 0; i < nret-1; ++i)
> -			outputs[i] = be32_to_cpu(rtas_args->rets[i+1]);
> -	ret = (nret > 0)? be32_to_cpu(rtas_args->rets[0]): 0;
> -
>  	unlock_rtas(s);
>  
>  	if (buff_copy) {
> @@ -950,7 +960,7 @@ int rtas_call_reentrant(int token, int nargs, int nret, int *outputs, ...)
>  	va_list list;
>  	struct rtas_args *args;
>  	unsigned long flags;
> -	int i, ret = 0;
> +	int ret;
>  
>  	if (!rtas.entry || token == RTAS_UNKNOWN_SERVICE)
>  		return -1;
> @@ -962,16 +972,9 @@ int rtas_call_reentrant(int token, int nargs, int nret, int *outputs, ...)
>  	args = local_paca->rtas_args_reentrant;
>  
>  	va_start(list, outputs);
> -	va_rtas_call_unlocked(args, token, nargs, nret, list);
> +	ret = va_raw_rtas_call(args, token, nargs, nret, outputs, list);
>  	va_end(list);
>  
> -	if (nret > 1 && outputs)
> -		for (i = 0; i < nret - 1; ++i)
> -			outputs[i] = be32_to_cpu(args->rets[i + 1]);
> -
> -	if (nret > 0)
> -		ret = be32_to_cpu(args->rets[0]);
> -
>  	local_irq_restore(flags);
>  	preempt_enable();
>  
> diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c b/arch/powerpc/platforms/pseries/hotplug-cpu.c
> index b81fc846d99c..17c05650b6b9 100644
> --- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
> +++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c
> @@ -53,7 +53,7 @@ static void rtas_stop_self(void)
>  
>  	BUG_ON(rtas_stop_self_token == RTAS_UNKNOWN_SERVICE);
>  
> -	rtas_call_unlocked(&args, rtas_stop_self_token, 0, 1, NULL);
> +	raw_rtas_call(&args, rtas_stop_self_token, 0, 1, NULL);
>  
>  	panic("Alas, I survived.\n");
>  }
> diff --git a/arch/powerpc/platforms/pseries/ras.c b/arch/powerpc/platforms/pseries/ras.c
> index 74c9b1b5bc66..b009ed7de1ee 100644
> --- a/arch/powerpc/platforms/pseries/ras.c
> +++ b/arch/powerpc/platforms/pseries/ras.c
> @@ -465,12 +465,7 @@ static void fwnmi_release_errinfo(void)
>  	struct rtas_args rtas_args;
>  	int ret;
>  
> -	/*
> -	 * On pseries, the machine check stack is limited to under 4GB, so
> -	 * args can be on-stack.
> -	 */
> -	rtas_call_unlocked(&rtas_args, ibm_nmi_interlock_token, 0, 1, NULL);
> -	ret = be32_to_cpu(rtas_args.rets[0]);
> +	ret = raw_rtas_call(&rtas_args, ibm_nmi_interlock_token, 0, 1, NULL);
>  	if (ret != 0)
>  		printk(KERN_ERR "FWNMI: nmi-interlock failed: %d\n", ret);
>  }
> diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
> index fd72753e8ad5..6f53e8bccc33 100644
> --- a/arch/powerpc/xmon/xmon.c
> +++ b/arch/powerpc/xmon/xmon.c
> @@ -410,7 +410,7 @@ static inline void disable_surveillance(void)
>  	if (set_indicator_token == RTAS_UNKNOWN_SERVICE)
>  		return;
>  
> -	rtas_call_unlocked(&args, set_indicator_token, 3, 1, NULL,
> +	raw_rtas_call(&args, set_indicator_token, 3, 1, NULL,
>  			   SURVEILLANCE_TOKEN, 0, 0);
>  
>  #endif /* CONFIG_PPC_PSERIES */