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

[Michael Ellerman <mpe@ellerman.id.au>]

Vishal Chourasia <vishalc@linux.ibm.com> writes:
> 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.

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

cheers