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

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,

Not sure what userspace API refers to here, can you be more specific?
I
don't think rtas_token() is exposed to user space.

Right, what I actually meant to refer to here is the fact that sys_rtas
takes a token, but when I wrote this I forgot that userspace doesn't
pass a string, rather librtas implements rtas_token itself using the
/proc interface to the device tree.

quoted

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.


-- 
Andrew Donnellan    OzLabs, ADL Canberra
ajd@linux.ibm.com   IBM Australia Limited