| Message ID | 1592208439-17594-3-git-send-email-krzk@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/3] spi: spi-fsl-dspi: Fix external abort on interrupt in exit paths | expand |
On Mon, Jun 15, 2020 at 10:07:19AM +0200, Krzysztof Kozlowski wrote: > Testing events during freeing of disabled shared interrupts > (CONFIG_DEBUG_SHIRQ) leads to false positives. The driver disabled > interrupts on purpose to be sure that they will not fire during device > removal. Surely the whole issue with shared IRQs that's being tested for here is that when the interrupt is shared some other device connected to the same interrupt line may trigger an interrupt regardless of what's going on with this device?
On Mon, Jun 15, 2020 at 01:08:44PM +0100, Mark Brown wrote: > On Mon, Jun 15, 2020 at 10:07:19AM +0200, Krzysztof Kozlowski wrote: > > Testing events during freeing of disabled shared interrupts > > (CONFIG_DEBUG_SHIRQ) leads to false positives. The driver disabled > > interrupts on purpose to be sure that they will not fire during device > > removal. > > Surely the whole issue with shared IRQs that's being tested for here is > that when the interrupt is shared some other device connected to the > same interrupt line may trigger an interrupt regardless of what's going > on with this device? Yes. However if that device disabled the interrupt, it should not be fired for other users. In such case the testing does not point to a real issue. Anyway, this patch is not necessary with my v3 approach to SPI shared interrupts issue. Best regards, Krzysztof
On Tue, Jun 16, 2020 at 12:11:17PM +0200, Krzysztof Kozlowski wrote: > On Mon, Jun 15, 2020 at 01:08:44PM +0100, Mark Brown wrote: > > On Mon, Jun 15, 2020 at 10:07:19AM +0200, Krzysztof Kozlowski wrote: > > > Testing events during freeing of disabled shared interrupts > > > (CONFIG_DEBUG_SHIRQ) leads to false positives. The driver disabled > > > interrupts on purpose to be sure that they will not fire during device > > > removal. > > Surely the whole issue with shared IRQs that's being tested for here is > > that when the interrupt is shared some other device connected to the > > same interrupt line may trigger an interrupt regardless of what's going > > on with this device? > Yes. However if that device disabled the interrupt, it should not be > fired for other users. In such case the testing does not point to a > real issue. To be honest I'd say that if you're disabling a shared interrupt that's a bit of an issue regardless of anything else that's going on, it'll disrupt other devices connected to it.
Mark Brown <broonie@kernel.org> writes: > On Tue, Jun 16, 2020 at 12:11:17PM +0200, Krzysztof Kozlowski wrote: >> On Mon, Jun 15, 2020 at 01:08:44PM +0100, Mark Brown wrote: >> > On Mon, Jun 15, 2020 at 10:07:19AM +0200, Krzysztof Kozlowski wrote: >> > > Testing events during freeing of disabled shared interrupts >> > > (CONFIG_DEBUG_SHIRQ) leads to false positives. The driver disabled >> > > interrupts on purpose to be sure that they will not fire during device >> > > removal. > >> > Surely the whole issue with shared IRQs that's being tested for here is >> > that when the interrupt is shared some other device connected to the >> > same interrupt line may trigger an interrupt regardless of what's going >> > on with this device? > >> Yes. However if that device disabled the interrupt, it should not be >> fired for other users. In such case the testing does not point to a >> real issue. > > To be honest I'd say that if you're disabling a shared interrupt that's > a bit of an issue regardless of anything else that's going on, it'll > disrupt other devices connected to it. Correct. Shared interrupts are broken by design and I really can't understand why hardware people still insist on them. Thanks, tglx
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c index 761911168438..f19f0dedc30d 100644 --- a/kernel/irq/manage.c +++ b/kernel/irq/manage.c @@ -1775,12 +1775,14 @@ static struct irqaction *__free_irq(struct irq_desc *desc, void *dev_id) /* * It's a shared IRQ -- the driver ought to be prepared for an IRQ * event to happen even now it's being freed, so let's make sure that - * is so by doing an extra call to the handler .... + * is so by doing an extra call to the handler. + * Although the driver could disable the interrupts just before freeing + * just to avoid such trouble - don't test it then. * * ( We do this after actually deregistering it, to make sure that a * 'real' IRQ doesn't run in parallel with our fake. ) */ - if (action->flags & IRQF_SHARED) { + if (action->flags & IRQF_SHARED && !desc->depth) { local_irq_save(flags); action->handler(irq, dev_id); local_irq_restore(flags);
Testing events during freeing of disabled shared interrupts (CONFIG_DEBUG_SHIRQ) leads to false positives. The driver disabled interrupts on purpose to be sure that they will not fire during device removal. Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org> --- Changes since v1: 1. New patch. --- kernel/irq/manage.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)