Re: [PATCH 04/33] ACPI / PPTT: Stop acpi_count_levels() expecting callers to clear levels

[Dave Martin <Dave.Martin@arm.com>]

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:
> > On Fri, Aug 22, 2025 at 03:29:45PM +0000, James Morse wrote:
> >> 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

[...]

> >> @@ -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