Hi Sourabh,
Thanks for the review.
On 07/06/26 4:49 PM, Sourabh Jain wrote:
On 27/05/26 12:54, Narayana Murty N wrote:
RTAS_FN_IBM_OPEN_ERRINJCT returns results in special format:
rets[0] = session token (output)
rets[1] = status code
rets[2..] = additional outputs (if any)
Unlike standard RTAS calls where:
rets[0] = status code
rets[1..] = outputs
This patch adds special handling for OPEN_ERRINJCT to:
1. Check correct status position (rets[1]) for __fetch_rtas_last_error()
You can consider fixing the same for the RTAS syscall.
2. Copy all rets[0..nret-1] to outputs[] (including token at rets[0])
3. Return status from rets[1] instead of rets[0]
Reference: OpenPOWER PAPR documentation
https://files.openpower.foundation/s/XFgfMaqLMD5Bcm8
Signed-off-by: Narayana Murty N <[email protected]>
---
arch/powerpc/kernel/rtas.c | 47 ++++++++++++++++++++++++++++++++------
1 file changed, 40 insertions(+), 7 deletions(-)
diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
index 8d81c1e7a8db..a2dd94eed9d0 100644
--- a/arch/powerpc/kernel/rtas.c
+++ b/arch/powerpc/kernel/rtas.c
@@ -1183,7 +1183,7 @@ int rtas_call(int token, int nargs, int nret,
int *outputs, ...)
unsigned long flags;
struct rtas_args *args;
char *buff_copy = NULL;
- int ret;
+ int ret = 0;
if (!rtas.entry || token == RTAS_UNKNOWN_SERVICE)
return -1;
@@ -1213,15 +1213,48 @@ int rtas_call(int token, int nargs, int nret,
int *outputs, ...)
va_rtas_call_unlocked(args, token, nargs, nret, list);
va_end(list);
+ /*
+ * Special handling for RTAS_FN_IBM_OPEN_ERRINJCT:
+ * Per PAPR, ibm,open-errinjct has a unique return format:
+ * rets[0] = injection session token (output parameter)
+ * rets[1] = status code
+ *
+ * This differs from standard RTAS calls which return:
+ * rets[0] = status code
+ * rets[1..] = output parameters
+ *
+ * We must extract status from rets[1] (not rets[0]) to correctly
+ * detect errors and trigger __fetch_rtas_last_error() when
status == -1.
+ */
/* A -1 return code indicates that the last command couldn't
- be completed due to a hardware error. */
The above comment should be moved to next if block, if (ret == -1).
- if (be32_to_cpu(args->rets[0]) == -1)
+ * be completed due to a hardware error.
+ */
+ if (token == rtas_function_token(RTAS_FN_IBM_OPEN_ERRINJCT) &&
nret > 1)
+ ret = be32_to_cpu(args->rets[1]);
+ else if (nret > 0)
+ ret = be32_to_cpu(args->rets[0]);
+
+ 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;
+ /* Copy all return values to caller's outputs buffer if provided */
+ if (nret > 1 && outputs != NULL) {
+ if (token == rtas_function_token(RTAS_FN_IBM_OPEN_ERRINJCT)) {
+ /* Special case: rets[0]=token, rets[1]=status,
rets[2..]=outputs */
+ for (i = 0; i < nret; ++i)
+ outputs[i] = be32_to_cpu(args->rets[i]);
+ } else {
+ /* Normal case: rets[0]=status, rets[1..]=outputs */
+ for (i = 0; i < nret - 1; ++i)
+ outputs[i] = be32_to_cpu(args->rets[i + 1]);
I am surprised that status is never copied to the output buffer even
though it is part
of the output as per PAPR. But status is copied for ibm,open-errinjct,
which is okay.
For normal RTAS calls, |rtas_call()|returns |rets[0]|, the firmware
status, as its function
return value and copies only |rets[1..]|into the caller's
|outputs[]|buffer. I intended to
preserve this existing API.
For |ibm,open-errinjct|, I will follow the same API convention: return
the status from |rets[1]|,
copy the session token from |rets[0]|to |outputs[0]|, and copy any
additional output values from |
|
|rets[2..]|to the subsequent entries in |outputs[]|. Therefore, the
status does not need to
be copied into |outputs[]|.
+ }
+ } else {
+ /* Either no outputs to copy (nret <= 1) or caller
+ * didn't provide output buffer ensure ret contains
+ * the status code for standard RTAS calls.
+ */
+ ret = (nret > 0) ? be32_to_cpu(args->rets[0]) : 0;
What is the need to find ret again? Isn't it already done above?
You are right. |ret|has already been determined above, so the
|else|block is unnecessary.
+ }
lockdep_unpin_lock(&rtas_lock, cookie);
raw_spin_unlock_irqrestore(&rtas_lock, flags);
