[PATCH] MIPS: CM: Drop WARN_ON(vp != 0)

[PATCH] MIPS: CM: Drop WARN_ON(vp != 0)

[James Hogan]

From: James Hogan <jhogan@kernel.org>

Since commit 68923cdc2eb3 ("MIPS: CM: Add cluster & block args to
mips_cm_lock_other()"), mips_smp_send_ipi_mask() has used
mips_cm_lock_other_cpu() with each CPU number, rather than
mips_cm_lock_other() with the first VPE in each core. Prior to r6,
multicore multithreaded systems such as dual-core dual-thread
interAptivs with CPU Idle enabled (e.g. MIPS Creator Ci40) results in
mips_cm_lock_other() repeatedly hitting WARN_ON(vp != 0).

There doesn't appear to be anything fundamentally wrong about passing a
non-zero VP/VPE number, even if it is a core's region that is locked
into the other region before r6, so remove that particular WARN_ON().

Fixes: 68923cdc2eb3 ("MIPS: CM: Add cluster & block args to mips_cm_lock_other()")
Signed-off-by: James Hogan <jhogan@kernel.org>
Reviewed-by: Paul Burton <paul.burton@mips.com>
Cc: Ralf Baechle <ralf@linux-mips.org>
Cc: linux-mips@linux-mips.org
Cc: <stable@vger.kernel.org> # 4.14+
---
 arch/mips/kernel/mips-cm.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/mips/kernel/mips-cm.c b/arch/mips/kernel/mips-cm.c
index dd5567b1e305..8f5bd04f320a 100644
--- a/arch/mips/kernel/mips-cm.c
+++ b/arch/mips/kernel/mips-cm.c
@@ -292,7 +292,6 @@ void mips_cm_lock_other(unsigned int cluster, unsigned int core,
 				  *this_cpu_ptr(&cm_core_lock_flags));
 	} else {
 		WARN_ON(cluster != 0);
-		WARN_ON(vp != 0);
 		WARN_ON(block != CM_GCR_Cx_OTHER_BLOCK_LOCAL);
 
 		/*
-- 
2.14.1

[PATCH] MIPS: CM: Drop WARN_ON(vp != 0)

[James Hogan]

From: James Hogan <jhogan@kernel.org>

Since commit 68923cdc2eb3 ("MIPS: CM: Add cluster & block args to
mips_cm_lock_other()"), mips_smp_send_ipi_mask() has used
mips_cm_lock_other_cpu() with each CPU number, rather than
mips_cm_lock_other() with the first VPE in each core. Prior to r6,
multicore multithreaded systems such as dual-core dual-thread
interAptivs with CPU Idle enabled (e.g. MIPS Creator Ci40) results in
mips_cm_lock_other() repeatedly hitting WARN_ON(vp != 0).

There doesn't appear to be anything fundamentally wrong about passing a
non-zero VP/VPE number, even if it is a core's region that is locked
into the other region before r6, so remove that particular WARN_ON().

Fixes: 68923cdc2eb3 ("MIPS: CM: Add cluster & block args to mips_cm_lock_other()")
Signed-off-by: James Hogan <jhogan@kernel.org>
Reviewed-by: Paul Burton <paul.burton@mips.com>
Cc: Ralf Baechle <ralf@linux-mips.org>
Cc: linux-mips@linux-mips.org
Cc: <stable@vger.kernel.org> # 4.14+
---
 arch/mips/kernel/mips-cm.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/mips/kernel/mips-cm.c b/arch/mips/kernel/mips-cm.c
index dd5567b1e305..8f5bd04f320a 100644
--- a/arch/mips/kernel/mips-cm.c
+++ b/arch/mips/kernel/mips-cm.c
@@ -292,7 +292,6 @@ void mips_cm_lock_other(unsigned int cluster, unsigned int core,
 				  *this_cpu_ptr(&cm_core_lock_flags));
 	} else {
 		WARN_ON(cluster != 0);
-		WARN_ON(vp != 0);
 		WARN_ON(block != CM_GCR_Cx_OTHER_BLOCK_LOCAL);
 
 		/*
-- 
2.14.1

Re: [PATCH] MIPS: CM: Drop WARN_ON(vp != 0)

[Ralf Baechle]

On Tue, Dec 05, 2017 at 10:28:22PM +0000, James Hogan wrote:

> Since commit 68923cdc2eb3 ("MIPS: CM: Add cluster & block args to
> mips_cm_lock_other()"), mips_smp_send_ipi_mask() has used
> mips_cm_lock_other_cpu() with each CPU number, rather than
> mips_cm_lock_other() with the first VPE in each core. Prior to r6,
> multicore multithreaded systems such as dual-core dual-thread
> interAptivs with CPU Idle enabled (e.g. MIPS Creator Ci40) results in
> mips_cm_lock_other() repeatedly hitting WARN_ON(vp != 0).
> 
> There doesn't appear to be anything fundamentally wrong about passing a
> non-zero VP/VPE number, even if it is a core's region that is locked
> into the other region before r6, so remove that particular WARN_ON().
> 
> Fixes: 68923cdc2eb3 ("MIPS: CM: Add cluster & block args to mips_cm_lock_other()")
> Signed-off-by: James Hogan <jhogan@kernel.org>
> Reviewed-by: Paul Burton <paul.burton@mips.com>
> Cc: Ralf Baechle <ralf@linux-mips.org>
> Cc: linux-mips@linux-mips.org
> Cc: <stable@vger.kernel.org> # 4.14+
> ---
>  arch/mips/kernel/mips-cm.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/arch/mips/kernel/mips-cm.c b/arch/mips/kernel/mips-cm.c
> index dd5567b1e305..8f5bd04f320a 100644
> --- a/arch/mips/kernel/mips-cm.c
> +++ b/arch/mips/kernel/mips-cm.c
> @@ -292,7 +292,6 @@ void mips_cm_lock_other(unsigned int cluster, unsigned int core,
>  				  *this_cpu_ptr(&cm_core_lock_flags));
>  	} else {
>  		WARN_ON(cluster != 0);
> -		WARN_ON(vp != 0);

I think the reason is that for a while the combination of SMP/CMP with
MT was at the bottom of priorities and nobody really cared about it so
a WARN_ON was thrown in.  Which in this case might well itself be a bug!

  Ralf

Re: [PATCH] MIPS: CM: Drop WARN_ON(vp != 0)

[Paul Burton]

Hi Ralf,

On Wed, Dec 06, 2017 at 02:57:45PM +0100, Ralf Baechle wrote:
> On Tue, Dec 05, 2017 at 10:28:22PM +0000, James Hogan wrote:
> 
> > Since commit 68923cdc2eb3 ("MIPS: CM: Add cluster & block args to
> > mips_cm_lock_other()"), mips_smp_send_ipi_mask() has used
> > mips_cm_lock_other_cpu() with each CPU number, rather than
> > mips_cm_lock_other() with the first VPE in each core. Prior to r6,
> > multicore multithreaded systems such as dual-core dual-thread
> > interAptivs with CPU Idle enabled (e.g. MIPS Creator Ci40) results in
> > mips_cm_lock_other() repeatedly hitting WARN_ON(vp != 0).
> > 
> > There doesn't appear to be anything fundamentally wrong about passing a
> > non-zero VP/VPE number, even if it is a core's region that is locked
> > into the other region before r6, so remove that particular WARN_ON().
> > 
> > Fixes: 68923cdc2eb3 ("MIPS: CM: Add cluster & block args to mips_cm_lock_other()")
> > Signed-off-by: James Hogan <jhogan@kernel.org>
> > Reviewed-by: Paul Burton <paul.burton@mips.com>
> > Cc: Ralf Baechle <ralf@linux-mips.org>
> > Cc: linux-mips@linux-mips.org
> > Cc: <stable@vger.kernel.org> # 4.14+
> > ---
> >  arch/mips/kernel/mips-cm.c | 1 -
> >  1 file changed, 1 deletion(-)
> > 
> > diff --git a/arch/mips/kernel/mips-cm.c b/arch/mips/kernel/mips-cm.c
> > index dd5567b1e305..8f5bd04f320a 100644
> > --- a/arch/mips/kernel/mips-cm.c
> > +++ b/arch/mips/kernel/mips-cm.c
> > @@ -292,7 +292,6 @@ void mips_cm_lock_other(unsigned int cluster, unsigned int core,
> >  				  *this_cpu_ptr(&cm_core_lock_flags));
> >  	} else {
> >  		WARN_ON(cluster != 0);
> > -		WARN_ON(vp != 0);
> 
> I think the reason is that for a while the combination of SMP/CMP with
> MT was at the bottom of priorities and nobody really cared about it so
> a WARN_ON was thrown in.  Which in this case might well itself be a bug!
> 
>   Ralf

Well, the WARN_ON came about because:

 - Originally mips_cm_lock_other() had no VP argument, because the CM
   "other" register in CM < 3 has no VP field. ie. the CM "other" region
   always accesses registers associated with a core rather than a VP(E).

 - With CM >= 3 the "other" register (renamed "redirect") gained a VP
   field & the ability to control the CPC & GIC redirect regions, which
   were formerly controlled by separate "other" registers in the CPC &
   GIC register interfaces. In the GIC case the other/redirect region
   actually targets a VP, not a core.

 - Since the CM < 3 other register has no VP field the WARN_ON made
   sense - if code is actually trying to access registers associated
   with a particular VP then that's a bug because that's simply not
   possible. The warning called out those cases by requiring CM < 3
   paths to explicitly pass VP=0.

That was all well & good until I added the mips_cm_lock_other_cpu()
wrapper for convenience. It passes the CPUs cluster, core & VP(E)
numbers to mips_cm_lock_other() which is problematic if it's used in
paths that may run on CM < 3. There is only one such case right now, in
mips_smp_send_ipi_mask() when powering up a CPU in a non-coherent idle
state. So effectively you have to have the CPS cpuidle driver enabled to
hit this.

A potential alternative to this patch would be to have
mips_cm_lock_other_cpu() check the CM version & use VP=0 for CM < 3, but
I think the utility of that is questionable so dropping the WARN_ON as
James has done seems reasonable.

We actually do test mainline, mips-for-linux-next & our internal kernel
branches a bunch on affected systems, most notably interAptiv in the
context of either the Boston or Ci40 platforms. Unfortunately that
didn't catch the bug here because the cpuidle driver isn't enabled in
the kernel configs that our CI system builds. That's something we'll
change. If that weren't the case then we ought to have caught this once
it hit -next. The patchset for multi-cluster support has also shrunk
quite a lot with the merging of the initial set, so our internal
branches are now closer to what will next be submitted which also helps
since we might spot issues even earlier in tests of those internal
branches.

Having the mips-for-linux-next branch updated regularly throughout the
cycle would help immensely in giving us the best chance of catching bugs
like this in a timely fashion, since when it all happens right before
the merge window we don't get much testing time on the code actually
hitting mainline until after it has gone in.

Anyway, hopefully that post-mortem shines some light on what went wrong
here!

Thanks,
    Paul

Re: [PATCH] MIPS: CM: Drop WARN_ON(vp != 0)

[Paul Burton]

Hi Ralf,

On Wed, Dec 06, 2017 at 02:57:45PM +0100, Ralf Baechle wrote:
> On Tue, Dec 05, 2017 at 10:28:22PM +0000, James Hogan wrote:
> 
> > Since commit 68923cdc2eb3 ("MIPS: CM: Add cluster & block args to
> > mips_cm_lock_other()"), mips_smp_send_ipi_mask() has used
> > mips_cm_lock_other_cpu() with each CPU number, rather than
> > mips_cm_lock_other() with the first VPE in each core. Prior to r6,
> > multicore multithreaded systems such as dual-core dual-thread
> > interAptivs with CPU Idle enabled (e.g. MIPS Creator Ci40) results in
> > mips_cm_lock_other() repeatedly hitting WARN_ON(vp != 0).
> > 
> > There doesn't appear to be anything fundamentally wrong about passing a
> > non-zero VP/VPE number, even if it is a core's region that is locked
> > into the other region before r6, so remove that particular WARN_ON().
> > 
> > Fixes: 68923cdc2eb3 ("MIPS: CM: Add cluster & block args to mips_cm_lock_other()")
> > Signed-off-by: James Hogan <jhogan@kernel.org>
> > Reviewed-by: Paul Burton <paul.burton@mips.com>
> > Cc: Ralf Baechle <ralf@linux-mips.org>
> > Cc: linux-mips@linux-mips.org
> > Cc: <stable@vger.kernel.org> # 4.14+
> > ---
> >  arch/mips/kernel/mips-cm.c | 1 -
> >  1 file changed, 1 deletion(-)
> > 
> > diff --git a/arch/mips/kernel/mips-cm.c b/arch/mips/kernel/mips-cm.c
> > index dd5567b1e305..8f5bd04f320a 100644
> > --- a/arch/mips/kernel/mips-cm.c
> > +++ b/arch/mips/kernel/mips-cm.c
> > @@ -292,7 +292,6 @@ void mips_cm_lock_other(unsigned int cluster, unsigned int core,
> >  				  *this_cpu_ptr(&cm_core_lock_flags));
> >  	} else {
> >  		WARN_ON(cluster != 0);
> > -		WARN_ON(vp != 0);
> 
> I think the reason is that for a while the combination of SMP/CMP with
> MT was at the bottom of priorities and nobody really cared about it so
> a WARN_ON was thrown in.  Which in this case might well itself be a bug!
> 
>   Ralf

Well, the WARN_ON came about because:

 - Originally mips_cm_lock_other() had no VP argument, because the CM
   "other" register in CM < 3 has no VP field. ie. the CM "other" region
   always accesses registers associated with a core rather than a VP(E).

 - With CM >= 3 the "other" register (renamed "redirect") gained a VP
   field & the ability to control the CPC & GIC redirect regions, which
   were formerly controlled by separate "other" registers in the CPC &
   GIC register interfaces. In the GIC case the other/redirect region
   actually targets a VP, not a core.

 - Since the CM < 3 other register has no VP field the WARN_ON made
   sense - if code is actually trying to access registers associated
   with a particular VP then that's a bug because that's simply not
   possible. The warning called out those cases by requiring CM < 3
   paths to explicitly pass VP=0.

That was all well & good until I added the mips_cm_lock_other_cpu()
wrapper for convenience. It passes the CPUs cluster, core & VP(E)
numbers to mips_cm_lock_other() which is problematic if it's used in
paths that may run on CM < 3. There is only one such case right now, in
mips_smp_send_ipi_mask() when powering up a CPU in a non-coherent idle
state. So effectively you have to have the CPS cpuidle driver enabled to
hit this.

A potential alternative to this patch would be to have
mips_cm_lock_other_cpu() check the CM version & use VP=0 for CM < 3, but
I think the utility of that is questionable so dropping the WARN_ON as
James has done seems reasonable.

We actually do test mainline, mips-for-linux-next & our internal kernel
branches a bunch on affected systems, most notably interAptiv in the
context of either the Boston or Ci40 platforms. Unfortunately that
didn't catch the bug here because the cpuidle driver isn't enabled in
the kernel configs that our CI system builds. That's something we'll
change. If that weren't the case then we ought to have caught this once
it hit -next. The patchset for multi-cluster support has also shrunk
quite a lot with the merging of the initial set, so our internal
branches are now closer to what will next be submitted which also helps
since we might spot issues even earlier in tests of those internal
branches.

Having the mips-for-linux-next branch updated regularly throughout the
cycle would help immensely in giving us the best chance of catching bugs
like this in a timely fashion, since when it all happens right before
the merge window we don't get much testing time on the code actually
hitting mainline until after it has gone in.

Anyway, hopefully that post-mortem shines some light on what went wrong
here!

Thanks,
    Paul