[PATCH 0/7] Misc GIC fixes

[PATCH 0/7] Misc GIC fixes

[Markos Chandras]

Hi,

These patches address a number of issues with the existing GIC IRQ code.

The patchset is based on 3.16-rc5

Jeffrey Deans (7):
  MIPS: GIC: move GIC interrupt bitmap declarations
  MIPS: GIC: Move GIC_NUM_INTRS into platform irq.h
  MIPS: GIC: Remove GIC_FLAG_IPI
  MIPS: GIC: Prevent array overrun
  MIPS: GIC: Generalise check for pending interrupts
  MIPS: Malta: Fix dispatching of GIC interrupts
  MIPS: GIC: Fix GICBIS macro

 arch/mips/include/asm/gic.h            | 41 +++++++++++++---------------------
 arch/mips/include/asm/mach-malta/irq.h |  1 +
 arch/mips/include/asm/mach-sead3/irq.h |  1 +
 arch/mips/kernel/irq-gic.c             | 38 ++++++++++++++++++++++++-------
 arch/mips/mti-malta/malta-int.c        | 27 +++++++++++++++-------
 5 files changed, 66 insertions(+), 42 deletions(-)

-- 
2.0.0

[PATCH 0/7] Misc GIC fixes

[Markos Chandras]

Hi,

These patches address a number of issues with the existing GIC IRQ code.

The patchset is based on 3.16-rc5

Jeffrey Deans (7):
  MIPS: GIC: move GIC interrupt bitmap declarations
  MIPS: GIC: Move GIC_NUM_INTRS into platform irq.h
  MIPS: GIC: Remove GIC_FLAG_IPI
  MIPS: GIC: Prevent array overrun
  MIPS: GIC: Generalise check for pending interrupts
  MIPS: Malta: Fix dispatching of GIC interrupts
  MIPS: GIC: Fix GICBIS macro

 arch/mips/include/asm/gic.h            | 41 +++++++++++++---------------------
 arch/mips/include/asm/mach-malta/irq.h |  1 +
 arch/mips/include/asm/mach-sead3/irq.h |  1 +
 arch/mips/kernel/irq-gic.c             | 38 ++++++++++++++++++++++++-------
 arch/mips/mti-malta/malta-int.c        | 27 +++++++++++++++-------
 5 files changed, 66 insertions(+), 42 deletions(-)

-- 
2.0.0

[PATCH 1/7] MIPS: GIC: move GIC interrupt bitmap declarations

[Markos Chandras]

From: Jeffrey Deans <jeffrey.deans@imgtec.com>

Several bitmaps are declared in arch/mips/include/asm/gic.h, but the
scope of their use is limited to arch/mips/kernel/irq-gic.c. Move the
declarations from the header file to the C file.

Signed-off-by: Jeffrey Deans <jeffrey.deans@imgtec.com>
Signed-off-by: Markos Chandras <markos.chandras@imgtec.com>
---
 arch/mips/include/asm/gic.h | 12 ------------
 arch/mips/kernel/irq-gic.c  | 12 ++++++++++++
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/arch/mips/include/asm/gic.h b/arch/mips/include/asm/gic.h
index 10f6a99f92c2..5b0e6a4b2c30 100644
--- a/arch/mips/include/asm/gic.h
+++ b/arch/mips/include/asm/gic.h
@@ -306,18 +306,6 @@
 	GICWRITE(GIC_REG_ADDR(SHARED, GIC_SH_MAP_TO_VPE_REG_OFF(intr, vpe)), \
 		 GIC_SH_MAP_TO_VPE_REG_BIT(vpe))
 
-struct gic_pcpu_mask {
-	DECLARE_BITMAP(pcpu_mask, GIC_NUM_INTRS);
-};
-
-struct gic_pending_regs {
-	DECLARE_BITMAP(pending, GIC_NUM_INTRS);
-};
-
-struct gic_intrmask_regs {
-	DECLARE_BITMAP(intrmask, GIC_NUM_INTRS);
-};
-
 /*
  * Interrupt Meta-data specification. The ipiflag helps
  * in building ipi_map.
diff --git a/arch/mips/kernel/irq-gic.c b/arch/mips/kernel/irq-gic.c
index 88e4c323382c..a1dea3ea59a0 100644
--- a/arch/mips/kernel/irq-gic.c
+++ b/arch/mips/kernel/irq-gic.c
@@ -28,6 +28,18 @@ unsigned int gic_irq_flags[GIC_NUM_INTRS];
 /* The index into this array is the vector # of the interrupt. */
 struct gic_shared_intr_map gic_shared_intr_map[GIC_NUM_INTRS];
 
+struct gic_pcpu_mask {
+	DECLARE_BITMAP(pcpu_mask, GIC_NUM_INTRS);
+};
+
+struct gic_pending_regs {
+	DECLARE_BITMAP(pending, GIC_NUM_INTRS);
+};
+
+struct gic_intrmask_regs {
+	DECLARE_BITMAP(intrmask, GIC_NUM_INTRS);
+};
+
 static struct gic_pcpu_mask pcpu_masks[NR_CPUS];
 static struct gic_pending_regs pending_regs[NR_CPUS];
 static struct gic_intrmask_regs intrmask_regs[NR_CPUS];
-- 
2.0.0

[PATCH 1/7] MIPS: GIC: move GIC interrupt bitmap declarations

[Markos Chandras]

From: Jeffrey Deans <jeffrey.deans@imgtec.com>

Several bitmaps are declared in arch/mips/include/asm/gic.h, but the
scope of their use is limited to arch/mips/kernel/irq-gic.c. Move the
declarations from the header file to the C file.

Signed-off-by: Jeffrey Deans <jeffrey.deans@imgtec.com>
Signed-off-by: Markos Chandras <markos.chandras@imgtec.com>
---
 arch/mips/include/asm/gic.h | 12 ------------
 arch/mips/kernel/irq-gic.c  | 12 ++++++++++++
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/arch/mips/include/asm/gic.h b/arch/mips/include/asm/gic.h
index 10f6a99f92c2..5b0e6a4b2c30 100644
--- a/arch/mips/include/asm/gic.h
+++ b/arch/mips/include/asm/gic.h
@@ -306,18 +306,6 @@
 	GICWRITE(GIC_REG_ADDR(SHARED, GIC_SH_MAP_TO_VPE_REG_OFF(intr, vpe)), \
 		 GIC_SH_MAP_TO_VPE_REG_BIT(vpe))
 
-struct gic_pcpu_mask {
-	DECLARE_BITMAP(pcpu_mask, GIC_NUM_INTRS);
-};
-
-struct gic_pending_regs {
-	DECLARE_BITMAP(pending, GIC_NUM_INTRS);
-};
-
-struct gic_intrmask_regs {
-	DECLARE_BITMAP(intrmask, GIC_NUM_INTRS);
-};
-
 /*
  * Interrupt Meta-data specification. The ipiflag helps
  * in building ipi_map.
diff --git a/arch/mips/kernel/irq-gic.c b/arch/mips/kernel/irq-gic.c
index 88e4c323382c..a1dea3ea59a0 100644
--- a/arch/mips/kernel/irq-gic.c
+++ b/arch/mips/kernel/irq-gic.c
@@ -28,6 +28,18 @@ unsigned int gic_irq_flags[GIC_NUM_INTRS];
 /* The index into this array is the vector # of the interrupt. */
 struct gic_shared_intr_map gic_shared_intr_map[GIC_NUM_INTRS];
 
