Vishal Chourasia [off-list ref] writes:

On 17/11/23 4:52 am, Michael Ellerman wrote:

quoted

Vishal Chourasia [off-list ref] writes:

quoted

On 15/11/23 5:46 pm, Aneesh Kumar K V wrote:

quoted

On 11/15/23 5:23 PM, Vishal Chourasia wrote:

quoted

On 15/11/23 1:39 pm, Aneesh Kumar K.V wrote:

quoted

Vishal Chourasia [off-list ref] writes:

quoted

This patch modifies the ARCH_HIBERNATION_POSSIBLE option to ensure that it
correctly depends on these PowerPC configurations being enabled. As a result,
it prevents the HOTPLUG_CPU from being selected when the required dependencies
are not satisfied.

This change aligns the dependency tree with the expected hardware support for
CPU hot-plugging under PowerPC architectures, ensuring that the kernel
configuration steps do not lead to inconsistent states.

Signed-off-by: Vishal Chourasia <redacted>
---
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 beingDuring 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'
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 | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 6f105ee4f3cf..bf99ff9869f6 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig

@@ -380,8 +380,9 @@ 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

I am wondering whether it should be switched to using select from
config PPC? 

selecting ARCH_HIBERNATION_POSSIBLE based on value of config PPC
will not guarantee config PPC_PSERIES being set

PPC_PSERIES can be set to N, even when config PPC is set.

I understand what you meant before. Having ARCH_HIBERNATION_POSSIBLE under config PPC makes more sense.

quoted

quoted

quoted

grep -A 5 -i "config ppc_pseries" arch/powerpc/platforms/pseries/Kconfig
config PPC_PSERIES
        depends on PPC64 && PPC_BOOK3S
        bool "IBM pSeries & new (POWER5-based) iSeries"
        select HAVE_PCSPKR_PLATFORM
        select MPIC
        select OF_DYNAMIC

modified   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

Though, even with these changes I was able to reproduce same warnings. (using steps from above)
It's because one can enable HIBERNATION manually.

But how? You shouldn't be able to enable it manually, it depends on
ARCH_HIBERNATION_POSSIBLE which shouldn't be enabled.

For the above to work you also need to make it default n, eg:

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 6f105ee4f3cf..dd2a9b938188 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig

@@ -380,8 +380,7 @@ 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 n

 config ARCH_SUSPEND_POSSIBLE
        def_bool y

Ran make randconfig bunch of times.

# make KCONFIG_SEED=0x97C94A3C randconfig
make[1]: Entering directory ''
  GEN     Makefile
KCONFIG_SEED=0x97C94A3C

WARNING: unmet direct dependencies detected for HOTPLUG_CPU
  Depends on [n]: SMP [=y] && (PPC_PSERIES [=n] || PPC_PMAC [=n] ||
                                       PPC_POWERNV [=n] || FSL_SOC_BOOKE [=n])
  Selected by [y]:
  - PM_SLEEP_SMP [=y] && SMP [=y] && (ARCH_SUSPEND_POSSIBLE [=y]
                                    || ARCH_HIBERNATION_POSSIBLE [=n]) && PM_SLEEP [=y]
#
# configuration written to .config
#
make[1]: Leaving directory ''

As per my understanding,

"Depends on" clause of the config HOTPLUG_CPU as CPU hotplugging is only
available for SMP systems and is only available for following power platforms
(PPC_PSERIES || PPC_PMAC || PPC_POWERNV  || FSL_SOC_BOOKE)

Also, config HOTPLUG_CPU  is being selected by config PM_SLEEP_SMP
config PM_SLEEP_SMP
        def_bool y
        depends on SMP
        depends on ARCH_SUSPEND_POSSIBLE || ARCH_HIBERNATION_POSSIBLE
        depends on PM_SLEEP
        select HOTPLUG_CPU <----- Here

Power management functionality depends and vary upon arch (and in powerpc case, platforms)
supporting suspend/hibernation

There are some platforms which support suspend and hence ARCH_SUSPEND_POSSIBLE
is set, leading to PM_SLEEP_SMP being set, and ultimately, config HOTPLUG_CPU gets set.
But, CPU hotplug is not supported for such platform and hence the conflict.

Yeah. We need to restrict ARCH_SUSPEND_POSSIBLE in a similar way to
ARCH_HIBERNATION_POSSIBLE.

cheers