Re: [RFC PATCH 06/36] ACPI / PPTT: Stop acpi_count_levels() expecting callers to clear levels

[James Morse <james.morse@arm.com>]

Hi Jonathan,

On 16/07/2025 16:51, Jonathan Cameron wrote:
> On Fri, 11 Jul 2025 18:36:18 +0000
> James Morse <james.morse@arm.com> 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.
>>
>> 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.
>>
>> Explicitly zero the levels variable, so the count always starts at zero.
>> This saves any additional callers having to work out they need to do this.

> This is all a bit fiddly as we now end up with that initialized in various
> different places.

I've debugged this one a few times, (turns out I'm forgetful) ... I figured doing this
was better than adding a comment to warn others.
As its static, I figured it was something the compiler can optimise out if there is a
duplicate. (I couldn't find any initialisation I could remove because of this)


> Perhaps simpler to have acpi_count_levels() return the
> number of levels rather than void. Then return number of levels rather
> than 0 on success from acpi_get_cache_info(). Negative error codes used
> for failure just like now.
> 
> That would leave only a local variable in acpi_count_levels being
> initialized to 0 and passed to acpi_find_cache_level() before being
> returned when the loop terminates.
> 
> I think that sequence then makes it such that we can't fail to
> initialize it at without the compiler noticing and screaming.
> 
> Requires a few changes from if (ret) to if (ret < 0) at callers
> of acpi_get_cache_info() but looks simple (says the person who
> hasn't actually coded it!)

Breaking the symmetry between levels and split_levels is an argument against this.

I think within pptt.c this is fine, because 'level's is used internally as
'starting_level', and this expectation it was initialised to zero is a nasty surprise.

But exposing that from acpi_get_cache_info() looks stranger - and would need to touch
users in cacheinfo, arm64, riscv.

I've updated acpi_count_levels() to look as you describe - that at least makes it harder
to miss this in future. (not sure whether it saves anything)


>> diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c
>> index 13619b1b821b..13ca2eee3b98 100644
>> --- a/drivers/acpi/pptt.c
>> +++ b/drivers/acpi/pptt.c
>> @@ -183,7 +183,7 @@ acpi_find_cache_level(struct acpi_table_header *table_hdr,
>>   * @cpu_node: processor node we wish to count caches for
>>   * @levels: Number of levels if success.
>>   * @split_levels:	Number of split cache levels (data/instruction) if
>> - *			success. Can by NULL.
>> + *			success. Can be NULL.
> 
> Grumpy reviewer hat.  Unrelated cleanup up - good to have but not in this patch where
> it's a distraction.

I was hoping diff would keep it as one hunk. Happy to leave it tyopd!


Thanks,

James