+struct gic_pcpu_mask {
+	DECLARE_BITMAP(pcpu_mask, GIC_NUM_INTRS);
+};
+
+struct gic_pending_regs {
+	DECLARE_BITMAP(pending, GIC_NUM_INTRS);
+};
+
+struct gic_intrmask_regs {
+	DECLARE_BITMAP(intrmask, GIC_NUM_INTRS);
+};
+
 static struct gic_pcpu_mask pcpu_masks[NR_CPUS];
 static struct gic_pending_regs pending_regs[NR_CPUS];
 static struct gic_intrmask_regs intrmask_regs[NR_CPUS];
-- 
2.0.0

[PATCH 2/7] MIPS: GIC: Move GIC_NUM_INTRS into platform irq.h

[Markos Chandras]

From: Jeffrey Deans <jeffrey.deans@imgtec.com>

The value of GIC_NUM_INTRS is platform-specific. Using a default value
from gic.h will result in incorrect behaviour on some systems, so
require a suitable definition to be present in the platform's irq.h.

Signed-off-by: Jeffrey Deans <jeffrey.deans@imgtec.com>
Signed-off-by: Markos Chandras <markos.chandras@imgtec.com>
---
 arch/mips/include/asm/gic.h            | 4 ++--
 arch/mips/include/asm/mach-malta/irq.h | 1 +
 arch/mips/include/asm/mach-sead3/irq.h | 1 +
 3 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/mips/include/asm/gic.h b/arch/mips/include/asm/gic.h
index 5b0e6a4b2c30..80804c16bb9d 100644
--- a/arch/mips/include/asm/gic.h
+++ b/arch/mips/include/asm/gic.h
@@ -14,6 +14,8 @@
 #include <linux/bitmap.h>
 #include <linux/threads.h>
 
+#include <irq.h>
+
 #undef	GICISBYTELITTLEENDIAN
 
 /* Constants */
@@ -22,8 +24,6 @@
 #define GIC_TRIG_EDGE			1
 #define GIC_TRIG_LEVEL			0
 
-#define GIC_NUM_INTRS			(24 + NR_CPUS * 2)
-
 #define MSK(n) ((1 << (n)) - 1)
 #define REG32(addr)		(*(volatile unsigned int *) (addr))
 #define REG(base, offs)		REG32((unsigned long)(base) + offs##_##OFS)
diff --git a/arch/mips/include/asm/mach-malta/irq.h b/arch/mips/include/asm/mach-malta/irq.h
index 47cfe64efbb0..f2c13d211abb 100644
--- a/arch/mips/include/asm/mach-malta/irq.h
+++ b/arch/mips/include/asm/mach-malta/irq.h
@@ -2,6 +2,7 @@
 #define __ASM_MACH_MIPS_IRQ_H
 
 
+#define GIC_NUM_INTRS (24 + NR_CPUS * 2)
 #define NR_IRQS 256
 
 #include_next <irq.h>
diff --git a/arch/mips/include/asm/mach-sead3/irq.h b/arch/mips/include/asm/mach-sead3/irq.h
index 5d154cfbcf4c..d8106f75b9af 100644
--- a/arch/mips/include/asm/mach-sead3/irq.h
+++ b/arch/mips/include/asm/mach-sead3/irq.h
@@ -1,6 +1,7 @@
 #ifndef __ASM_MACH_MIPS_IRQ_H
 #define __ASM_MACH_MIPS_IRQ_H
 
+#define GIC_NUM_INTRS (24 + NR_CPUS * 2)
 #define NR_IRQS 256
 
 
-- 
2.0.0

[PATCH 2/7] MIPS: GIC: Move GIC_NUM_INTRS into platform irq.h

[Markos Chandras]

From: Jeffrey Deans <jeffrey.deans@imgtec.com>

The value of GIC_NUM_INTRS is platform-specific. Using a default value
from gic.h will result in incorrect behaviour on some systems, so
require a suitable definition to be present in the platform's irq.h.

Signed-off-by: Jeffrey Deans <jeffrey.deans@imgtec.com>
Signed-off-by: Markos Chandras <markos.chandras@imgtec.com>
---
 arch/mips/include/asm/gic.h            | 4 ++--
 arch/mips/include/asm/mach-malta/irq.h | 1 +
 arch/mips/include/asm/mach-sead3/irq.h | 1 +
 3 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/mips/include/asm/gic.h b/arch/mips/include/asm/gic.h
index 5b0e6a4b2c30..80804c16bb9d 100644
--- a/arch/mips/include/asm/gic.h
+++ b/arch/mips/include/asm/gic.h
@@ -14,6 +14,8 @@
 #include <linux/bitmap.h>
 #include <linux/threads.h>
 
+#include <irq.h>
+
 #undef	GICISBYTELITTLEENDIAN
 
 /* Constants */
@@ -22,8 +24,6 @@
 #define GIC_TRIG_EDGE			1
 #define GIC_TRIG_LEVEL			0
 
-#define GIC_NUM_INTRS			(24 + NR_CPUS * 2)
-
 #define MSK(n) ((1 << (n)) - 1)
 #define REG32(addr)		(*(volatile unsigned int *) (addr))
 #define REG(base, offs)		REG32((unsigned long)(base) + offs##_##OFS)
diff --git a/arch/mips/include/asm/mach-malta/irq.h b/arch/mips/include/asm/mach-malta/irq.h
index 47cfe64efbb0..f2c13d211abb 100644
--- a/arch/mips/include/asm/mach-malta/irq.h
+++ b/arch/mips/include/asm/mach-malta/irq.h
@@ -2,6 +2,7 @@
 #define __ASM_MACH_MIPS_IRQ_H
 
 
+#define GIC_NUM_INTRS (24 + NR_CPUS * 2)
 #define NR_IRQS 256
 
 #include_next <irq.h>
diff --git a/arch/mips/include/asm/mach-sead3/irq.h b/arch/mips/include/asm/mach-sead3/irq.h
index 5d154cfbcf4c..d8106f75b9af 100644
--- a/arch/mips/include/asm/mach-sead3/irq.h
+++ b/arch/mips/include/asm/mach-sead3/irq.h
@@ -1,6 +1,7 @@
 #ifndef __ASM_MACH_MIPS_IRQ_H
 #define __ASM_MACH_MIPS_IRQ_H
 
+#define GIC_NUM_INTRS (24 + NR_CPUS * 2)
 #define NR_IRQS 256
 
 
-- 
2.0.0

[PATCH 3/7] MIPS: GIC: Remove GIC_FLAG_IPI

[Markos Chandras]

From: Jeffrey Deans <jeffrey.deans@imgtec.com>

irq-gic.c:gic_get_int() masks out interrupts from the pending set which
aren’t in the pcpu_mask. Only interrupts marked with GIC_FLAG_IPI were
set in pcpu_mask, meaning that peripheral interrupts also had to be
marked as IPIs. Remove the use of GIC_FLAG_IPI and allow the flags
member of struct gic_intr_map to be zero.

