Disabling an interrupt in the handler locks the system up

Hello,

On my platform, one HW block pulls the interrupt line high as long as it remains idle, and low when it is busy.

The device tree node is:

		test at 22222 {
			compatible = "vendor,testme";
			interrupts = <23 IRQ_TYPE_LEVEL_HIGH>;
		};

I wrote a minimal driver which registers the irq.
And in the interrupt handler, I disable said irq.

Since the irq is IRQ_TYPE_LEVEL_HIGH, it will fire as soon as it is registered (because the block is idle).

Here is the code I've been running, request_irq doesn't return.

#include <linux/module.h>
#include <linux/of_irq.h>
#include <linux/interrupt.h>
#include <linux/platform_device.h>

static irqreturn_t test_isr(int irq, void *dev)

{
	printk("irq=%d dev=%p\n", irq, dev);
	disable_irq_nosync(irq);
	printk("IRQ %d NOW DISABLED\n", irq);
	return IRQ_HANDLED;
}

static int test_probe(struct platform_device *pdev)

{
	unsigned res, virq;
	printk("ENTER %s\n", __func__);

	virq = irq_of_parse_and_map(pdev->dev.of_node, 0);
	printk("virq = %u\n", virq);

	res = request_irq(virq, test_isr, 0, "testme", (void *)0x42);
	printk("request_irq = %d\n", res);

	return 0;
}

static const struct of_device_id test_ids[] = {
	{ .compatible = "vendor,testme" },
	{ /* sentinel */ }
};

static struct platform_driver test_driver = {
	.probe	= test_probe,
	.driver	= {
		.name		= "test",
		.of_match_table	= test_ids,
	},
};

module_platform_driver(test_driver);
MODULE_LICENSE("GPL");

