[PATCH] irqchip/versatile-fpga: Handle chained IRQs properly

[PATCH] irqchip/versatile-fpga: Handle chained IRQs properly

[Sungbo Eo]

Enclose the chained handler with chained_irq_{enter,exit}(), so that the
muxed interrupts get properly acked.

This patch also fixes a reboot bug on OX820 SoC, where the jiffies timer
interrupt is never acked. The kernel waits a clock tick forever in
calibrate_delay_converge(), which leads to a boot hang.

Signed-off-by: Sungbo Eo <mans0n@gorani.run>
Cc: Neil Armstrong <narmstrong@baylibre.com>
---
 drivers/irqchip/irq-versatile-fpga.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/irqchip/irq-versatile-fpga.c b/drivers/irqchip/irq-versatile-fpga.c
index 928858dada75..08faab2fec3e 100644
--- a/drivers/irqchip/irq-versatile-fpga.c
+++ b/drivers/irqchip/irq-versatile-fpga.c
@@ -6,6 +6,7 @@
 #include <linux/irq.h>
 #include <linux/io.h>
 #include <linux/irqchip.h>
+#include <linux/irqchip/chained_irq.h>
 #include <linux/irqchip/versatile-fpga.h>
 #include <linux/irqdomain.h>
 #include <linux/module.h>