Signed-off-by: Jeffrey Deans <jeffrey.deans@imgtec.com>
Signed-off-by: Markos Chandras <markos.chandras@imgtec.com>
---
 arch/mips/include/asm/gic.h     | 3 +--
 arch/mips/kernel/irq-gic.c      | 7 +++----
 arch/mips/mti-malta/malta-int.c | 2 +-
 3 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/arch/mips/include/asm/gic.h b/arch/mips/include/asm/gic.h
index 80804c16bb9d..394d366b8fc1 100644
--- a/arch/mips/include/asm/gic.h
+++ b/arch/mips/include/asm/gic.h
@@ -317,8 +317,7 @@ struct gic_intr_map {
 	unsigned int polarity;	/* Polarity : +/-	*/
 	unsigned int trigtype;	/* Trigger  : Edge/Levl */
 	unsigned int flags;	/* Misc flags	*/
-#define GIC_FLAG_IPI	       0x01
-#define GIC_FLAG_TRANSPARENT   0x02
+#define GIC_FLAG_TRANSPARENT   0x01
 };
 
 /*
diff --git a/arch/mips/kernel/irq-gic.c b/arch/mips/kernel/irq-gic.c
index a1dea3ea59a0..71cf45a335b6 100644
--- a/arch/mips/kernel/irq-gic.c
+++ b/arch/mips/kernel/irq-gic.c
@@ -311,9 +311,10 @@ static void __init gic_setup_intr(unsigned int intr, unsigned int cpu,
 
 	/* Init Intr Masks */
 	GIC_CLR_INTR_MASK(intr);
+
 	/* Initialise per-cpu Interrupt software masks */
-	if (flags & GIC_FLAG_IPI)
-		set_bit(intr, pcpu_masks[cpu].pcpu_mask);
+	set_bit(intr, pcpu_masks[cpu].pcpu_mask);
+
 	if ((flags & GIC_FLAG_TRANSPARENT) && (cpu_has_veic == 0))
 		GIC_SET_INTR_MASK(intr);
 	if (trigtype == GIC_TRIG_EDGE)
