linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/2] powerpc/8xx: Fix NR_IRQ bugs and refactor 8xx interrupt controller
@ 2012-04-16 20:13 Grant Likely
  2012-04-16 20:13 ` [PATCH 2/2] irqdomain/powerpc: Fix broken NR_IRQ references Grant Likely
                   ` (2 more replies)
  0 siblings, 3 replies; 13+ messages in thread
From: Grant Likely @ 2012-04-16 20:13 UTC (permalink / raw)
  To: linux-kernel, linuxppc-dev; +Cc: Grant Likely, Benjamin Herrenschmidt

The mpc8xx driver uses a reference to NR_IRQS that is buggy.  It uses
NR_IRQs for the array size of the ppc_cached_irq_mask bitmap, but
NR_IRQs could be smaller than the number of hardware irqs that
ppc_cached_irq_mask tracks.

Also, while fixing that problem, it became apparent that the interrupt
controller only supports 32 interrupt numbers, but it is written as if
it supports multiple register banks which is more complicated.

This patch pulls out the buggy reference to NR_IRQs and fixes the size
of the ppc_cached_irq_mask to match the number of HW irqs.  It also
drops the now-unnecessary code since ppc_cached_irq_mask is no longer
an array.

Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 arch/powerpc/sysdev/mpc8xx_pic.c |   61 +++++++++++++-------------------------
 1 file changed, 21 insertions(+), 40 deletions(-)

diff --git a/arch/powerpc/sysdev/mpc8xx_pic.c b/arch/powerpc/sysdev/mpc8xx_pic.c
index d5f5416..91cade8 100644
--- a/arch/powerpc/sysdev/mpc8xx_pic.c
+++ b/arch/powerpc/sysdev/mpc8xx_pic.c
@@ -18,69 +18,47 @@
 extern int cpm_get_irq(struct pt_regs *regs);
 
 static struct irq_domain *mpc8xx_pic_host;
-#define NR_MASK_WORDS   ((NR_IRQS + 31) / 32)
-static unsigned long ppc_cached_irq_mask[NR_MASK_WORDS];
+static unsigned long ppc_cached_irq_mask;
 static sysconf8xx_t __iomem *siu_reg;
 
 int cpm_get_irq(struct pt_regs *regs);
 
-static void mpc8xx_unmask_irq(struct irq_data *d)
+static inline unsigned long mpc8xx_irqd_to_bit(struct irq_data *d)
 {
-	int	bit, word;
-	unsigned int irq_nr = (unsigned int)irqd_to_hwirq(d);
-
-	bit = irq_nr & 0x1f;
-	word = irq_nr >> 5;
+	return 0x80000000 >> irqd_to_hwirq(d);
+}
 
-	ppc_cached_irq_mask[word] |= (1 << (31-bit));
-	out_be32(&siu_reg->sc_simask, ppc_cached_irq_mask[word]);
+static void mpc8xx_unmask_irq(struct irq_data *d)
+{
+	ppc_cached_irq_mask |= mpc8xx_irqd_to_bit(d);
+	out_be32(&siu_reg->sc_simask, ppc_cached_irq_mask);
 }
 
 static void mpc8xx_mask_irq(struct irq_data *d)
 {
-	int	bit, word;
-	unsigned int irq_nr = (unsigned int)irqd_to_hwirq(d);
-
-	bit = irq_nr & 0x1f;
-	word = irq_nr >> 5;
-
-	ppc_cached_irq_mask[word] &= ~(1 << (31-bit));
-	out_be32(&siu_reg->sc_simask, ppc_cached_irq_mask[word]);
+	ppc_cached_irq_mask &= ~mpc8xx_irqd_to_bit(d);
+	out_be32(&siu_reg->sc_simask, ppc_cached_irq_mask);
 }
 
 static void mpc8xx_ack(struct irq_data *d)
 {
-	int	bit;
-	unsigned int irq_nr = (unsigned int)irqd_to_hwirq(d);
-
-	bit = irq_nr & 0x1f;
-	out_be32(&siu_reg->sc_sipend, 1 << (31-bit));
+	out_be32(&siu_reg->sc_sipend, mpc8xx_irqd_to_bit(d));
 }
 
 static void mpc8xx_end_irq(struct irq_data *d)
 {
-	int bit, word;
-	unsigned int irq_nr = (unsigned int)irqd_to_hwirq(d);
-
-	bit = irq_nr & 0x1f;
-	word = irq_nr >> 5;
-
-	ppc_cached_irq_mask[word] |= (1 << (31-bit));
-	out_be32(&siu_reg->sc_simask, ppc_cached_irq_mask[word]);
+	ppc_cached_irq_mask |= mpc8xx_irqd_to_bit(d);
+	out_be32(&siu_reg->sc_simask, ppc_cached_irq_mask);
 }
 
 static int mpc8xx_set_irq_type(struct irq_data *d, unsigned int flow_type)
 {
-	if (flow_type & IRQ_TYPE_EDGE_FALLING) {
-		irq_hw_number_t hw = (unsigned int)irqd_to_hwirq(d);
+	/* only external IRQ senses are programmable */
+	if ((flow_type & IRQ_TYPE_EDGE_FALLING) && !(irqd_to_hwirq(d) & 1)) {
 		unsigned int siel = in_be32(&siu_reg->sc_siel);
-
-		/* only external IRQ senses are programmable */
-		if ((hw & 1) == 0) {
-			siel |= (0x80000000 >> hw);
-			out_be32(&siu_reg->sc_siel, siel);
-			__irq_set_handler_locked(d->irq, handle_edge_irq);
-		}
+		siel |= mpc8xx_irqd_to_bit(d);
+		out_be32(&siu_reg->sc_siel, siel);
+		__irq_set_handler_locked(d->irq, handle_edge_irq);
 	}
 	return 0;
 }
@@ -132,6 +110,9 @@ static int mpc8xx_pic_host_xlate(struct irq_domain *h, struct device_node *ct,
 		IRQ_TYPE_EDGE_FALLING,
 	};
 
+	if (intspec[0] > 0x1f)
+		return 0;
+
 	*out_hwirq = intspec[0];
 	if (intsize > 1 && intspec[1] < 4)
 		*out_flags = map_pic_senses[intspec[1]];
-- 
1.7.9.5


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 2/2] irqdomain/powerpc: Fix broken NR_IRQ references
  2012-04-16 20:13 [PATCH 1/2] powerpc/8xx: Fix NR_IRQ bugs and refactor 8xx interrupt controller Grant Likely
