[PATCH] MIPS: smp.c: Fix uninitialised temp_foreign_map

[PATCH] MIPS: smp.c: Fix uninitialised temp_foreign_map

[James Hogan]

When calculate_cpu_foreign_map() recalculates the cpu_foreign_map
cpumask it uses the local variable temp_foreign_map without initialising
it to zero. Since the calculation only ever sets bits in this cpumask
any existing bits at that memory location will remain set and find their
way into cpu_foreign_map too. This could potentially lead to cache
operations suboptimally doing smp calls to multiple VPEs in the same
core, even though the VPEs share primary caches.

Therefore initialise temp_foreign_map using cpumask_clear() before use.

Fixes: cccf34e9411c ("MIPS: c-r4k: Fix cache flushing for MT cores")
Signed-off-by: James Hogan <james.hogan@imgtec.com>
Cc: Ralf Baechle <ralf@linux-mips.org>
Cc: Paul Burton <paul.burton@imgtec.com>
Cc: linux-mips@linux-mips.org
---
 arch/mips/kernel/smp.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/mips/kernel/smp.c b/arch/mips/kernel/smp.c
index bd4385a8e6e8..2b521e07b860 100644
--- a/arch/mips/kernel/smp.c
+++ b/arch/mips/kernel/smp.c
@@ -121,6 +121,7 @@ static inline void calculate_cpu_foreign_map(void)
 	cpumask_t temp_foreign_map;
 
 	/* Re-calculate the mask */