@@ -352,8 +353,6 @@ static void __init gic_basic_init(int numintrs, int numvpes,
 		cpu = intrmap[i].cpunum;
 		if (cpu == GIC_UNUSED)
 			continue;
-		if (cpu == 0 && i != 0 && intrmap[i].flags == 0)
-			continue;
 		gic_setup_intr(i,
 			intrmap[i].cpunum,
 			intrmap[i].pin + pin_offset,
diff --git a/arch/mips/mti-malta/malta-int.c b/arch/mips/mti-malta/malta-int.c
index ecc2785f7858..4ab919141737 100644
--- a/arch/mips/mti-malta/malta-int.c
+++ b/arch/mips/mti-malta/malta-int.c
@@ -427,7 +427,7 @@ static void __init fill_ipi_map1(int baseintr, int cpu, int cpupin)
 	gic_intr_map[intr].pin = cpupin;
 	gic_intr_map[intr].polarity = GIC_POL_POS;
 	gic_intr_map[intr].trigtype = GIC_TRIG_EDGE;
-	gic_intr_map[intr].flags = GIC_FLAG_IPI;
+	gic_intr_map[intr].flags = 0;
 	ipi_map[cpu] |= (1 << (cpupin + 2));
 }
 
-- 
2.0.0

[PATCH 3/7] MIPS: GIC: Remove GIC_FLAG_IPI

[Markos Chandras]

From: Jeffrey Deans <jeffrey.deans@imgtec.com>

irq-gic.c:gic_get_int() masks out interrupts from the pending set which
aren’t in the pcpu_mask. Only interrupts marked with GIC_FLAG_IPI were
set in pcpu_mask, meaning that peripheral interrupts also had to be
marked as IPIs. Remove the use of GIC_FLAG_IPI and allow the flags
member of struct gic_intr_map to be zero.

Signed-off-by: Jeffrey Deans <jeffrey.deans@imgtec.com>
Signed-off-by: Markos Chandras <markos.chandras@imgtec.com>
---
 arch/mips/include/asm/gic.h     | 3 +--
 arch/mips/kernel/irq-gic.c      | 7 +++----
 arch/mips/mti-malta/malta-int.c | 2 +-
 3 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/arch/mips/include/asm/gic.h b/arch/mips/include/asm/gic.h
index 80804c16bb9d..394d366b8fc1 100644
--- a/arch/mips/include/asm/gic.h
+++ b/arch/mips/include/asm/gic.h
@@ -317,8 +317,7 @@ struct gic_intr_map {
 	unsigned int polarity;	/* Polarity : +/-	*/
 	unsigned int trigtype;	/* Trigger  : Edge/Levl */
 	unsigned int flags;	/* Misc flags	*/
-#define GIC_FLAG_IPI	       0x01
-#define GIC_FLAG_TRANSPARENT   0x02
+#define GIC_FLAG_TRANSPARENT   0x01
 };
 
 /*
diff --git a/arch/mips/kernel/irq-gic.c b/arch/mips/kernel/irq-gic.c
index a1dea3ea59a0..71cf45a335b6 100644
--- a/arch/mips/kernel/irq-gic.c
+++ b/arch/mips/kernel/irq-gic.c
@@ -311,9 +311,10 @@ static void __init gic_setup_intr(unsigned int intr, unsigned int cpu,
 
 	/* Init Intr Masks */
 	GIC_CLR_INTR_MASK(intr);
+
 	/* Initialise per-cpu Interrupt software masks */
-	if (flags & GIC_FLAG_IPI)
-		set_bit(intr, pcpu_masks[cpu].pcpu_mask);
+	set_bit(intr, pcpu_masks[cpu].pcpu_mask);
+
 	if ((flags & GIC_FLAG_TRANSPARENT) && (cpu_has_veic == 0))
 		GIC_SET_INTR_MASK(intr);
 	if (trigtype == GIC_TRIG_EDGE)
@@ -352,8 +353,6 @@ static void __init gic_basic_init(int numintrs, int numvpes,
 		cpu = intrmap[i].cpunum;
 		if (cpu == GIC_UNUSED)
 			continue;
-		if (cpu == 0 && i != 0 && intrmap[i].flags == 0)
-			continue;
 		gic_setup_intr(i,
 			intrmap[i].cpunum,
 			intrmap[i].pin + pin_offset,
diff --git a/arch/mips/mti-malta/malta-int.c b/arch/mips/mti-malta/malta-int.c
index ecc2785f7858..4ab919141737 100644
--- a/arch/mips/mti-malta/malta-int.c
+++ b/arch/mips/mti-malta/malta-int.c
@@ -427,7 +427,7 @@ static void __init fill_ipi_map1(int baseintr, int cpu, int cpupin)
 	gic_intr_map[intr].pin = cpupin;
 	gic_intr_map[intr].polarity = GIC_POL_POS;
 	gic_intr_map[intr].trigtype = GIC_TRIG_EDGE;
-	gic_intr_map[intr].flags = GIC_FLAG_IPI;
+	gic_intr_map[intr].flags = 0;
 	ipi_map[cpu] |= (1 << (cpupin + 2));
 }
 
-- 
2.0.0

[PATCH 4/7] MIPS: GIC: Prevent array overrun

[Markos Chandras]

From: Jeffrey Deans <jeffrey.deans@imgtec.com>

A GIC interrupt which is declared as having a GIC_MAP_TO_NMI_MSK
mapping causes the cpu parameter to gic_setup_intr() to be increased
to 32, causing memory corruption when pcpu_masks[] is written to again
later in the function.

Cc: stable@vger.kernel.org
Signed-off-by: Jeffrey Deans <jeffrey.deans@imgtec.com>
Signed-off-by: Markos Chandras <markos.chandras@imgtec.com>
---
 arch/mips/kernel/irq-gic.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/mips/kernel/irq-gic.c b/arch/mips/kernel/irq-gic.c
index 71cf45a335b6..9932aef91abb 100644
--- a/arch/mips/kernel/irq-gic.c
+++ b/arch/mips/kernel/irq-gic.c
@@ -281,11 +281,13 @@ static void __init gic_setup_intr(unsigned int intr, unsigned int cpu,
 
 	/* Setup Intr to Pin mapping */
 	if (pin & GIC_MAP_TO_NMI_MSK) {
+		int i;
+
 		GICWRITE(GIC_REG_ADDR(SHARED, GIC_SH_MAP_TO_PIN(intr)), pin);
 		/* FIXME: hack to route NMI to all cpu's */
-		for (cpu = 0; cpu < NR_CPUS; cpu += 32) {
+		for (i = 0; i < NR_CPUS; i += 32) {
 			GICWRITE(GIC_REG_ADDR(SHARED,
-					  GIC_SH_MAP_TO_VPE_REG_OFF(intr, cpu)),
+					  GIC_SH_MAP_TO_VPE_REG_OFF(intr, i)),
 				 0xffffffff);
 		}
 	} else {
-- 
2.0.0

[PATCH 4/7] MIPS: GIC: Prevent array overrun

[Markos Chandras]

From: Jeffrey Deans <jeffrey.deans@imgtec.com>

A GIC interrupt which is declared as having a GIC_MAP_TO_NMI_MSK
mapping causes the cpu parameter to gic_setup_intr() to be increased
to 32, causing memory corruption when pcpu_masks[] is written to again
later in the function.

Cc: stable@vger.kernel.org
Signed-off-by: Jeffrey Deans <jeffrey.deans@imgtec.com>
Signed-off-by: Markos Chandras <markos.chandras@imgtec.com>
---
 arch/mips/kernel/irq-gic.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/mips/kernel/irq-gic.c b/arch/mips/kernel/irq-gic.c
index 71cf45a335b6..9932aef91abb 100644
--- a/arch/mips/kernel/irq-gic.c
+++ b/arch/mips/kernel/irq-gic.c
@@ -281,11 +281,13 @@ static void __init gic_setup_intr(unsigned int intr, unsigned int cpu,
 
 	/* Setup Intr to Pin mapping */
 	if (pin & GIC_MAP_TO_NMI_MSK) {
+		int i;
+
 		GICWRITE(GIC_REG_ADDR(SHARED, GIC_SH_MAP_TO_PIN(intr)), pin);
 		/* FIXME: hack to route NMI to all cpu's */
-		for (cpu = 0; cpu < NR_CPUS; cpu += 32) {
+		for (i = 0; i < NR_CPUS; i += 32) {
 			GICWRITE(GIC_REG_ADDR(SHARED,
-					  GIC_SH_MAP_TO_VPE_REG_OFF(intr, cpu)),
+					  GIC_SH_MAP_TO_VPE_REG_OFF(intr, i)),
 				 0xffffffff);
 		}
 	} else {
-- 
2.0.0

[PATCH 5/7] MIPS: GIC: Generalise check for pending interrupts

[Markos Chandras]

From: Jeffrey Deans <jeffrey.deans@imgtec.com>

Move most of the functionality of gic_get_int() into a new function
gic_get_int_mask() which takes a bitmask of interrupts in which the
caller is interested, and returns the subset which are pending for the
current CPU.

This allows CP0 IRQ dispatch routines to check only the GIC interrupts
which are routed to a particular CPU interrupt input.

gic_get_int() is reimplemented using gic_get_int_mask() and is retained
for use by any platforms for which gic_get_int() is sufficient.

Signed-off-by: Jeffrey Deans <jeffrey.deans@imgtec.com>
Signed-off-by: Markos Chandras <markos.chandras@imgtec.com>
---
 arch/mips/include/asm/gic.h |  1 +
 arch/mips/kernel/irq-gic.c  | 13 +++++++++++--
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/arch/mips/include/asm/gic.h b/arch/mips/include/asm/gic.h
index 394d366b8fc1..8b30befd99d6 100644
--- a/arch/mips/include/asm/gic.h
+++ b/arch/mips/include/asm/gic.h
@@ -373,6 +373,7 @@ extern unsigned int plat_ipi_call_int_xlate(unsigned int);
 extern unsigned int plat_ipi_resched_int_xlate(unsigned int);
 extern void gic_bind_eic_interrupt(int irq, int set);
 extern unsigned int gic_get_timer_pending(void);
+extern void gic_get_int_mask(unsigned long *dst, const unsigned long *src);
 extern unsigned int gic_get_int(void);
 extern void gic_enable_interrupt(int irq_vec);
 extern void gic_disable_interrupt(int irq_vec);
diff --git a/arch/mips/kernel/irq-gic.c b/arch/mips/kernel/irq-gic.c
index 9932aef91abb..9e9d8b9a5b97 100644
--- a/arch/mips/kernel/irq-gic.c
+++ b/arch/mips/kernel/irq-gic.c
@@ -189,7 +189,7 @@ unsigned int gic_compare_int(void)
 		return 0;
 }
 
-unsigned int gic_get_int(void)
+void gic_get_int_mask(unsigned long *dst, const unsigned long *src)
 {
 	unsigned int i;
 	unsigned long *pending, *intrmask, *pcpu_mask;
@@ -214,8 +214,17 @@ unsigned int gic_get_int(void)
 
 	bitmap_and(pending, pending, intrmask, GIC_NUM_INTRS);
 	bitmap_and(pending, pending, pcpu_mask, GIC_NUM_INTRS);
+	bitmap_and(dst, src, pending, GIC_NUM_INTRS);
+}
+
+unsigned int gic_get_int(void)
+{
+	DECLARE_BITMAP(interrupts, GIC_NUM_INTRS);
+
+	bitmap_fill(interrupts, GIC_NUM_INTRS);
+	gic_get_int_mask(interrupts, interrupts);
 
-	return find_first_bit(pending, GIC_NUM_INTRS);
+	return find_first_bit(interrupts, GIC_NUM_INTRS);
 }
 
 static void gic_mask_irq(struct irq_data *d)
-- 
2.0.0

[PATCH 5/7] MIPS: GIC: Generalise check for pending interrupts

[Markos Chandras]

From: Jeffrey Deans <jeffrey.deans@imgtec.com>

Move most of the functionality of gic_get_int() into a new function
gic_get_int_mask() which takes a bitmask of interrupts in which the
caller is interested, and returns the subset which are pending for the
current CPU.

This allows CP0 IRQ dispatch routines to check only the GIC interrupts
which are routed to a particular CPU interrupt input.

gic_get_int() is reimplemented using gic_get_int_mask() and is retained
for use by any platforms for which gic_get_int() is sufficient.

Signed-off-by: Jeffrey Deans <jeffrey.deans@imgtec.com>
Signed-off-by: Markos Chandras <markos.chandras@imgtec.com>
---
 arch/mips/include/asm/gic.h |  1 +
 arch/mips/kernel/irq-gic.c  | 13 +++++++++++--
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/arch/mips/include/asm/gic.h b/arch/mips/include/asm/gic.h
index 394d366b8fc1..8b30befd99d6 100644
--- a/arch/mips/include/asm/gic.h
+++ b/arch/mips/include/asm/gic.h
@@ -373,6 +373,7 @@ extern unsigned int plat_ipi_call_int_xlate(unsigned int);
 extern unsigned int plat_ipi_resched_int_xlate(unsigned int);
 extern void gic_bind_eic_interrupt(int irq, int set);
 extern unsigned int gic_get_timer_pending(void);
+extern void gic_get_int_mask(unsigned long *dst, const unsigned long *src);
 extern unsigned int gic_get_int(void);
 extern void gic_enable_interrupt(int irq_vec);
 extern void gic_disable_interrupt(int irq_vec);
diff --git a/arch/mips/kernel/irq-gic.c b/arch/mips/kernel/irq-gic.c
index 9932aef91abb..9e9d8b9a5b97 100644
--- a/arch/mips/kernel/irq-gic.c
+++ b/arch/mips/kernel/irq-gic.c
@@ -189,7 +189,7 @@ unsigned int gic_compare_int(void)
 		return 0;
 }
 
-unsigned int gic_get_int(void)
+void gic_get_int_mask(unsigned long *dst, const unsigned long *src)
 {
 	unsigned int i;
 	unsigned long *pending, *intrmask, *pcpu_mask;
@@ -214,8 +214,17 @@ unsigned int gic_get_int(void)
 
 	bitmap_and(pending, pending, intrmask, GIC_NUM_INTRS);
 	bitmap_and(pending, pending, pcpu_mask, GIC_NUM_INTRS);
+	bitmap_and(dst, src, pending, GIC_NUM_INTRS);
+}
+
+unsigned int gic_get_int(void)
+{
+	DECLARE_BITMAP(interrupts, GIC_NUM_INTRS);
+
+	bitmap_fill(interrupts, GIC_NUM_INTRS);
+	gic_get_int_mask(interrupts, interrupts);
 
-	return find_first_bit(pending, GIC_NUM_INTRS);
+	return find_first_bit(interrupts, GIC_NUM_INTRS);
 }
 
 static void gic_mask_irq(struct irq_data *d)
-- 
2.0.0

[PATCH 6/7] MIPS: Malta: Fix dispatching of GIC interrupts

[Markos Chandras]

From: Jeffrey Deans <jeffrey.deans@imgtec.com>

The Malta malta_ipi_irqdispatch() routine now checks only IPI interrupts
when handling IPIs. It could previously call do_IRQ() for non-IPIs, and
also call do_IRQ() with an invalid IRQ number if there were no pending
GIC interrupts when gic_get_int() was called.

Signed-off-by: Jeffrey Deans <jeffrey.deans@imgtec.com>
Signed-off-by: Markos Chandras <markos.chandras@imgtec.com>
---
 arch/mips/mti-malta/malta-int.c | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/arch/mips/mti-malta/malta-int.c b/arch/mips/mti-malta/malta-int.c
index 4ab919141737..e4f43baa8f67 100644
--- a/arch/mips/mti-malta/malta-int.c
+++ b/arch/mips/mti-malta/malta-int.c
@@ -42,6 +42,10 @@ static unsigned int ipi_map[NR_CPUS];
 
 static DEFINE_RAW_SPINLOCK(mips_irq_lock);
 
+#ifdef CONFIG_MIPS_GIC_IPI
+DECLARE_BITMAP(ipi_ints, GIC_NUM_INTRS);
+#endif
+
 static inline int mips_pcibios_iack(void)
 {
 	int irq;
@@ -125,16 +129,22 @@ static void malta_hw0_irqdispatch(void)
 
 static void malta_ipi_irqdispatch(void)
 {
-	int irq;
+#ifdef CONFIG_MIPS_GIC_IPI
+	unsigned long irq;
+	DECLARE_BITMAP(pending, GIC_NUM_INTRS);
 
-	if (gic_compare_int())
-		do_IRQ(MIPS_GIC_IRQ_BASE);
+	gic_get_int_mask(pending, ipi_ints);
+
+	irq = find_first_bit(pending, GIC_NUM_INTRS);
 
-	irq = gic_get_int();
-	if (irq < 0)
-		return;	 /* interrupt has already been cleared */
+	while (irq < GIC_NUM_INTRS) {
+		do_IRQ(MIPS_GIC_IRQ_BASE + irq);
 
-	do_IRQ(MIPS_GIC_IRQ_BASE + irq);
+		irq = find_next_bit(pending, GIC_NUM_INTRS, irq + 1);
+	}
+#endif
+	if (gic_compare_int())
+		do_IRQ(MIPS_GIC_IRQ_BASE);
 }
 
 static void corehi_irqdispatch(void)
@@ -429,6 +439,7 @@ static void __init fill_ipi_map1(int baseintr, int cpu, int cpupin)
 	gic_intr_map[intr].trigtype = GIC_TRIG_EDGE;
 	gic_intr_map[intr].flags = 0;
 	ipi_map[cpu] |= (1 << (cpupin + 2));
+	bitmap_set(ipi_ints, intr, 1);
 }
 
 static void __init fill_ipi_map(void)
-- 
2.0.0

[PATCH 6/7] MIPS: Malta: Fix dispatching of GIC interrupts

[Markos Chandras]

From: Jeffrey Deans <jeffrey.deans@imgtec.com>

The Malta malta_ipi_irqdispatch() routine now checks only IPI interrupts
when handling IPIs. It could previously call do_IRQ() for non-IPIs, and
also call do_IRQ() with an invalid IRQ number if there were no pending
GIC interrupts when gic_get_int() was called.

Signed-off-by: Jeffrey Deans <jeffrey.deans@imgtec.com>
Signed-off-by: Markos Chandras <markos.chandras@imgtec.com>
---
 arch/mips/mti-malta/malta-int.c | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/arch/mips/mti-malta/malta-int.c b/arch/mips/mti-malta/malta-int.c
index 4ab919141737..e4f43baa8f67 100644
--- a/arch/mips/mti-malta/malta-int.c
+++ b/arch/mips/mti-malta/malta-int.c
@@ -42,6 +42,10 @@ static unsigned int ipi_map[NR_CPUS];
 
 static DEFINE_RAW_SPINLOCK(mips_irq_lock);
 
+#ifdef CONFIG_MIPS_GIC_IPI
+DECLARE_BITMAP(ipi_ints, GIC_NUM_INTRS);
+#endif
+
 static inline int mips_pcibios_iack(void)
 {
 	int irq;
@@ -125,16 +129,22 @@ static void malta_hw0_irqdispatch(void)
 
 static void malta_ipi_irqdispatch(void)
 {
-	int irq;
+#ifdef CONFIG_MIPS_GIC_IPI
+	unsigned long irq;
+	DECLARE_BITMAP(pending, GIC_NUM_INTRS);
 
-	if (gic_compare_int())
-		do_IRQ(MIPS_GIC_IRQ_BASE);
+	gic_get_int_mask(pending, ipi_ints);
+
+	irq = find_first_bit(pending, GIC_NUM_INTRS);
 
-	irq = gic_get_int();
-	if (irq < 0)
-		return;	 /* interrupt has already been cleared */
+	while (irq < GIC_NUM_INTRS) {
+		do_IRQ(MIPS_GIC_IRQ_BASE + irq);
 
-	do_IRQ(MIPS_GIC_IRQ_BASE + irq);
+		irq = find_next_bit(pending, GIC_NUM_INTRS, irq + 1);
+	}
+#endif
+	if (gic_compare_int())
+		do_IRQ(MIPS_GIC_IRQ_BASE);
 }
 
 static void corehi_irqdispatch(void)
@@ -429,6 +439,7 @@ static void __init fill_ipi_map1(int baseintr, int cpu, int cpupin)
 	gic_intr_map[intr].trigtype = GIC_TRIG_EDGE;
 	gic_intr_map[intr].flags = 0;
 	ipi_map[cpu] |= (1 << (cpupin + 2));
+	bitmap_set(ipi_ints, intr, 1);
 }
 
 static void __init fill_ipi_map(void)
-- 
2.0.0

Re: [PATCH 6/7] MIPS: Malta: Fix dispatching of GIC interrupts

[Florian Fainelli]

On 07/17/2014 01:20 AM, Markos Chandras wrote:
> From: Jeffrey Deans <jeffrey.deans@imgtec.com>
> 
> The Malta malta_ipi_irqdispatch() routine now checks only IPI interrupts
> when handling IPIs. It could previously call do_IRQ() for non-IPIs, and
> also call do_IRQ() with an invalid IRQ number if there were no pending
> GIC interrupts when gic_get_int() was called.
> 
> Signed-off-by: Jeffrey Deans <jeffrey.deans@imgtec.com>
> Signed-off-by: Markos Chandras <markos.chandras@imgtec.com>
> ---
>  arch/mips/mti-malta/malta-int.c | 25 ++++++++++++++++++-------
>  1 file changed, 18 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/mips/mti-malta/malta-int.c b/arch/mips/mti-malta/malta-int.c
> index 4ab919141737..e4f43baa8f67 100644
> --- a/arch/mips/mti-malta/malta-int.c
> +++ b/arch/mips/mti-malta/malta-int.c
> @@ -42,6 +42,10 @@ static unsigned int ipi_map[NR_CPUS];
>  
>  static DEFINE_RAW_SPINLOCK(mips_irq_lock);
>  
> +#ifdef CONFIG_MIPS_GIC_IPI
> +DECLARE_BITMAP(ipi_ints, GIC_NUM_INTRS);
> +#endif

I caught this one too late (it is already in mips-for-linux-next), but
it looks like we could make ipi_ints static.
--
Florian

[PATCH 7/7] MIPS: GIC: Fix GICBIS macro

[Markos Chandras]

From: Jeffrey Deans <jeffrey.deans@imgtec.com>

The GICBIS macro could update the GIC registers incorrectly, depending
on the data value passed in:

* Bits were only OR'd into the register data, so register fields could
  not be cleared.

* Bits were OR'd into the register data without masking the data to the
  correct field width, corrupting adjacent bits.

Signed-off-by: Jeffrey Deans <jeffrey.deans@imgtec.com>
Signed-off-by: Markos Chandras <markos.chandras@imgtec.com>
---
 arch/mips/include/asm/gic.h | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/arch/mips/include/asm/gic.h b/arch/mips/include/asm/gic.h
index 8b30befd99d6..3f20b2111d56 100644
--- a/arch/mips/include/asm/gic.h
+++ b/arch/mips/include/asm/gic.h
@@ -43,18 +43,17 @@
 #ifdef GICISBYTELITTLEENDIAN
 #define GICREAD(reg, data)	((data) = (reg), (data) = le32_to_cpu(data))
 #define GICWRITE(reg, data)	((reg) = cpu_to_le32(data))
-#define GICBIS(reg, bits)			\
-	({unsigned int data;			\
-		GICREAD(reg, data);		\
-		data |= bits;			\
-		GICWRITE(reg, data);		\
-	})
-
 #else
 #define GICREAD(reg, data)	((data) = (reg))
 #define GICWRITE(reg, data)	((reg) = (data))
-#define GICBIS(reg, bits)	((reg) |= (bits))
 #endif
+#define GICBIS(reg, mask, bits)			\
+	do { u32 data;				\
+		GICREAD((reg), data);		\
+		data &= ~(mask);		\
+		data |= ((bits) & (mask));	\
+		GICWRITE((reg), data);		\
+	} while (0)
 
 
 /* GIC Address Space */
@@ -170,13 +169,15 @@
 #define GIC_SH_SET_POLARITY_OFS		0x0100
 #define GIC_SET_POLARITY(intr, pol) \
 	GICBIS(GIC_REG_ADDR(SHARED, GIC_SH_SET_POLARITY_OFS + \
-		GIC_INTR_OFS(intr)), (pol) << GIC_INTR_BIT(intr))
+		GIC_INTR_OFS(intr)), (1 << GIC_INTR_BIT(intr)), \
+		(pol) << GIC_INTR_BIT(intr))
 
 /* Triggering : Reset Value is always 0 */
 #define GIC_SH_SET_TRIGGER_OFS		0x0180
 #define GIC_SET_TRIGGER(intr, trig) \
 	GICBIS(GIC_REG_ADDR(SHARED, GIC_SH_SET_TRIGGER_OFS + \
-		GIC_INTR_OFS(intr)), (trig) << GIC_INTR_BIT(intr))
+		GIC_INTR_OFS(intr)), (1 << GIC_INTR_BIT(intr)), \
+		(trig) << GIC_INTR_BIT(intr))
 
 /* Mask manipulation */
 #define GIC_SH_SMASK_OFS		0x0380
