Benjamin Herrenschmidt <b...@kernel.crashing.org> writes:
> On Fri, 2017-10-06 at 18:46 +1100, Cyril Bur wrote:
...
>> diff --git a/arch/powerpc/kernel/cputable.c b/arch/powerpc/kernel/cputable.c
>> index 760872916013..2cb01b48123a 100644
>> --- a/arch/powerpc/kernel/cputable.c
>> +++ b/arch/powerpc/kernel/cputable.c
>> @@ -2301,6 +2302,17 @@ void __init identify_cpu_name(unsigned int pvr)
>> }
>> }
>>
>> +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
>> +bool tm_suspend_supported(void)
>> +{
>> + if (cpu_has_feature(CPU_FTR_TM)) {
>> + if (pvr_version_is(PVR_POWER9) && ppc_tm_state !=
>> TM_STATE_NO_SUSPEND)
>> + return false;
>> + return true;
>> + }
>
> Hrm... so if state is "NO SUSPEND" you return "true" ? Isn't this
> backward ? Or I don't understand what this is about...
Yeah it's a bit confuzzled.
I literally wrote it for you on a post-it Cyril! >:D
tm_suspend_supported() should be called tm_no_suspend_mode(). Where "no
suspend mode" is the new "mode" we're adding where suspend is not supported.
Then tm_no_suspend_mode() should be:
+ if (pvr_version_is(PVR_POWER9) && ppc_tm_state == TM_STATE_NO_SUSPEND)
+ return true;
+ return false;
And then all the extra checks and warnings in patch 3 just use it like:
WARN_ON(tm_no_suspend_mode());
Because they're in paths where we shouldn't get to if suspend is
disabled.
I don't think we need to check CPU_FTR_TM because it's only called from
TM paths anyway. But we could add that to be paranoid. Or probably
better, when TM is forced off (below) we set ppc_tm_state to off.
>> diff --git a/arch/powerpc/kernel/setup_64.c b/arch/powerpc/kernel/setup_64.c
>> index e37c26d2e54b..227ac600a1b7 100644
>> --- a/arch/powerpc/kernel/setup_64.c
>> +++ b/arch/powerpc/kernel/setup_64.c
>> @@ -265,11 +267,13 @@ early_param("ppc_tm", parse_ppc_tm);
>>
>> static void check_disable_tm(void)
>> {
>> - if (cpu_has_feature(CPU_FTR_TM) && ppc_tm_state == TM_STATE_OFF) {
>> - printk(KERN_NOTICE "Disabling hardware transactional memory
>> (HTM)\n");
>> - cur_cpu_spec->cpu_user_features2 &=
>> - ~(PPC_FEATURE2_HTM_NOSC | PPC_FEATURE2_HTM);
>> - cur_cpu_spec->cpu_features &= ~CPU_FTR_TM;
>> + if (cpu_has_feature(CPU_FTR_TM)) {
>> + if (ppc_tm_state == TM_STATE_OFF || (!tm_suspend_supported())) {
>> + printk(KERN_NOTICE "Disabling hardware transactional
>> memory (HTM)\n");
>> + cur_cpu_spec->cpu_user_features2 &=
>> + ~(PPC_FEATURE2_HTM_NOSC | PPC_FEATURE2_HTM);
>> + cur_cpu_spec->cpu_features &= ~CPU_FTR_TM;
>> + }
>
> So that code translates to if TM is off or doesn't support suspend,
> disable TM. Are we sure that's really what we meant here ?
It should be:
+ if (!cpu_has_feature(CPU_FTR_TM))
+ return;
+
+ if (ppc_tm_state == TM_STATE_OFF || \
+ (pvr_version_is(PVR_POWER9) && ppc_tm_state !=
TM_STATE_NO_SUSPEND)) {
+ printk(KERN_NOTICE "Disabling hardware transactional memory
(HTM)\n");
+ cur_cpu_spec->cpu_user_features2 &= ~(PPC_FEATURE2_HTM_NOSC |
PPC_FEATURE2_HTM);
+ cur_cpu_spec->cpu_features &= ~CPU_FTR_TM;
+ }
And as I mentioned above perhaps we should also do:
+ ppc_tm_state = TM_STATE_OFF;
cheers
