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
