Re: [PATCH v3 1/5] powerpc/rtas: Handle ibm,open-errinjct return format

On 21/07/26 09:08, Narayana Murty N wrote:
> PAPR specifies that ibm,open-errinjct has a unique return-cell layout:
>
>    rets[0] = injection session token (output parameter)
>    rets[1] = status code
>
> This differs from every other RTAS call, where:
>
>    rets[0] = status code
>    rets[1..] = output parameters
>
> As a result, the existing rtas_call() convention — return value is the
> RTAS status, outputs[] receives the non-status output values — must be
> preserved while correctly extracting status from rets[1] for this one
> call.
>
> Add rtas_token_is_open_errinjct() and rtas_status_from_args() helpers.
> rtas_status_from_args() selects the correct status cell based on the
> token, and the output-copy loop in rtas_call() is updated so that for
> ibm,open-errinjct:
>
>    rtas_call() return = rets[1]   (RTAS status)
>    outputs[0]        = rets[0]   (session token)
>
> For all other calls the behaviour is unchanged: return value is rets[0]
> and outputs[] receives rets[1..nret-1].
>
> The sys_rtas userspace path is not modified: copy_to_user() still
> copies raw RTAS return cells (rets[0..nret-1]) to userspace.
>
> Callers passing a single output int (nret == 2) are safe because we
> write at most nret-1 values into outputs[], never all nret cells.
>
> Also move the '/* A -1 return code...*/' comment to immediately precede
> the if (ret == -1) check it describes, and remove the redundant stale
> else branch that re-assigned ret.
>
> Reference: OpenPOWER PAPR documentation
>             https://files.openpower.foundation/s/XFgfMaqLMD5Bcm8
>
> Signed-off-by: Narayana Murty N <[email protected]>
> ---
>   arch/powerpc/kernel/rtas.c | 51 ++++++++++++++++++++++++++++++++------
>   1 file changed, 44 insertions(+), 7 deletions(-)
>
> diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
> index 8d81c1e7a8db..27d53f34494d 100644
> --- a/arch/powerpc/kernel/rtas.c
> +++ b/arch/powerpc/kernel/rtas.c
> @@ -1117,6 +1117,29 @@ static bool token_is_restricted_errinjct(s32 token)
>   	       token == rtas_function_token(RTAS_FN_IBM_ERRINJCT);
>   }
>   
> +/**
> + * rtas_token_is_open_errinjct() - Test whether @token identifies ibm,open-errinjct.
> + */
> +static bool rtas_token_is_open_errinjct(int token)
> +{
> +	return token == rtas_function_token(RTAS_FN_IBM_OPEN_ERRINJCT);
> +}

The above function is a god candidate for an inline function.

> +
> +/**
> + * rtas_status_from_args() - Extract the RTAS status code from a completed
> + *                           call's return-cell array.
> + *
> + * For ibm,open-errinjct the status lives in rets[1]; for every other
> + * RTAS function it lives in rets[0].
> + */
> +static int rtas_status_from_args(int token, struct rtas_args *args, int nret)
> +{
> +	if (rtas_token_is_open_errinjct(token) && nret > 1)

Do we know what RTAS returns when nret is less than 2 for the 
ibm,open-errinjct RTAS call?

I assume RTAS treats this as a parameter error. However, since there is 
no rets buffer available
to store the status code, I'm not sure how RTAS would convey the 
parameter error back to the kernel.

The way the RTAS status code is extracted when nret is less than 2 for 
the ibm,open-errinjct RTAS
call seems problematic me, especially when nret == 1. When nret == 1, 
this function returns args->rets[0],
which is supposed to contain the session token.

Are we sure that RTAS places the call status in rets[0] when nret == 1 
for the ibm,open-errinjct RTAS call?

- Sourabh Jain

> +		return be32_to_cpu(args->rets[1]);
> +
> +	return nret > 0 ? be32_to_cpu(args->rets[0]) : 0;
> +}
> +
>   /**
>    * rtas_call() - Invoke an RTAS firmware function.
>    * @token: Identifies the function being invoked.
> @@ -1213,15 +1236,29 @@ int rtas_call(int token, int nargs, int nret, int *outputs, ...)
>   	va_rtas_call_unlocked(args, token, nargs, nret, 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(args->rets[0]) == -1)
> +	ret = rtas_status_from_args(token, args, nret);
> +
> +	/*
> +	 * A -1 return code indicates that the last command couldn't
> +	 * be completed due to a hardware error.
> +	 */
> +	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(args->rets[i + 1]);
> -	ret = (nret > 0) ? be32_to_cpu(args->rets[0]) : 0;
> +	if (nret > 1 && outputs != NULL) {
> +		if (rtas_token_is_open_errinjct(token)) {
> +			/*
> +			 * ibm,open-errinjct: rets[0]=session token, rets[1]=status.
> +			 * Expose session token in outputs[0]; skip rets[1] (status).
> +			 */
> +			outputs[0] = be32_to_cpu(args->rets[0]);
> +			for (i = 1; i < nret - 1; ++i)
> +				outputs[i] = be32_to_cpu(args->rets[i + 1]);
> +		} else {
> +			for (i = 0; i < nret - 1; ++i)
> +				outputs[i] = be32_to_cpu(args->rets[i + 1]);
> +		}
> +	}
>   
>   	lockdep_unpin_lock(&rtas_lock, cookie);
>   	raw_spin_unlock_irqrestore(&rtas_lock, flags);