Disabling an interrupt in the handler locks the system up

Disabling an interrupt in the handler locks the system up

[Mason]

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@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.

Disabling an interrupt in the handler locks the system up

[Mason]

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.

Re: Disabling an interrupt in the handler locks the system up

[Marc Zyngier]

On 21/10/16 17:37, Mason wrote:
> 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@22222 {
> 			compatible = "vendor,testme";
> 			interrupts = <23 IRQ_TYPE_LEVEL_HIGH>;
> 		};

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

> 
> 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.

[...]

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

[...]

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

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...

Disabling an interrupt in the handler locks the system up

[Marc Zyngier]

On 21/10/16 17:37, Mason wrote:
> 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 assume that this is for the sake of the discussion, and that you do
not actually intend to put together such a monstrosity.

> 
> 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.

[...]

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

[...]

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

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...

Re: Disabling an interrupt in the handler locks the system up

[Mason]

On 21/10/2016 19:46, Marc Zyngier wrote:

> On 21/10/16 17:37, Mason wrote:
>
>> 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@22222 {
>> 			compatible = "vendor,testme";
>> 			interrupts = <23 IRQ_TYPE_LEVEL_HIGH>;
>> 		};
> 
> I assume that this is for the sake of the discussion, and that you do
> not actually intend to put together such a monstrosity.

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

