* [PATCH] irqchip: sun4i: Fix irq 0 not working
@ 2014-03-11 15:51 ` Hans de Goede
0 siblings, 0 replies; 6+ messages in thread
From: Hans de Goede @ 2014-03-11 15:51 UTC (permalink / raw)
To: Thomas Gleixner, Maxime Ripard
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, devicetree,
linux-sunxi-/JYPxA39Uh5TLH3MbocFFw, Hans de Goede
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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
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] 6+ messages in thread
* [PATCH] irqchip: sun4i: Fix irq 0 not working
@ 2014-03-11 15:51 ` Hans de Goede
0 siblings, 0 replies; 6+ 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] 6+ messages in thread
* Re: [PATCH] irqchip: sun4i: Fix irq 0 not working
2014-03-11 15:51 ` Hans de Goede
@ 2014-03-12 10:09 ` Maxime Ripard
-1 siblings, 0 replies; 6+ messages in thread
From: Maxime Ripard @ 2014-03-12 10:09 UTC (permalink / raw)
To: Hans de Goede
Cc: Thomas Gleixner,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, devicetree,
linux-sunxi-/JYPxA39Uh5TLH3MbocFFw
[-- Attachment #1: Type: text/plain, Size: 2014 bytes --]
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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
> 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
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] irqchip: sun4i: Fix irq 0 not working
@ 2014-03-12 10:09 ` Maxime Ripard
0 siblings, 0 replies; 6+ 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] 6+ messages in thread
* Re: [PATCH] irqchip: sun4i: Fix irq 0 not working
2014-03-12 10:09 ` Maxime Ripard
@ 2014-03-12 13:45 ` Hans de Goede
-1 siblings, 0 replies; 6+ messages in thread
From: Hans de Goede @ 2014-03-12 13:45 UTC (permalink / raw)
To: Maxime Ripard
Cc: Thomas Gleixner,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, devicetree,
linux-sunxi-/JYPxA39Uh5TLH3MbocFFw
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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> ---
>> 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] 6+ messages in thread
* [PATCH] irqchip: sun4i: Fix irq 0 not working
@ 2014-03-12 13:45 ` Hans de Goede
0 siblings, 0 replies; 6+ 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] 6+ messages in thread
end of thread, other threads:[~2014-03-12 13:45 UTC | newest]
Thread overview: 6+ 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-11 15:51 ` Hans de Goede
[not found] ` <1394553060-30298-1-git-send-email-hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2014-03-12 10:09 ` Maxime Ripard
2014-03-12 10:09 ` Maxime Ripard
2014-03-12 13:45 ` Hans de Goede
2014-03-12 13:45 ` Hans de Goede
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.