* [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 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 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 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: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 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 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: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