>> 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.
> 
> [...]
> 
>> And here's what I get when I try to load the module:
>> (I'm using the default CONFIG_RCU_CPU_STALL_TIMEOUT=21)
> 
> [...]
> 
>> Are we not supposed to disable the irq in the handler?
> 
> 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.

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.

Disabling an interrupt in the handler locks the system up

[Mason]

On 21/10/2016 19:46, Marc Zyngier wrote:

> On 21/10/16 17:37, Mason wrote:
>
>> 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 assume that this is for the sake of the discussion, and that you do
> not actually intend to put together such a monstrosity.

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

>> 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.
> 
> [...]
> 
>> And here's what I get when I try to load the module:
>> (I'm using the default CONFIG_RCU_CPU_STALL_TIMEOUT=21)
> 
> [...]
> 
>> Are we not supposed to disable the irq in the handler?
> 
> 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.

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.

Re: Disabling an interrupt in the handler locks the system up

[Marc Zyngier]

On Fri, 21 Oct 2016 20:39:41 +0200
Mason <slash.tmp@free.fr> wrote:

> On 21/10/2016 19:46, Marc Zyngier wrote:
> 
> > On 21/10/16 17:37, Mason wrote:
> >  
> >> 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@22222 {
> >> 			compatible = "vendor,testme";
> >> 			interrupts = <23 IRQ_TYPE_LEVEL_HIGH>;
> >> 		};  
> > 
> > I assume that this is for the sake of the discussion, and that you do
> > not actually intend to put together such a monstrosity.  
> 
> It's just missing a reg properties to be a valid node, right?

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.

> 
> >> 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.  
> > 
> > [...]
> >   
> >> And here's what I get when I try to load the module:
> >> (I'm using the default CONFIG_RCU_CPU_STALL_TIMEOUT=21)  
> > 
> > [...]
> >   
> >> Are we not supposed to disable the irq in the handler?  
> > 
> > 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.  
> 
> 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?

That's possible.

> Which call-back handles enabling/disabling interrupts?

How about irq_unmask/irq_mask?

Thanks,

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

Disabling an interrupt in the handler locks the system up

[Marc Zyngier]

On Fri, 21 Oct 2016 20:39:41 +0200
Mason <slash.tmp@free.fr> wrote:

> On 21/10/2016 19:46, Marc Zyngier wrote:
> 
> > On 21/10/16 17:37, Mason wrote:
> >  
> >> 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 assume that this is for the sake of the discussion, and that you do
> > not actually intend to put together such a monstrosity.  
> 
> It's just missing a reg properties to be a valid node, right?

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.

> 
> >> 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.  
> > 
> > [...]
> >   
> >> And here's what I get when I try to load the module:
> >> (I'm using the default CONFIG_RCU_CPU_STALL_TIMEOUT=21)  
> > 
> > [...]
> >   
> >> Are we not supposed to disable the irq in the handler?  
> > 
> > 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.  
> 
> 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?

That's possible.

> Which call-back handles enabling/disabling interrupts?

How about irq_unmask/irq_mask?

Thanks,

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

Re: Disabling an interrupt in the handler locks the system up

[Mason]

On 21/10/2016 21:14, Marc Zyngier wrote:

> Mason wrote:
> 
>> On 21/10/2016 19:46, Marc Zyngier wrote:
>>
>>> On 21/10/16 17:37, Mason wrote:
>>>  
>>>> 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@22222 {
>>>> 			compatible = "vendor,testme";
>>>> 			interrupts = <23 IRQ_TYPE_LEVEL_HIGH>;
>>>> 		};  
>>>
>>> I assume that this is for the sake of the discussion, and that you do
>>> not actually intend to put together such a monstrosity.  
>>
>> It's just missing a reg properties to be a valid node, right?
> 
> 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.

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 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.  
>>>
>>> [...]
>>>   
>>>> And here's what I get when I try to load the module:
>>>> (I'm using the default CONFIG_RCU_CPU_STALL_TIMEOUT=21)  
>>>
>>> [...]
>>>   
>>>> Are we not supposed to disable the irq in the handler?  
>>>
>>> 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.  
>>
>> 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?
> 
> That's possible.
> 
>> Which call-back handles enabling/disabling interrupts?
> 
> How about irq_unmask/irq_mask?

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.

Disabling an interrupt in the handler locks the system up

[Mason]

On 21/10/2016 21:14, Marc Zyngier wrote:

> Mason wrote:
> 
>> On 21/10/2016 19:46, Marc Zyngier wrote:
>>
>>> On 21/10/16 17:37, Mason wrote:
>>>  
>>>> 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 assume that this is for the sake of the discussion, and that you do
>>> not actually intend to put together such a monstrosity.  
>>
>> It's just missing a reg properties to be a valid node, right?
> 
> 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.

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 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.  
>>>
>>> [...]
>>>   
>>>> And here's what I get when I try to load the module:
>>>> (I'm using the default CONFIG_RCU_CPU_STALL_TIMEOUT=21)  
>>>
>>> [...]
>>>   
>>>> Are we not supposed to disable the irq in the handler?  
>>>
>>> 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.  
>>
>> 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?
> 
> That's possible.
> 
>> Which call-back handles enabling/disabling interrupts?
> 
> How about irq_unmask/irq_mask?

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.

Re: Disabling an interrupt in the handler locks the system up

[Thomas Gleixner]

On Fri, 21 Oct 2016, Mason wrote:
> On 21/10/2016 21:14, Marc Zyngier wrote:
> > 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.
> 
> 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.)

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

Disabling an interrupt in the handler locks the system up

[Thomas Gleixner]

On Fri, 21 Oct 2016, Mason wrote:
> On 21/10/2016 21:14, Marc Zyngier wrote:
> > 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.
> 
> 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.)

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

Re: Disabling an interrupt in the handler locks the system up

[Mason]

On 21/10/2016 21:49, Thomas Gleixner wrote:
> On Fri, 21 Oct 2016, Mason wrote:
>> On 21/10/2016 21:14, Marc Zyngier wrote:
>>> 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.
>>
>> 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.)
> 
> 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.

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.

Disabling an interrupt in the handler locks the system up

[Mason]

On 21/10/2016 21:49, Thomas Gleixner wrote:
> On Fri, 21 Oct 2016, Mason wrote:
>> On 21/10/2016 21:14, Marc Zyngier wrote:
>>> 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.
>>
>> 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.)
> 
> 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.

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.

Re: Disabling an interrupt in the handler locks the system up

[Marc Zyngier]

On Fri, 21 Oct 2016 22:27:23 +0200
Mason <slash.tmp@free.fr> wrote:

> On 21/10/2016 21:49, Thomas Gleixner wrote:
> > On Fri, 21 Oct 2016, Mason wrote:  
> >> On 21/10/2016 21:14, Marc Zyngier wrote:  
> >>> 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.  
> >>
> >> 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.)  
> > 
> > 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.  
> 
> 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.

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

> 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).

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").

> 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.)

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.

Disabling an interrupt in the handler locks the system up

[Marc Zyngier]

On Fri, 21 Oct 2016 22:27:23 +0200
Mason <slash.tmp@free.fr> wrote:

> On 21/10/2016 21:49, Thomas Gleixner wrote:
> > On Fri, 21 Oct 2016, Mason wrote:  
> >> On 21/10/2016 21:14, Marc Zyngier wrote:  
> >>> 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.  
> >>
> >> 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.)  
> > 
> > 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.  
> 
> 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.

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

