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 <[email protected]>
>>> Cc: Neil Armstrong <[email protected]>
>>> ---
>>> 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...
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 <[email protected]>
Cc: Neil Armstrong <[email protected]>
---
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
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 <[email protected]>
> Cc: Neil Armstrong <[email protected]>
> ---
> 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...
On Thu, Mar 19, 2020 at 3:36 AM Sungbo Eo <[email protected]> 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 <[email protected]>
> Cc: Neil Armstrong <[email protected]>
> ---
> v2: moved readl below chained_irq_enter()
> added Fixes tag
Reviewed-by: Linus Walleij <[email protected]>
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
The following commit has been merged into the irq/core branch of tip:
Commit-ID: 486562da598c59e9f835b551d7cf19507de2d681
Gitweb: https://git.kernel.org/tip/486562da598c59e9f835b551d7cf19507de2d681
Author: Sungbo Eo <[email protected]>
AuthorDate: Thu, 19 Mar 2020 11:34:48 +09:00
Committer: Marc Zyngier <[email protected]>
CommitterDate: Thu, 19 Mar 2020 08:37:44
irqchip/versatile-fpga: Handle chained IRQs properly
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 <[email protected]>
Signed-off-by: Marc Zyngier <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
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 928858d..70e2cff 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);
}
/*