-- 
2.0.0

[PATCH 7/7] MIPS: GIC: Fix GICBIS macro

[Markos Chandras]

From: Jeffrey Deans <jeffrey.deans@imgtec.com>

The GICBIS macro could update the GIC registers incorrectly, depending
on the data value passed in:

* Bits were only OR'd into the register data, so register fields could
  not be cleared.

* Bits were OR'd into the register data without masking the data to the
  correct field width, corrupting adjacent bits.

Signed-off-by: Jeffrey Deans <jeffrey.deans@imgtec.com>
Signed-off-by: Markos Chandras <markos.chandras@imgtec.com>
---
 arch/mips/include/asm/gic.h | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/arch/mips/include/asm/gic.h b/arch/mips/include/asm/gic.h
index 8b30befd99d6..3f20b2111d56 100644
--- a/arch/mips/include/asm/gic.h
+++ b/arch/mips/include/asm/gic.h
@@ -43,18 +43,17 @@
 #ifdef GICISBYTELITTLEENDIAN
 #define GICREAD(reg, data)	((data) = (reg), (data) = le32_to_cpu(data))
 #define GICWRITE(reg, data)	((reg) = cpu_to_le32(data))
-#define GICBIS(reg, bits)			\
-	({unsigned int data;			\
-		GICREAD(reg, data);		\
-		data |= bits;			\
-		GICWRITE(reg, data);		\
-	})
-
 #else
 #define GICREAD(reg, data)	((data) = (reg))
 #define GICWRITE(reg, data)	((reg) = (data))