> 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).

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").

> 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.)

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.

Re: Disabling an interrupt in the handler locks the system up

[Mason]

On 22/10/2016 13:37, Marc Zyngier wrote:

> Mason wrote:
> 
>> 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.
> 
> That's how the CPU interprets it, but this is even more basic than
> that, see below.
> 
>> 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).
> 
> 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").

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.

>> 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.)
> 
> 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.

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.

> - 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.

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?

> 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.

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.

Disabling an interrupt in the handler locks the system up

[Mason]

On 22/10/2016 13:37, Marc Zyngier wrote:

> Mason wrote:
> 
>> 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.
> 
> That's how the CPU interprets it, but this is even more basic than
> that, see below.
> 
>> 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).
> 
> 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").

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.

>> 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.)
> 
> 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.

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.

> - 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.

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?

> 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.

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.

Re: Disabling an interrupt in the handler locks the system up

[Marc Zyngier]

On 23/10/16 00:10, Mason wrote:
> On 22/10/2016 13:37, Marc Zyngier wrote:
> 
>> Mason wrote:
>>
>>> 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.
>>
>> That's how the CPU interprets it, but this is even more basic than
>> that, see below.
>>
>>> 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).
>>
>> 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").
> 
> 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.
> 
>>> 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.)
>>
>> 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.
> 
> 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.

Then this is unambiguously a rising edge interrupt.

> 
>> - 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.
> 
> 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.

Because that's exactly what that is.

> 
> 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?

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.

>> 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.
> 
> 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

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.

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

Indeed.

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

Disabling an interrupt in the handler locks the system up

[Marc Zyngier]

On 23/10/16 00:10, Mason wrote:
> On 22/10/2016 13:37, Marc Zyngier wrote:
> 
>> Mason wrote:
>>
>>> 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.
>>
>> That's how the CPU interprets it, but this is even more basic than
>> that, see below.
>>
>>> 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).
>>
>> 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").
> 
> 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.
> 
>>> 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.)
>>
>> 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.
> 
> 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.

Then this is unambiguously a rising edge interrupt.

> 
>> - 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.
> 
> 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.

Because that's exactly what that is.

> 
> 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?

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.

>> 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.
> 
> 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

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.

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

Indeed.

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

Re: Disabling an interrupt in the handler locks the system up

[Mason]

On 23/10/2016 01:10, Mason wrote:

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

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.

Disabling an interrupt in the handler locks the system up

[Mason]

On 23/10/2016 01:10, Mason wrote:

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

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.

Re: Disabling an interrupt in the handler locks the system up

[Thomas Gleixner]

On Mon, 24 Oct 2016, Mason wrote:
> 
> For the record, setting the IRQ_DISABLE_UNLAZY flag for this device
> makes the system lock-up disappear.

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

Disabling an interrupt in the handler locks the system up

[Thomas Gleixner]

On Mon, 24 Oct 2016, Mason wrote:
> 
> For the record, setting the IRQ_DISABLE_UNLAZY flag for this device
> makes the system lock-up disappear.

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

Re: Disabling an interrupt in the handler locks the system up

[Sebastian Frias]

Hi Thomas,

On 10/24/2016 06:55 PM, Thomas Gleixner wrote:
> On Mon, 24 Oct 2016, Mason wrote:
>>
>> For the record, setting the IRQ_DISABLE_UNLAZY flag for this device
>> makes the system lock-up disappear.
> 
> 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.
> 

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

Disabling an interrupt in the handler locks the system up

[Sebastian Frias]

Hi Thomas,

On 10/24/2016 06:55 PM, Thomas Gleixner wrote:
> On Mon, 24 Oct 2016, Mason wrote:
>>
>> For the record, setting the IRQ_DISABLE_UNLAZY flag for this device
>> makes the system lock-up disappear.
> 
> 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.
> 

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

Re: Disabling an interrupt in the handler locks the system up

[Mason]

On 25/10/2016 10:29, Sebastian Frias wrote:

> On 10/24/2016 06:55 PM, Thomas Gleixner wrote:
>
>> On Mon, 24 Oct 2016, Mason wrote:
>>
>>> For the record, setting the IRQ_DISABLE_UNLAZY flag for this device
>>> makes the system lock-up disappear.
>>
>> 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.
> 
> 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()"

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.

Disabling an interrupt in the handler locks the system up

[Mason]