@ 2012-04-16 20:13 ` Grant Likely
  2012-04-19  0:45   ` Michael Ellerman
  2012-04-16 20:13 ` [PATCH 1/2] powerpc/8xx: Fix NR_IRQ bugs and refactor 8xx interrupt controller Grant Likely
  2012-04-16 23:03 ` Benjamin Herrenschmidt
  2 siblings, 1 reply; 13+ messages in thread
From: Grant Likely @ 2012-04-16 20:13 UTC (permalink / raw)
  To: linux-kernel, linuxppc-dev; +Cc: Grant Likely, Benjamin Herrenschmidt

The switch from using irq_map to irq_alloc_desc*() for managing irq
number allocations introduced new bugs in some of the powerpc
interrupt code.  Several functions rely on the value of NR_IRQS to
determine the maximum irq number that could get allocated.  However,
with sparse_irq and using irq_alloc_desc*() the maximum possible irq
number is now specified with 'nr_irqs' which may be a number larger
than NR_IRQS.  This has caused breakage on powermac when
CONFIG_NR_IRQS is set to 32.

This patch removes most of the direct references to NR_IRQS in the
powerpc code and replaces them with either a nr_irqs reference or by
using the common for_each_irq_desc() macro.  The powerpc-specific
for_each_irq() macro is removed at the same time.

Also, the Cell axon_msi driver is refactored to remove the global
build assumption on the size of NR_IRQS and instead add a limit to the
maximum irq number when calling irq_domain_add_nomap().

Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 arch/powerpc/include/asm/irq.h               |    4 ----
 arch/powerpc/kernel/irq.c                    |    6 +-----
 arch/powerpc/kernel/machine_kexec.c          |    7 ++-----
 arch/powerpc/platforms/cell/axon_msi.c       |    7 ++-----
 arch/powerpc/platforms/cell/beat_interrupt.c |    2 +-
 arch/powerpc/platforms/powermac/pic.c        |    6 +++---
 arch/powerpc/sysdev/cpm2_pic.c               |    3 +--
 arch/powerpc/sysdev/xics/xics-common.c       |    7 +++----
 8 files changed, 13 insertions(+), 29 deletions(-)

diff --git a/arch/powerpc/include/asm/irq.h b/arch/powerpc/include/asm/irq.h
index e648af9..0e40843 100644
--- a/arch/powerpc/include/asm/irq.h
+++ b/arch/powerpc/include/asm/irq.h
@@ -18,10 +18,6 @@
 #include <linux/atomic.h>
 
 
-/* Define a way to iterate across irqs. */
-#define for_each_irq(i) \
-	for ((i) = 0; (i) < NR_IRQS; ++(i))
-
 extern atomic_t ppc_n_lost_interrupts;
 
 /* This number is used when no interrupt has been assigned */
diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c
index 5ec1b23..43eb74f 100644
--- a/arch/powerpc/kernel/irq.c
+++ b/arch/powerpc/kernel/irq.c
@@ -330,14 +330,10 @@ void migrate_irqs(void)
 
 	alloc_cpumask_var(&mask, GFP_KERNEL);
 