And here's what I get when I try to load the module:
(I'm using the default CONFIG_RCU_CPU_STALL_TIMEOUT=21)

$ insmod test.ko
[   91.571065] ENTER test_probe
[   91.574208] virq = 21
[   91.576902] irq=21 dev=00000042
[   91.580061] IRQ 21 NOW DISABLED
[  102.038544] nfs: server 172.27.64.1 not responding, still trying
[  112.571855] INFO: rcu_preempt self-detected stall on CPU
[  112.577201]  0-...: (6298 ticks this GP) idle=c45/140000000000002/0 softirq=1295/1295 fqs=2045 
[  112.585942]   (t=6300 jiffies g=208 c=207 q=20)
[  112.590497] Task dump for CPU 0:
[  112.593735] insmod          R running      0   951    929 0x00000002
[  112.600124] Backtrace: 
[  112.602601] [<c010b9a4>] (dump_backtrace) from [<c010bba0>] (show_stack+0x18/0x1c)
[  112.610207]  r7:c0702434[  112.612571]  r6:c07024c8
 r5:000003a1[  112.616163]  r4:df570dc0
[  112.618701] 
[  112.620202] [<c010bb88>] (show_stack) from [<c0143a28>] (sched_show_task+0xbc/0x110)
[  112.627989] [<c014396c>] (sched_show_task) from [<c0145ef0>] (dump_cpu_task+0x40/0x48)
[  112.635943]  r5:00000000[  112.638307]  r4:00000000
[  112.640845] 
[  112.642347] [<c0145eb0>] (dump_cpu_task) from [<c01a8130>] (rcu_dump_cpu_stacks+0xb0/0xcc)
[  112.650651]  r5:00000000[  112.653014]  r4:c070a840
[  112.655553] 
[  112.657055] [<c01a8080>] (rcu_dump_cpu_stacks) from [<c01743ec>] (rcu_check_callbacks+0x8f8/0x9b0)
[  112.666058]  r10:c07024c0[  112.668509]  r9:c0702518
 r8:1f5a3000[  112.672100]  r7:c070a840
 r6:c0702100[  112.675691]  r5:c0637f40
[  112.678230]  r4:dfbdaf40[  112.680592]  r3:ffff2458
[  112.683130] 
[  112.684629] [<c0173af4>] (rcu_check_callbacks) from [<c0177b70>] (update_process_times+0x3c/0x64)
[  112.693545]  r10:c0188c54[  112.695995]  r9:00000001
 r8:dfbd870c[  112.699588]  r7:0000001a
 r6:00000000[  112.703178]  r5:df570dc0
[  112.705717]  r4:ffffe000[  112.708080] 
[  112.709581] [<c0177b34>] (update_process_times) from [<c0188c50>] (tick_sched_handle+0x50/0x54)
[  112.718322]  r7:0000001a[  112.720684]  r6:35192494
 r5:def2dab8[  112.724274]  r4:dfbd88f8
[  112.726812] 
[  112.728309] [<c0188c00>] (tick_sched_handle) from [<c0188cb0>] (tick_sched_timer+0x5c/0xa0)
[  112.736705] [<c0188c54>] (tick_sched_timer) from [<c0178bf4>] (__hrtimer_run_queues+0x11c/0x1ac)
[  112.745533]  r7:00000000[  112.747896]  r6:dfbd8700
 r5:dfbd88f8[  112.751486]  r4:dfbd86c0
[  112.754025] 
[  112.755519] [<c0178ad8>] (__hrtimer_run_queues) from [<c0178e88>] (hrtimer_interrupt+0xbc/0x20c)
[  112.764347]  r10:dfbd8738[  112.766798]  r9:dfbd8778
 r8:dfbd8758[  112.770389]  r7:dfbd86d4
 r6:ffffffff[  112.773979]  r5:00000003
[  112.776518]  r4:dfbd86c0[  112.778881] 
[  112.780378] [<c0178dcc>] (hrtimer_interrupt) from [<c010ed0c>] (twd_handler+0x38/0x48)
[  112.788332]  r10:def2dab8[  112.790782]  r9:df402400
 r8:00000001[  112.794373]  r7:df408700
 r6:00000013[  112.797964]  r5:c0702744
[  112.800504]  r4:00000001[  112.802866] 
[  112.804362] [<c010ecd4>] (twd_handler) from [<c016a134>] (handle_percpu_devid_irq+0x94/0x14c)
[  112.812928]  r5:c0702744[  112.815291]  r4:df405300
[  112.817829] 
[  112.819327] [<c016a0a0>] (handle_percpu_devid_irq) from [<c0165134>] (generic_handle_irq+0x2c/0x3c)
[  112.828417]  r7:def2dbd0[  112.830780]  r6:00000013
 r5:00000000[  112.834370]  r4:c0635ec4
[  112.836909] 
[  112.838404] [<c0165108>] (generic_handle_irq) from [<c01656dc>] (__handle_domain_irq+0x84/0xf4)
[  112.847150] [<c0165658>] (__handle_domain_irq) from [<c01014ac>] (gic_handle_irq+0x50/0x94)
[  112.855541]  r10:def2dbd0[  112.857992]  r9:e0803100
 r8:e0802100[  112.861583]  r7:def2dab8
 r6:e080210c[  112.865173]  r5:c0702744
[  112.867712]  r4:c070ff50[  112.870076]  r3:def2dab8
[  112.872614] 
[  112.874107] [<c010145c>] (gic_handle_irq) from [<c010c70c>] (__irq_svc+0x6c/0xa8)
[  112.881626] Exception stack(0xdef2dab8 to 0xdef2db00)
[  112.886699] daa0:                                                       00000000 c057c9b4
[  112.894919] dac0: c07216c0 00000000 00000202 ffffe000 00000013 00000000 00000001 df402400
[  112.903140] dae0: def2dbd0 def2db64 def2daf8 def2db08 c030db68 c0121068 20000113 ffffffff
[  112.911356]  r9:def2c000[  112.913719]  r8:00000001
 r7:def2daec[  112.917310]  r6:ffffffff
 r5:20000113[  112.920901]  r4:c0121068
[  112.923448] [<c0120fc0>] (__do_softirq) from [<c0121520>] (irq_exit+0xd4/0x110)
[  112.930792]  r10:def2dbd0[  112.933242]  r9:df402400
 r8:00000001[  112.936833]  r7:00000000
 r6:00000013[  112.940423]  r5:00000000
[  112.942962]  r4:c0635ec4[  112.945324] 
[  112.946819] [<c012144c>] (irq_exit) from [<c01656e0>] (__handle_domain_irq+0x88/0xf4)
[  112.954690] [<c0165658>] (__handle_domain_irq) from [<c01014ac>] (gic_handle_irq+0x50/0x94)
[  112.963080]  r10:00000000[  112.965531]  r9:e0803100
 r8:e0802100[  112.969122]  r7:def2dbd0
 r6:e080210c[  112.972714]  r5:c0702744
[  112.975253]  r4:c070ff50[  112.977617]  r3:def2dbd0
[  112.980154] 
[  112.981648] [<c010145c>] (gic_handle_irq) from [<c010c70c>] (__irq_svc+0x6c/0xa8)
[  112.989166] Exception stack(0xdef2dbd0 to 0xdef2dc18)
[  112.994240] dbc0:                                     df506260 60000013 00000000 00000005
[  113.002460] dbe0: dedbd680 df506200 00000015 df506260 60000013 df506238 00000000 def2dc2c
[  113.010679] dc00: def2dc30 def2dc20 c0167f90 c04d68d0 60000013 ffffffff
[  113.017324]  r9:def2c000[  113.019687]  r8:60000013
 r7:def2dc04[  113.023277]  r6:ffffffff
 r5:60000013[  113.026868]  r4:c04d68d0
[  113.029418] [<c04d68a8>] (_raw_spin_unlock_irqrestore) from [<c0167f90>] (__setup_irq+0x440/0x5f0)
[  113.038426] [<c0167b50>] (__setup_irq) from [<c0168300>] (request_threaded_irq+0xf0/0x188)
[  113.046730]  r10:00000000[  113.049179]  r9:00000015
 r8:df506210[  113.052770]  r7:00000000
 r6:df506200[  113.056362]  r5:00000000
[  113.058901]  r4:dedbd680[  113.061263] 
[  113.062767] [<c0168210>] (request_threaded_irq) from [<bf0040b8>] (test_probe+0x74/0x90 [zozo])
[  113.071508]  r10:00000000[  113.073959]  r9:21242a5c
 r8:00000001[  113.077550]  r7:fffffdfb
 r6:bf004320[  113.081142]  r5:df516810
[  113.083681]  r4:00000015[  113.086044]  r3:00000000
[  113.088582] 
[  113.090085] [<bf004044>] (test_probe [zozo]) from [<c03595e8>] (platform_drv_probe+0x54/0xb8)
[  113.098652]  r4:c0752d30[  113.101014] 
[  113.102510] [<c0359594>] (platform_drv_probe) from [<c0357b88>] (driver_probe_device+0x228/0x2b8)
[  113.111425]  r7:bf004320[  113.113789]  r6:df516844
 r5:df516810[  113.117379]  r4:c0752d30
[  113.119917] 
[  113.121412] [<c0357960>] (driver_probe_device) from [<c0357cd8>] (__driver_attach+0xc0/0xc4)
[  113.129891]  r9:21242a5c[  113.132255]  r8:00000001
 r7:00000000[  113.135845]  r6:df516844
 r5:bf004320[  113.139436]  r4:df516810
[  113.141980] [<c0357c18>] (__driver_attach) from [<c0355d00>] (bus_for_each_dev+0x70/0xa4)
[  113.150196]  r7:00000000[  113.152560]  r6:c0357c18
 r5:bf004320[  113.156150]  r4:00000000
[  113.158689] 
[  113.160183] [<c0355c90>] (bus_for_each_dev) from [<c03573d4>] (driver_attach+0x24/0x28)
[  113.168224]  r6:c0714090[  113.170587]  r5:df7efc80
 r4:bf004320[  113.174178] 
[  113.175673] [<c03573b0>] (driver_attach) from [<c0356fa0>] (bus_add_driver+0x1a8/0x220)
[  113.183719] [<c0356df8>] (bus_add_driver) from [<c0358498>] (driver_register+0x80/0x100)
[  113.191848]  r7:bf004380[  113.194211]  r6:dedbdc80
 r5:bf006000[  113.197801]  r4:bf004320
[  113.200339] 
[  113.201834] [<c0358418>] (driver_register) from [<c0359548>] (__platform_driver_register+0x48/0x50)
[  113.210924]  r5:bf006000[  113.213287]  r4:c0714090
[  113.215826] 
[  113.217323] [<c0359500>] (__platform_driver_register) from [<bf006020>] (test_driver_init+0x20/0x24 [zozo])
[  113.227113]  r5:bf006000[  113.229475]  r4:ffffe000
[  113.232014] 
[  113.233511] [<bf006000>] (test_driver_init [zozo]) from [<c01017f4>] (do_one_initcall+0x4c/0x17c)
[  113.242432] [<c01017a8>] (do_one_initcall) from [<c01a81b4>] (do_init_module+0x68/0x3a4)
[  113.250561]  r10:dedbd5c8[  113.253011]  r9:21242a5c
 r8:00000001[  113.256602]  r7:bf004380
 r6:dedbdc80[  113.260193]  r5:00000001
[  113.262732]  r4:bf004380[  113.265094] 
[  113.266591] [<c01a814c>] (do_init_module) from [<c01925f0>] (load_module+0x1dc0/0x2180)
[  113.274633]  r6:dedbd5c0[  113.276997]  r5:00000001
 r4:def2df34[  113.280587] 
[  113.282082] [<c0190830>] (load_module) from [<c0192b08>] (SyS_init_module+0x158/0x170)
[  113.290037]  r10:00000051[  113.292487]  r9:def2c000
 r8:01bbf008[  113.296078]  r7:e08682cc
 r6:00000000[  113.299667]  r5:01bc02e4
[  113.302207]  r4:000012cc[  113.304569] 
[  113.306065] [<c01929b0>] (SyS_init_module) from [<c0107ba0>] (ret_fast_syscall+0x0/0x3c)
[  113.314194]  r10:00000000[  113.316645]  r9:def2c000
 r8:c0107d64[  113.320236]  r7:00000080
 r6:be98bb54[  113.323828]  r5:be98bc6d
[  113.326368]  r4:000012cc[  113.328729] 

Are we not supposed to disable the irq in the handler?

Regards.

I assume that this is for the sake of the discussion, and that you do not actually intend to put together such a monstrosity.

[...]

[...]

You can. It then depends on what your interrupt controller does to actually ensure that the interrupt is disabled. Only you can trace it on your HW to find out.

Thanks,

	M.

Jazz is not dead. It just smells funny...

It's just missing a reg properties to be a valid node, right?

I'm using an upstream driver on v4.9-rc1

http://lxr.free-electrons.com/source/drivers/irqchip/irq-tango.c

Given that the system locks up, is it possible there is a bug in the driver?

Which call-back handles enabling/disabling interrupts?

Regards.

If connecting a device that signals its interrupt as level low to an input line configured as level high doesn't strike you as a major issue, nothing will. At that point, you can put anything you want in your DT.

That's possible.

How about irq_unmask/irq_mask?

Thanks,

	M.

Jazz is not dead. It just smells funny.

If I understand correctly, you are saying that I should have specified IRQ_TYPE_LEVEL_LOW, instead of IRQ_TYPE_LEVEL_HIGH?

If the HW outputs 1 when idle, and 0 when busy, that is level low? (Sorry if this is obvious, I'm absolutely clueless in this subject matter.)

I tried following the source from disable_irq_nosync() as far down as I could.

disable_irq_nosync ->
__disable_irq_nosync ->
__disable_irq ->
irq_disable -> ??

http://lxr.free-electrons.com/source/kernel/irq/chip.c#L232

I don't know if desc->irq_data.chip->irq_disable is defined by the driver I'm using?

I don't know how the trail goes to irq_mask?

Regards.

We describe the level which is raising the interrupt. So in your case the line goes to 0 when the interrupt is active, so the level is LOW.

Thanks,

tglx

I see. I'll try that on Monday.

In my mental picture of interrupts (which is obviously so incomplete as to be wrong) interrupts are a way for hardware to tell the CPU that they urgently need the CPU's attention.

Obviously, the hardware being idle (line high) is not an urgent matter which interests the CPU. Likewise, I'm not sure the CPU cares that the hardware is busy (line low). It seems to me the interesting event from the CPU's perspective is when the hardware completes a "task" (transition from low to high).

So I had originally configured the interrupt as IRQ_TYPE_EDGE_RISING. (There is an edge detection block in the irqchip, but the HW designer warned me that at low frequencies, it is possible to "miss" some edges, and we should prefer level triggers if possible.)

Regards.

That's how the CPU interprets it, but this is even more basic than that, see below.

There is no such thing as "busy" when it comes to interrupts. An interrupt signals the CPU that some device-specific condition has been satisfied. It could be "I've received a packet" or "Battery is about to explode", depending if the device is a network controller or a temperature sensor. The interrupt doesn't describe the process that leads to that condition (packet being received or temperature rising), but the condition itself.

In your cases, as the device seems to do some form of processing (you're talking about task completion), then the interrupt seems to describe exactly this ("I'm done").

Level and edge are not interchangeable. They do describe very different thing:

- Level indicates a persistent state, which implies that the device needs to be serviced so that this condition can be cleared (the UART has received a character, and won't be able to received another until it has been read by the CPU). Once the device has been serviced and that condition cleared, it will lower its interrupt line.

- Edge is indicative of an event having occurred ("I'm done") that doesn't require any action from the CPU. Because the device can continue its life without being poked by the CPU, it can continue delivering interrupts even if the first one hasn't been serviced. Being edge triggered, the signals get coalesced into a single interrupt. For example, the temperature sensor will say "Temperature rising" multiple times before the battery explodes, and it is the CPU's job to go and read the sensor to find out by how much it has risen.

If your device only sends a pulse, then it is edge triggered, and it should be treated as such, no matter what your HW guy is saying. This usually involves looking at the device to find out how many times the interrupt has been generated (assuming the device is some kind of processing element). Of course, this is racy (interrupts can still be generated whilst you're processing them), and you should design your interrupt handler to take care of the possible race.

So, to make it short: find out how your device works, and configure your interrupt controller in a similar way. Write your device driver with the interrupt policy in mind (state vs event). Keep it simple.

Thanks,

	M.

Jazz is not dead. It just smells funny.

The device is a graphics engine, which can be programmed to perform some operation on one or several frame buffers stored in memory. It outputs its state (idle vs busy) on interrupt line 23.

With this graphics engine, there is nothing the CPU can do to change what the engine outputs on the interrupt line:

When the graphics engine is idle, the line remains high, forever. When the graphics engine is busy, the line remains low, until all operations have been performed (engine idle).

All the CPU can do is mask the interrupt line at the interrupt controller, as far as I understand.

It is clear that the block does not send a pulse on the interrupt line.

For reasons I don't understand, Linux didn't hang when I set the IRQ type to IRQ_TYPE_EDGE_RISING, so it seemed better than locking up the system.

I'm also fuzzy on what purpose the edge detector is supposed to serve... I had the impression is what supposed to "capture" an edge, to turn it into a level?

Thomas said "We describe the level which is raising the interrupt". But I'm not sure I want the state "engine is busy" to raise an interrupt. "engine is idle" makes more sense. But you said it's stupid to set IRQ_TYPE_LEVEL_HIGH... /me confused

Maybe the fact that disable_irq locks the system up is an orthogonal issue that needs to be fixed anyway.

Regards.

Then this is unambiguously a rising edge interrupt.

Because that's exactly what that is.

If you care to read the explanation I've given above, you'll realize that you cannot turn an edge into a level, because they don't represent the same thing (state vs event). The interrupt controller will latch on a rising edge (for example), and present that information to the CPU, no matter what the line does after that.

Because you insist on considering as a level something that is an edge. Once you try to understand the nature of the signal the device is providing, then you may stop getting confused.

You definitely don't want to generate an interrupt when the device is idle, because that's a state on which you cannot act (apart from constantly generating job for your graphics engine). What you want to detect is the *transition* from busy to idle (event). Nothing else matters.

Indeed.

	M.

Jazz is not dead. It just smells funny...

disable_irq_nosync() eventually calls irq_disable()

void irq_disable(struct irq_desc *desc)

{
	irq_state_set_disabled(desc);
	if (desc->irq_data.chip->irq_disable) {
		desc->irq_data.chip->irq_disable(&desc->irq_data);
		irq_state_set_masked(desc);
	} else if (irq_settings_disable_unlazy(desc)) {
		mask_irq(desc);
	}
}

irq_disable() is a NOP on my platform, because the intc driver does not implement irq_disable, and the second test is false as well in this instance.

The function's description is interesting.

/**
* irq_disable - Mark interrupt disabled
* @desc: irq descriptor which should be disabled
*
* If the chip does not implement the irq_disable callback, we
* use a lazy disable approach. That means we mark the interrupt
* disabled, but leave the hardware unmasked. That's an
* optimization because we avoid the hardware access for the
* common case where no interrupt happens after we marked it
* disabled. If an interrupt happens, then the interrupt flow
* handler masks the line at the hardware level and marks it
* pending.
*
* If the interrupt chip does not implement the irq_disable callback,
* a driver can disable the lazy approach for a particular irq line by
* calling 'irq_set_status_flags(irq, IRQ_DISABLE_UNLAZY)'. This can
* be used for devices which cannot disable the interrupt at the
* device level under certain circumstances and have to use
* disable_irq[_nosync] instead.
*/

(I assume "chip" and "interrupt chip" refer to the same abstraction.)

I took a look at commit e9849777d0e27, but my brain dumped core on the notions of "disabling unlazy" and "disabling a disable".

* IRQ_DISABLE_UNLAZY - Disable lazy irq disable

For the record, setting the IRQ_DISABLE_UNLAZY flag for this device makes the system lock-up disappear.

Regards.

The way how lazy irq disabling works is:

1) Interrupt is marked disabled in software, but the hardware is not masked

2) If the interrupt fires befor the interrupt is reenabled, then it's masked at the hardware level in the low level interrupt flow handler.

I have no idea why that does not work on your hardware. You might instrument handle_level_irq() to see what effect that has.

Thanks,

tglx

Hi Thomas,

Would you mind explaining what is the intention behind? Because it does not seem obvious why there isn't a direct map between "disable_irq*()" and "mask_irq()"

Thanks in advance.
Best regards,

Sebastian

I had a similar, but slightly different question:

What is the difference between struct irq_chip's

* @irq_shutdown: shut down the interrupt (defaults to ->disable if NULL)
* @irq_disable: disable the interrupt
* @irq_mask: mask an interrupt source

(enable seems to default to unmask, but disable does not default to mask)

Regards.

Two reasons for this:

1) If you mask edge type interrupts then you might race with an incoming interrupt which then gets lost and eventually you won't get another interrupt from that device. Even if there is no race, then on many irq chips edge type interrupts are not latched when the interrupt line is masked. That also can result in a stale interrupt line.

With the lazy disabling we mask only if an interrupt fires while it's disabled in software. We note that it is pending and resend it when the interrupt gets reenabled.

2) Accessing irq chip hardware can be slow and we have situations where interrupts are disabled/enabled fast. So it's an optimization to avoid the hardware access, which is sensible as we do not expect an interrupt to fire in most cases. If it fires then we mask it when the interrupt handler sees the disabled flag.

That should really work on any hardware and the IRQ_DISABLE_UNLAZY flag is just there to deal with devices which are known to keep the (level based) irq line active. In that case we know that we always take an interrupt to mask it right away, so we can avoid the overhead.

Though you should not set that flag on edge type interrupts, unless your hardware guarantees to avoid the issues described in #1.

Hope that helps.

Thanks,

tglx

One important difference between disable and mask is that disable is perfectly allowed not to care about pending signals, whereas mask must preserve an interrupt becoming pending whilst masked.

Thanks,

	M.

Jazz is not dead. It just smells funny...

(For my information)

Is it correct to say that "mask" is supposed to defer any interrupt until sometime later; while "disable" will simply discard incoming interrupts, losing them forever.

Is the irq_mask() call-back exposed via some module-visible API?

include/linux/interrupt.h documents mostly enable/disable variants.

extern void disable_irq_nosync(unsigned int irq);
extern bool disable_hardirq(unsigned int irq);
extern void disable_irq(unsigned int irq);
extern void disable_percpu_irq(unsigned int irq);
extern void enable_irq(unsigned int irq);
extern void enable_percpu_irq(unsigned int irq, unsigned int type); extern bool irq_percpu_is_enabled(unsigned int irq); extern void irq_wake_thread(unsigned int irq, void *dev_id);

Regards.

No. And there is no reason to do so.

Thanks,

tglx