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

From: Nathan Lynch <nathanl@linux.ibm.com>
To: Nick Child <nnac123@linux.ibm.com>, linuxppc-dev@lists.ozlabs.org
Cc: tyreld@linux.ibm.com, ldufour@linux.ibm.com, ajd@linux.ibm.com,
	npiggin@gmail.com
Subject: Re: [PATCH 10/13] powerpc/rtas: improve function information lookups
Date: Mon, 28 Nov 2022 15:57:13 -0600	[thread overview]
Message-ID: <87wn7e91za.fsf@linux.ibm.com> (raw)
In-Reply-To: <43451bb2-7ed9-e538-4334-2f4a462c7805@linux.ibm.com>

Nick Child <nnac123@linux.ibm.com> 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 <nnac123@linux.ibm.com>

Thanks for testing and reviewing!