* [PATCH] irqchip: sun4i: Fix irq 0 not working @ 2014-03-11 15:51 Hans de Goede 2014-03-12 10:09 ` Maxime Ripard 0 siblings, 1 reply; 3+ messages in thread From: Hans de Goede @ 2014-03-11 15:51 UTC (permalink / raw) To: linux-arm-kernel SUN4I_IRQ_VECTOR_REG containing 0 can mean one of 2 things: 1) irq 0 pending 2) no more irqs pending So we must loop always atleast once to make irq 0 work, otherwise irq 0 will never get serviced and we end up with a hard hang because sun4i_handle_irq gets re-entered constantly. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/irqchip/irq-sun4i.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/drivers/irqchip/irq-sun4i.c b/drivers/irqchip/irq-sun4i.c index a5438d8..3761bf1 100644 --- a/drivers/irqchip/irq-sun4i.c +++ b/drivers/irqchip/irq-sun4i.c @@ -140,10 +140,16 @@ static asmlinkage void __exception_irq_entry sun4i_handle_irq(struct pt_regs *re { u32 irq, hwirq; + /* + * hwirq == 0 can mean one of 2 things: + * 1) irq 0 pending + * 2) no more irqs pending + * So loop always atleast once to make irq 0 work. + */ hwirq = readl(sun4i_irq_base + SUN4I_IRQ_VECTOR_REG) >> 2; - while (hwirq != 0) { + do { irq = irq_find_mapping(sun4i_irq_domain, hwirq); handle_IRQ(irq, regs); hwirq = readl(sun4i_irq_base + SUN4I_IRQ_VECTOR_REG) >> 2; - } + } while (hwirq != 0); } -- 1.9.0 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* [PATCH] irqchip: sun4i: Fix irq 0 not working 2014-03-11 15:51 [PATCH] irqchip: sun4i: Fix irq 0 not working Hans de Goede @ 2014-03-12 10:09 ` Maxime Ripard 2014-03-12 13:45 ` Hans de Goede 0 siblings, 1 reply; 3+ messages in thread From: Maxime Ripard @ 2014-03-12 10:09 UTC (permalink / raw) To: linux-arm-kernel Hi, On Tue, Mar 11, 2014 at 04:51:00PM +0100, Hans de Goede wrote: > SUN4I_IRQ_VECTOR_REG containing 0 can mean one of 2 things: > 1) irq 0 pending > 2) no more irqs pending > > So we must loop always atleast once to make irq 0 work, otherwise irq 0 > will never get serviced and we end up with a hard hang because > sun4i_handle_irq gets re-entered constantly. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/irqchip/irq-sun4i.c | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/drivers/irqchip/irq-sun4i.c b/drivers/irqchip/irq-sun4i.c > index a5438d8..3761bf1 100644 > --- a/drivers/irqchip/irq-sun4i.c > +++ b/drivers/irqchip/irq-sun4i.c > @@ -140,10 +140,16 @@ static asmlinkage void __exception_irq_entry sun4i_handle_irq(struct pt_regs *re > { > u32 irq, hwirq; > > + /* > + * hwirq == 0 can mean one of 2 things: > + * 1) irq 0 pending > + * 2) no more irqs pending 3) spurious interrupt. > + * So loop always atleast once to make irq 0 work. > + */ > hwirq = readl(sun4i_irq_base + SUN4I_IRQ_VECTOR_REG) >> 2; > - while (hwirq != 0) { > + do { I'd at least lookup in the pending register to see if the interrupt 0 was actually triggered. Otherwise, you could end up with spurious handler calls on the interrupt 0. > irq = irq_find_mapping(sun4i_irq_domain, hwirq); > handle_IRQ(irq, regs); > hwirq = readl(sun4i_irq_base + SUN4I_IRQ_VECTOR_REG) >> 2; And you end up with the same issue if there's a first != 0 interrupt, and then the interrupt 0. What about something like: while (1) { hwirq = readl(sun4i_irq_base + SUN4I_IRQ_VECTOR_REG) >> 2; if (!hwirq) if (!(readl(sun4i_irq_base + SUN4I_IRQ_PENDING_REG(0)) & BIT(0))) break; irq = irq_find_mapping(sun4i_irq_domain, hwirq); handle_IRQ(irq, regs); } -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 836 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140312/7d937b9c/attachment.sig> ^ permalink raw reply [flat|nested] 3+ messages in thread
* [PATCH] irqchip: sun4i: Fix irq 0 not working 2014-03-12 10:09 ` Maxime Ripard @ 2014-03-12 13:45 ` Hans de Goede 0 siblings, 0 replies; 3+ messages in thread From: Hans de Goede @ 2014-03-12 13:45 UTC (permalink / raw) To: linux-arm-kernel Hi, On 03/12/2014 11:09 AM, Maxime Ripard wrote: > Hi, > > On Tue, Mar 11, 2014 at 04:51:00PM +0100, Hans de Goede wrote: >> SUN4I_IRQ_VECTOR_REG containing 0 can mean one of 2 things: >> 1) irq 0 pending >> 2) no more irqs pending >> >> So we must loop always atleast once to make irq 0 work, otherwise irq 0 >> will never get serviced and we end up with a hard hang because >> sun4i_handle_irq gets re-entered constantly. >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> drivers/irqchip/irq-sun4i.c | 10 ++++++++-- >> 1 file changed, 8 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/irqchip/irq-sun4i.c b/drivers/irqchip/irq-sun4i.c >> index a5438d8..3761bf1 100644 >> --- a/drivers/irqchip/irq-sun4i.c >> +++ b/drivers/irqchip/irq-sun4i.c >> @@ -140,10 +140,16 @@ static asmlinkage void __exception_irq_entry sun4i_handle_irq(struct pt_regs *re >> { >> u32 irq, hwirq; >> >> + /* >> + * hwirq == 0 can mean one of 2 things: >> + * 1) irq 0 pending >> + * 2) no more irqs pending > > 3) spurious interrupt. > >> + * So loop always atleast once to make irq 0 work. >> + */ >> hwirq = readl(sun4i_irq_base + SUN4I_IRQ_VECTOR_REG) >> 2; >> - while (hwirq != 0) { >> + do { > > I'd at least lookup in the pending register to see if the interrupt 0 > was actually triggered. Otherwise, you could end up with spurious > handler calls on the interrupt 0. Yes, I was already worrying about this myself after sending the patch, and considered reading pending too. > >> irq = irq_find_mapping(sun4i_irq_domain, hwirq); >> handle_IRQ(irq, regs); >> hwirq = readl(sun4i_irq_base + SUN4I_IRQ_VECTOR_REG) >> 2; > > And you end up with the same issue if there's a first != 0 interrupt, > and then the interrupt 0. No, before my fix sun4i_handle_irq would be called continuously since we were never handling irq 0, so if this happens we will simply drop out of sun4i_handle_irq only to immediately get recalled, this does make this scenario more expensive, but things will still work, while it saves an also not cheap read from SUN4I_IRQ_PENDING_REG(0) for each regular interrupt. Note I agree the spurious irq case is an issue, as said that has me worried too. > What about something like: > > while (1) { > hwirq = readl(sun4i_irq_base + SUN4I_IRQ_VECTOR_REG) >> 2; > if (!hwirq) > if (!(readl(sun4i_irq_base + SUN4I_IRQ_PENDING_REG(0)) & BIT(0))) > break; > > irq = irq_find_mapping(sun4i_irq_domain, hwirq); > handle_IRQ(irq, regs); > } Yes that should work nicely, but for the straight path it means reading pending once for each interrupt. I agree we need to read pending before calling handle_IRQ for irq 0, but we only need to do so if the first read from SUN4I_IRQ_VECTOR_REG == 0, on any subsequent reads from SUN4I_IRQ_VECTOR_REG returning 0 we can exit immediately, in the worst case we'll get called again, and then do the right thing. IE something like this: hwirq = readl(sun4i_irq_base + SUN4I_IRQ_VECTOR_REG) >> 2; /* Ensure hwirq == 0 is because of irq 0 pending */ if (hwirq == 0 && !(readl(sun4i_irq_base + SUN4I_IRQ_PENDING_REG(0)) & BIT(0))) return; do { irq = irq_find_mapping(sun4i_irq_domain, hwirq); handle_IRQ(irq, regs); hwirq = readl(sun4i_irq_base + SUN4I_IRQ_VECTOR_REG) >> 2; } while (hwirq); Note untested, and this might be unnecessary optimization. So let me know which version you prefer and I'll give it a test run. Regards, Hans ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2014-03-12 13:45 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-03-11 15:51 [PATCH] irqchip: sun4i: Fix irq 0 not working Hans de Goede 2014-03-12 10:09 ` Maxime Ripard 2014-03-12 13:45 ` Hans de Goede
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).