On 24/11/23 8:09 am, Michael Ellerman wrote:
> Hi Vishal,
>
> I think our wires got crossed here somewhere :)
>
> Vishal Chourasia <vish...@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 <sri...@linux.vnet.ibm.com>
>> Suggested-by: Aneesh Kumar K V <aneesh.ku...@linux.ibm.com>
>> Suggested-by: Michael Ellerman <m...@ellerman.id.au>
>> Signed-off-by: Vishal Chourasia <vish...@linux.ibm.com>
>>
>> v2: https://lore.kernel.org/all/20231122092303.223719-1-vish...@linux.ibm.com
>> v1: https://lore.kernel.org/all/20231114082046.6018-1-vish...@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.