On 25/10/2016 10:29, Sebastian Frias wrote:

> On 10/24/2016 06:55 PM, Thomas Gleixner wrote:
>
>> On Mon, 24 Oct 2016, Mason wrote:
>>
>>> For the record, setting the IRQ_DISABLE_UNLAZY flag for this device
>>> makes the system lock-up disappear.
>>
>> 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.
> 
> 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()"

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.

Re: Disabling an interrupt in the handler locks the system up

[Thomas Gleixner]

On Tue, 25 Oct 2016, Sebastian Frias wrote:
> On 10/24/2016 06:55 PM, Thomas Gleixner wrote:
> > On Mon, 24 Oct 2016, Mason wrote:
> >>
> >> For the record, setting the IRQ_DISABLE_UNLAZY flag for this device
> >> makes the system lock-up disappear.
> > 
> > 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.
> > 
> 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()"

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

Disabling an interrupt in the handler locks the system up

[Thomas Gleixner]

On Tue, 25 Oct 2016, Sebastian Frias wrote:
> On 10/24/2016 06:55 PM, Thomas Gleixner wrote:
> > On Mon, 24 Oct 2016, Mason wrote:
> >>
> >> For the record, setting the IRQ_DISABLE_UNLAZY flag for this device
> >> makes the system lock-up disappear.
> > 
> > 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.
> > 
> 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()"

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

Re: Disabling an interrupt in the handler locks the system up

[Marc Zyngier]

On 25/10/16 09:36, Mason wrote:
> On 25/10/2016 10:29, Sebastian Frias wrote:
> 
>> On 10/24/2016 06:55 PM, Thomas Gleixner wrote:
>>
>>> On Mon, 24 Oct 2016, Mason wrote:
>>>
>>>> For the record, setting the IRQ_DISABLE_UNLAZY flag for this device
>>>> makes the system lock-up disappear.
>>>
>>> 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.
>>
>> 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()"
> 
> 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

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...

Disabling an interrupt in the handler locks the system up

[Marc Zyngier]

On 25/10/16 09:36, Mason wrote:
> On 25/10/2016 10:29, Sebastian Frias wrote:
> 
>> On 10/24/2016 06:55 PM, Thomas Gleixner wrote:
>>
>>> On Mon, 24 Oct 2016, Mason wrote:
>>>
>>>> For the record, setting the IRQ_DISABLE_UNLAZY flag for this device
>>>> makes the system lock-up disappear.
>>>
>>> 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.
>>
>> 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()"
> 
> 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

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...

Re: Disabling an interrupt in the handler locks the system up

[Mason]

On 25/10/2016 12:45, Marc Zyngier wrote:
> On 25/10/16 09:36, Mason wrote:
>> On 25/10/2016 10:29, Sebastian Frias wrote:
>>
>>> On 10/24/2016 06:55 PM, Thomas Gleixner wrote:
>>>
>>>> On Mon, 24 Oct 2016, Mason wrote:
>>>>
>>>>> For the record, setting the IRQ_DISABLE_UNLAZY flag for this device
>>>>> makes the system lock-up disappear.
>>>>
>>>> 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.
>>>
>>> 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()"
>>
>> 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
> 
> 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.

(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.

Disabling an interrupt in the handler locks the system up

[Mason]

On 25/10/2016 12:45, Marc Zyngier wrote:
> On 25/10/16 09:36, Mason wrote:
>> On 25/10/2016 10:29, Sebastian Frias wrote:
>>
>>> On 10/24/2016 06:55 PM, Thomas Gleixner wrote:
>>>
>>>> On Mon, 24 Oct 2016, Mason wrote:
>>>>
>>>>> For the record, setting the IRQ_DISABLE_UNLAZY flag for this device
>>>>> makes the system lock-up disappear.
>>>>
>>>> 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.
>>>
>>> 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()"
>>
>> 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
> 
> 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.

(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.

Re: Disabling an interrupt in the handler locks the system up

[Thomas Gleixner]

On Tue, 25 Oct 2016, Mason wrote:
> Is the irq_mask() call-back exposed via some module-visible API?

No. And there is no reason to do so.

Thanks,

	tglx

Disabling an interrupt in the handler locks the system up

[Thomas Gleixner]

On Tue, 25 Oct 2016, Mason wrote:
> Is the irq_mask() call-back exposed via some module-visible API?

No. And there is no reason to do so.

Thanks,

	tglx