[PATCH 10/13] powerpc/rtas: improve function information lookups
The core RTAS support code and its clients perform two types of lookup
for RTAS firmware function information.
First, mapping a known function name to a token. The typical use case
invokes rtas_token() to retrieve the token value to pass to
rtas_call(). rtas_token() relies on of_get_property(), which performs
a linear search of the /rtas node's property list under a lock with
IRQs disabled.
Second, and less common: given a token value, looking up some
information about the function. The primary example is the sys_rtas
filter path, which linearly scans a small table to match the token to
a rtas_filter struct. Another use case to come is RTAS entry/exit
tracepoints, which will require efficient lookup of function names
from token values. Currently there is no general API for this.
We need something much like the existing rtas_filters table, but more
general and organized to facilitate efficient lookups.
Introduce:
* A new rtas_function type, aggregating function name, token,
and filter. Other function characteristics could be added in the
future.
* An array of rtas_function, where each element corresponds to a known
RTAS function. All information in the table is static save the token
values, which are derived from the device tree at boot. The array is
sorted by function name to allow binary search.
* A named constant for each known RTAS function, used to index the
function array. These also will be used in a client-facing API to be
added later.
* An xarray that maps valid tokens to rtas_function objects.
Fold the existing rtas_filter table into the new rtas_function array,
with the appropriate adjustments to block_rtas_call(). Remove
now-redundant fields from struct rtas_filter.
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.
Signed-off-by: Nathan Lynch
---
arch/powerpc/include/asm/rtas.h | 87
arch/powerpc/kernel/rtas.c | 735 +++-
2 files changed, 709 insertions(+), 113 deletions(-)
diff --git a/arch/powerpc/include/asm/rtas.h b/arch/powerpc/include/asm/rtas.h
index 479a95cb2770..14fe79217c26 100644
--- a/arch/powerpc/include/asm/rtas.h
+++ b/arch/powerpc/include/asm/rtas.h
@@ -16,6 +16,93 @@
* Copyright (C) 2001 PPC 64 Team, IBM Corp
*/
+#define rtas_fnidx(x_) RTAS_FNIDX__ ## x_
+
+enum rtas_function_index {
+ rtas_fnidx(CHECK_EXCEPTION),
+ rtas_fnidx(DISPLAY_CHARACTER),
+ rtas_fnidx(EVENT_SCAN),
+ rtas_fnidx(FREEZE_TIME_BASE),
+ rtas_fnidx(GET_POWER_LEVEL),
+ rtas_fnidx(GET_SENSOR_STATE),
+ rtas_fnidx(GET_TERM_CHAR),
+ rtas_fnidx(GET_TIME_OF_DAY),
+ rtas_fnidx(IBM_ACTIVATE_FIRMWARE),
+ rtas_fnidx(IBM_CBE_START_PTCAL),
+ rtas_fnidx(IBM_CBE_STOP_PTCAL),
+ rtas_fnidx(IBM_CHANGE_MSI),
+ rtas_fnidx(IBM_CLOSE_ERRINJCT),
+ rtas_fnidx(IBM_CONFIGURE_BRIDGE),
+ rtas_fnidx(IBM_CONFIGURE_CONNECTOR),
+ rtas_fnidx(IBM_CONFIGURE_KERNEL_DUMP),
+ rtas_fnidx(IBM_CONFIGURE_PE),
+ rtas_fnidx(IBM_CREATE_PE_DMA_WINDOW),
+ rtas_fnidx(IBM_DISPLAY_MESSAGE),
+ rtas_fnidx(IBM_ERRINJCT),
+ rtas_fnidx(IBM_EXTI2C),
+ rtas_fnidx(IBM_GET_CONFIG_ADDR_INFO),
+ rtas_fnidx(IBM_GET_CONFIG_ADDR_INFO2),
+ rtas_fnidx(IBM_GET_DYNAMIC_SENSOR_STATE),
+ rtas_fnidx(IBM_GET_INDICES),
+ rtas_fnidx(IBM_GET_RIO_TOPOLOGY),
+ rtas_fnidx(IBM_GET_SYSTEM_PARAMETER),
+ rtas_fnidx(IBM_GET_VPD),
+ rtas_fnidx(IBM_GET_XIVE),
+ rtas_fnidx(IBM_INT_OFF),
+ rtas_fnidx(IBM_INT_ON),
+ rtas_fnidx(IBM_IO_QUIESCE_ACK),
+ rtas_fnidx(IBM_LPAR_PERFTOOLS),
+ rtas_fnidx(IBM_MANAGE_FLASH_IMAGE),
+ rtas_fnidx(IBM_MANAGE_STORAGE_PRESERVATION),
+ rtas_fnidx(IBM_NMI_INTERLOCK),
+ rtas_fnidx(IBM_NMI_REGISTER),
+ rtas_fnidx(IBM_OPEN_ERRINJCT),
+ rtas_fnidx(IBM_OPEN_SRIOV_ALLOW_UNFREEZE),
+ rtas_fnidx(IBM_OPEN_SRIOV_MAP_PE_NUMBER),
+ rtas_fnidx(IBM_OS_TERM),
+ rtas_fnidx(IBM_PARTNER_CONTROL),
+ rtas_fnidx(IBM_PHYSICAL_ATTESTATION),
+ rtas_fnidx(IBM_PLATFORM_DUMP),
+ rtas_fnidx(IBM_POWER_OFF_UPS),
+ rtas_fnidx(IBM_QUERY_INTERRUPT_SOURCE_NUMBER),
+ rtas_fnidx(IBM_QUERY_PE_DMA_WINDOW),
+ rtas_fnidx(IBM_READ_PCI_CONFIG),
+ rtas_fnidx(IBM_READ_SLOT_RESET_STATE),
+ rtas_fnidx(IBM_READ_SLOT_RESET_STATE2),
+ rtas_fnidx(IBM_REMOVE_PE_DMA_WINDOW),
+ rtas_fnidx(IBM_RESET_PE_DMA_WINDOWS),
+ rtas_fnidx(IBM_SCAN_LOG_DUMP),
+ rtas_fnidx(IBM_SET_DYNAMIC_INDICATOR),
+ rtas_fnidx(IBM_SET_EEH_OPTION),
+ rtas_fnidx(IBM_SET_SLOT_RESET),
+
Re: [PATCH 10/13] powerpc/rtas: improve function information lookups
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?
> +enum rtas_function_flags {
> + RTAS_FN_FLAG_BANNED_FOR_SYSCALL_ON_LE = (1 << 0),
> +};
This seems to be new, what's the justification?
--
Andrew DonnellanOzLabs, ADL Canberra
a...@linux.ibm.com IBM Australia Limited
Re: [PATCH 10/13] powerpc/rtas: improve function information lookups
On 11/22/22 20:51, Andrew Donnellan wrote:
On Fri, 2022-11-18 at 09:07 -0600, Nathan Lynch wrote:
+enum rtas_function_flags {
+ RTAS_FN_FLAG_BANNED_FOR_SYSCALL_ON_LE = (1 << 0),
+};
This seems to be new, what's the justification?
Seems to be a run-time replacement of:
#ifdef CONFIG_CPU_BIG_ENDIAN
{ "ibm,suspend-me", -1, -1, -1, -1, -1 },
{ "ibm,update-nodes", -1, 0, -1, -1, -1, 4096 },
{ "ibm,update-properties", -1, 0, -1, -1, -1, 4096 },
#endif
It looks to be handled logically:
+ if (IS_ENABLED(CONFIG_CPU_LITTLE_ENDIAN) &&
+ (func->flags & RTAS_FN_FLAG_BANNED_FOR_SYSCALL_ON_LE))
+ goto err;
Perhaps, also allow the addition of any future special cases
for rtas functions easier to maintain?
Re: [PATCH 10/13] powerpc/rtas: improve function information lookups
On 11/18/22 09:07, Nathan Lynch wrote:
+static int __init rtas_token_to_function_xarray_init(void)
+{
+ int err = 0;
+
+ for (size_t i = 0; i < ARRAY_SIZE(rtas_function_table); ++i) {
+ const struct rtas_function *func = &rtas_function_table[i];
+ const s32 token = func->token;
+
+ if (token == RTAS_UNKNOWN_SERVICE)
+ continue;
+
+ err = xa_err(xa_store(&rtas_token_to_function_xarray,
+ token, (void *)func, GFP_KERNEL));
+ if (err)
+ break;
+ }
+
+ return err;
+}
+arch_initcall(rtas_token_to_function_xarray_init);
+
+static const struct rtas_function *rtas_token_to_function(s32 token)
+{
+ const struct rtas_function *func;
+
+ if (WARN_ONCE(token < 0, "invalid token %d", token))
+ return NULL;
+
+ func = xa_load(&rtas_token_to_function_xarray, (unsigned long)token);
+
Why typecast token here and not in xa_store?
+static void __init rtas_function_table_init(void)
+{
+ struct property *prop;
+
+ for (size_t i = 0; i < ARRAY_SIZE(rtas_function_table); ++i) {
+ struct rtas_function *curr = &rtas_function_table[i];
+ struct rtas_function *prior;
+ int cmp;
+
+ curr->token = RTAS_UNKNOWN_SERVICE;
+
+ if (i == 0)
+ continue;
+ /*
+* Ensure table is sorted correctly for binary search
+* on function names.
+*/
+ prior = &rtas_function_table[i - 1];
+
+ cmp = strcmp(prior->name, curr->name);
+ if (cmp < 0)
+ continue;
+
+ if (cmp == 0) {
+ pr_err("'%s' has duplicate function table entries\n",
+ curr->name);
+ } else {
+ pr_err("function table unsorted: '%s' wrongly precedes
'%s'\n",
+ prior->name, curr->name);
+ }
+ }
Just a thought, would it be simpler to use sort()? you already have the
cmp_func implemented for bsearch().
As for the series as a whole:
I am no RTAS expert but was able to build, boot and mess around with new
tracepoints without errors:
Tested-by: Nick Child
Re: [PATCH 10/13] powerpc/rtas: improve function information lookups
On Wed, 2022-11-23 at 13:32 -0600, Nick Child wrote:
> On 11/22/22 20:51, Andrew Donnellan wrote:
> > On Fri, 2022-11-18 at 09:07 -0600, Nathan Lynch wrote:
> > > +enum rtas_function_flags {
> > > + RTAS_FN_FLAG_BANNED_FOR_SYSCALL_ON_LE = (1 << 0),
> > > +};
> >
> > This seems to be new, what's the justification?
> >
>
> Seems to be a run-time replacement of:
> #ifdef CONFIG_CPU_BIG_ENDIAN
> { "ibm,suspend-me", -1, -1, -1, -1, -1 },
> { "ibm,update-nodes", -1, 0, -1, -1, -1, 4096 },
> { "ibm,update-properties", -1, 0, -1, -1, -1, 4096 },
> #endif
>
> It looks to be handled logically:
> + if (IS_ENABLED(CONFIG_CPU_LITTLE_ENDIAN) &&
> + (func->flags & RTAS_FN_FLAG_BANNED_FOR_SYSCALL_ON_LE))
> + goto err;
>
> Perhaps, also allow the addition of any future special cases
> for rtas functions easier to maintain?
Makes sense, though I'm slightly confused about the original rationale
for the ifdef and why it's not being fixed in userspace.
Slightly clunky name though, something like
RTAS_FN_FLAG_SYSCALL_BE_ONLY might be less clunky?
--
Andrew DonnellanOzLabs, ADL Canberra
a...@linux.ibm.com IBM Australia Limited
Re: [PATCH 10/13] powerpc/rtas: improve function information lookups
Andrew Donnellan writes:
> On Wed, 2022-11-23 at 13:32 -0600, Nick Child wrote:
>> On 11/22/22 20:51, Andrew Donnellan wrote:
>> > On Fri, 2022-11-18 at 09:07 -0600, Nathan Lynch wrote:
>> > > +enum rtas_function_flags {
>> > > + RTAS_FN_FLAG_BANNED_FOR_SYSCALL_ON_LE = (1 << 0),
>> > > +};
>> >
>> > This seems to be new, what's the justification?
>> >
>>
>> Seems to be a run-time replacement of:
>> #ifdef CONFIG_CPU_BIG_ENDIAN
>> { "ibm,suspend-me", -1, -1, -1, -1, -1 },
>> { "ibm,update-nodes", -1, 0, -1, -1, -1, 4096 },
>> { "ibm,update-properties", -1, 0, -1, -1, -1, 4096 },
>> #endif
>>
>> It looks to be handled logically:
>> + if (IS_ENABLED(CONFIG_CPU_LITTLE_ENDIAN) &&
>> + (func->flags & RTAS_FN_FLAG_BANNED_FOR_SYSCALL_ON_LE))
>> + goto err;
>>
>> Perhaps, also allow the addition of any future special cases
>> for rtas functions easier to maintain?
>
> Makes sense, though I'm slightly confused about the original rationale
> for the ifdef and why it's not being fixed in userspace.
Nick C's explanation is correct. I will make the commit message more
explicit about the conversion, and document the flag in the code.
The original rationale:
commit de0f7349a0dd072e54b5fc04c305907b22d28a5f
Author: Nathan Lynch
Date: Mon Dec 7 15:51:33 2020 -0600
powerpc/rtas: prevent suspend-related sys_rtas use on LE
While drmgr has had work in some areas to make its RTAS syscall
interactions endian-neutral, its code for performing partition
migration via the syscall has never worked on LE. While it is able to
complete ibm,suspend-me successfully, it crashes when attempting the
subsequent ibm,update-nodes call.
drmgr is the only known (or plausible) user of ibm,suspend-me,
ibm,update-nodes, and ibm,update-properties, so allow them only in
big-endian configurations.
To summarize: we know these functions have never had working users via
sys_rtas on ppc64le, and we want to keep it that way.
> Slightly clunky name though, something like
> RTAS_FN_FLAG_SYSCALL_BE_ONLY might be less clunky?
RTAS_FN_FLAG_BANNED_FOR_SYSCALL_ON_LE is verbose, but I think it
communicates better that we are consciously imposing a policy in a
specific context.
Re: [PATCH 10/13] powerpc/rtas: improve function information lookups
Nick Child writes:
> On 11/18/22 09:07, Nathan Lynch wrote:
>> +static int __init rtas_token_to_function_xarray_init(void)
>> +{
>> +int err = 0;
>> +
>> +for (size_t i = 0; i < ARRAY_SIZE(rtas_function_table); ++i) {
>> +const struct rtas_function *func = &rtas_function_table[i];
>> +const s32 token = func->token;
>> +
>> +if (token == RTAS_UNKNOWN_SERVICE)
>> +continue;
>> +
>> +err = xa_err(xa_store(&rtas_token_to_function_xarray,
>> + token, (void *)func, GFP_KERNEL));
>> +if (err)
>> +break;
>> +}
>> +
>> +return err;
>> +}
>> +arch_initcall(rtas_token_to_function_xarray_init);
>> +
>> +static const struct rtas_function *rtas_token_to_function(s32 token)
>> +{
>> +const struct rtas_function *func;
>> +
>> +if (WARN_ONCE(token < 0, "invalid token %d", token))
>> +return NULL;
>> +
>> +func = xa_load(&rtas_token_to_function_xarray, (unsigned long)token);
>> +
> Why typecast token here and not in xa_store?
No good reason. I'll add it to the xa_store() call site.
>> +static void __init rtas_function_table_init(void)
>> +{
>> +struct property *prop;
>> +
>> +for (size_t i = 0; i < ARRAY_SIZE(rtas_function_table); ++i) {
>> +struct rtas_function *curr = &rtas_function_table[i];
>> +struct rtas_function *prior;
>> +int cmp;
>> +
>> +curr->token = RTAS_UNKNOWN_SERVICE;
>> +
>> +if (i == 0)
>> +continue;
>> +/*
>> + * Ensure table is sorted correctly for binary search
>> + * on function names.
>> + */
>> +prior = &rtas_function_table[i - 1];
>> +
>> +cmp = strcmp(prior->name, curr->name);
>> +if (cmp < 0)
>> +continue;
>> +
>> +if (cmp == 0) {
>> +pr_err("'%s' has duplicate function table entries\n",
>> + curr->name);
>> +} else {
>> +pr_err("function table unsorted: '%s' wrongly precedes
>> '%s'\n",
>> + prior->name, curr->name);
>> +}
>> +}
> Just a thought, would it be simpler to use sort()? you already have the
> cmp_func implemented for bsearch().
It's an option, but I think a tradeoff is that we would have to
sacrifice some const-ness in the data structures (i.e. remove the const
qualifier from struct rtas_function's fields). And the table has to be
in *some* order, so it may as well be sorted by name from the start.
That said, I don't love resorting to a boot-time check for this. We
could sidestep the issue by generating the C code for the table and
indexes at build time, but it's hard to justify the effort when the set
of RTAS functions changes very slowly over time.
> As for the series as a whole:
> I am no RTAS expert but was able to build, boot and mess around with new
> tracepoints without errors:
>
> Tested-by: Nick Child
Thanks for testing and reviewing!
Re: [PATCH 10/13] powerpc/rtas: improve function information lookups
Andrew Donnellan 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.
> 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_SCANrtas_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.
Re: [PATCH 10/13] powerpc/rtas: improve function information lookups
On Mon, 2022-11-28 at 18:08 -0600, Nathan Lynch wrote:
> Andrew Donnellan 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 DonnellanOzLabs, ADL Canberra
a...@linux.ibm.com IBM Australia Limited
Re: [PATCH 10/13] powerpc/rtas: improve function information lookups
Andrew Donnellan writes:
> On Mon, 2022-11-28 at 18:08 -0600, Nathan Lynch wrote:
>> Andrew Donnellan 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.