@@ -68,12 +69,15 @@ static void fpga_irq_unmask(struct irq_data *d)
 
 static void fpga_irq_handle(struct irq_desc *desc)
 {
+	struct irq_chip *chip = irq_desc_get_chip(desc);
 	struct fpga_irq_data *f = irq_desc_get_handler_data(desc);
 	u32 status = readl(f->base + IRQ_STATUS);
 
+	chained_irq_enter(chip, desc);
+
 	if (status == 0) {
 		do_bad_IRQ(desc);
-		return;
+		goto out;
 	}
 
 	do {
@@ -82,6 +86,9 @@ static void fpga_irq_handle(struct irq_desc *desc)
 		status &= ~(1 << irq);
 		generic_handle_irq(irq_find_mapping(f->domain, irq));
 	} while (status);
+
+out:
+	chained_irq_exit(chip, desc);
 }
 
 /*
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] irqchip/versatile-fpga: Handle chained IRQs properly

[Marc Zyngier]

Hi Sungbo,

On 2020-03-18 17:09, Sungbo Eo wrote:
> Enclose the chained handler with chained_irq_{enter,exit}(), so that 
> the
> muxed interrupts get properly acked.
> 
> This patch also fixes a reboot bug on OX820 SoC, where the jiffies 
> timer
> interrupt is never acked. The kernel waits a clock tick forever in
> calibrate_delay_converge(), which leads to a boot hang.

Nice catch.

> 
> Signed-off-by: Sungbo Eo <mans0n@gorani.run>
> Cc: Neil Armstrong <narmstrong@baylibre.com>
> ---
>  drivers/irqchip/irq-versatile-fpga.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/irqchip/irq-versatile-fpga.c
> b/drivers/irqchip/irq-versatile-fpga.c
> index 928858dada75..08faab2fec3e 100644
> --- a/drivers/irqchip/irq-versatile-fpga.c
> +++ b/drivers/irqchip/irq-versatile-fpga.c
> @@ -6,6 +6,7 @@
>  #include <linux/irq.h>
>  #include <linux/io.h>
>  #include <linux/irqchip.h>
> +#include <linux/irqchip/chained_irq.h>
>  #include <linux/irqchip/versatile-fpga.h>
>  #include <linux/irqdomain.h>
>  #include <linux/module.h>
> @@ -68,12 +69,15 @@ static void fpga_irq_unmask(struct irq_data *d)
> 
>  static void fpga_irq_handle(struct irq_desc *desc)
>  {
> +	struct irq_chip *chip = irq_desc_get_chip(desc);
>  	struct fpga_irq_data *f = irq_desc_get_handler_data(desc);
>  	u32 status = readl(f->base + IRQ_STATUS);
> 
> +	chained_irq_enter(chip, desc);
> +

It's probably not a big deal, but I'm not fond of starting talking to
the muxing irqchip before having done the chained_irq_enter() call.

Moving that read here would probably be safer.

>  	if (status == 0) {
>  		do_bad_IRQ(desc);
> -		return;
> +		goto out;
>  	}
> 
>  	do {
> @@ -82,6 +86,9 @@ static void fpga_irq_handle(struct irq_desc *desc)
>  		status &= ~(1 << irq);
>  		generic_handle_irq(irq_find_mapping(f->domain, irq));
>  	} while (status);
> +
> +out:
> +	chained_irq_exit(chip, desc);
>  }
> 
>  /*

Otherwise looks good. If you send it again with the above fixed
and a Fixes: tag, I'll queue it.

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] irqchip/versatile-fpga: Handle chained IRQs properly

[Sungbo Eo]

Hi Marc,

On 2020-03-19 02:48, Marc Zyngier wrote:
> Hi Sungbo,
> 
> On 2020-03-18 17:09, Sungbo Eo wrote:
>> Enclose the chained handler with chained_irq_{enter,exit}(), so that the
>> muxed interrupts get properly acked.
>>
>> This patch also fixes a reboot bug on OX820 SoC, where the jiffies timer
>> interrupt is never acked. The kernel waits a clock tick forever in
>> calibrate_delay_converge(), which leads to a boot hang.
> 
> Nice catch.
> 
>>
>> Signed-off-by: Sungbo Eo <mans0n@gorani.run>
>> Cc: Neil Armstrong <narmstrong@baylibre.com>
>> ---
>>  drivers/irqchip/irq-versatile-fpga.c | 9 ++++++++-
>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/irqchip/irq-versatile-fpga.c
>> b/drivers/irqchip/irq-versatile-fpga.c
>> index 928858dada75..08faab2fec3e 100644
>> --- a/drivers/irqchip/irq-versatile-fpga.c
>> +++ b/drivers/irqchip/irq-versatile-fpga.c
>> @@ -6,6 +6,7 @@
>>  #include <linux/irq.h>
>>  #include <linux/io.h>
>>  #include <linux/irqchip.h>
>> +#include <linux/irqchip/chained_irq.h>
>>  #include <linux/irqchip/versatile-fpga.h>
>>  #include <linux/irqdomain.h>
>>  #include <linux/module.h>
>> @@ -68,12 +69,15 @@ static void fpga_irq_unmask(struct irq_data *d)
>>
>>  static void fpga_irq_handle(struct irq_desc *desc)
>>  {
>> +    struct irq_chip *chip = irq_desc_get_chip(desc);
>>      struct fpga_irq_data *f = irq_desc_get_handler_data(desc);
>>      u32 status = readl(f->base + IRQ_STATUS);
>>
>> +    chained_irq_enter(chip, desc);
>> +
> 
> It's probably not a big deal, but I'm not fond of starting talking to
> the muxing irqchip before having done the chained_irq_enter() call.
> 
> Moving that read here would probably be safer.

Oops, I missed it. Thanks for pointing it out.

> 
>>      if (status == 0) {
>>          do_bad_IRQ(desc);
>> -        return;
>> +        goto out;
>>      }
>>
>>      do {
>> @@ -82,6 +86,9 @@ static void fpga_irq_handle(struct irq_desc *desc)
>>          status &= ~(1 << irq);
>>          generic_handle_irq(irq_find_mapping(f->domain, irq));
>>      } while (status);
>> +
>> +out:
>> +    chained_irq_exit(chip, desc);
>>  }
>>
>>  /*
> 
> Otherwise looks good. If you send it again with the above fixed
> and a Fixes: tag, I'll queue it.

It seems the handler had been broken from the very beginning. Could you 
give me a hint on how the tag should be like?

Thanks.

> 
> Thanks,
> 
>          M.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] irqchip/versatile-fpga: Handle chained IRQs properly

[Marc Zyngier]

On 2020-03-18 18:20, Sungbo Eo wrote:
> Hi Marc,
> 
> On 2020-03-19 02:48, Marc Zyngier wrote:
>> Hi Sungbo,
>> 
>> On 2020-03-18 17:09, Sungbo Eo wrote:
>>> Enclose the chained handler with chained_irq_{enter,exit}(), so that 
>>> the
>>> muxed interrupts get properly acked.
>>> 
>>> This patch also fixes a reboot bug on OX820 SoC, where the jiffies 
>>> timer
>>> interrupt is never acked. The kernel waits a clock tick forever in
>>> calibrate_delay_converge(), which leads to a boot hang.
>> 
>> Nice catch.
>> 
>>> 
>>> Signed-off-by: Sungbo Eo <mans0n@gorani.run>
>>> Cc: Neil Armstrong <narmstrong@baylibre.com>
>>> ---
>>>  drivers/irqchip/irq-versatile-fpga.c | 9 ++++++++-
>>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>> 
>>> diff --git a/drivers/irqchip/irq-versatile-fpga.c
>>> b/drivers/irqchip/irq-versatile-fpga.c
>>> index 928858dada75..08faab2fec3e 100644
>>> --- a/drivers/irqchip/irq-versatile-fpga.c
>>> +++ b/drivers/irqchip/irq-versatile-fpga.c
>>> @@ -6,6 +6,7 @@
>>>  #include <linux/irq.h>
>>>  #include <linux/io.h>
>>>  #include <linux/irqchip.h>
>>> +#include <linux/irqchip/chained_irq.h>
>>>  #include <linux/irqchip/versatile-fpga.h>
>>>  #include <linux/irqdomain.h>
>>>  #include <linux/module.h>
>>> @@ -68,12 +69,15 @@ static void fpga_irq_unmask(struct irq_data *d)
>>> 
>>>  static void fpga_irq_handle(struct irq_desc *desc)
>>>  {
>>> +    struct irq_chip *chip = irq_desc_get_chip(desc);
>>>      struct fpga_irq_data *f = irq_desc_get_handler_data(desc);
>>>      u32 status = readl(f->base + IRQ_STATUS);
>>> 
>>> +    chained_irq_enter(chip, desc);
>>> +
>> 
>> It's probably not a big deal, but I'm not fond of starting talking to
>> the muxing irqchip before having done the chained_irq_enter() call.
>> 
>> Moving that read here would probably be safer.
> 
> Oops, I missed it. Thanks for pointing it out.
> 
>> 
>>>      if (status == 0) {
>>>          do_bad_IRQ(desc);
>>> -        return;
>>> +        goto out;
>>>      }
>>> 
>>>      do {
>>> @@ -82,6 +86,9 @@ static void fpga_irq_handle(struct irq_desc *desc)
>>>          status &= ~(1 << irq);
>>>          generic_handle_irq(irq_find_mapping(f->domain, irq));
>>>      } while (status);
>>> +
>>> +out:
>>> +    chained_irq_exit(chip, desc);
>>>  }
>>> 
>>>  /*
>> 
>> Otherwise looks good. If you send it again with the above fixed
>> and a Fixes: tag, I'll queue it.
> 
> It seems the handler had been broken from the very beginning. Could
> you give me a hint on how the tag should be like?

Indeed, it has been broken forever. I'm tempted to say:

Fixes: c41b16f8c9d9d ("ARM: integrator/versatile: consolidate FPGA IRQ 
handling code")

even if it probably predates the introduction of the chained_irq_enter() 
helpers.
This will ensure this gets backported to older kernels...

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2] irqchip/versatile-fpga: Handle chained IRQs properly

[Sungbo Eo]

Enclose the chained handler with chained_irq_{enter,exit}(), so that the
muxed interrupts get properly acked.

This patch also fixes a reboot bug on OX820 SoC, where the jiffies timer
interrupt is never acked. The kernel waits a clock tick forever in
calibrate_delay_converge(), which leads to a boot hang.

Fixes: c41b16f8c9d9 ("ARM: integrator/versatile: consolidate FPGA IRQ handling code")
Signed-off-by: Sungbo Eo <mans0n@gorani.run>
Cc: Neil Armstrong <narmstrong@baylibre.com>
---
v2: moved readl below chained_irq_enter()
    added Fixes tag

 drivers/irqchip/irq-versatile-fpga.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/irqchip/irq-versatile-fpga.c b/drivers/irqchip/irq-versatile-fpga.c
index 928858dada75..70e2cfff8175 100644
--- a/drivers/irqchip/irq-versatile-fpga.c
+++ b/drivers/irqchip/irq-versatile-fpga.c
@@ -6,6 +6,7 @@
 #include <linux/irq.h>
 #include <linux/io.h>
 #include <linux/irqchip.h>
+#include <linux/irqchip/chained_irq.h>
 #include <linux/irqchip/versatile-fpga.h>
 #include <linux/irqdomain.h>
 #include <linux/module.h>
@@ -68,12 +69,16 @@ static void fpga_irq_unmask(struct irq_data *d)
 
 static void fpga_irq_handle(struct irq_desc *desc)
 {
+	struct irq_chip *chip = irq_desc_get_chip(desc);
 	struct fpga_irq_data *f = irq_desc_get_handler_data(desc);
-	u32 status = readl(f->base + IRQ_STATUS);
+	u32 status;
+
+	chained_irq_enter(chip, desc);
 
+	status = readl(f->base + IRQ_STATUS);
 	if (status == 0) {
 		do_bad_IRQ(desc);
-		return;
+		goto out;
 	}
 
 	do {
@@ -82,6 +87,9 @@ static void fpga_irq_handle(struct irq_desc *desc)
 		status &= ~(1 << irq);
 		generic_handle_irq(irq_find_mapping(f->domain, irq));
 	} while (status);
+
+out:
+	chained_irq_exit(chip, desc);
 }
 
 /*
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] irqchip/versatile-fpga: Handle chained IRQs properly

[Marc Zyngier]

On 2020-03-19 02:34, Sungbo Eo wrote:
> Enclose the chained handler with chained_irq_{enter,exit}(), so that 
> the
> muxed interrupts get properly acked.
> 
> This patch also fixes a reboot bug on OX820 SoC, where the jiffies 
> timer
> interrupt is never acked. The kernel waits a clock tick forever in
> calibrate_delay_converge(), which leads to a boot hang.
> 
> Fixes: c41b16f8c9d9 ("ARM: integrator/versatile: consolidate FPGA IRQ
> handling code")
> Signed-off-by: Sungbo Eo <mans0n@gorani.run>
> Cc: Neil Armstrong <narmstrong@baylibre.com>
> ---
> v2: moved readl below chained_irq_enter()
>     added Fixes tag
> 
>  drivers/irqchip/irq-versatile-fpga.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)

Queued for 5.7.

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] irqchip/versatile-fpga: Handle chained IRQs properly

[Linus Walleij]

On Thu, Mar 19, 2020 at 3:36 AM Sungbo Eo <mans0n@gorani.run> wrote:

> Enclose the chained handler with chained_irq_{enter,exit}(), so that the
> muxed interrupts get properly acked.
>
> This patch also fixes a reboot bug on OX820 SoC, where the jiffies timer
> interrupt is never acked. The kernel waits a clock tick forever in
> calibrate_delay_converge(), which leads to a boot hang.
>
> Fixes: c41b16f8c9d9 ("ARM: integrator/versatile: consolidate FPGA IRQ handling code")
> Signed-off-by: Sungbo Eo <mans0n@gorani.run>
> Cc: Neil Armstrong <narmstrong@baylibre.com>
> ---
> v2: moved readl below chained_irq_enter()
>     added Fixes tag

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

I wonder how Integrator keeps working so well despite
this. I can't test it right now but I'm sure it is fine.

Yours,
Linus Walleij

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel