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,
>
> 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.
>
> > 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
a...@linux.ibm.com IBM Australia Limited
