[PATCH v3 05/12] arm64, acpi, numa: NUMA support based on SRAT and SLIT

[Robert Richter]

On 23.01.16 17:39:20, Hanjun Guo wrote:

> @@ -385,10 +386,8 @@ void __init arm64_numa_init(void)
>  {
>  	int ret = -ENODEV;
>  
> -#ifdef CONFIG_OF_NUMA
>  	if (!numa_off)
> -		ret = numa_init(arm64_of_numa_init);
> -#endif
> +		ret = numa_init(acpi_disabled ? arm64_of_numa_init : arm64_acpi_numa_init);
>  
>  	if (ret)
>  		numa_init(dummy_numa_init);

Ok, this style is mostly flavor, some people want #ifdefs (my
preference), some not. In any case it must build with or without the
config option set. But first some words why I like #ifdefs:

 * Code is easier to understand as you don't need to look at any other
   location whether it is enabled or not.

 * You can't break the build if the options are not set. Thus, you
   also don't need to check if the function is implemented for the
   unset case (valid for the coder and also the reviewer). This makes
   things a lot easier.

 * Total number of lines of code that needs to be implement is
   smaller.

However, if we don't ifdef the code, we need empty functions stubs in
the header file for them.

Also, the conditional assignment does not reduce the complexity of the
paths. It just concentrates everything in a single line.

How about the following (similar to x86)?

----
	if (!numa_off) {
#ifdef CONFIG_ACPI_NUMA
		if (!numa_init(acpi_numa_init))
			return 0;
#endif
#ifdef CONFIG_OF_NUMA
		if (!numa_init(of_numa_init))
			return 0;
#endif
	}

	return numa_init(dummy_numa_init);
----

Pretty straight and nice.

Note: The !acpi_disabled check needs to be moved to the beginning of
acpi_numa_init(). Variable ret can be removed.

-Robert