Problem since commit c73bbaa4ec3e [rc-core: don't lock device at rc_register_device()]

DISCUSSION Heiner Kallweit 2 participants 7 messages Feb 27, 2016 – Mar 2, 2016 Media (V4L/DVB)

lore

Share & tools

lore

Heiner Kallweit

author

2016-02-27 12:14 UTC #0 lore raw

Since this commit I see the following error when the Nuvoton RC driver is loaded:

input: failed to attach handler kbd to device input3, error: -22

Error 22 (EINVAL) comes from the new check in rc_open().

Heiner Kallweit

author

2016-02-27 16:10 UTC #1 lore raw

Am 27.02.2016 um 13:14 schrieb Heiner Kallweit:

Quoted 6 lines

Since this commit I see the following error when the Nuvoton RC driver is loaded:
input: failed to attach handler kbd to device input3, error: -22
Error 22 (EINVAL) comes from the new check in rc_open().

Complete call chain seems to be:
rc_register_device
input_register_device
input_attach_handler
kbd_connect
input_open_device
ir_open
rc_open

rc_register_device calls input_register_device before dev->initialized = true, therefore the new check in rc_open fails. At a first glance I'd say that we have to remove this check from rc_open.

Regards, Heiner

Mauro Carvalho Chehab

2016-02-27 18:05 UTC #2 lore raw

Em Sat, 27 Feb 2016 17:10:41 +0100
Heiner Kallweit <[email protected]> escreveu:

Quoted 17 lines

Am 27.02.2016 um 13:14 schrieb Heiner Kallweit:
Quoted 6 lines
Since this commit I see the following error when the Nuvoton RC driver is loaded:
input: failed to attach handler kbd to device input3, error: -22
Error 22 (EINVAL) comes from the new check in rc_open().
Complete call chain seems to be:
rc_register_device
input_register_device
input_attach_handler
kbd_connect
input_open_device
ir_open
rc_open

Quoted 3 lines

rc_register_device calls input_register_device before dev->initialized = true, therefore the new check in rc_open fails. At a first glance I'd say that we have to remove this check from rc_open.

Hmm... maybe we could, instead, do:

	if (!rdev->initialized) {
		rval = -ERESTARTSYS;
		goto unlock;
	}

Quoted 3 lines

Regards, Heiner

Signature · 2 lines

Thanks,
Mauro

Heiner Kallweit

author

2016-02-27 18:39 UTC #3 lore raw

Am 27.02.2016 um 19:05 schrieb Mauro Carvalho Chehab:

Quoted 24 lines

Em Sat, 27 Feb 2016 17:10:41 +0100
Heiner Kallweit <[email protected]> escreveu:
Quoted 17 lines
Am 27.02.2016 um 13:14 schrieb Heiner Kallweit:
Quoted 6 lines
Since this commit I see the following error when the Nuvoton RC driver is loaded:
input: failed to attach handler kbd to device input3, error: -22
Error 22 (EINVAL) comes from the new check in rc_open().
Complete call chain seems to be:
rc_register_device
input_register_device
input_attach_handler
kbd_connect
input_open_device
ir_open
rc_open
Quoted 3 lines
rc_register_device calls input_register_device before dev->initialized = true, therefore the new check in rc_open fails. At a first glance I'd say that we have to remove this check from rc_open.

Quoted 7 lines

Hmm... maybe we could, instead, do:
	if (!rdev->initialized) {
		rval = -ERESTARTSYS;
		goto unlock;
	}

Looking at the source code of the functions in the call chain I see no special handling of ERESTARTSYS. It's treated like any other error, therefore I don't think this helps.

Quoted 7 lines

Quoted 5 lines
Regards, Heiner

Mauro Carvalho Chehab

2016-02-27 19:50 UTC #4 lore raw

Em Sat, 27 Feb 2016 19:39:16 +0100
Heiner Kallweit <[email protected]> escreveu:

Quoted 27 lines

Am 27.02.2016 um 19:05 schrieb Mauro Carvalho Chehab:
Quoted 26 lines
Em Sat, 27 Feb 2016 17:10:41 +0100
Heiner Kallweit <[email protected]> escreveu:
Quoted 17 lines
Am 27.02.2016 um 13:14 schrieb Heiner Kallweit:
Quoted 6 lines
Since this commit I see the following error when the Nuvoton RC driver is loaded:
input: failed to attach handler kbd to device input3, error: -22
Error 22 (EINVAL) comes from the new check in rc_open().
Complete call chain seems to be:
rc_register_device
input_register_device
input_attach_handler
kbd_connect
input_open_device
ir_open
rc_open
Quoted 3 lines
rc_register_device calls input_register_device before dev->initialized = true, therefore the new check in rc_open fails. At a first glance I'd say that we have to remove this check from rc_open.
Hmm... maybe we could, instead, do:

Quoted 8 lines

Quoted 5 lines
	if (!rdev->initialized) {
		rval = -ERESTARTSYS;
		goto unlock;
	}
Looking at the source code of the functions in the call chain I see no special handling of ERESTARTSYS. It's treated like any other error, therefore I don't think this helps.

The expected behavior is that the Kernel syscall code to handle ERESTARTSYS internally, and either return EAGAIN to userspace, or try again until it succeeds, depending on the open mode.

So, it seems a worth trial.

Thanks,
Mauro

Heiner Kallweit

author

2016-02-27 21:42 UTC #5 lore raw

Am 27.02.2016 um 20:50 schrieb Mauro Carvalho Chehab:

Quoted 35 lines

Em Sat, 27 Feb 2016 19:39:16 +0100
Heiner Kallweit <[email protected]> escreveu:
Quoted 32 lines
Am 27.02.2016 um 19:05 schrieb Mauro Carvalho Chehab:
Quoted 31 lines
Em Sat, 27 Feb 2016 17:10:41 +0100
Heiner Kallweit <[email protected]> escreveu:
Quoted 17 lines
Am 27.02.2016 um 13:14 schrieb Heiner Kallweit:
Quoted 6 lines
Since this commit I see the following error when the Nuvoton RC driver is loaded:
input: failed to attach handler kbd to device input3, error: -22
Error 22 (EINVAL) comes from the new check in rc_open().
Complete call chain seems to be:
rc_register_device
input_register_device
input_attach_handler
kbd_connect
input_open_device
ir_open
rc_open
Quoted 3 lines
rc_register_device calls input_register_device before dev->initialized = true, therefore the new check in rc_open fails. At a first glance I'd say that we have to remove this check from rc_open.
Hmm... maybe we could, instead, do:
	if (!rdev->initialized) {
		rval = -ERESTARTSYS;
		goto unlock;
	}

Quoted 8 lines

Quoted 3 lines
Looking at the source code of the functions in the call chain I see no special handling of ERESTARTSYS. It's treated like any other error, therefore I don't think this helps.
The expected behavior is that the Kernel syscall code to handle ERESTARTSYS internally, and either return EAGAIN to userspace, or try again until it succeeds, depending on the open mode.

I tested it and returning ERESTARTSYS instead of EINVAL doesn't help. The behavior is the same.
As far as I can see no syscall code is involved in this call chain.

Quoted 5 lines

So, it seems a worth trial.
Thanks,
Mauro

Mauro Carvalho Chehab

2016-03-02 10:59 UTC #6 lore raw

Em Sat, 27 Feb 2016 22:42:24 +0100
Heiner Kallweit <[email protected]> escreveu:

Quoted 40 lines

Am 27.02.2016 um 20:50 schrieb Mauro Carvalho Chehab:
Quoted 39 lines
Em Sat, 27 Feb 2016 19:39:16 +0100
Heiner Kallweit <[email protected]> escreveu:
Quoted 35 lines
Am 27.02.2016 um 19:05 schrieb Mauro Carvalho Chehab:
Quoted 24 lines
Em Sat, 27 Feb 2016 17:10:41 +0100
Heiner Kallweit <[email protected]> escreveu:
Quoted 17 lines
Am 27.02.2016 um 13:14 schrieb Heiner Kallweit:
Quoted 6 lines
Since this commit I see the following error when the Nuvoton RC driver is loaded:
input: failed to attach handler kbd to device input3, error: -22
Error 22 (EINVAL) comes from the new check in rc_open().
Complete call chain seems to be:
rc_register_device
input_register_device
input_attach_handler
kbd_connect
input_open_device
ir_open
rc_open
Quoted 3 lines
rc_register_device calls input_register_device before dev->initialized = true, therefore the new check in rc_open fails. At a first glance I'd say that we have to remove this check from rc_open.
Quoted 7 lines
Hmm... maybe we could, instead, do:
	if (!rdev->initialized) {
		rval = -ERESTARTSYS;
		goto unlock;
	}
Looking at the source code of the functions in the call chain I see no special handling of ERESTARTSYS. It's treated like any other error, therefore I don't think this helps.

Quoted 9 lines

Quoted 4 lines
The expected behavior is that the Kernel syscall code to handle ERESTARTSYS internally, and either return EAGAIN to userspace, or try again until it succeeds, depending on the open mode.
I tested it and returning ERESTARTSYS instead of EINVAL doesn't help. The behavior is the same.
As far as I can see no syscall code is involved in this call chain.
Quoted 1 line
So, it seems a worth trial.

Yeah, this didn't work. The problem is that rc_open can be called too early, by the time the input device got registered.

I have a patch for it. Will send on a separate e-mail

Yet, there's something still not right: after removing nuvoton_cir, a new modprobe to the driver causes a dead lock:

[  242.151822] INFO: task modprobe:2599 blocked for more than 120 seconds.
[  242.151952]       Not tainted 4.5.0-rc3+ #49
[  242.151955] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
[  242.151958] modprobe        D ffff8803b1037610     0  2599   2598 0x00000000
[  242.151964]  ffff8803b1037610 ffffffff82aa27f8 ffff8803b10375c8 ffffffff812404fa
[  242.151970]  0000000000000282 ffff8803bf076340 ffff8803c689f258 ffffffff00000000
[  242.151975]  ffff8803c2198000 ffff8803bf076000 ffff8803b1030000 ffffed0076206001
[  242.151981] Call Trace:
[  242.151988]  [<ffffffff812404fa>] ? trace_hardirqs_on_caller+0x40a/0x590
[  242.151992]  [<ffffffff822dc6dc>] schedule+0x9c/0x1c0
[  242.151996]  [<ffffffff8115fd02>] __request_region+0x2c2/0x440
[  242.152002]  [<ffffffff8115fa40>] ? free_resource+0x180/0x180
[  242.152005]  [<ffffffff812f6eb5>] ? is_module_address+0x15/0x30
[  242.152009]  [<ffffffff812381d3>] ? static_obj+0x33/0x50
[  242.152012]  [<ffffffff81239680>] ? lockdep_init_map+0xf0/0x1470
[  242.152016]  [<ffffffff811d3980>] ? wake_up_q+0xe0/0xe0
[  242.152023]  [<ffffffffa125a4c8>] nvt_probe+0x518/0x26a0 [nuvoton_cir]
[  242.152031]  [<ffffffff81b913a0>] ? compare_pnp_id+0x90/0x210
[  242.152041]  [<ffffffffa1259fb0>] ? nvt_tx_ir+0x3f0/0x3f0 [nuvoton_cir]
[  242.152047]  [<ffffffff81b91735>] pnp_device_probe+0x125/0x1f0
[  242.152054]  [<ffffffff81ce7e7a>] driver_probe_device+0x21a/0xc30
[  242.152060]  [<ffffffff81ce8890>] ? driver_probe_device+0xc30/0xc30
[  242.152066]  [<ffffffff81ce89b1>] __driver_attach+0x121/0x160
[  242.152072]  [<ffffffff81ce21bf>] bus_for_each_dev+0x11f/0x1a0
[  242.152077]  [<ffffffff81ce20a0>] ? subsys_dev_iter_exit+0x10/0x10
[  242.152083]  [<ffffffff822e7be7>] ? _raw_spin_unlock+0x27/0x40
[  242.152089]  [<ffffffff81ce6cdd>] driver_attach+0x3d/0x50
[  242.152095]  [<ffffffff81ce5df9>] bus_add_driver+0x4c9/0x770
[  242.152101]  [<ffffffffa0cd0000>] ? 0xffffffffa0cd0000
[  242.152107]  [<ffffffffa0cd0000>] ? 0xffffffffa0cd0000
[  242.152112]  [<ffffffff81cea39c>] driver_register+0x18c/0x3b0
[  242.152119]  [<ffffffff81b912c5>] pnp_register_driver+0x75/0xa0
[  242.152126]  [<ffffffffa0cd0010>] nvt_driver_init+0x10/0x1000 [nuvoton_cir]
[  242.152132]  [<ffffffff810021b1>] do_one_initcall+0x141/0x300
[  242.152138]  [<ffffffff81002070>] ? try_to_run_init_process+0x40/0x40
[  242.152145]  [<ffffffff8155f696>] ? kasan_unpoison_shadow+0x36/0x50
[  242.152150]  [<ffffffff8155f696>] ? kasan_unpoison_shadow+0x36/0x50
[  242.152156]  [<ffffffff8155f7a7>] ? __asan_register_globals+0x87/0xa0
[  242.152164]  [<ffffffff8144d8eb>] do_init_module+0x1d0/0x5ad
[  242.152170]  [<ffffffff812f27b6>] load_module+0x6666/0x9ba0
[  242.152177]  [<ffffffff812e9e20>] ? symbol_put_addr+0x50/0x50
[  242.152191]  [<ffffffff812ec150>] ? module_frob_arch_sections+0x20/0x20
[  242.152197]  [<ffffffff815be010>] ? open_exec+0x50/0x50
[  242.152204]  [<ffffffff8116716b>] ? ns_capable+0x5b/0xd0
[  242.152210]  [<ffffffff812f5fe8>] SyS_finit_module+0x108/0x130
[  242.152215]  [<ffffffff812f5ee0>] ? SyS_init_module+0x1f0/0x1f0
[  242.152221]  [<ffffffff81004044>] ? lockdep_sys_exit_thunk+0x12/0x14
[  242.152228]  [<ffffffff822e8636>] entry_SYSCALL_64_fastpath+0x16/0x76
[  242.152233] 2 locks held by modprobe/2599:
[  242.152237]  #0:  (&dev->mutex){......}, at: [<ffffffff81ce8933>] __driver_attach+0xa3/0x160
[  242.152252]  #1:  (&dev->mutex){......}, at: [<ffffffff81ce8941>] __driver_attach+0xb1/0x160