-	for_each_irq(irq) {
+	for_each_irq_desc(irq, desc) {
 		struct irq_data *data;
 		struct irq_chip *chip;
 
-		desc = irq_to_desc(irq);
-		if (!desc)
-			continue;
-
 		data = irq_desc_get_irq_data(desc);
 		if (irqd_is_per_cpu(data))
 			continue;
diff --git a/arch/powerpc/kernel/machine_kexec.c b/arch/powerpc/kernel/machine_kexec.c
index c957b12..5df7777 100644
--- a/arch/powerpc/kernel/machine_kexec.c
+++ b/arch/powerpc/kernel/machine_kexec.c
@@ -23,14 +23,11 @@
 
 void machine_kexec_mask_interrupts(void) {
 	unsigned int i;
+	struct irq_desc *desc;
 
-	for_each_irq(i) {
-		struct irq_desc *desc = irq_to_desc(i);
+	for_each_irq_desc(i, desc) {
 		struct irq_chip *chip;
 
-		if (!desc)
-			continue;
-
 		chip = irq_desc_get_chip(desc);
 		if (!chip)
 			continue;
diff --git a/arch/powerpc/platforms/cell/axon_msi.c b/arch/powerpc/platforms/cell/axon_msi.c
index d09f3e8..fc9df1a 100644
--- a/arch/powerpc/platforms/cell/axon_msi.c
+++ b/arch/powerpc/platforms/cell/axon_msi.c
@@ -114,7 +114,7 @@ static void axon_msi_cascade(unsigned int irq, struct irq_desc *desc)
 		pr_devel("axon_msi: woff %x roff %x msi %x\n",
 			  write_offset, msic->read_offset, msi);
 
-		if (msi < NR_IRQS && irq_get_chip_data(msi) == msic) {
+		if (msi < nr_irqs && irq_get_chip_data(msi) == msic) {
 			generic_handle_irq(msi);
 			msic->fifo_virt[idx] = cpu_to_le32(0xffffffff);
 		} else {
@@ -276,9 +276,6 @@ static int axon_msi_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
 	if (rc)
 		return rc;
 
-	/* We rely on being able to stash a virq in a u16 */
-	BUILD_BUG_ON(NR_IRQS > 65536);
-
 	list_for_each_entry(entry, &dev->msi_list, list) {
 		virq = irq_create_direct_mapping(msic->irq_domain);
 		if (virq == NO_IRQ) {
@@ -392,7 +389,7 @@ static int axon_msi_probe(struct platform_device *device)
 	}
 	memset(msic->fifo_virt, 0xff, MSIC_FIFO_SIZE_BYTES);
 
-	msic->irq_domain = irq_domain_add_nomap(dn, 0, &msic_host_ops, msic);
+	msic->irq_domain = irq_domain_add_nomap(dn, 65536, &msic_host_ops, msic);
 	if (!msic->irq_domain) {
 		printk(KERN_ERR "axon_msi: couldn't allocate irq_domain for %s\n",
 		       dn->full_name);
diff --git a/arch/powerpc/platforms/cell/beat_interrupt.c b/arch/powerpc/platforms/cell/beat_interrupt.c
index f9a48af..8c6dc42 100644
--- a/arch/powerpc/platforms/cell/beat_interrupt.c
+++ b/arch/powerpc/platforms/cell/beat_interrupt.c
@@ -248,6 +248,6 @@ void beatic_deinit_IRQ(void)
 {
 	int	i;
 
-	for (i = 1; i < NR_IRQS; i++)
+	for (i = 1; i < nr_irqs; i++)
 		beat_destruct_irq_plug(i);
 }
diff --git a/arch/powerpc/platforms/powermac/pic.c b/arch/powerpc/platforms/powermac/pic.c
index 66ad93d..c4e6305 100644
--- a/arch/powerpc/platforms/powermac/pic.c
+++ b/arch/powerpc/platforms/powermac/pic.c
@@ -57,9 +57,9 @@ static int max_real_irqs;
 
 static DEFINE_RAW_SPINLOCK(pmac_pic_lock);
 
-#define NR_MASK_WORDS	((NR_IRQS + 31) / 32)
-static unsigned long ppc_lost_interrupts[NR_MASK_WORDS];
-static unsigned long ppc_cached_irq_mask[NR_MASK_WORDS];
+/* The max irq number this driver deals with is 128; see max_irqs */
+static DECLARE_BITMAP(ppc_lost_interrupts, 128);
+static DECLARE_BITMAP(ppc_cached_irq_mask, 128);
 static int pmac_irq_cascade = -1;
 static struct irq_domain *pmac_pic_host;
 
diff --git a/arch/powerpc/sysdev/cpm2_pic.c b/arch/powerpc/sysdev/cpm2_pic.c
index d3be961..10386b6 100644
--- a/arch/powerpc/sysdev/cpm2_pic.c
+++ b/arch/powerpc/sysdev/cpm2_pic.c
@@ -51,8 +51,7 @@
 static intctl_cpm2_t __iomem *cpm2_intctl;
 
 static struct irq_domain *cpm2_pic_host;
-#define NR_MASK_WORDS   ((NR_IRQS + 31) / 32)
-static unsigned long ppc_cached_irq_mask[NR_MASK_WORDS];
+static unsigned long ppc_cached_irq_mask[2]; /* 2 32-bit registers */
 
 static const u_char irq_to_siureg[] = {
 	1, 1, 1, 1, 1, 1, 1, 1,
diff --git a/arch/powerpc/sysdev/xics/xics-common.c b/arch/powerpc/sysdev/xics/xics-common.c
index 1d7067d..9049d9f 100644
--- a/arch/powerpc/sysdev/xics/xics-common.c
+++ b/arch/powerpc/sysdev/xics/xics-common.c
@@ -188,6 +188,7 @@ void xics_migrate_irqs_away(void)
 {
 	int cpu = smp_processor_id(), hw_cpu = hard_smp_processor_id();
 	unsigned int irq, virq;
+	struct irq_desc *desc;
 
 	/* If we used to be the default server, move to the new "boot_cpuid" */
 	if (hw_cpu == xics_default_server)
@@ -202,8 +203,7 @@ void xics_migrate_irqs_away(void)
 	/* Allow IPIs again... */
 	icp_ops->set_priority(DEFAULT_PRIORITY);
 
-	for_each_irq(virq) {
-		struct irq_desc *desc;
+	for_each_irq_desc(virq, desc) {
 		struct irq_chip *chip;
 		long server;
 		unsigned long flags;
@@ -212,9 +212,8 @@ void xics_migrate_irqs_away(void)
 		/* We can't set affinity on ISA interrupts */
 		if (virq < NUM_ISA_INTERRUPTS)
 			continue;
-		desc = irq_to_desc(virq);
 		/* We only need to migrate enabled IRQS */
-		if (!desc || !desc->action)
+		if (!desc->action)
 			continue;
 		if (desc->irq_data.domain != xics_host)
 			continue;
-- 
1.7.9.5


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] powerpc/8xx: Fix NR_IRQ bugs and refactor 8xx interrupt controller
  2012-04-16 20:13 [PATCH 1/2] powerpc/8xx: Fix NR_IRQ bugs and refactor 8xx interrupt controller Grant Likely
  2012-04-16 20:13 ` [PATCH 2/2] irqdomain/powerpc: Fix broken NR_IRQ references Grant Likely
@ 2012-04-16 20:13 ` Grant Likely
  2012-04-16 23:03   ` Benjamin Herrenschmidt
  2012-04-16 23:03 ` Benjamin Herrenschmidt
  2 siblings, 1 reply; 13+ messages in thread
From: Grant Likely @ 2012-04-16 20:13 UTC (permalink / raw)
  To: linux-kernel, linuxppc-dev; +Cc: Grant Likely, Benjamin Herrenschmidt

Ben, I can take these two patches via my irqdomain tree if you prefer.

g.

On Mon, Apr 16, 2012 at 2:13 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
> The mpc8xx driver uses a reference to NR_IRQS that is buggy.  It uses
> NR_IRQs for the array size of the ppc_cached_irq_mask bitmap, but
> NR_IRQs could be smaller than the number of hardware irqs that
> ppc_cached_irq_mask tracks.
>
> Also, while fixing that problem, it became apparent that the interrupt
> controller only supports 32 interrupt numbers, but it is written as if
> it supports multiple register banks which is more complicated.
>
> This patch pulls out the buggy reference to NR_IRQs and fixes the size
> of the ppc_cached_irq_mask to match the number of HW irqs.  It also
> drops the now-unnecessary code since ppc_cached_irq_mask is no longer
> an array.
>
> Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
>  arch/powerpc/sysdev/mpc8xx_pic.c |   61 +++++++++++++-------------------------
>  1 file changed, 21 insertions(+), 40 deletions(-)
>
> diff --git a/arch/powerpc/sysdev/mpc8xx_pic.c b/arch/powerpc/sysdev/mpc8xx_pic.c
> index d5f5416..91cade8 100644
> --- a/arch/powerpc/sysdev/mpc8xx_pic.c
> +++ b/arch/powerpc/sysdev/mpc8xx_pic.c
> @@ -18,69 +18,47 @@
>  extern int cpm_get_irq(struct pt_regs *regs);
>
>  static struct irq_domain *mpc8xx_pic_host;
> -#define NR_MASK_WORDS   ((NR_IRQS + 31) / 32)
> -static unsigned long ppc_cached_irq_mask[NR_MASK_WORDS];
> +static unsigned long ppc_cached_irq_mask;
>  static sysconf8xx_t __iomem *siu_reg;
>
>  int cpm_get_irq(struct pt_regs *regs);
>
> -static void mpc8xx_unmask_irq(struct irq_data *d)
> +static inline unsigned long mpc8xx_irqd_to_bit(struct irq_data *d)
>  {
> -       int     bit, word;
> -       unsigned int irq_nr = (unsigned int)irqd_to_hwirq(d);
> -
> -       bit = irq_nr & 0x1f;
> -       word = irq_nr >> 5;
> +       return 0x80000000 >> irqd_to_hwirq(d);
> +}
>
> -       ppc_cached_irq_mask[word] |= (1 << (31-bit));
> -       out_be32(&siu_reg->sc_simask, ppc_cached_irq_mask[word]);
> +static void mpc8xx_unmask_irq(struct irq_data *d)
> +{
> +       ppc_cached_irq_mask |= mpc8xx_irqd_to_bit(d);
> +       out_be32(&siu_reg->sc_simask, ppc_cached_irq_mask);
>  }
>
>  static void mpc8xx_mask_irq(struct irq_data *d)
>  {
> -       int     bit, word;
> -       unsigned int irq_nr = (unsigned int)irqd_to_hwirq(d);
> -
> -       bit = irq_nr & 0x1f;
> -       word = irq_nr >> 5;
> -
> -       ppc_cached_irq_mask[word] &= ~(1 << (31-bit));
> -       out_be32(&siu_reg->sc_simask, ppc_cached_irq_mask[word]);
> +       ppc_cached_irq_mask &= ~mpc8xx_irqd_to_bit(d);
> +       out_be32(&siu_reg->sc_simask, ppc_cached_irq_mask);
>  }
>
>  static void mpc8xx_ack(struct irq_data *d)
>  {
> -       int     bit;
> -       unsigned int irq_nr = (unsigned int)irqd_to_hwirq(d);
> -
> -       bit = irq_nr & 0x1f;
> -       out_be32(&siu_reg->sc_sipend, 1 << (31-bit));
> +       out_be32(&siu_reg->sc_sipend, mpc8xx_irqd_to_bit(d));
>  }
>
>  static void mpc8xx_end_irq(struct irq_data *d)
>  {
> -       int bit, word;
> -       unsigned int irq_nr = (unsigned int)irqd_to_hwirq(d);
> -
> -       bit = irq_nr & 0x1f;
> -       word = irq_nr >> 5;
> -
> -       ppc_cached_irq_mask[word] |= (1 << (31-bit));
> -       out_be32(&siu_reg->sc_simask, ppc_cached_irq_mask[word]);
> +       ppc_cached_irq_mask |= mpc8xx_irqd_to_bit(d);
> +       out_be32(&siu_reg->sc_simask, ppc_cached_irq_mask);
>  }
>
>  static int mpc8xx_set_irq_type(struct irq_data *d, unsigned int flow_type)
>  {
> -       if (flow_type & IRQ_TYPE_EDGE_FALLING) {
> -               irq_hw_number_t hw = (unsigned int)irqd_to_hwirq(d);
> +       /* only external IRQ senses are programmable */
> +       if ((flow_type & IRQ_TYPE_EDGE_FALLING) && !(irqd_to_hwirq(d) & 1)) {
>                unsigned int siel = in_be32(&siu_reg->sc_siel);
> -
> -               /* only external IRQ senses are programmable */
> -               if ((hw & 1) == 0) {
> -                       siel |= (0x80000000 >> hw);
> -                       out_be32(&siu_reg->sc_siel, siel);
> -                       __irq_set_handler_locked(d->irq, handle_edge_irq);
> -               }
> +               siel |= mpc8xx_irqd_to_bit(d);
> +               out_be32(&siu_reg->sc_siel, siel);
> +               __irq_set_handler_locked(d->irq, handle_edge_irq);
>        }
>        return 0;
>  }
> @@ -132,6 +110,9 @@ static int mpc8xx_pic_host_xlate(struct irq_domain *h, struct device_node *ct,
>                IRQ_TYPE_EDGE_FALLING,
>        };
>
> +       if (intspec[0] > 0x1f)
> +               return 0;
> +
>        *out_hwirq = intspec[0];
>        if (intsize > 1 && intspec[1] < 4)
>                *out_flags = map_pic_senses[intspec[1]];
> --
> 1.7.9.5
>



-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] powerpc/8xx: Fix NR_IRQ bugs and refactor 8xx interrupt controller
  2012-04-16 20:13 [PATCH 1/2] powerpc/8xx: Fix NR_IRQ bugs and refactor 8xx interrupt controller Grant Likely
  2012-04-16 20:13 ` [PATCH 2/2] irqdomain/powerpc: Fix broken NR_IRQ references Grant Likely
  2012-04-16 20:13 ` [PATCH 1/2] powerpc/8xx: Fix NR_IRQ bugs and refactor 8xx interrupt controller Grant Likely
@ 2012-04-16 23:03 ` Benjamin Herrenschmidt
  2012-04-16 23:23   ` Grant Likely
  2012-04-19 18:22   ` Grant Likely
  2 siblings, 2 replies; 13+ messages in thread
From: Benjamin Herrenschmidt @ 2012-04-16 23:03 UTC (permalink / raw)
  To: Grant Likely; +Cc: linux-kernel, linuxppc-dev

On Mon, 2012-04-16 at 14:13 -0600, Grant Likely wrote:
> The mpc8xx driver uses a reference to NR_IRQS that is buggy.  It uses
> NR_IRQs for the array size of the ppc_cached_irq_mask bitmap, but
> NR_IRQs could be smaller than the number of hardware irqs that
> ppc_cached_irq_mask tracks.
> 
> Also, while fixing that problem, it became apparent that the interrupt
> controller only supports 32 interrupt numbers, but it is written as if
> it supports multiple register banks which is more complicated.
> 
> This patch pulls out the buggy reference to NR_IRQs and fixes the size
> of the ppc_cached_irq_mask to match the number of HW irqs.  It also
> drops the now-unnecessary code since ppc_cached_irq_mask is no longer
> an array.

Can you rename ppc_cached_irq_mask while at it ? I think it was written
that way because it was all copy/pasted from powermac/pic.c which -does-
need it to be an array (and has the same bugs btw) :-) 

I wonder if we can get rid of the cached mask alltogether... I have the
feeling that we could just rely on the irq_desc fields nowadays for
that... this code is ancient.

Cheers,
Ben.

> Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
>  arch/powerpc/sysdev/mpc8xx_pic.c |   61 +++++++++++++-------------------------
>  1 file changed, 21 insertions(+), 40 deletions(-)
> 
> diff --git a/arch/powerpc/sysdev/mpc8xx_pic.c b/arch/powerpc/sysdev/mpc8xx_pic.c
> index d5f5416..91cade8 100644
> --- a/arch/powerpc/sysdev/mpc8xx_pic.c
> +++ b/arch/powerpc/sysdev/mpc8xx_pic.c
> @@ -18,69 +18,47 @@
>  extern int cpm_get_irq(struct pt_regs *regs);
>  
>  static struct irq_domain *mpc8xx_pic_host;
> -#define NR_MASK_WORDS   ((NR_IRQS + 31) / 32)
> -static unsigned long ppc_cached_irq_mask[NR_MASK_WORDS];
> +static unsigned long ppc_cached_irq_mask;
>  static sysconf8xx_t __iomem *siu_reg;
>  
>  int cpm_get_irq(struct pt_regs *regs);
>  
> -static void mpc8xx_unmask_irq(struct irq_data *d)
> +static inline unsigned long mpc8xx_irqd_to_bit(struct irq_data *d)
>  {
> -	int	bit, word;
> -	unsigned int irq_nr = (unsigned int)irqd_to_hwirq(d);
> -
> -	bit = irq_nr & 0x1f;
> -	word = irq_nr >> 5;
> +	return 0x80000000 >> irqd_to_hwirq(d);
> +}
>  
> -	ppc_cached_irq_mask[word] |= (1 << (31-bit));
> -	out_be32(&siu_reg->sc_simask, ppc_cached_irq_mask[word]);
> +static void mpc8xx_unmask_irq(struct irq_data *d)
> +{
> +	ppc_cached_irq_mask |= mpc8xx_irqd_to_bit(d);
> +	out_be32(&siu_reg->sc_simask, ppc_cached_irq_mask);
>  }
>  
>  static void mpc8xx_mask_irq(struct irq_data *d)
>  {
> -	int	bit, word;
> -	unsigned int irq_nr = (unsigned int)irqd_to_hwirq(d);
> -
> -	bit = irq_nr & 0x1f;
> -	word = irq_nr >> 5;
> -
> -	ppc_cached_irq_mask[word] &= ~(1 << (31-bit));
> -	out_be32(&siu_reg->sc_simask, ppc_cached_irq_mask[word]);
> +	ppc_cached_irq_mask &= ~mpc8xx_irqd_to_bit(d);
> +	out_be32(&siu_reg->sc_simask, ppc_cached_irq_mask);
>  }
>  
>  static void mpc8xx_ack(struct irq_data *d)
>  {
> -	int	bit;
> -	unsigned int irq_nr = (unsigned int)irqd_to_hwirq(d);
> -
> -	bit = irq_nr & 0x1f;
> -	out_be32(&siu_reg->sc_sipend, 1 << (31-bit));
> +	out_be32(&siu_reg->sc_sipend, mpc8xx_irqd_to_bit(d));
>  }
>  
>  static void mpc8xx_end_irq(struct irq_data *d)
>  {
> -	int bit, word;
> -	unsigned int irq_nr = (unsigned int)irqd_to_hwirq(d);
> -
> -	bit = irq_nr & 0x1f;
> -	word = irq_nr >> 5;
> -
> -	ppc_cached_irq_mask[word] |= (1 << (31-bit));
> -	out_be32(&siu_reg->sc_simask, ppc_cached_irq_mask[word]);
> +	ppc_cached_irq_mask |= mpc8xx_irqd_to_bit(d);
> +	out_be32(&siu_reg->sc_simask, ppc_cached_irq_mask);
>  }
>  
>  static int mpc8xx_set_irq_type(struct irq_data *d, unsigned int flow_type)
>  {
> -	if (flow_type & IRQ_TYPE_EDGE_FALLING) {
> -		irq_hw_number_t hw = (unsigned int)irqd_to_hwirq(d);
> +	/* only external IRQ senses are programmable */
> +	if ((flow_type & IRQ_TYPE_EDGE_FALLING) && !(irqd_to_hwirq(d) & 1)) {
>  		unsigned int siel = in_be32(&siu_reg->sc_siel);
> -
> -		/* only external IRQ senses are programmable */
> -		if ((hw & 1) == 0) {
> -			siel |= (0x80000000 >> hw);
> -			out_be32(&siu_reg->sc_siel, siel);
> -			__irq_set_handler_locked(d->irq, handle_edge_irq);
> -		}
> +		siel |= mpc8xx_irqd_to_bit(d);
> +		out_be32(&siu_reg->sc_siel, siel);
> +		__irq_set_handler_locked(d->irq, handle_edge_irq);
>  	}
>  	return 0;
>  }
> @@ -132,6 +110,9 @@ static int mpc8xx_pic_host_xlate(struct irq_domain *h, struct device_node *ct,
>  		IRQ_TYPE_EDGE_FALLING,
>  	};
>  
> +	if (intspec[0] > 0x1f)
> +		return 0;
> +
>  	*out_hwirq = intspec[0];
>  	if (intsize > 1 && intspec[1] < 4)
>  		*out_flags = map_pic_senses[intspec[1]];



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] powerpc/8xx: Fix NR_IRQ bugs and refactor 8xx interrupt controller
  2012-04-16 20:13 ` [PATCH 1/2] powerpc/8xx: Fix NR_IRQ bugs and refactor 8xx interrupt controller Grant Likely
@ 2012-04-16 23:03   ` Benjamin Herrenschmidt
  2012-04-16 23:21     ` Grant Likely
  2012-04-17  0:03     ` Joakim Tjernlund
  0 siblings, 2 replies; 13+ messages in thread
From: Benjamin Herrenschmidt @ 2012-04-16 23:03 UTC (permalink / raw)
  To: Grant Likely; +Cc: linux-kernel, linuxppc-dev

On Mon, 2012-04-16 at 14:13 -0600, Grant Likely wrote:
> Ben, I can take these two patches via my irqdomain tree if you prefer.

Let me give them a test run first.

Cheers,
Ben.

> g.
> 
> On Mon, Apr 16, 2012 at 2:13 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
> > The mpc8xx driver uses a reference to NR_IRQS that is buggy.  It uses
> > NR_IRQs for the array size of the ppc_cached_irq_mask bitmap, but
> > NR_IRQs could be smaller than the number of hardware irqs that
> > ppc_cached_irq_mask tracks.
> >
> > Also, while fixing that problem, it became apparent that the interrupt
> > controller only supports 32 interrupt numbers, but it is written as if
> > it supports multiple register banks which is more complicated.
> >
> > This patch pulls out the buggy reference to NR_IRQs and fixes the size
> > of the ppc_cached_irq_mask to match the number of HW irqs.  It also
> > drops the now-unnecessary code since ppc_cached_irq_mask is no longer
> > an array.
> >
> > Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
> > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > ---
> >  arch/powerpc/sysdev/mpc8xx_pic.c |   61 +++++++++++++-------------------------
> >  1 file changed, 21 insertions(+), 40 deletions(-)
> >
> > diff --git a/arch/powerpc/sysdev/mpc8xx_pic.c b/arch/powerpc/sysdev/mpc8xx_pic.c
> > index d5f5416..91cade8 100644
> > --- a/arch/powerpc/sysdev/mpc8xx_pic.c
> > +++ b/arch/powerpc/sysdev/mpc8xx_pic.c
> > @@ -18,69 +18,47 @@
> >  extern int cpm_get_irq(struct pt_regs *regs);
> >
> >  static struct irq_domain *mpc8xx_pic_host;
> > -#define NR_MASK_WORDS   ((NR_IRQS + 31) / 32)
> > -static unsigned long ppc_cached_irq_mask[NR_MASK_WORDS];
> > +static unsigned long ppc_cached_irq_mask;
> >  static sysconf8xx_t __iomem *siu_reg;
> >
> >  int cpm_get_irq(struct pt_regs *regs);
> >
> > -static void mpc8xx_unmask_irq(struct irq_data *d)
> > +static inline unsigned long mpc8xx_irqd_to_bit(struct irq_data *d)
> >  {
> > -       int     bit, word;
> > -       unsigned int irq_nr = (unsigned int)irqd_to_hwirq(d);
> > -
> > -       bit = irq_nr & 0x1f;
> > -       word = irq_nr >> 5;
> > +       return 0x80000000 >> irqd_to_hwirq(d);
> > +}
> >
> > -       ppc_cached_irq_mask[word] |= (1 << (31-bit));
> > -       out_be32(&siu_reg->sc_simask, ppc_cached_irq_mask[word]);
> > +static void mpc8xx_unmask_irq(struct irq_data *d)
> > +{
> > +       ppc_cached_irq_mask |= mpc8xx_irqd_to_bit(d);
> > +       out_be32(&siu_reg->sc_simask, ppc_cached_irq_mask);
> >  }
> >
> >  static void mpc8xx_mask_irq(struct irq_data *d)
> >  {
> > -       int     bit, word;
> > -       unsigned int irq_nr = (unsigned int)irqd_to_hwirq(d);
> > -
> > -       bit = irq_nr & 0x1f;
> > -       word = irq_nr >> 5;
> > -
> > -       ppc_cached_irq_mask[word] &= ~(1 << (31-bit));
> > -       out_be32(&siu_reg->sc_simask, ppc_cached_irq_mask[word]);
> > +       ppc_cached_irq_mask &= ~mpc8xx_irqd_to_bit(d);
> > +       out_be32(&siu_reg->sc_simask, ppc_cached_irq_mask);
> >  }
> >
> >  static void mpc8xx_ack(struct irq_data *d)
> >  {
> > -       int     bit;
> > -       unsigned int irq_nr = (unsigned int)irqd_to_hwirq(d);
> > -
> > -       bit = irq_nr & 0x1f;
> > -       out_be32(&siu_reg->sc_sipend, 1 << (31-bit));
> > +       out_be32(&siu_reg->sc_sipend, mpc8xx_irqd_to_bit(d));
> >  }
> >
> >  static void mpc8xx_end_irq(struct irq_data *d)
> >  {
> > -       int bit, word;
> > -       unsigned int irq_nr = (unsigned int)irqd_to_hwirq(d);
> > -
> > -       bit = irq_nr & 0x1f;
> > -       word = irq_nr >> 5;
> > -
> > -       ppc_cached_irq_mask[word] |= (1 << (31-bit));
> > -       out_be32(&siu_reg->sc_simask, ppc_cached_irq_mask[word]);
> > +       ppc_cached_irq_mask |= mpc8xx_irqd_to_bit(d);
> > +       out_be32(&siu_reg->sc_simask, ppc_cached_irq_mask);
> >  }
> >
> >  static int mpc8xx_set_irq_type(struct irq_data *d, unsigned int flow_type)
> >  {
> > -       if (flow_type & IRQ_TYPE_EDGE_FALLING) {
> > -               irq_hw_number_t hw = (unsigned int)irqd_to_hwirq(d);
> > +       /* only external IRQ senses are programmable */
> > +       if ((flow_type & IRQ_TYPE_EDGE_FALLING) && !(irqd_to_hwirq(d) & 1)) {
> >                unsigned int siel = in_be32(&siu_reg->sc_siel);
> > -
> > -               /* only external IRQ senses are programmable */
> > -               if ((hw & 1) == 0) {
> > -                       siel |= (0x80000000 >> hw);
> > -                       out_be32(&siu_reg->sc_siel, siel);
> > -                       __irq_set_handler_locked(d->irq, handle_edge_irq);
> > -               }
> > +               siel |= mpc8xx_irqd_to_bit(d);
> > +               out_be32(&siu_reg->sc_siel, siel);
> > +               __irq_set_handler_locked(d->irq, handle_edge_irq);
> >        }
> >        return 0;
> >  }
> > @@ -132,6 +110,9 @@ static int mpc8xx_pic_host_xlate(struct irq_domain *h, struct device_node *ct,
> >                IRQ_TYPE_EDGE_FALLING,
> >        };
> >
> > +       if (intspec[0] > 0x1f)
> > +               return 0;
> > +
> >        *out_hwirq = intspec[0];
> >        if (intsize > 1 && intspec[1] < 4)
> >                *out_flags = map_pic_senses[intspec[1]];
> > --
> > 1.7.9.5
> >
> 
> 
> 



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] powerpc/8xx: Fix NR_IRQ bugs and refactor 8xx interrupt controller
  2012-04-16 23:03   ` Benjamin Herrenschmidt
@ 2012-04-16 23:21     ` Grant Likely
  2012-04-17  0:03     ` Joakim Tjernlund
  1 sibling, 0 replies; 13+ messages in thread
From: Grant Likely @ 2012-04-16 23:21 UTC (permalink / raw)
  To: Benjamin Herrenschmidt; +Cc: linux-kernel, linuxppc-dev

On Mon, Apr 16, 2012 at 5:03 PM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> On Mon, 2012-04-16 at 14:13 -0600, Grant Likely wrote:
>> Ben, I can take these two patches via my irqdomain tree if you prefer.
>
> Let me give them a test run first.

Yes of course.  I won't merge before they've been checked out.

g.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] powerpc/8xx: Fix NR_IRQ bugs and refactor 8xx interrupt controller
  2012-04-16 23:03 ` Benjamin Herrenschmidt
@ 2012-04-16 23:23   ` Grant Likely
  2012-04-19 18:22   ` Grant Likely
  1 sibling, 0 replies; 13+ messages in thread
From: Grant Likely @ 2012-04-16 23:23 UTC (permalink / raw)
  To: Benjamin Herrenschmidt; +Cc: linux-kernel, linuxppc-dev

On Mon, Apr 16, 2012 at 5:03 PM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> On Mon, 2012-04-16 at 14:13 -0600, Grant Likely wrote:
>> The mpc8xx driver uses a reference to NR_IRQS that is buggy.  It uses
>> NR_IRQs for the array size of the ppc_cached_irq_mask bitmap, but
>> NR_IRQs could be smaller than the number of hardware irqs that
>> ppc_cached_irq_mask tracks.
>>
>> Also, while fixing that problem, it became apparent that the interrupt
>> controller only supports 32 interrupt numbers, but it is written as if
>> it supports multiple register banks which is more complicated.
>>
>> This patch pulls out the buggy reference to NR_IRQs and fixes the size
>> of the ppc_cached_irq_mask to match the number of HW irqs.  It also
>> drops the now-unnecessary code since ppc_cached_irq_mask is no longer
>> an array.
>
> Can you rename ppc_cached_irq_mask while at it ? I think it was written
> that way because it was all copy/pasted from powermac/pic.c which -does-
> need it to be an array (and has the same bugs btw) :-)
>
> I wonder if we can get rid of the cached mask alltogether... I have the
> feeling that we could just rely on the irq_desc fields nowadays for
> that... this code is ancient.

The irq_desc fields aren't ideal because they are per-irq and would
need to be &'ed together for each mask/unmask/ack operation.  We could
store something in the irq_domain I suppose, but I'm not convinced it
is worth it for an interrupt controller that will only ever have one
instance in the system.  If I were to refactor any deeper than I have,
then I'm move this driver over to use irq_generic_chip instead.

g.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] powerpc/8xx: Fix NR_IRQ bugs and refactor 8xx interrupt controller
  2012-04-16 23:03   ` Benjamin Herrenschmidt
  2012-04-16 23:21     ` Grant Likely
@ 2012-04-17  0:03     ` Joakim Tjernlund
  2012-04-17  1:00       ` Benjamin Herrenschmidt
  1 sibling, 1 reply; 13+ messages in thread
From: Joakim Tjernlund @ 2012-04-17  0:03 UTC (permalink / raw)
  To: Benjamin Herrenschmidt; +Cc: Grant Likely, linux-kernel, linuxppc-dev

>
> On Mon, 2012-04-16 at 14:13 -0600, Grant Likely wrote:
> > Ben, I can take these two patches via my irqdomain tree if you prefer.
>
> Let me give them a test run first.

Hi Ben

If you are going to test 8xx, could you revert e0908085fc2391c85b85fb814ae1df377c8e0dcb,
powerpc/8xx: Fix regression introduced by cache coherency rewrite ?

It should not be needed after I fixed up the 8xx TLB code and added a workaround
for buggy dcbX instructions.

 Jocke


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] powerpc/8xx: Fix NR_IRQ bugs and refactor 8xx interrupt controller
  2012-04-17  0:03     ` Joakim Tjernlund
@ 2012-04-17  1:00       ` Benjamin Herrenschmidt
  2012-04-17  5:33         ` Joakim Tjernlund
  0 siblings, 1 reply; 13+ messages in thread
From: Benjamin Herrenschmidt @ 2012-04-17  1:00 UTC (permalink / raw)
  To: Joakim Tjernlund; +Cc: Grant Likely, linux-kernel, linuxppc-dev

On Tue, 2012-04-17 at 02:03 +0200, Joakim Tjernlund wrote:
> 
> If you are going to test 8xx, could you revert
> e0908085fc2391c85b85fb814ae1df377c8e0dcb,
> powerpc/8xx: Fix regression introduced by cache coherency rewrite ?
> 
> It should not be needed after I fixed up the 8xx TLB code and added a
> workaround
> for buggy dcbX instructions. 

Have you actually verified that things still work after reverting it ?

Cheers,
Ben.



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] powerpc/8xx: Fix NR_IRQ bugs and refactor 8xx interrupt controller
  2012-04-17  1:00       ` Benjamin Herrenschmidt
@ 2012-04-17  5:33         ` Joakim Tjernlund
  0 siblings, 0 replies; 13+ messages in thread
From: Joakim Tjernlund @ 2012-04-17  5:33 UTC (permalink / raw)
  To: Benjamin Herrenschmidt; +Cc: Grant Likely, linux-kernel, linuxppc-dev

Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote on 2012/04/17 03:00:40:
>
> On Tue, 2012-04-17 at 02:03 +0200, Joakim Tjernlund wrote:
> >
> > If you are going to test 8xx, could you revert
> > e0908085fc2391c85b85fb814ae1df377c8e0dcb,
> > powerpc/8xx: Fix regression introduced by cache coherency rewrite ?
> >
> > It should not be needed after I fixed up the 8xx TLB code and added a
> > workaround
> > for buggy dcbX instructions.
>
> Have you actually verified that things still work after reverting it ?

Nope, I haven't moved our 8xx to 3.x. We are still on
2.4 and it works there.

 Jocke


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] irqdomain/powerpc: Fix broken NR_IRQ references
  2012-04-16 20:13 ` [PATCH 2/2] irqdomain/powerpc: Fix broken NR_IRQ references Grant Likely
@ 2012-04-19  0:45   ` Michael Ellerman
  2012-04-19 18:21     ` Grant Likely
  0 siblings, 1 reply; 13+ messages in thread
From: Michael Ellerman @ 2012-04-19  0:45 UTC (permalink / raw)
  To: Grant Likely; +Cc: linux-kernel, linuxppc-dev

On Mon, 2012-04-16 at 14:13 -0600, Grant Likely wrote:

> diff --git a/arch/powerpc/platforms/cell/axon_msi.c b/arch/powerpc/platforms/cell/axon_msi.c
> index d09f3e8..fc9df1a 100644
> --- a/arch/powerpc/platforms/cell/axon_msi.c
> +++ b/arch/powerpc/platforms/cell/axon_msi.c
> @@ -276,9 +276,6 @@ static int axon_msi_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
>  	if (rc)
>  		return rc;
>  
> -	/* We rely on being able to stash a virq in a u16 */

Would be nice to move the comment to the hunk below rather than just
deleting it. Otherwise the 65536 looks a bit random.

> -	BUILD_BUG_ON(NR_IRQS > 65536);
> -
>  	list_for_each_entry(entry, &dev->msi_list, list) {
>  		virq = irq_create_direct_mapping(msic->irq_domain);
>  		if (virq == NO_IRQ) {
> @@ -392,7 +389,7 @@ static int axon_msi_probe(struct platform_device *device)
>  	}
>  	memset(msic->fifo_virt, 0xff, MSIC_FIFO_SIZE_BYTES);
>  
> -	msic->irq_domain = irq_domain_add_nomap(dn, 0, &msic_host_ops, msic);
> +	msic->irq_domain = irq_domain_add_nomap(dn, 65536, &msic_host_ops, msic);
>  	if (!msic->irq_domain) {
>  		printk(KERN_ERR "axon_msi: couldn't allocate irq_domain for %s\n",
>  		       dn->full_name);

cheers


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] irqdomain/powerpc: Fix broken NR_IRQ references
  2012-04-19  0:45   ` Michael Ellerman
@ 2012-04-19 18:21     ` Grant Likely
  0 siblings, 0 replies; 13+ messages in thread
From: Grant Likely @ 2012-04-19 18:21 UTC (permalink / raw)
  To: Michael Ellerman; +Cc: linux-kernel, linuxppc-dev

On Thu, 19 Apr 2012 10:45:01 +1000, Michael Ellerman <michael@ellerman.id.au> wrote:
> On Mon, 2012-04-16 at 14:13 -0600, Grant Likely wrote:
> 
> > diff --git a/arch/powerpc/platforms/cell/axon_msi.c b/arch/powerpc/platforms/cell/axon_msi.c
> > index d09f3e8..fc9df1a 100644
> > --- a/arch/powerpc/platforms/cell/axon_msi.c
> > +++ b/arch/powerpc/platforms/cell/axon_msi.c
> > @@ -276,9 +276,6 @@ static int axon_msi_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
> >  	if (rc)
> >  		return rc;
> >  
> > -	/* We rely on being able to stash a virq in a u16 */
> 
> Would be nice to move the comment to the hunk below rather than just
> deleting it. Otherwise the 65536 looks a bit random.

Done.

g.


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] powerpc/8xx: Fix NR_IRQ bugs and refactor 8xx interrupt controller
  2012-04-16 23:03 ` Benjamin Herrenschmidt
  2012-04-16 23:23   ` Grant Likely
@ 2012-04-19 18:22   ` Grant Likely
  1 sibling, 0 replies; 13+ messages in thread
From: Grant Likely @ 2012-04-19 18:22 UTC (permalink / raw)
  To: Benjamin Herrenschmidt; +Cc: linux-kernel, linuxppc-dev

On Tue, 17 Apr 2012 09:03:02 +1000, Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
> On Mon, 2012-04-16 at 14:13 -0600, Grant Likely wrote:
> > The mpc8xx driver uses a reference to NR_IRQS that is buggy.  It uses
> > NR_IRQs for the array size of the ppc_cached_irq_mask bitmap, but
> > NR_IRQs could be smaller than the number of hardware irqs that
> > ppc_cached_irq_mask tracks.
> > 
> > Also, while fixing that problem, it became apparent that the interrupt
> > controller only supports 32 interrupt numbers, but it is written as if
> > it supports multiple register banks which is more complicated.
> > 
> > This patch pulls out the buggy reference to NR_IRQs and fixes the size
> > of the ppc_cached_irq_mask to match the number of HW irqs.  It also
> > drops the now-unnecessary code since ppc_cached_irq_mask is no longer
> > an array.
> 
> Can you rename ppc_cached_irq_mask while at it ? I think it was written
> that way because it was all copy/pasted from powermac/pic.c which -does-
> need it to be an array (and has the same bugs btw) :-) 

Done; and I think I fixed the pic.c bugs in patch 2.

g.


^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2012-04-19 18:22 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-04-16 20:13 [PATCH 1/2] powerpc/8xx: Fix NR_IRQ bugs and refactor 8xx interrupt controller Grant Likely
2012-04-16 20:13 ` [PATCH 2/2] irqdomain/powerpc: Fix broken NR_IRQ references Grant Likely
2012-04-19  0:45   ` Michael Ellerman
2012-04-19 18:21     ` Grant Likely
2012-04-16 20:13 ` [PATCH 1/2] powerpc/8xx: Fix NR_IRQ bugs and refactor 8xx interrupt controller Grant Likely
2012-04-16 23:03   ` Benjamin Herrenschmidt
2012-04-16 23:21     ` Grant Likely
2012-04-17  0:03     ` Joakim Tjernlund
2012-04-17  1:00       ` Benjamin Herrenschmidt
2012-04-17  5:33         ` Joakim Tjernlund
2012-04-16 23:03 ` Benjamin Herrenschmidt
2012-04-16 23:23   ` Grant Likely
2012-04-19 18:22   ` Grant Likely

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).