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

[Narayana Murty N <nnmlinux@linux.ibm.com>]

Hi Sourabh,

Thanks for the review.

On 04/08/26 11:22 AM, Sourabh Jain wrote:
> 
> 
> 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 <nnmlinux@linux.ibm.com>
>> ---
>>   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.

Agreed. I updated this in v4 and made the helper static inline.

>> +
>> +/**
>> + * 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?
> 
The rets[0] contains a status for
ibm,open-errinjct when fewer than two return cells are provided. For this
RTAS call, rets[0] is the session token and rets[1] is the status. So if
nret < 2, the kernel cannot reliably extract the status.

I fixed this in v4 by treating ibm,open-errinjct with nret < 2 as an
invalid rtas_call() usage instead of falling back to rets[0]. That avoids
mistaking a session token for a status code.

For normal callers using nret == 2, the behavior remains:

rtas_call() return value = rets[1] status
outputs[0] = rets[0] session token

The sys_rtas() path remains unchanged and continues to expose the raw RTAS
return cells to userspace.

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


Thanks,
Narayana