linux-kernel - RE: [PATCH v2 3/3] ACPI/NUMA: Remove architecture dependent remainings

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]

Date: Tue, 19 Mar 2024 13:44:53 -0700
From: Dan Williams <dan.j.williams@...el.com>
To: Robert Richter <rrichter@....com>, "Rafael J. Wysocki" <rafael@...nel.org>
CC: Dave Hansen <dave.hansen@...ux.intel.com>, Dan Williams
	<dan.j.williams@...el.com>, Alison Schofield <alison.schofield@...el.com>,
	<linux-acpi@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<linux-cxl@...r.kernel.org>, Robert Richter <rrichter@....com>, Len Brown
	<lenb@...nel.org>
Subject: RE: [PATCH v2 3/3] ACPI/NUMA: Remove architecture dependent
 remainings

Robert Richter wrote:
> With the removal of the Itanium architecture [1] the last architecture
> dependent functions:
> 
>  acpi_numa_slit_init(), acpi_numa_memory_affinity_init()
> 
> were removed. Remove its remainings in the header files too an make
> them static.

It would have helped to clarify that this is a refactoring and absorbing
logic into the helpers to validate the correctness of the conversion
approach. I almost asked for it to be split to make that clearer, but
figured it out eventually.

> [1] cf8e8658100d arch: Remove Itanium (IA-64) architecture

Checkpatch does not like this format and has some other things to say
about this legacy code being touched.

> 
> Signed-off-by: Robert Richter <rrichter@....com>
> ---
>  drivers/acpi/numa/srat.c | 68 +++++++++++++---------------------------
>  include/linux/acpi.h     |  5 ---
>  2 files changed, 21 insertions(+), 52 deletions(-)
> 
> diff --git a/drivers/acpi/numa/srat.c b/drivers/acpi/numa/srat.c
> index 50ae8557e8d1..910609a9754b 100644
> --- a/drivers/acpi/numa/srat.c
> +++ b/drivers/acpi/numa/srat.c
> @@ -208,16 +208,21 @@ int __init srat_disabled(void)
>  	return acpi_numa < 0;
>  }
>  
> -#if defined(CONFIG_X86) || defined(CONFIG_ARM64) || defined(CONFIG_LOONGARCH)
>  /*
>   * Callback for SLIT parsing.  pxm_to_node() returns NUMA_NO_NODE for
>   * I/O localities since SRAT does not list them.  I/O localities are
>   * not supported at this point.
>   */
> -void __init acpi_numa_slit_init(struct acpi_table_slit *slit)
> +static int __init acpi_parse_slit(struct acpi_table_header *table)
>  {
> +	struct acpi_table_slit *slit = (struct acpi_table_slit *)table;
>  	int i, j;
>  
> +	if (!slit_valid(slit)) {
> +		pr_info("SLIT table looks invalid. Not used.\n");
> +		return -EINVAL;
> +	}
> +
>  	for (i = 0; i < slit->locality_count; i++) {
>  		const int from_node = pxm_to_node(i);
>  
> @@ -234,19 +239,25 @@ void __init acpi_numa_slit_init(struct acpi_table_slit *slit)
>  				slit->entry[slit->locality_count * i + j]);
>  		}
>  	}
> +
> +	return 0;
>  }
>  
> -/*
> - * Default callback for parsing of the Proximity Domain <-> Memory
> - * Area mappings
> - */
> -int __init
> -acpi_numa_memory_affinity_init(struct acpi_srat_mem_affinity *ma)
> +static int __initdata parsed_numa_memblks;

Checkpatch wants __initdata moved after the symbol name.

Otherwise this conversion looks correct.

> +static int __init
> +acpi_parse_memory_affinity(union acpi_subtable_headers * header,
> +			   const unsigned long table_end)
>  {
> +	struct acpi_srat_mem_affinity *ma;
>  	u64 start, end;
>  	u32 hotpluggable;
>  	int node, pxm;
>  
> +	ma = (struct acpi_srat_mem_affinity *)header;
> +
> +	acpi_table_print_srat_entry(&header->common);
> +
>  	if (srat_disabled())
>  		goto out_err;
>  	if (ma->header.length < sizeof(struct acpi_srat_mem_affinity)) {
> @@ -293,6 +304,8 @@ acpi_numa_memory_affinity_init(struct acpi_srat_mem_affinity *ma)
>  
>  	max_possible_pfn = max(max_possible_pfn, PFN_UP(end - 1));
>  
> +	parsed_numa_memblks++;
> +
>  	return 0;
>  out_err_bad_srat:
>  	bad_srat();
> @@ -448,27 +461,6 @@ static int __init acpi_parse_cfmws(union acpi_subtable_headers *header,
>  	(*fake_pxm)++;
>  	return 0;
>  }
> -#else
> -static inline void acpi_table_print_cedt(void) {}

Would be nice to move this patch before the printing patch otherwise
feels icky to add a line in patch2 that gets deleted in patch3.