Re: [PATCH v2 1/5] powerpc/rtas: Handle special return format for RTAS_FN_IBM_OPEN_ERRINJCT

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