From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate.crashing.org (gate.crashing.org [63.228.1.57]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id 76EBFB6EEB for ; Tue, 24 Apr 2012 13:13:22 +1000 (EST) Message-ID: <1335237195.24897.0.camel@pasglop> Subject: Re: [PATCH v2 1/2] powerpc/8xx: Fix NR_IRQ bugs and refactor 8xx interrupt controller From: Benjamin Herrenschmidt To: Grant Likely Date: Tue, 24 Apr 2012 13:13:15 +1000 In-Reply-To: <1335220202-32533-2-git-send-email-grant.likely@secretlab.ca> References: <1335220202-32533-1-git-send-email-grant.likely@secretlab.ca> <1335220202-32533-2-git-send-email-grant.likely@secretlab.ca> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Cc: linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , On Mon, 2012-04-23 at 16:30 -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. Joakim, any chance you can test this ASAP ? :-) Thanks ! Cheers, Ben. > 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. > > v2: - Rename ppc_cached_irq_mask to mpc8xx_cached_irq_mask > - Drop duplicate forward declaration > > Signed-off-by: Grant Likely > Cc: Benjamin Herrenschmidt > --- > arch/powerpc/sysdev/mpc8xx_pic.c | 61 +++++++++++++------------------------- > 1 file changed, 20 insertions(+), 41 deletions(-) > > diff --git a/arch/powerpc/sysdev/mpc8xx_pic.c b/arch/powerpc/sysdev/mpc8xx_pic.c > index d5f5416..b724622 100644 > --- a/arch/powerpc/sysdev/mpc8xx_pic.c > +++ b/arch/powerpc/sysdev/mpc8xx_pic.c > @@ -18,69 +18,45 @@ > 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 mpc8xx_cached_irq_mask; > static sysconf8xx_t __iomem *siu_reg; > > -int cpm_get_irq(struct pt_regs *regs); > +static inline unsigned long mpc8xx_irqd_to_bit(struct irq_data *d) > +{ > + return 0x80000000 >> irqd_to_hwirq(d); > +} > > static void mpc8xx_unmask_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]); > + mpc8xx_cached_irq_mask |= mpc8xx_irqd_to_bit(d); > + out_be32(&siu_reg->sc_simask, mpc8xx_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]); > + mpc8xx_cached_irq_mask &= ~mpc8xx_irqd_to_bit(d); > + out_be32(&siu_reg->sc_simask, mpc8xx_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]); > + mpc8xx_cached_irq_mask |= mpc8xx_irqd_to_bit(d); > + out_be32(&siu_reg->sc_simask, mpc8xx_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 +108,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]];