Vishal Chourasia <vish...@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 <vish...@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 <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 | 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
