Andrew Donnellan <a...@linux.ibm.com> writes:
> On Mon, 2022-11-28 at 18:08 -0600, Nathan Lynch wrote:
>> Andrew Donnellan <a...@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.