-#define GICBIS(reg, bits)	((reg) |= (bits))
 #endif
+#define GICBIS(reg, mask, bits)			\
+	do { u32 data;				\
+		GICREAD((reg), data);		\
+		data &= ~(mask);		\
+		data |= ((bits) & (mask));	\
+		GICWRITE((reg), data);		\
+	} while (0)
 
 
 /* GIC Address Space */
@@ -170,13 +169,15 @@
 #define GIC_SH_SET_POLARITY_OFS		0x0100
 #define GIC_SET_POLARITY(intr, pol) \
 	GICBIS(GIC_REG_ADDR(SHARED, GIC_SH_SET_POLARITY_OFS + \
-		GIC_INTR_OFS(intr)), (pol) << GIC_INTR_BIT(intr))
+		GIC_INTR_OFS(intr)), (1 << GIC_INTR_BIT(intr)), \
+		(pol) << GIC_INTR_BIT(intr))
 
 /* Triggering : Reset Value is always 0 */
 #define GIC_SH_SET_TRIGGER_OFS		0x0180
 #define GIC_SET_TRIGGER(intr, trig) \
 	GICBIS(GIC_REG_ADDR(SHARED, GIC_SH_SET_TRIGGER_OFS + \
-		GIC_INTR_OFS(intr)), (trig) << GIC_INTR_BIT(intr))
+		GIC_INTR_OFS(intr)), (1 << GIC_INTR_BIT(intr)), \
+		(trig) << GIC_INTR_BIT(intr))
 
 /* Mask manipulation */
 #define GIC_SH_SMASK_OFS		0x0380
