Re: [PATCH v2 18/19] powerpc/rtas: introduce rtas_function_token() API

Michael Ellerman [off-list ref] writes:

Nathan Lynch via B4 Submission Endpoint
[off-list ref] writes:

quoted

From: Nathan Lynch <redacted>

Users of rtas_token() supply a string argument that can't be validated
at build time. A typo or misspelling has to be caught by inspection or
by observing wrong behavior at runtime.

Since the core RTAS code now has consolidated the names of all
possible RTAS functions and mapped them to their tokens, token lookup
can be implemented using symbolic constants to index a static array.

So introduce rtas_function_token(), a replacement API which does that,
along with a rtas_service_present()-equivalent helper,
rtas_function_implemented(). Callers supply an opaque predefined
function handle which is used internally to index the function
table. Typos or other inappropriate arguments yield build errors, and
the function handle is a type that can't be easily confused with RTAS
tokens or other integer types.

Why not go all the way and have the rtas_call() signature be:

  int rtas_call(rtas_fn_handle_t fn, int, int, int *, ...);


And have it do the token lookup internally? That way a caller can never
inadvertantly pass a random integer to rtas_call().

And instead of eg:

	error = rtas_call(rtas_function_token(RTAS_FN_GET_TIME_OF_DAY), 0, 8, ret);

we'd just need:

	error = rtas_call(RTAS_FN_GET_TIME_OF_DAY, 0, 8, ret);


Doing the conversion all at once might be tricky. So maybe we need to add
rtas_fn_call() which takes rtas_fn_handle_t so we can convert cases individually?

Anyway just a thought. I guess we could merge this as-is and then do a
further change to use rtas_fn_handle_t later.

You read my mind :-) But I want to go further and make the eventual
replacement for rtas_call() non-variadic, which will eliminate another
class of usage error.

Getting more ambitious: the ideal situation IMO would be that every use
of rtas_call() or its replacement is tidily contained in a C function in
kernel/rtas.c, where complexities like retries and error code
translation can be performed in a uniform way.

Anyway, a transition away from rtas_call(), whatever form it takes,
probably needs to happen incrementally.

quoted

diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
index 41c430dc40c2..17e59306ce63 100644
--- a/arch/powerpc/kernel/rtas.c
+++ b/arch/powerpc/kernel/rtas.c

@@ -453,6 +453,26 @@ static struct rtas_function rtas_function_table[] __ro_after_init = {
 	},
 };
 
+/**
+ * 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;

This needs:

+	// If RTAS is not present or not initialised (yet) return unknown
+	if (!rtas.dev)
+		return RTAS_UNKNOWN_SERVICE;
+

Otherwise powernv breaks because it looks up tokens and gets back 0,
because we never got as far as rtas_function_table_init() (to set all the
tokens to RTAS_UNKNOWN_SERVICE), because we bailed out at the start of
rtas_initialize() when we found no /rtas node.

Oh! OK, thanks.