gpio: pca953x: Survive spurious interrupts
PATCH 1/1 gpio: pca953x: Survive spurious interrupts
The pca953x driver never checks the result of irq_find_mapping(), which returns 0 when no mapping is found. When a spurious interrupt is delivered (which can happen under obscure circumstances), the kernel explodes as it still tries to handle the error code as a real interrupt.
Handle this particular case and warn on spurious interrupts.
--- drivers/gpio/gpio-pca953x.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-)
drivers/gpio/gpio-pca953x.c +15 -2
@@ -824,8 +824,21 @@ static irqreturn_t pca953x_irq_handler(int irq, void *devid) ret =pca953x_irq_pending (chip, pending);mutex_unlock (&chip->i2c_lock); -for_each_set_bit (level, pending, gc->ngpio) -handle_nested_irq (irq_find_mapping (gc->irq.domain, level)); +if (ret) { + ret =0 ; + +for_each_set_bit (level, pending, gc->ngpio) { +int nested_irq =irq_find_mapping (gc->irq.domain, level); + +if (unlikely (nested_irq <=0 )) { +dev_warn_ratelimited (gc->parent,"unmapped interrupt %d\n" , level); +continue ; + } + +handle_nested_irq (nested_irq); + ret =1 ; + } + }return IRQ_RETVAL (ret); }
Signature · 1 line
2.28.0
Quoted 9 lines · Marc Zyngier
The pca953x driver never checks the result of
irq_find_mapping(), which returns 0 when no mapping is found. When a spurious interrupt is delivered (which can happen under obscure circumstances), the kernel explodes as it still tries to handle the error code as a real interrupt.Handle this particular case and warn on spurious interrupts.
Signed-off-by Marc Zyngier <[email protected]>
Patch applied for fixes.
Yours,
Linus Walleij
Quoted 9 lines · Linus Walleij
Quoted 6 lines · Marc Zyngier
The pca953x driver never checks the result of
irq_find_mapping(), which returns 0 when no mapping is found. When a spurious interrupt is delivered (which can happen under obscure circumstances), the kernel explodes as it still tries to handle the error code as a real interrupt.
Quoted 3 lines
Quoted 3 lines
Handle this particular case and warn on spurious interrupts.
Signed-off-by Marc Zyngier <[email protected]>
Wait, doesn't actually [1] fix the reported issue? Marc, can you confirm this?
[1]: e43c26e12dd4 ("gpio: pca953x: Fix uninitialized pending variable")
Signature · 2 lines
With Best Regards, Andy Shevchenko
Quoted 10 lines · Andy Shevchenko
Quoted 9 lines · Linus Walleij
Quoted 6 lines · Marc Zyngier
The pca953x driver never checks the result of
irq_find_mapping(), which returns 0 when no mapping is found. When a spurious interrupt is delivered (which can happen under obscure circumstances), the kernel explodes as it still tries to handle the error code as a real interrupt.
Quoted 5 lines
Quoted 3 lines
Quoted 3 lines
Handle this particular case and warn on spurious interrupts.
Signed-off-by Marc Zyngier <[email protected]>Wait, doesn't actually [1] fix the reported issue?
Not at all.
Quoted 3 lines
Marc, can you confirm this?
[1]:
e43c26e12dd4("gpio: pca953x: Fix uninitialized pending variable")
Different bug, really. If an interrupt is *really* pending, and no mapping established yet, feeding the result of irq_find_mapping() to handle_nested_irq() will lead to a panic.
Recently seen on a Tegra system suffering from even more pathological bugs.
M.
Signature · 1 line
Jazz is not dead. It just smells funny...
Quoted 19 lines · Marc Zyngier
Quoted 10 lines · Andy Shevchenko
Quoted 8 lines · Linus Walleij
Quoted 6 lines · Marc Zyngier
The pca953x driver never checks the result of
irq_find_mapping(), which returns 0 when no mapping is found. When a spurious interrupt is delivered (which can happen under obscure circumstances), the kernel explodes as it still tries to handle the error code as a real interrupt.Quoted 5 lines
Quoted 3 lines
Quoted 3 lines
Handle this particular case and warn on spurious interrupts.
Signed-off-by Marc Zyngier <[email protected]>Wait, doesn't actually [1] fix the reported issue?
Not at all.
Quoted 7 lines
Quoted 3 lines
Marc, can you confirm this?
[1]:
e43c26e12dd4("gpio: pca953x: Fix uninitialized pending variable")Different bug, really. If an interrupt is *really* pending, and no mapping established yet, feeding the result of
irq_find_mapping()tohandle_nested_irq()will lead to a panic.
I don't understand. We have plenty of drivers doing exactly the way without checking this returned code. What circumstances makes the mapping be absent?
Shouldn't we rather change this:
girq->handler = handle_simple_irq;
to this:
girq->handler = handle_bad_irq;?
Quoted 2 lines
Recently seen on a Tegra system suffering from even more pathological bugs.
Signature · 2 lines
With Best Regards, Andy Shevchenko
Quoted 22 lines · Andy Shevchenko
Quoted 21 lines · Marc Zyngier
Quoted 10 lines · Andy Shevchenko
Quoted 8 lines · Linus Walleij
Quoted 6 lines · Marc Zyngier
The pca953x driver never checks the result of
irq_find_mapping(), which returns 0 when no mapping is found. When a spurious interrupt is delivered (which can happen under obscure circumstances), the kernel explodes as it still tries to handle the error code as a real interrupt.Quoted 5 lines
Quoted 3 lines
Quoted 3 lines
Handle this particular case and warn on spurious interrupts.
Signed-off-by Marc Zyngier <[email protected]>Wait, doesn't actually [1] fix the reported issue?
Not at all.
Quoted 2 lines
Marc, can you confirm this?
Quoted 8 lines
Quoted 5 lines
Quoted 1 line
[1]:
e43c26e12dd4("gpio: pca953x: Fix uninitialized pending variable")Different bug, really. If an interrupt is *really* pending, and no mapping established yet, feeding the result of
irq_find_mapping()tohandle_nested_irq()will lead to a panic.I don't understand. We have plenty of drivers doing exactly the way without checking this returned code.
I'm sure we do. Most driver code is buggy as hell, but I don't see that as a reason to cargo-cult the crap. The API is crystal clear that it can return 0 for no mapping, and 0 isn't a valid interrupt.
Quoted 1 line
What circumstances makes the mapping be absent?
Other bugs in the system ([1]), spurious interrupts (which can *always* happen).
Quoted 6 lines
Shouldn't we rather change this:
girq->handler = handle_simple_irq; to this: girq->handler = handle_bad_irq;?
I don't understand what you are trying to achieve with that, apart from maybe breaking the driver. The right way to handle spurious interrupts is by telling the core code that the interrupt wasn't handled, and to let the spurious interrupt code do its magic.
M.
[1] https://lore.kernel.org/r/[email protected]
Signature · 1 line
Jazz is not dead. It just smells funny...
Quoted 29 lines · Marc Zyngier
Quoted 28 lines · Andy Shevchenko
Quoted 26 lines · Marc Zyngier
Quoted 10 lines · Andy Shevchenko
Quoted 8 lines · Linus Walleij
Quoted 6 lines · Marc Zyngier
The pca953x driver never checks the result of
irq_find_mapping(), which returns 0 when no mapping is found. When a spurious interrupt is delivered (which can happen under obscure circumstances), the kernel explodes as it still tries to handle the error code as a real interrupt.Quoted 5 lines
Quoted 3 lines
Quoted 3 lines
Handle this particular case and warn on spurious interrupts.
Signed-off-by Marc Zyngier <[email protected]>Wait, doesn't actually [1] fix the reported issue?
Not at all.
Quoted 3 lines
Marc, can you confirm this?
[1]:
e43c26e12dd4("gpio: pca953x: Fix uninitialized pending variable")Different bug, really. If an interrupt is *really* pending, and no mapping established yet, feeding the result of
irq_find_mapping()tohandle_nested_irq()will lead to a panic.
Quoted 6 lines
Quoted 2 lines
I don't understand. We have plenty of drivers doing exactly the way without checking this returned code.
I'm sure we do. Most driver code is buggy as hell, but I don't see that as a reason to cargo-cult the crap. The API is crystal clear that it can return 0 for no mapping, and 0 isn't a valid interrupt.
Yes, and the problem here is that we got this response from IRQ core, which we shouldn't.
Quoted 12 lines
Quoted 1 line
What circumstances makes the mapping be absent?
Other bugs in the system ([1]), spurious interrupts (which can *always* happen).
Quoted 6 lines
Shouldn't we rather change this:
girq->handler = handle_simple_irq; to this: girq->handler = handle_bad_irq;?
Quoted 5 lines
I don't understand what you are trying to achieve with that, apart from maybe breaking the driver. The right way to handle spurious interrupts is by telling the core code that the interrupt wasn't handled, and to let
the spurious interrupt code do its magic.
handle_bad_irq() is exactly for handling spurious IRQs as far as we believe documentation. So, by default the driver assigns (should assign) handle_bad_irq() to all IRQs as a default handler. If, by any chance, we got it, we already have a proper handler in place. The read handler is assigned whenever the IRQ core is called to register it (by means of ->irq_set_type() callback). My understanding that GPIO IRQ drivers are designed (should be designed) in this way. The approach will make us sure that we don't have spurious interrupts with assigned handlers.
Quoted 1 line
Signature · 2 lines
With Best Regards, Andy Shevchenko
Quoted 33 lines · Andy Shevchenko
Quoted 32 lines · Marc Zyngier
Quoted 30 lines · Andy Shevchenko
Quoted 19 lines · Marc Zyngier
Quoted 10 lines · Andy Shevchenko
Quoted 8 lines · Linus Walleij
Quoted 6 lines · Marc Zyngier
The pca953x driver never checks the result of
irq_find_mapping(), which returns 0 when no mapping is found. When a spurious interrupt is delivered (which can happen under obscure circumstances), the kernel explodes as it still tries to handle the error code as a real interrupt.Quoted 5 lines
Quoted 3 lines
Quoted 3 lines
Handle this particular case and warn on spurious interrupts.
Signed-off-by Marc Zyngier <[email protected]>Wait, doesn't actually [1] fix the reported issue?
Not at all.
Quoted 7 lines
Quoted 3 lines
Marc, can you confirm this?
[1]:
e43c26e12dd4("gpio: pca953x: Fix uninitialized pending variable")Different bug, really. If an interrupt is *really* pending, and no mapping established yet, feeding the result of
irq_find_mapping()tohandle_nested_irq()will lead to a panic.I don't understand. We have plenty of drivers doing exactly the way without checking this returned code.
Quoted 6 lines
Quoted 3 lines
I'm sure we do. Most driver code is buggy as hell, but I don't see that as a reason to cargo-cult the crap. The API is crystal clear that it can return 0 for no mapping, and 0 isn't a valid interrupt.
Yes, and the problem here is that we got this response from IRQ core, which we shouldn't.
What do you mean? There is no mapping at all. and all the core code can tell you is exactly that. If you think that using an error code as a valid input to another function is OK, we have a much bigger problem.
Quoted 19 lines
Quoted 13 lines
Quoted 2 lines
What circumstances makes the mapping be absent?
Other bugs in the system ([1]), spurious interrupts (which can *always* happen).
Quoted 6 lines
Shouldn't we rather change this:
girq->handler = handle_simple_irq; to this: girq->handler = handle_bad_irq;?
Quoted 5 lines
I don't understand what you are trying to achieve with that, apart from maybe breaking the driver. The right way to handle spurious interrupts is by telling the core code that the interrupt wasn't handled, and to let
the spurious interrupt code do its magic.
Quoted 9 lines
handle_bad_irq()is exactly for handling spurious IRQs as far as we believe documentation. So, by default the driver assigns (should assign)handle_bad_irq()to all IRQs as a default handler. If, by any chance, we got it, we already have a proper handler in place. The read handler is assigned whenever the IRQ core is called to register it (by means of ->irq_set_type() callback). My understanding that GPIO IRQ drivers are designed (should be designed) in this way. The approach will make us sure that we don't have spurious interrupts with assigned handlers.
I can't see how setting this to anything else can work, given that handle_nested_irq() knows nothing about this flow (it doesn't use any).
M.
Signature · 1 line
Jazz is not dead. It just smells funny...
Quoted 40 lines · Marc Zyngier
Quoted 39 lines · Andy Shevchenko
Quoted 37 lines · Marc Zyngier
Quoted 22 lines · Andy Shevchenko
Quoted 21 lines · Marc Zyngier
Quoted 10 lines · Andy Shevchenko
Quoted 8 lines · Linus Walleij
Quoted 6 lines · Marc Zyngier
The pca953x driver never checks the result of
irq_find_mapping(), which returns 0 when no mapping is found. When a spurious interrupt is delivered (which can happen under obscure circumstances), the kernel explodes as it still tries to handle the error code as a real interrupt.Quoted 5 lines
Quoted 3 lines
Quoted 3 lines
Handle this particular case and warn on spurious interrupts.
Signed-off-by Marc Zyngier <[email protected]>Wait, doesn't actually [1] fix the reported issue?
Not at all.
Quoted 2 lines
Marc, can you confirm this?
Quoted 8 lines
Quoted 5 lines
Quoted 1 line
[1]:
e43c26e12dd4("gpio: pca953x: Fix uninitialized pending variable")Different bug, really. If an interrupt is *really* pending, and no mapping established yet, feeding the result of
irq_find_mapping()tohandle_nested_irq()will lead to a panic.I don't understand. We have plenty of drivers doing exactly the way without checking this returned code.
I'm sure we do. Most driver code is buggy as hell, but I don't see that
as a reason to cargo-cult the crap. The API is crystal clear that it can
return 0 for no mapping, and 0 isn't a valid interrupt.
Quoted 7 lines
Quoted 2 lines
Yes, and the problem here is that we got this response from IRQ core, which we shouldn't.
What do you mean? There is no mapping at all. and all the core code can tell you is exactly that. If you think that using an error code as a valid input to another function is OK, we have a much bigger problem.
Of course it's not okay. And that's what puzzles me. We shouldn't get bit set in pending if there is no requested IRQ (handler assigned). I think there is a bug indeed, but I'm not sure it is in the code you are patching. Rather in the code when we are preparing a pending bitmap.
Shouldn't we have unused (unassigned interrupts) being masked in the first place?
I can imagine that we have the chip preconfigured by firmware and when ->probe() happens the enabled IRQs should be left untouched, but is it the case?
I guess you are using a non-latched version of the GPIO expander (I don't have such for a test).
I need to look at this closer...
Since Linus already applied this we will live with it now, but it would be really helpful if you may dump the traces of non-working case before this patch to analyze (I would like to see all regmap IO for this chip).
Quoted 33 lines
Quoted 20 lines
Quoted 13 lines
Quoted 1 line
What circumstances makes the mapping be absent?
Other bugs in the system ([1]), spurious interrupts (which can *always*
happen).Quoted 6 lines
Shouldn't we rather change this:
girq->handler = handle_simple_irq; to this: girq->handler = handle_bad_irq;?
Quoted 6 lines
I don't understand what you are trying to achieve with that, apart from
maybe breaking the driver. The right way to handle spurious interrupts is by telling the core code that the interrupt wasn't handled, and to let
the spurious interrupt code do its magic.Quoted 9 lines
handle_bad_irq()is exactly for handling spurious IRQs as far as we believe documentation. So, by default the driver assigns (should assign)handle_bad_irq()to all IRQs as a default handler. If, by any chance, we got it, we already have a proper handler in place. The read handler is assigned whenever the IRQ core is called to register it (by means of ->irq_set_type() callback). My understanding that GPIO IRQ drivers are designed (should be designed) in this way. The approach will make us sure that we don't have spurious interrupts with assigned handlers.I can't see how setting this to anything else can work, given that
handle_nested_irq()knows nothing about this flow (it doesn't use any).
Signature · 2 lines
With Best Regards, Andy Shevchenko