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

From: Andrew Donnellan <ajd@linux.ibm.com>
To: Nathan Lynch <nathanl@linux.ibm.com>, linuxppc-dev@lists.ozlabs.org
Cc: tyreld@linux.ibm.com, nnac123@linux.ibm.com,
	ldufour@linux.ibm.com, npiggin@gmail.com
Subject: Re: [PATCH 10/13] powerpc/rtas: improve function information lookups
Date: Tue, 29 Nov 2022 18:23:09 +1100	[thread overview]
Message-ID: <099d99683c12b137ec2a4b0158ae3fd390fc1cd3.camel@linux.ibm.com> (raw)
In-Reply-To: <87r0xm8vvy.fsf@linux.ibm.com>

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