[PATCH] genirq: Check irq disabled & masked states in irq_shutdown

[PATCH] genirq: Check irq disabled & masked states in irq_shutdown

[Jeffy Chen]

If irq is already disabled and masked, we would hit a unbalanced irq
shutdown/disable/mask when freeing it.

Add a state check in irq_shutdown to prevent this.

Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
---

 kernel/irq/chip.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index 686be4b..816da03 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -206,14 +206,20 @@ int irq_startup(struct irq_desc *desc, bool resend)
 
 void irq_shutdown(struct irq_desc *desc)
 {
-	irq_state_set_disabled(desc);
 	desc->depth = 1;
+
+	if (unlikely(irqd_irq_disabled(&desc->irq_data) &&
+		irqd_irq_masked(&desc->irq_data)))
+		goto out;
+
+	irq_state_set_disabled(desc);
 	if (desc->irq_data.chip->irq_shutdown)
 		desc->irq_data.chip->irq_shutdown(&desc->irq_data);
 	else if (desc->irq_data.chip->irq_disable)
 		desc->irq_data.chip->irq_disable(&desc->irq_data);
 	else
 		desc->irq_data.chip->irq_mask(&desc->irq_data);
+out:
 	irq_domain_deactivate_irq(&desc->irq_data);
 	irq_state_set_masked(desc);
 }
-- 
2.1.4

Re: [PATCH] genirq: Check irq disabled & masked states in irq_shutdown

[Thomas Gleixner]

On Fri, 26 May 2017, Jeffy Chen wrote:

> If irq is already disabled and masked, we would hit a unbalanced irq
> shutdown/disable/mask when freeing it.

Errr? What exactly is unbalanced? None of the called functions has any
counter or whatever.

Can you please explain what you are trying to fix?

Thanks,

	tglx

Re: [PATCH] genirq: Check irq disabled & masked states in irq_shutdown

[jeffy]

Hi Thomas,

On 05/26/2017 09:20 PM, Thomas Gleixner wrote:
> On Fri, 26 May 2017, Jeffy Chen wrote:
>
>> If irq is already disabled and masked, we would hit a unbalanced irq
>> shutdown/disable/mask when freeing it.
>
> Errr? What exactly is unbalanced? None of the called functions has any
> counter or whatever.
>
> Can you please explain what you are trying to fix?

sorry, i'll try to rewrite the commit message.

for example when a driver(drivers/net/wireless/marvell/mwifiex/main.c) 
try to do these:

devm_request_irq->irq_startup->irq_enable
disable_irq                                     <-- disabled and masked
devm_free_irq->irq_shutdown                     <-- disable it again

and the pinctrl-rockchip driver would enable/disable gpio clk in 
irq_enable/irq_disable, so it would try to disable a disabled clk(due to 
unbalanced irq disable)


>
> Thanks,
>
> 	tglx
>
>
>

Re: [PATCH] genirq: Check irq disabled & masked states in irq_shutdown

[Thomas Gleixner]

On Sat, 27 May 2017, jeffy wrote:
> On 05/26/2017 09:20 PM, Thomas Gleixner wrote:
> > On Fri, 26 May 2017, Jeffy Chen wrote:
> > 
> > > If irq is already disabled and masked, we would hit a unbalanced irq
> > > shutdown/disable/mask when freeing it.
> > 
> > Errr? What exactly is unbalanced? None of the called functions has any
> > counter or whatever.
> > 
> > Can you please explain what you are trying to fix?
> 
> sorry, i'll try to rewrite the commit message.
> 
> for example when a driver(drivers/net/wireless/marvell/mwifiex/main.c) try to
> do these:
> 
> devm_request_irq->irq_startup->irq_enable
> disable_irq                                     <-- disabled and masked
> devm_free_irq->irq_shutdown                     <-- disable it again

This driver is broken as hell. It requests the interrupt _BEFORE_ the whole
thing is initialized. If there is a pending interrupt on that line, it will
explode nicely before it is able to disable the irq. But that's a different
problem.

Thanks,

	tglx

Re: [PATCH] genirq: Check irq disabled & masked states in irq_shutdown

[Brian Norris]

Hi,

To address a tangent brought up here:

On Sat, May 27, 2017 at 10:16:37AM +0200, Thomas Gleixner wrote:
> On Sat, 27 May 2017, jeffy wrote:
> > for example when a driver(drivers/net/wireless/marvell/mwifiex/main.c) try to
> > do these:
> > 
> > devm_request_irq->irq_startup->irq_enable
> > disable_irq                                     <-- disabled and masked
> > devm_free_irq->irq_shutdown                     <-- disable it again
> 
> This driver is broken as hell.

No argument on the general statement :)

> It requests the interrupt _BEFORE_ the whole
> thing is initialized. If there is a pending interrupt on that line, it will
> explode nicely before it is able to disable the irq. But that's a different
> problem.

For that particular interrupt, it's mostly an informational interrupt
regarding wakeups. We don't do anything that could blow up there, except
report a (spurious) wakeup event. (And this spurious wakeup event only
occurs because the Wifi firmware may toggle its "wake" pin even when the
system is already awake. A weird behavior...)

