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

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

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 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 | 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


cheers