Hi James,

On Thu, Aug 28, 2025 at 04:57:15PM +0100, James Morse wrote:

Hi Dave,

On 27/08/2025 11:49, Dave Martin wrote:

quoted

On Fri, Aug 22, 2025 at 03:29:45PM +0000, James Morse wrote:

quoted

acpi_count_levels() passes the number of levels back via a pointer argument.
It also passes this to acpi_find_cache_level() as the starting_level, and
preserves this value as it walks up the cpu_node tree counting the levels.

This means the caller must initialise 'levels' due to acpi_count_levels()
internals. The only caller acpi_get_cache_info() happens to have already
initialised levels to zero, which acpi_count_levels() depends on to get the
correct result.

Two results are passed back from acpi_count_levels(), unlike split_levels,
levels is not optional.

Split these two results up. The mandatory 'levels' is always returned,
which hides the internal details from the caller, and avoids having
duplicated initialisation in all callers. split_levels remains an
optional argument passed back.

Nit: I found all this a bit hard to follow.

This seems to boil down to:

--8<--

In acpi_count_levels(), the initial value of *levels passed by the
caller is really an implementation detail of acpi_count_levels(), so it
is unreasonable to expect the callers of this function to know what to
pass in for this parameter.  The only sensible initial value is 0,
which is what the only upstream caller (acpi_get_cache_info()) passes.

Use a local variable for the starting cache level in acpi_count_levels(),
and pass the result back to the caller via the function return value.

Gid rid of the levels parameter, which has no remaining purpose.

Fix acpi_get_cache_info() to match.

-->8--

I've taken this instead,

OK

[...]

quoted

quoted

@@ -731,7 +735,7 @@ int acpi_get_cache_info(unsigned int cpu, unsigned int *levels,
 	if (!cpu_node)
 		return -ENOENT;
 
-	acpi_count_levels(table, cpu_node, levels, split_levels);
+	*levels = acpi_count_levels(table, cpu_node, split_levels);
 
 	pr_debug("Cache Setup: last_level=%d split_levels=%d\n",
 		 *levels, split_levels ? *split_levels : -1);

Otherwise, looks reasonable to me.

(But see my comments on the next patches re whether we really need this.)

It was enough fun to debug that I'd like to save anyone else the trouble!

Fair enough.

Cheers
---Dave