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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250716165142.00002c46@huawei.com>
Date: Wed, 16 Jul 2025 16:51:42 +0100
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: James Morse <james.morse@....com>
CC: <linux-kernel@...r.kernel.org>, <linux-arm-kernel@...ts.infradead.org>,
	"Rob Herring" <robh@...nel.org>, Ben Horgan <ben.horgan@....com>, Rohit
 Mathew <rohit.mathew@....com>, Shanker Donthineni <sdonthineni@...dia.com>,
	"Zeng Heng" <zengheng4@...wei.com>, Lecopzer Chen <lecopzerc@...dia.com>,
	"Carl Worth" <carl@...amperecomputing.com>,
	<shameerali.kolothum.thodi@...wei.com>, D Scott Phillips OS
	<scott@...amperecomputing.com>, <lcherian@...vell.com>,
	<bobo.shaobowang@...wei.com>, <tan.shaopeng@...itsu.com>,
	<baolin.wang@...ux.alibaba.com>, Jamie Iles <quic_jiles@...cinc.com>, Xin Hao
	<xhao@...ux.alibaba.com>, <peternewman@...gle.com>, <dfustini@...libre.com>,
	<amitsinght@...vell.com>, David Hildenbrand <david@...hat.com>, Rex Nie
	<rex.nie@...uarmicro.com>, Dave Martin <dave.martin@....com>, Koba Ko
	<kobak@...dia.com>, Sudeep Holla <sudeep.holla@....com>
Subject: Re: [RFC PATCH 06/36] ACPI / PPTT: Stop acpi_count_levels()
 expecting callers to clear levels

On Fri, 11 Jul 2025 18:36:18 +0000
James Morse <james.morse@....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.

Hi James,

This is all a bit fiddly as we now end up with that initialized in various
different places. 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!)

> 
> Signed-off-by: James Morse <james.morse@....com>
> Reviewed-by: Sudeep Holla <sudeep.holla@....com>
> ---
>  drivers/acpi/pptt.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> 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.

>   *
>   * Given a processor node containing a processing unit, walk into it and count
>   * how many levels exist solely for it, and then walk up each level until we hit
> @@ -196,6 +196,8 @@ static void acpi_count_levels(struct acpi_table_header *table_hdr,
>  			      struct acpi_pptt_processor *cpu_node,
>  			      unsigned int *levels, unsigned int *split_levels)
>  {
> +	*levels = 0;
> +
>  	do {
>  		acpi_find_cache_level(table_hdr, cpu_node, levels, split_levels, 0, 0);
>  		cpu_node = fetch_pptt_node(table_hdr, cpu_node->parent);