Re: [PATCH] powerpc: Restrict ARCH_HIBERNATION_POSSIBLE to supported configurations

[Vishal Chourasia <vishalc@linux.ibm.com>]

On 17/11/23 4:52 am, Michael Ellerman wrote:
> Vishal Chourasia <vishalc@linux.ibm.com> writes:
>> On 15/11/23 5:46 pm, Aneesh Kumar K V wrote:
>>> On 11/15/23 5:23 PM, Vishal Chourasia wrote:
>>>> On 15/11/23 1:39 pm, Aneesh Kumar K.V wrote:
>>>>> Vishal Chourasia <vishalc@linux.ibm.com> writes:
>>>>>
>>>>>> 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 <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 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.
>>>>> 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.


>
> cheers