-- 
2.0.0

Re: [PATCH 7/7] MIPS: GIC: Fix GICBIS macro

[Sergei Shtylyov]

Hello.

On 07/17/2014 12:20 PM, Markos Chandras wrote:

> From: Jeffrey Deans <jeffrey.deans@imgtec.com>

> The GICBIS macro could update the GIC registers incorrectly, depending
> on the data value passed in:

> * Bits were only OR'd into the register data, so register fields could
>    not be cleared.

> * Bits were OR'd into the register data without masking the data to the
>    correct field width, corrupting adjacent bits.

> Signed-off-by: Jeffrey Deans <jeffrey.deans@imgtec.com>
> Signed-off-by: Markos Chandras <markos.chandras@imgtec.com>
> ---
>   arch/mips/include/asm/gic.h | 21 +++++++++++----------
>   1 file changed, 11 insertions(+), 10 deletions(-)

> diff --git a/arch/mips/include/asm/gic.h b/arch/mips/include/asm/gic.h
> index 8b30befd99d6..3f20b2111d56 100644
> --- a/arch/mips/include/asm/gic.h
> +++ b/arch/mips/include/asm/gic.h
> @@ -43,18 +43,17 @@
>   #ifdef GICISBYTELITTLEENDIAN
>   #define GICREAD(reg, data)	((data) = (reg), (data) = le32_to_cpu(data))
>   #define GICWRITE(reg, data)	((reg) = cpu_to_le32(data))
> -#define GICBIS(reg, bits)			\
> -	({unsigned int data;			\
> -		GICREAD(reg, data);		\
> -		data |= bits;			\
> -		GICWRITE(reg, data);		\
> -	})
> -
>   #else
>   #define GICREAD(reg, data)	((data) = (reg))
>   #define GICWRITE(reg, data)	((reg) = (data))
> -#define GICBIS(reg, bits)	((reg) |= (bits))
>   #endif
> +#define GICBIS(reg, mask, bits)			\
> +	do { u32 data;				\
> +		GICREAD((reg), data);		\

    Why () only around 'reg', not around 'data'?

> +		data &= ~(mask);		\
> +		data |= ((bits) & (mask));	\

    Outer () not needed.

> +		GICWRITE((reg), data);		\

    Again, why no () around 'data'?

> +	} while (0)

WBR, Sergei

Re: [PATCH 7/7] MIPS: GIC: Fix GICBIS macro

[Jeffrey Deans]

Hi Sergei,

On 17/07/14 13:47, Sergei Shtylyov wrote:
> Hello.
>
> On 07/17/2014 12:20 PM, Markos Chandras wrote:
>
>> From: Jeffrey Deans <jeffrey.deans@imgtec.com>
>
>> The GICBIS macro could update the GIC registers incorrectly, depending
>> on the data value passed in:
>
>> * Bits were only OR'd into the register data, so register fields could
>>    not be cleared.
>
>> * Bits were OR'd into the register data without masking the data to the
>>    correct field width, corrupting adjacent bits.
>
>> Signed-off-by: Jeffrey Deans <jeffrey.deans@imgtec.com>
>> Signed-off-by: Markos Chandras <markos.chandras@imgtec.com>
>> ---
>>   arch/mips/include/asm/gic.h | 21 +++++++++++----------
>>   1 file changed, 11 insertions(+), 10 deletions(-)
>
>> diff --git a/arch/mips/include/asm/gic.h b/arch/mips/include/asm/gic.h
>> index 8b30befd99d6..3f20b2111d56 100644
>> --- a/arch/mips/include/asm/gic.h
>> +++ b/arch/mips/include/asm/gic.h
>> @@ -43,18 +43,17 @@
>>   #ifdef GICISBYTELITTLEENDIAN
>>   #define GICREAD(reg, data)    ((data) = (reg), (data) =
>> le32_to_cpu(data))
>>   #define GICWRITE(reg, data)    ((reg) = cpu_to_le32(data))
>> -#define GICBIS(reg, bits)            \
>> -    ({unsigned int data;            \
>> -        GICREAD(reg, data);        \
>> -        data |= bits;            \
>> -        GICWRITE(reg, data);        \
>> -    })
>> -
>>   #else
>>   #define GICREAD(reg, data)    ((data) = (reg))
>>   #define GICWRITE(reg, data)    ((reg) = (data))
>> -#define GICBIS(reg, bits)    ((reg) |= (bits))
>>   #endif
>> +#define GICBIS(reg, mask, bits)            \
>> +    do { u32 data;                \
>> +        GICREAD((reg), data);        \
>
>     Why () only around 'reg', not around 'data'?

Brackets aren't necessary around "data" because it is declared at the 
start of the "do" code block, so it can't expand to anything else within 
that scope.

>
>> +        data &= ~(mask);        \
>> +        data |= ((bits) & (mask));    \
>
>     Outer () not needed.

Agreed.

>
>> +        GICWRITE((reg), data);        \
>
>     Again, why no () around 'data'?
>
>> +    } while (0)

As above.

>
> WBR, Sergei
>

Thanks for reviewing!

Jeffrey

Re: [PATCH 7/7] MIPS: GIC: Fix GICBIS macro

[Jeffrey Deans]

Hi Sergei,

