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

Andrew Donnellan [off-list ref] writes:

On Mon, 2022-11-28 at 18:08 -0600, Nathan Lynch wrote:

quoted

Andrew Donnellan [off-list ref] writes:

quoted

On Fri, 2022-11-18 at 09:07 -0600, Nathan Lynch wrote:

quoted

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.