Re: [PATCH 2/3] powerpc/tm: P9 disabled suspend mode workaround

Benjamin Herrenschmidt [off-list ref] writes:

On Fri, 2017-10-06 at 18:46 +1100, Cyril Bur wrote:

...

quoted

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.

quoted

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