So yes, the pattern isn't great, but no, it's not going to blow up,
AFAIK.

However, if you were to look at the same driver's .../mwifiex/pcie.c,
you would see a similar problem, and you *would* be right if you claimed
that things could blow up badly there! mwifiex_pcie_request_irq() is
called much too early, and if an interrupt gets queued up at the wrong
time, we won't handle it very nicely.

Anyway, I just thought I'd mention it, in case someone else following
this thread is curious. Coincidentally, I'm already working on patching
this on linux-wireless@.

Side note: for issues like the first problem above, I wonder why there
isn't a flag that once could pass to request_irq() that suggests the IRQ
should be initially disabled? I know this wouldn't work for shared
interrupts (but request_irq() could reject that combination, no?), but
it seems like there are plenty of cases where it might be useful. Some
devices simply don't have a device-level interrupt mask, and always
expect to have a dedicated interrupt. With the status quo, a driver for
such a device has to defer their request_irq() until
sometimes-inconvient times [1], or else accept some subpar behavior (see
above "spurious wakeup reporting").

Regards,
Brian

[1] Note that, for one, request_irq() can fail, whereas enable_irq()
    cannot.

Re: [PATCH] genirq: Check irq disabled & masked states in irq_shutdown

[Brian Norris]

Sorry to respond to myself. Thomas, your reply to another mail in this
series helped me to notice:

On Tue, May 30, 2017 at 04:19:58PM -0700, Brian Norris wrote:
> Side note: for issues like the first problem above, I wonder why there
> isn't a flag that once could pass to request_irq() that suggests the IRQ
> should be initially disabled?

Is that what IRQ_NOAUTOEN is for?

> I know this wouldn't work for shared
> interrupts (but request_irq() could reject that combination, no?)

Hehe, but then I see this, for example, when grepping around:

drivers/usb/dwc3/dwc3-omap.c:

        irq_set_status_flags(omap->irq, IRQ_NOAUTOEN);
        ret = devm_request_threaded_irq(dev, omap->irq, dwc3_omap_interrupt,
                                        dwc3_omap_interrupt_thread, IRQF_SHARED,
                                        "dwc3-omap", omap);

IIUC, that's quite broken, no?

Brian

Re: [PATCH] genirq: Check irq disabled & masked states in irq_shutdown

[Thomas Gleixner]

On Tue, 30 May 2017, Brian Norris wrote:
> Sorry to respond to myself. Thomas, your reply to another mail in this
> series helped me to notice:
> 
> On Tue, May 30, 2017 at 04:19:58PM -0700, Brian Norris wrote:
> > Side note: for issues like the first problem above, I wonder why there
> > isn't a flag that once could pass to request_irq() that suggests the IRQ
> > should be initially disabled?
> 
> Is that what IRQ_NOAUTOEN is for?

Yes.

> > I know this wouldn't work for shared
> > interrupts (but request_irq() could reject that combination, no?)
> 
> Hehe, but then I see this, for example, when grepping around:
> 
> drivers/usb/dwc3/dwc3-omap.c:
> 
>         irq_set_status_flags(omap->irq, IRQ_NOAUTOEN);
>         ret = devm_request_threaded_irq(dev, omap->irq, dwc3_omap_interrupt,
>                                         dwc3_omap_interrupt_thread, IRQF_SHARED,
>                                         "dwc3-omap", omap);
> 
> IIUC, that's quite broken, no?

Indeed. Because the interrupt could have been requested by some other
driver already. In that case IRQ_NOAUTOEN has no effect at all.

We probably should check that in __setup_irq() and yell at people.

Thanks,

	tglx

Re: [PATCH] genirq: Check irq disabled & masked states in irq_shutdown

[Thomas Gleixner]

On Tue, 30 May 2017, Brian Norris wrote:
> On Sat, May 27, 2017 at 10:16:37AM +0200, Thomas Gleixner wrote:
> > On Sat, 27 May 2017, jeffy wrote:
> > > for example when a driver(drivers/net/wireless/marvell/mwifiex/main.c) try to
> > > do these:
> > > 
> > > devm_request_irq->irq_startup->irq_enable
> > > disable_irq                                     <-- disabled and masked
> > > devm_free_irq->irq_shutdown                     <-- disable it again
> > 
> > This driver is broken as hell.
> 
> No argument on the general statement :)
> 
> > It requests the interrupt _BEFORE_ the whole
> > thing is initialized. If there is a pending interrupt on that line, it will
> > explode nicely before it is able to disable the irq. But that's a different
> > problem.
> 
> For that particular interrupt, it's mostly an informational interrupt
> regarding wakeups. We don't do anything that could blow up there, except
> report a (spurious) wakeup event. (And this spurious wakeup event only
> occurs because the Wifi firmware may toggle its "wake" pin even when the
> system is already awake. A weird behavior...)
> 
> So yes, the pattern isn't great, but no, it's not going to blow up,
> AFAIK.

Fair enough.

Thanks,

	tglx