+	cpumask_clear(&temp_foreign_map);
 	for_each_online_cpu(i) {
 		core_present = 0;
 		for_each_cpu(k, &temp_foreign_map)
-- 
2.4.10

[PATCH] MIPS: smp.c: Fix uninitialised temp_foreign_map

[James Hogan]

When calculate_cpu_foreign_map() recalculates the cpu_foreign_map
cpumask it uses the local variable temp_foreign_map without initialising
it to zero. Since the calculation only ever sets bits in this cpumask
any existing bits at that memory location will remain set and find their
way into cpu_foreign_map too. This could potentially lead to cache
operations suboptimally doing smp calls to multiple VPEs in the same
core, even though the VPEs share primary caches.

Therefore initialise temp_foreign_map using cpumask_clear() before use.

Fixes: cccf34e9411c ("MIPS: c-r4k: Fix cache flushing for MT cores")
Signed-off-by: James Hogan <james.hogan@imgtec.com>
Cc: Ralf Baechle <ralf@linux-mips.org>
Cc: Paul Burton <paul.burton@imgtec.com>
Cc: linux-mips@linux-mips.org
---
 arch/mips/kernel/smp.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/mips/kernel/smp.c b/arch/mips/kernel/smp.c
index bd4385a8e6e8..2b521e07b860 100644
--- a/arch/mips/kernel/smp.c
+++ b/arch/mips/kernel/smp.c
@@ -121,6 +121,7 @@ static inline void calculate_cpu_foreign_map(void)
 	cpumask_t temp_foreign_map;
 
 	/* Re-calculate the mask */
+	cpumask_clear(&temp_foreign_map);
 	for_each_online_cpu(i) {
 		core_present = 0;
 		for_each_cpu(k, &temp_foreign_map)
-- 
2.4.10

Re: [PATCH] MIPS: smp.c: Fix uninitialised temp_foreign_map

[Matt Redfearn]

Hi James,

On 04/03/16 10:10, James Hogan wrote:
> When calculate_cpu_foreign_map() recalculates the cpu_foreign_map
> cpumask it uses the local variable temp_foreign_map without initialising
> it to zero. Since the calculation only ever sets bits in this cpumask
> any existing bits at that memory location will remain set and find their
> way into cpu_foreign_map too. This could potentially lead to cache
> operations suboptimally doing smp calls to multiple VPEs in the same
> core, even though the VPEs share primary caches.
>
> Therefore initialise temp_foreign_map using cpumask_clear() before use.
>
> Fixes: cccf34e9411c ("MIPS: c-r4k: Fix cache flushing for MT cores")

cccf34e9411c was CC'd to stable 3.15+, should this fix do the same?

Thanks,
Matt


> Signed-off-by: James Hogan <james.hogan@imgtec.com>
> Cc: Ralf Baechle <ralf@linux-mips.org>
> Cc: Paul Burton <paul.burton@imgtec.com>
> Cc: linux-mips@linux-mips.org
> ---
>   arch/mips/kernel/smp.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/arch/mips/kernel/smp.c b/arch/mips/kernel/smp.c
> index bd4385a8e6e8..2b521e07b860 100644
> --- a/arch/mips/kernel/smp.c
> +++ b/arch/mips/kernel/smp.c
> @@ -121,6 +121,7 @@ static inline void calculate_cpu_foreign_map(void)
>   	cpumask_t temp_foreign_map;
>   
>   	/* Re-calculate the mask */
> +	cpumask_clear(&temp_foreign_map);
>   	for_each_online_cpu(i) {
>   		core_present = 0;
>   		for_each_cpu(k, &temp_foreign_map)

Re: [PATCH] MIPS: smp.c: Fix uninitialised temp_foreign_map

[Matt Redfearn]

Hi James,

On 04/03/16 10:10, James Hogan wrote:
> When calculate_cpu_foreign_map() recalculates the cpu_foreign_map
> cpumask it uses the local variable temp_foreign_map without initialising
> it to zero. Since the calculation only ever sets bits in this cpumask
> any existing bits at that memory location will remain set and find their
> way into cpu_foreign_map too. This could potentially lead to cache
> operations suboptimally doing smp calls to multiple VPEs in the same
> core, even though the VPEs share primary caches.
>
> Therefore initialise temp_foreign_map using cpumask_clear() before use.
>
> Fixes: cccf34e9411c ("MIPS: c-r4k: Fix cache flushing for MT cores")

cccf34e9411c was CC'd to stable 3.15+, should this fix do the same?

Thanks,
Matt


> Signed-off-by: James Hogan <james.hogan@imgtec.com>
> Cc: Ralf Baechle <ralf@linux-mips.org>
> Cc: Paul Burton <paul.burton@imgtec.com>
> Cc: linux-mips@linux-mips.org
> ---
>   arch/mips/kernel/smp.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/arch/mips/kernel/smp.c b/arch/mips/kernel/smp.c
> index bd4385a8e6e8..2b521e07b860 100644
> --- a/arch/mips/kernel/smp.c
> +++ b/arch/mips/kernel/smp.c
> @@ -121,6 +121,7 @@ static inline void calculate_cpu_foreign_map(void)
>   	cpumask_t temp_foreign_map;
>   
>   	/* Re-calculate the mask */
> +	cpumask_clear(&temp_foreign_map);
>   	for_each_online_cpu(i) {
>   		core_present = 0;
>   		for_each_cpu(k, &temp_foreign_map)

Re: [PATCH] MIPS: smp.c: Fix uninitialised temp_foreign_map

[James Hogan]

[-- Attachment #1: Type: text/plain, Size: 2229 bytes --]

Hi Matt,

On Fri, Mar 04, 2016 at 01:43:37PM +0000, Matt Redfearn wrote:
> Hi James,
> 
> On 04/03/16 10:10, James Hogan wrote:
> > When calculate_cpu_foreign_map() recalculates the cpu_foreign_map
> > cpumask it uses the local variable temp_foreign_map without initialising
> > it to zero. Since the calculation only ever sets bits in this cpumask
> > any existing bits at that memory location will remain set and find their
> > way into cpu_foreign_map too. This could potentially lead to cache
> > operations suboptimally doing smp calls to multiple VPEs in the same
> > core, even though the VPEs share primary caches.
> >
> > Therefore initialise temp_foreign_map using cpumask_clear() before use.
> >
> > Fixes: cccf34e9411c ("MIPS: c-r4k: Fix cache flushing for MT cores")
> 
> cccf34e9411c was CC'd to stable 3.15+, should this fix do the same?

I originally didn't think it was needed for stable, since it only causes
a few unnecessary IPIs. However having thought some more about it, I
think it could result in cpu_foreign_map containing VPEs that are
offline, and not ones online, which could result in the IPI *not* being
called on a given core at all when it should.

E.g.
if the initial undefined value is 0x2 (CPU 1) and 2nd VPE of 1st core is
offline, then CPU 0 wouldn't be set (since CPU 1 is a sibling) and
neither VPE in core 0 would get the IPI.

So yeh, maybe it should.

Cheers
James

> 
> Thanks,
> Matt
> 
> 
> > Signed-off-by: James Hogan <james.hogan@imgtec.com>
> > Cc: Ralf Baechle <ralf@linux-mips.org>
> > Cc: Paul Burton <paul.burton@imgtec.com>
> > Cc: linux-mips@linux-mips.org
> > ---
> >   arch/mips/kernel/smp.c | 1 +
> >   1 file changed, 1 insertion(+)
> >
> > diff --git a/arch/mips/kernel/smp.c b/arch/mips/kernel/smp.c
> > index bd4385a8e6e8..2b521e07b860 100644
> > --- a/arch/mips/kernel/smp.c
> > +++ b/arch/mips/kernel/smp.c
> > @@ -121,6 +121,7 @@ static inline void calculate_cpu_foreign_map(void)
> >   	cpumask_t temp_foreign_map;
> >   
> >   	/* Re-calculate the mask */
> > +	cpumask_clear(&temp_foreign_map);
> >   	for_each_online_cpu(i) {
> >   		core_present = 0;
> >   		for_each_cpu(k, &temp_foreign_map)
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] MIPS: smp.c: Fix uninitialised temp_foreign_map

[James Hogan]

[-- Attachment #1: Type: text/plain, Size: 2229 bytes --]

Hi Matt,

On Fri, Mar 04, 2016 at 01:43:37PM +0000, Matt Redfearn wrote:
> Hi James,
> 
> On 04/03/16 10:10, James Hogan wrote:
> > When calculate_cpu_foreign_map() recalculates the cpu_foreign_map
> > cpumask it uses the local variable temp_foreign_map without initialising
> > it to zero. Since the calculation only ever sets bits in this cpumask
> > any existing bits at that memory location will remain set and find their
> > way into cpu_foreign_map too. This could potentially lead to cache
> > operations suboptimally doing smp calls to multiple VPEs in the same
> > core, even though the VPEs share primary caches.
> >
> > Therefore initialise temp_foreign_map using cpumask_clear() before use.
> >
> > Fixes: cccf34e9411c ("MIPS: c-r4k: Fix cache flushing for MT cores")
> 
> cccf34e9411c was CC'd to stable 3.15+, should this fix do the same?

I originally didn't think it was needed for stable, since it only causes
a few unnecessary IPIs. However having thought some more about it, I
think it could result in cpu_foreign_map containing VPEs that are
offline, and not ones online, which could result in the IPI *not* being
called on a given core at all when it should.

E.g.
if the initial undefined value is 0x2 (CPU 1) and 2nd VPE of 1st core is
offline, then CPU 0 wouldn't be set (since CPU 1 is a sibling) and
neither VPE in core 0 would get the IPI.

So yeh, maybe it should.

Cheers
James

> 
> Thanks,
> Matt
> 
> 
> > Signed-off-by: James Hogan <james.hogan@imgtec.com>
> > Cc: Ralf Baechle <ralf@linux-mips.org>
> > Cc: Paul Burton <paul.burton@imgtec.com>
> > Cc: linux-mips@linux-mips.org
> > ---
> >   arch/mips/kernel/smp.c | 1 +
> >   1 file changed, 1 insertion(+)
> >
> > diff --git a/arch/mips/kernel/smp.c b/arch/mips/kernel/smp.c
> > index bd4385a8e6e8..2b521e07b860 100644
> > --- a/arch/mips/kernel/smp.c
> > +++ b/arch/mips/kernel/smp.c
> > @@ -121,6 +121,7 @@ static inline void calculate_cpu_foreign_map(void)
> >   	cpumask_t temp_foreign_map;
> >   
> >   	/* Re-calculate the mask */
> > +	cpumask_clear(&temp_foreign_map);
> >   	for_each_online_cpu(i) {
> >   		core_present = 0;
> >   		for_each_cpu(k, &temp_foreign_map)
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH] MIPS: smp.c: Fix uninitialised temp_foreign_map

[miles.chen]

From: James Hogan <james.hogan@imgtec.com>

When calculate_cpu_foreign_map() recalculates the cpu_foreign_map
cpumask it uses the local variable temp_foreign_map without initialising
it to zero. Since the calculation only ever sets bits in this cpumask
any existing bits at that memory location will remain set and find their
way into cpu_foreign_map too. This could potentially lead to cache
operations suboptimally doing smp calls to multiple VPEs in the same
core, even though the VPEs share primary caches.

Therefore initialise temp_foreign_map using cpumask_clear() before use.

Fixes: cccf34e9411c ("MIPS: c-r4k: Fix cache flushing for MT cores")
Signed-off-by: James Hogan <james.hogan@imgtec.com>
Cc: Paul Burton <paul.burton@imgtec.com>
Cc: linux-mips@linux-mips.org
Patchwork: https://patchwork.linux-mips.org/patch/12759/
Signed-off-by: Ralf Baechle <ralf@linux-mips.org>
---
 arch/mips/kernel/smp.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/mips/kernel/smp.c b/arch/mips/kernel/smp.c
index bd4385a..2b521e0 100644
--- a/arch/mips/kernel/smp.c
+++ b/arch/mips/kernel/smp.c
@@ -121,6 +121,7 @@ static inline void calculate_cpu_foreign_map(void)
 	cpumask_t temp_foreign_map;
 
 	/* Re-calculate the mask */
+	cpumask_clear(&temp_foreign_map);
 	for_each_online_cpu(i) {
 		core_present = 0;
 		for_each_cpu(k, &temp_foreign_map)
-- 
1.9.1

[PATCH] MIPS: smp.c: Fix uninitialised temp_foreign_map

[miles.chen]

From: James Hogan <james.hogan@imgtec.com>

When calculate_cpu_foreign_map() recalculates the cpu_foreign_map
cpumask it uses the local variable temp_foreign_map without initialising
it to zero. Since the calculation only ever sets bits in this cpumask
any existing bits at that memory location will remain set and find their
way into cpu_foreign_map too. This could potentially lead to cache
operations suboptimally doing smp calls to multiple VPEs in the same
core, even though the VPEs share primary caches.

Therefore initialise temp_foreign_map using cpumask_clear() before use.

Fixes: cccf34e9411c ("MIPS: c-r4k: Fix cache flushing for MT cores")
Signed-off-by: James Hogan <james.hogan@imgtec.com>
Cc: Paul Burton <paul.burton@imgtec.com>
Cc: linux-mips@linux-mips.org
Patchwork: https://patchwork.linux-mips.org/patch/12759/
Signed-off-by: Ralf Baechle <ralf@linux-mips.org>
---
 arch/mips/kernel/smp.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/mips/kernel/smp.c b/arch/mips/kernel/smp.c
index bd4385a..2b521e0 100644
--- a/arch/mips/kernel/smp.c
+++ b/arch/mips/kernel/smp.c
@@ -121,6 +121,7 @@ static inline void calculate_cpu_foreign_map(void)
 	cpumask_t temp_foreign_map;
 
 	/* Re-calculate the mask */
+	cpumask_clear(&temp_foreign_map);
 	for_each_online_cpu(i) {
 		core_present = 0;
 		for_each_cpu(k, &temp_foreign_map)
-- 
1.9.1

Re: [PATCH] MIPS: smp.c: Fix uninitialised temp_foreign_map

[Ralf Baechle]

Please fix your scripts to not repost patches.

  Ralf

On Mon, Mar 14, 2016 at 02:12:58PM +0800, miles.chen@mediatek.com wrote:

> From: miles.chen@mediatek.com
> To: Miles <miles.chen@mediatek.com>
> CC: James Hogan <james.hogan@imgtec.com>, Paul Burton
>  <paul.burton@imgtec.com>, linux-mips@linux-mips.org, Ralf Baechle
>  <ralf@linux-mips.org>
> Subject: [PATCH] MIPS: smp.c: Fix uninitialised temp_foreign_map
> Content-Type: text/plain
> 
> From: James Hogan <james.hogan@imgtec.com>
> 
> When calculate_cpu_foreign_map() recalculates the cpu_foreign_map
> cpumask it uses the local variable temp_foreign_map without initialising
> it to zero. Since the calculation only ever sets bits in this cpumask
> any existing bits at that memory location will remain set and find their
> way into cpu_foreign_map too. This could potentially lead to cache
> operations suboptimally doing smp calls to multiple VPEs in the same
> core, even though the VPEs share primary caches.
> 
> Therefore initialise temp_foreign_map using cpumask_clear() before use.
> 
> Fixes: cccf34e9411c ("MIPS: c-r4k: Fix cache flushing for MT cores")
> Signed-off-by: James Hogan <james.hogan@imgtec.com>
> Cc: Paul Burton <paul.burton@imgtec.com>
> Cc: linux-mips@linux-mips.org
> Patchwork: https://patchwork.linux-mips.org/patch/12759/
> Signed-off-by: Ralf Baechle <ralf@linux-mips.org>
> ---
>  arch/mips/kernel/smp.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/mips/kernel/smp.c b/arch/mips/kernel/smp.c
> index bd4385a..2b521e0 100644
> --- a/arch/mips/kernel/smp.c
> +++ b/arch/mips/kernel/smp.c
> @@ -121,6 +121,7 @@ static inline void calculate_cpu_foreign_map(void)
>  	cpumask_t temp_foreign_map;
>  
>  	/* Re-calculate the mask */
> +	cpumask_clear(&temp_foreign_map);
>  	for_each_online_cpu(i) {
>  		core_present = 0;
>  		for_each_cpu(k, &temp_foreign_map)
> -- 
> 1.9.1

  Ralf