2017-03-06 16:07:39

by Charles Keepax

Subject: [PATCH] genirq: Fix handling of nested shared IRQs

When an IRQ is nested the nested handler is called directly from within the
threaded handler of the parent IRQ, however, the code in handle_nested_irq
only calls a single handler. This means when a shared IRQ is nested only
the first of the shared IRQ handlers will be run. This patch adds a loop
to move through and process all the handlers associated with the IRQ in
handle_nested_irq.

Signed-off-by: Charles Keepax <[email protected]>
---
kernel/irq/chip.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index be3c34e..c6c7f11 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -348,9 +348,12 @@ void handle_nested_irq(unsigned int irq)
irqd_set(&desc->irq_data, IRQD_IRQ_INPROGRESS);
raw_spin_unlock_irq(&desc->lock);

- action_ret = action->thread_fn(action->irq, action->dev_id);
- if (!noirqdebug)
- note_interrupt(desc, action_ret);
+ do {
+ action_ret = action->thread_fn(action->irq, action->dev_id);
+ if (!noirqdebug)
+ note_interrupt(desc, action_ret);
+ action = action->next;
+ } while (action);

raw_spin_lock_irq(&desc->lock);
irqd_clear(&desc->irq_data, IRQD_IRQ_INPROGRESS);
--
2.1.4

2017-03-06 18:42:18

by Thomas Gleixner

Subject: Re: [PATCH] genirq: Fix handling of nested shared IRQs

On Mon, 6 Mar 2017, Charles Keepax wrote:

> When an IRQ is nested the nested handler is called directly from within the
> threaded handler of the parent IRQ, however, the code in handle_nested_irq
> only calls a single handler. This means when a shared IRQ is nested only
> the first of the shared IRQ handlers will be run. This patch adds a loop
> to move through and process all the handlers associated with the IRQ in
> handle_nested_irq.

That was never meant to deal with nested shaed interrupts, so the $subject
is misleading. This is not a fix, it's a functional extension.

Aside of that, please structure the changelog in paragraphs instead of one
big lump.

1) Context
2) Problem
3) Solution

See also Documentation/process/submitting-patches.rst and please search
there for "This patch" .....

> @@ -348,9 +348,12 @@ void handle_nested_irq(unsigned int irq)
> irqd_set(&desc->irq_data, IRQD_IRQ_INPROGRESS);
> raw_spin_unlock_irq(&desc->lock);
>
> - action_ret = action->thread_fn(action->irq, action->dev_id);
> - if (!noirqdebug)
> - note_interrupt(desc, action_ret);
> + do {

for_each_action_of_desc() please

> + action_ret = action->thread_fn(action->irq, action->dev_id);
> + if (!noirqdebug)
> + note_interrupt(desc, action_ret);

That's wrong. See __handle_irq_event_percpu() for the correct handling of
shared interrupts vs. note_interrupt()

Thanks,

tglx

2017-03-07 09:24:00

by Charles Keepax

Subject: Re: [PATCH] genirq: Fix handling of nested shared IRQs

On Mon, Mar 06, 2017 at 07:05:39PM +0100, Thomas Gleixner wrote:
> On Mon, 6 Mar 2017, Charles Keepax wrote:
>
> > When an IRQ is nested the nested handler is called directly from within the
> > threaded handler of the parent IRQ, however, the code in handle_nested_irq
> > only calls a single handler. This means when a shared IRQ is nested only
> > the first of the shared IRQ handlers will be run. This patch adds a loop
> > to move through and process all the handlers associated with the IRQ in
> > handle_nested_irq.
>
> That was never meant to deal with nested shaed interrupts, so the $subject
> is misleading. This is not a fix, it's a functional extension.
>
> Aside of that, please structure the changelog in paragraphs instead of one
> big lump.
>
> 1) Context
> 2) Problem
> 3) Solution
>
> See also Documentation/process/submitting-patches.rst and please search
> there for "This patch" .....
>
> > @@ -348,9 +348,12 @@ void handle_nested_irq(unsigned int irq)
> > irqd_set(&desc->irq_data, IRQD_IRQ_INPROGRESS);
> > raw_spin_unlock_irq(&desc->lock);
> >
> > - action_ret = action->thread_fn(action->irq, action->dev_id);
> > - if (!noirqdebug)
> > - note_interrupt(desc, action_ret);
> > + do {
>
> for_each_action_of_desc() please
>
> > + action_ret = action->thread_fn(action->irq, action->dev_id);
> > + if (!noirqdebug)
> > + note_interrupt(desc, action_ret);
>
> That's wrong. See __handle_irq_event_percpu() for the correct handling of
> shared interrupts vs. note_interrupt()
>

Thanks for the pointers, I will look at those and respin the
patch.

Thanks,
Charles