diff mbox series

[v3] powerpc: Adjust config HOTPLUG_CPU dependency

Message ID 20231122101040.231850-1-vishalc@linux.ibm.com (mailing list archive)
State New
Headers show
Series [v3] powerpc: Adjust config HOTPLUG_CPU dependency | expand

Checks

Context Check Description
snowpatch_ozlabs/github-powerpc_ppctests success Successfully ran 8 jobs.
snowpatch_ozlabs/github-powerpc_selftests success Successfully ran 8 jobs.
snowpatch_ozlabs/github-powerpc_sparse success Successfully ran 4 jobs.
snowpatch_ozlabs/github-powerpc_clang success Successfully ran 6 jobs.
snowpatch_ozlabs/github-powerpc_kernel_qemu success Successfully ran 23 jobs.

Commit Message

Vishal Chourasia Nov. 22, 2023, 10:10 a.m. UTC 
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@linux.vnet.ibm.com>
Suggested-by: Aneesh Kumar K V <aneesh.kumar@linux.ibm.com>
Suggested-by: Michael Ellerman <mpe@ellerman.id.au>
Signed-off-by: Vishal Chourasia <vishalc@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(-)

Comments

Michael Ellerman Nov. 24, 2023, 2:39 a.m. UTC | #1 
Hi Vishal,

I think our wires got crossed here somewhere :)

Vishal Chourasia <vishalc@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@linux.vnet.ibm.com>
> Suggested-by: Aneesh Kumar K V <aneesh.kumar@linux.ibm.com>
> Suggested-by: Michael Ellerman <mpe@ellerman.id.au>
> Signed-off-by: Vishal Chourasia <vishalc@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.
Vishal Chourasia Nov. 24, 2023, 9:42 p.m. UTC | #2 
On 24/11/23 8:09 am, Michael Ellerman wrote:
> Hi Vishal,
> 
> I think our wires got crossed here somewhere :)
> 
> Vishal Chourasia <vishalc@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@linux.vnet.ibm.com>
>> Suggested-by: Aneesh Kumar K V <aneesh.kumar@linux.ibm.com>
>> Suggested-by: Michael Ellerman <mpe@ellerman.id.au>
>> Signed-off-by: Vishal Chourasia <vishalc@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.
> 
Tried and it works. No warnings to be seen.

But, consider this scenario

 SMP [=y]
 CONFIG_PPC64 [=y]
 PPC_BOOK3S_32 [=y]
 CONFIG_CPU_BIG_ENDIAN [=y]
 PPC_MPC52xx [=y]
 PPC_EFIKA [=y]

With above config setting in place, config hotplug_cpu will be set to 'N' based
on your patch. And, If we consider current way of things, config hotplug_cpu
will be set to 'Y'

Below patch fixes the warning by making arch_hibernation_possible depend upon
all of the necessary platforms and extends the dependency of hotplug_cpu on
arch_suspend_possible


diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 6f105ee4f3cf..13e32494bbf1 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -380,8 +380,8 @@ 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 PPC_PSERIES || PPC_PMAC || PPC_POWERNV || FSL_SOC_BOOKE

 config ARCH_SUSPEND_POSSIBLE
        def_bool y
@@ -566,10 +566,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 ARCH_HIBERNATION_POSSIBLE || ARCH_SUSPEND_POSSIBLE
+
 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.


What is the "correct value" of config HOTPLUG_CPU (Y/N) given the config
settings (from above)? I am finding it hard to justify N over Y. Can you explain
a bit more?

Your patch produces N, where as the patch i have provided in this mail would
sets it to Y.

does cpu hotplug code path calls into suspend code logic for any
platforms upon which arch_suspend_possible config depends.. ex. PPC_EFIKA, etc.
diff mbox series

Patch

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 6f105ee4f3cf..87c8134da3da 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -156,6 +156,7 @@  config PPC
 	select ARCH_HAS_UACCESS_FLUSHCACHE
 	select ARCH_HAS_UBSAN_SANITIZE_ALL
 	select ARCH_HAVE_NMI_SAFE_CMPXCHG
+	select ARCH_HIBERNATION_POSSIBLE        if (PPC_PSERIES || PPC_PMAC || PPC_POWERNV || FSL_SOC_BOOKE)
 	select ARCH_KEEP_MEMBLOCK
 	select ARCH_MHP_MEMMAP_ON_MEMORY_ENABLE	if PPC_RADIX_MMU
 	select ARCH_MIGHT_HAVE_PC_PARPORT
@@ -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)
 	select ARCH_USE_BUILTIN_BSWAP
 	select ARCH_USE_CMPXCHG_LOCKREF		if PPC64
 	select ARCH_USE_MEMTEST
@@ -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)
 	help
 	  Say Y here to be able to disable and re-enable individual
 	  CPUs at runtime on SMP machines.