Re: [PATCH 10/13] powerpc/rtas: improve function information lookups

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

Andrew Donnellan <ajd@linux.ibm.com> writes:
> On Mon, 2022-11-28 at 18:08 -0600, Nathan Lynch wrote:
>> Andrew Donnellan <ajd@linux.ibm.com> writes:
>> > On Fri, 2022-11-18 at 09:07 -0600, Nathan Lynch wrote:
>> > > Convert rtas_token() to use a lockless binary search on the
>> > > function table. Fall back to the old behavior for lookups against
>> > > names that are not known to be RTAS functions, but issue a
>> > > warning.  rtas_token() is for function names; it is not a general
>> > > facility for accessing arbitrary properties of the /rtas
>> > > node. All known misuses of rtas_token() have been converted to
>> > > more appropriate of_ APIs in preceding changes.
>> > 
>> > For in-kernel users, why not go all the way: make rtas_token()
>> > static and use it purely for the userspace API, and switch kernel
>> > users over to using rtas_function_index directly?
>> 
>> Well, I have work in progress to transition all rtas_token() users to
>> a different API, but using opaque symbolic handles rather than
>> exposing the indexes. Something like:
>> 
>> /*
>>  * Opaque handle for client code to refer to RTAS functions. All
>> valid
>>  * function handles are build-time constants prefixed with RTAS_FN_.
>>  */
>> typedef struct {
>>         enum rtas_function_index index;
>> } rtas_fn_handle_t;
>> 
>> #define rtas_fn_handle(x_) (const rtas_fn_handle_t) { .index =
>> rtas_fnidx(x_), }
>> 
>> #define RTAS_FN_CHECK_EXCEPTION                  
>> rtas_fn_handle(CHECK_EXCEPTION)
>> #define RTAS_FN_DISPLAY_CHARACTER                
>> rtas_fn_handle(DISPLAY_CHARACTER)
>> #define RTAS_FN_EVENT_SCAN                       
>> rtas_fn_handle(EVENT_SCAN)
>> 
>> ...
>> 
>> /**
>>  * 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;
>> 
>>         return rtas_function_table[index].token;
>> }
>> 
>> But that's a tree-wide change that would touch various drivers, and I
>> feel like the current series is what I can reasonably hope to get
>> applied right now.
>
> It's not clear to me what the benefit of adding this additional layer
> of indirection would be.

One benefit would be that the type system won't let you use a token
(int) where you meant to use a handle (struct), and vice versa. I
anticipate that property being valuable for making API changes. But also
it's just good interface hygiene: how the token lookup is implemented
isn't the concern of code outside of rtas.c.