[PATCH v3] powerpc: Adjust config HOTPLUG_CPU dependency

[Michael Ellerman]

Hi Vishal,

I think our wires got crossed here somewhere :)

Vishal Chourasia <vishalc at linux.ibm.com> writes:
> Changed HOTPLUG_CPU dependency to SMP and either ARCH_HIBERNATION_POSSIBLE or
> ARCH_SUSPEND_POSSIBLE, aligning with systems' suspend/hibernation capabilities.
> This update link CPU hotplugging more logically with platforms' capabilities.
>
> configs ARCH_HIBERNATION_POSSIBLE and ARCH_SUSPEND_POSSIBLE are now selected
> only if required platform dependencies are met.
>
> Reported-by: Srikar Dronamraju <srikar at linux.vnet.ibm.com>
> Suggested-by: Aneesh Kumar K V <aneesh.kumar at linux.ibm.com>
> Suggested-by: Michael Ellerman <mpe at ellerman.id.au>
> Signed-off-by: Vishal Chourasia <vishalc at linux.ibm.com>
>
> v2: https://lore.kernel.org/all/20231122092303.223719-1-vishalc@linux.ibm.com
> v1: https://lore.kernel.org/all/20231114082046.6018-1-vishalc@linux.ibm.com
> ---
> During the configuration process with 'make randconfig' followed by
> 'make olddefconfig', we observed a warning indicating an unmet direct
> dependency for the HOTPLUG_CPU option. The dependency in question relates to
> various PowerPC configurations (PPC_PSERIES, PPC_PMAC, PPC_POWERNV,
> FSL_SOC_BOOKE) which were not enabled, yet the HOTPLUG_CPU was being
> erroneously selected due to an implicit assumption by the PM_SLEEP_SMP option.
> This misalignment in dependencies could potentially lead to inconsistent kernel
> configuration states, especially when considering the necessary hardware
> support for CPU hot-plugging on PowerPC platforms. The patch aims to correct
> this by ensuring that ARCH_HIBERNATION_POSSIBLE is contingent upon the
> appropriate PowerPC configurations being active.
>
> steps to reproduce (before applying the patch):
>
> Run 'make pseries_le_defconfig'
> Run 'make menuconfig'
> Enable hibernation [ Kernel options -> Hibernation (aka 'suspend to disk') ] 
> Disable [ Platform support -> IBM PowerNV (Non-Virtualized) platform support ]
> Disable [ Platform support -> IBM pSeries & new (POWER5-based) iSeries ]
> Enable SMP [ Processor support -> Symmetric multi-processing support ]
> Save the config
> Run 'make olddefconfig'
>
>  arch/powerpc/Kconfig | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index 6f105ee4f3cf..87c8134da3da 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -166,6 +167,7 @@ config PPC
>  	select ARCH_STACKWALK
>  	select ARCH_SUPPORTS_ATOMIC_RMW
>  	select ARCH_SUPPORTS_DEBUG_PAGEALLOC	if PPC_BOOK3S || PPC_8xx || 40x
> +	select ARCH_SUSPEND_POSSIBLE            if (ADB_PMU || PPC_EFIKA || PPC_LITE5200 || PPC_83xx || (PPC_85xx && !PPC_E500MC) || PPC_86xx || PPC_PSERIES || 44x || 40x)

I know Aneesh suggested moving symbols to under PPC, but I think this is
too big and complicated to be under PPC.

> @@ -381,13 +383,9 @@ config DEFAULT_UIMAGE
>  
>  config ARCH_HIBERNATION_POSSIBLE
>  	bool
> -	default y
>  config ARCH_SUSPEND_POSSIBLE
> -	def_bool y
> -	depends on ADB_PMU || PPC_EFIKA || PPC_LITE5200 || PPC_83xx || \
> -		   (PPC_85xx && !PPC_E500MC) || PPC_86xx || PPC_PSERIES \
> -		   || 44x || 40x
> +	bool
>  
>  config ARCH_SUSPEND_NONZERO_CPU
>  	def_bool y
> @@ -568,8 +566,7 @@ config ARCH_USING_PATCHABLE_FUNCTION_ENTRY
>  
>  config HOTPLUG_CPU
>  	bool "Support for enabling/disabling CPUs"
> -	depends on SMP && (PPC_PSERIES || \
> -		PPC_PMAC || PPC_POWERNV || FSL_SOC_BOOKE)
> +	depends on SMP && (ARCH_HIBERNATION_POSSIBLE || ARCH_SUSPEND_POSSIBLE)

It's good to fix these warnings, but IMHO the result is that the
dependencies are now backward.

HOTPLUG_CPU should retain its original dependency list. It's easier to
reason directly about "what platforms support CPU hotplug?", oh it's
pseries/powernv/powermac/85xx, because they implement cpu_disable().

If there's some dependency from suspend/hibernate on CPU hotplug, then
those symbols (suspend/hibernate) should depend on something to do with
CPU hotplug.

Can you try the patch below?

Though, going back to your original reproduction case, that kernel is
configured for Book3S 64, but with no platforms enabled, which is a
non-sensical configuration (it can't boot on any actual machines). So
possibly the real root cause is that, and we should find some way to
block creating a config that has no platforms enabled.

cheers


diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 6f105ee4f3cf..9fe656a17017 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -380,11 +380,12 @@ config DEFAULT_UIMAGE
 	  Used to allow a board to specify it wants a uImage built by default
 
 config ARCH_HIBERNATION_POSSIBLE
-	bool
-	default y
+	def_bool y
+	depends on !SMP || HAVE_HOTPLUG_CPU
 
 config ARCH_SUSPEND_POSSIBLE
 	def_bool y
+	depends on !SMP || HAVE_HOTPLUG_CPU
 	depends on ADB_PMU || PPC_EFIKA || PPC_LITE5200 || PPC_83xx || \
 		   (PPC_85xx && !PPC_E500MC) || PPC_86xx || PPC_PSERIES \
 		   || 44x || 40x
@@ -566,10 +567,14 @@ config ARCH_USING_PATCHABLE_FUNCTION_ENTRY
 	def_bool $(success,$(srctree)/arch/powerpc/tools/gcc-check-fpatchable-function-entry.sh $(CC) -mlittle-endian) if PPC64 && CPU_LITTLE_ENDIAN
 	def_bool $(success,$(srctree)/arch/powerpc/tools/gcc-check-fpatchable-function-entry.sh $(CC) -mbig-endian) if PPC64 && CPU_BIG_ENDIAN
 
+config HAVE_HOTPLUG_CPU
+	def_bool y
+	depends on SMP
+	depends on PPC_PSERIES || PPC_PMAC || PPC_POWERNV || FSL_SOC_BOOKE
+
 config HOTPLUG_CPU
 	bool "Support for enabling/disabling CPUs"
-	depends on SMP && (PPC_PSERIES || \
-		PPC_PMAC || PPC_POWERNV || FSL_SOC_BOOKE)
+	depends on HAVE_HOTPLUG_CPU
 	help
 	  Say Y here to be able to disable and re-enable individual
 	  CPUs at runtime on SMP machines.