Re: [PATCH v2 18/19] powerpc/rtas: introduce rtas_function_token() API

[Nathan Lynch <nathanl@linux.ibm.com>]

Michael Ellerman <mpe@ellerman.id.au> writes:
> Nathan Lynch via B4 Submission Endpoint
> <devnull+nathanl.linux.ibm.com@kernel.org> writes:
>> From: Nathan Lynch <nathanl@linux.ibm.com>
>>
>> Users of rtas_token() supply a string argument that can't be validated
>> at build time. A typo or misspelling has to be caught by inspection or
>> by observing wrong behavior at runtime.
>>
>> Since the core RTAS code now has consolidated the names of all
>> possible RTAS functions and mapped them to their tokens, token lookup
>> can be implemented using symbolic constants to index a static array.
>>
>> So introduce rtas_function_token(), a replacement API which does that,
>> along with a rtas_service_present()-equivalent helper,
>> rtas_function_implemented(). Callers supply an opaque predefined
>> function handle which is used internally to index the function
>> table. Typos or other inappropriate arguments yield build errors, and
>> the function handle is a type that can't be easily confused with RTAS
>> tokens or other integer types.
>
> Why not go all the way and have the rtas_call() signature be:
>
>   int rtas_call(rtas_fn_handle_t fn, int, int, int *, ...);
>
>
> And have it do the token lookup internally? That way a caller can never
> inadvertantly pass a random integer to rtas_call().
>
> And instead of eg:
>
> 	error = rtas_call(rtas_function_token(RTAS_FN_GET_TIME_OF_DAY), 0, 8, ret);
>
> we'd just need:
>
> 	error = rtas_call(RTAS_FN_GET_TIME_OF_DAY, 0, 8, ret);
>
>
> Doing the conversion all at once might be tricky. So maybe we need to add
> rtas_fn_call() which takes rtas_fn_handle_t so we can convert cases individually?
>
> Anyway just a thought. I guess we could merge this as-is and then do a
> further change to use rtas_fn_handle_t later.

You read my mind :-) But I want to go further and make the eventual
replacement for rtas_call() non-variadic, which will eliminate another
class of usage error.

Getting more ambitious: the ideal situation IMO would be that every use
of rtas_call() or its replacement is tidily contained in a C function in
kernel/rtas.c, where complexities like retries and error code
translation can be performed in a uniform way.

Anyway, a transition away from rtas_call(), whatever form it takes,
probably needs to happen incrementally.

>> diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
>> index 41c430dc40c2..17e59306ce63 100644
>> --- a/arch/powerpc/kernel/rtas.c
>> +++ b/arch/powerpc/kernel/rtas.c
>> @@ -453,6 +453,26 @@ static struct rtas_function rtas_function_table[] __ro_after_init = {
>>  	},
>>  };
>>  
>> +/**
>> + * rtas_function_token() - RTAS function token lookup.
>> + * @handle: Function handle, e.g. RTAS_FN_EVENT_SCAN.
>> + *
>> + * Context: Any context.
>> + * Return: the token value for the function if implemented by this platform,
>> + *         otherwise RTAS_UNKNOWN_SERVICE.
>> + */
>> +s32 rtas_function_token(const rtas_fn_handle_t handle)
>> +{
>> +	const size_t index = handle.index;
>> +	const bool out_of_bounds = index >= ARRAY_SIZE(rtas_function_table);
>> +
>> +	if (WARN_ONCE(out_of_bounds, "invalid function index %zu", index))
>> +		return RTAS_UNKNOWN_SERVICE;
>
> This needs:
>
> +	// If RTAS is not present or not initialised (yet) return unknown
> +	if (!rtas.dev)
> +		return RTAS_UNKNOWN_SERVICE;
> +
>
> Otherwise powernv breaks because it looks up tokens and gets back 0,
> because we never got as far as rtas_function_table_init() (to set all the
> tokens to RTAS_UNKNOWN_SERVICE), because we bailed out at the start of
> rtas_initialize() when we found no /rtas node.

Oh! OK, thanks.