On 17/07/14 13:47, Sergei Shtylyov wrote:
> Hello.
>
> On 07/17/2014 12:20 PM, Markos Chandras wrote:
>
>> From: Jeffrey Deans <jeffrey.deans@imgtec.com>
>
>> The GICBIS macro could update the GIC registers incorrectly, depending
>> on the data value passed in:
>
>> * Bits were only OR'd into the register data, so register fields could
>>    not be cleared.
>
>> * Bits were OR'd into the register data without masking the data to the
>>    correct field width, corrupting adjacent bits.
>
>> Signed-off-by: Jeffrey Deans <jeffrey.deans@imgtec.com>
>> Signed-off-by: Markos Chandras <markos.chandras@imgtec.com>
>> ---
>>   arch/mips/include/asm/gic.h | 21 +++++++++++----------
>>   1 file changed, 11 insertions(+), 10 deletions(-)
>
>> diff --git a/arch/mips/include/asm/gic.h b/arch/mips/include/asm/gic.h
>> index 8b30befd99d6..3f20b2111d56 100644
>> --- a/arch/mips/include/asm/gic.h
>> +++ b/arch/mips/include/asm/gic.h
>> @@ -43,18 +43,17 @@
>>   #ifdef GICISBYTELITTLEENDIAN
>>   #define GICREAD(reg, data)    ((data) = (reg), (data) =
>> le32_to_cpu(data))
>>   #define GICWRITE(reg, data)    ((reg) = cpu_to_le32(data))
>> -#define GICBIS(reg, bits)            \
>> -    ({unsigned int data;            \
>> -        GICREAD(reg, data);        \
>> -        data |= bits;            \
>> -        GICWRITE(reg, data);        \
>> -    })
>> -
>>   #else
>>   #define GICREAD(reg, data)    ((data) = (reg))
>>   #define GICWRITE(reg, data)    ((reg) = (data))
>> -#define GICBIS(reg, bits)    ((reg) |= (bits))
>>   #endif
>> +#define GICBIS(reg, mask, bits)            \
>> +    do { u32 data;                \
>> +        GICREAD((reg), data);        \
>
>     Why () only around 'reg', not around 'data'?

Brackets aren't necessary around "data" because it is declared at the 
start of the "do" code block, so it can't expand to anything else within 
that scope.

>
>> +        data &= ~(mask);        \
>> +        data |= ((bits) & (mask));    \
>
>     Outer () not needed.

Agreed.

>
>> +        GICWRITE((reg), data);        \
>
>     Again, why no () around 'data'?
>
>> +    } while (0)

As above.

>
> WBR, Sergei
>

Thanks for reviewing!

Jeffrey

Re: [PATCH 7/7] MIPS: GIC: Fix GICBIS macro

[Markos Chandras]

On 07/18/2014 08:54 AM, Jeffrey Deans wrote:
> 
>>
>>> +        data &= ~(mask);        \
>>> +        data |= ((bits) & (mask));    \
>>
>>     Outer () not needed.
> 
> Agreed.
> 
Thanks for the review. This is a minor fix and I see no reason for v2.
Ralf could you please remove the outer () when you get to apply this patch?

Thanks!

-- 
markos

Re: [PATCH 7/7] MIPS: GIC: Fix GICBIS macro

[Markos Chandras]

On 07/18/2014 08:54 AM, Jeffrey Deans wrote:
> 
>>
>>> +        data &= ~(mask);        \
>>> +        data |= ((bits) & (mask));    \
>>
>>     Outer () not needed.
> 
> Agreed.
> 
Thanks for the review. This is a minor fix and I see no reason for v2.
Ralf could you please remove the outer () when you get to apply this patch?

Thanks!

-- 
markos

Re: [PATCH 7/7] MIPS: GIC: Fix GICBIS macro

[Sergei Shtylyov]

Hello.

On 07/18/2014 11:54 AM, Jeffrey Deans wrote:

>>> From: Jeffrey Deans <jeffrey.deans@imgtec.com>

>>> The GICBIS macro could update the GIC registers incorrectly, depending
>>> on the data value passed in:

>>> * Bits were only OR'd into the register data, so register fields could
>>>    not be cleared.

>>> * Bits were OR'd into the register data without masking the data to the
>>>    correct field width, corrupting adjacent bits.

>>> Signed-off-by: Jeffrey Deans <jeffrey.deans@imgtec.com>
>>> Signed-off-by: Markos Chandras <markos.chandras@imgtec.com>
>>> ---
>>>   arch/mips/include/asm/gic.h | 21 +++++++++++----------
>>>   1 file changed, 11 insertions(+), 10 deletions(-)

>>> diff --git a/arch/mips/include/asm/gic.h b/arch/mips/include/asm/gic.h
>>> index 8b30befd99d6..3f20b2111d56 100644
>>> --- a/arch/mips/include/asm/gic.h
>>> +++ b/arch/mips/include/asm/gic.h
>>> @@ -43,18 +43,17 @@
>>>   #ifdef GICISBYTELITTLEENDIAN
>>>   #define GICREAD(reg, data)    ((data) = (reg), (data) =
>>> le32_to_cpu(data))
>>>   #define GICWRITE(reg, data)    ((reg) = cpu_to_le32(data))
>>> -#define GICBIS(reg, bits)            \
>>> -    ({unsigned int data;            \
>>> -        GICREAD(reg, data);        \
>>> -        data |= bits;            \
>>> -        GICWRITE(reg, data);        \
>>> -    })
>>> -
>>>   #else
>>>   #define GICREAD(reg, data)    ((data) = (reg))
>>>   #define GICWRITE(reg, data)    ((reg) = (data))
>>> -#define GICBIS(reg, bits)    ((reg) |= (bits))
>>>   #endif
>>> +#define GICBIS(reg, mask, bits)            \
>>> +    do { u32 data;                \
>>> +        GICREAD((reg), data);        \

>>     Why () only around 'reg', not around 'data'?

> Brackets aren't necessary around "data" because it is declared at the start of
> the "do" code block, so it can't expand to anything else within that scope.

    Oh, I was not attentive enough, sorry about that... :-<
    However, it makes sense to at least put that declaration at a separate line.

WBR, Sergei

Re: [PATCH 7/7] MIPS: GIC: Fix GICBIS macro

[Ralf Baechle]

On Fri, Jul 18, 2014 at 10:05:29PM +0400, Sergei Shtylyov wrote:

> >>>+#define GICBIS(reg, mask, bits)            \
> >>>+    do { u32 data;                \
> >>>+        GICREAD((reg), data);        \
> 
> >>    Why () only around 'reg', not around 'data'?
> 
> >Brackets aren't necessary around "data" because it is declared at the start of
> >the "do" code block, so it can't expand to anything else within that scope.
> 
>    Oh, I was not attentive enough, sorry about that... :-<
>    However, it makes sense to at least put that declaration at a separate line.

And it's not safe against multiple evaluation of macro arguments.  Imagine
what's going to happen if GICBIS is called as something like

	GICBIS(++a, ++b, c);

That'll expand to:

    do { u32 data;
            GICREAD((++a), data);
            data &= ~(++b);
            data |= ((bits) & (++b));
            GICWRITE((++a), data);
    } while (0)

Paranoia?

  Ralf