Re: [PATCH] Input: uinput - Allow uinput_request to be interrupted

From: Marcos Paulo de Souza
Date: Thu Feb 21 2019 - 06:11:55 EST

Next message: Mike Rapoport: "Re: [PATCH v2] parisc: use memblock_alloc() instead of custom get_memblock()"
Previous message: Stephen Rothwell: "linux-next: Fixes tags need some work in the net-next tree"
In reply to: Dmitry Torokhov: "Re: [PATCH] Input: uinput - Allow uinput_request to be interrupted"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Dmitry,

On 2/18/19 5:15 PM, Dmitry Torokhov wrote:

On Mon, Feb 18, 2019 at 03:21:10PM +0100, Rodrigo Rivas Costa wrote:

On Sun, Feb 17, 2019 at 09:42:52PM -0300, Marcos Paulo de Souza wrote:

- if (!wait_for_completion_timeout(&request->done, 30 * HZ)) {
+ if (!wait_for_completion_interruptible_timeout(&request->done,
+ 30 * HZ)) {
retval = -ETIMEDOUT;
goto out;
}

Now this function can succeed or fail because of ETIMEDOUT or an
interrupt. I think you should return -EINTR or maybe -ESYSRESTART if
interrupted.

Rodrigo, you are right. Marcos, could you please send updated patch that
returns different error code for timeout vs interrupt condition?

Sure. But now I found another issue: If we start fftest and press Ctrl-C, it works (as Rodrigo and we all tested), but if we press -1 (Stopping effects), it gets stuck in the same old way. Now I'm running lockdep and trying to fix this missing case (lockdep added bellow).

This lockdep warning is shown before the fftest can even allow the user to choose between his available options or press -1.

I dropped the patch for now.

Thanks.

[ÂÂ 11.528465] WARNING: possible circular locking dependency detected [37/7926]
[ÂÂ 11.530419] 5.0.0-rc7+ #5 Not tainted
[ÂÂ 11.531368] ------------------------------------------------------
[ÂÂ 11.533295] fftest/200 is trying to acquire lock:
[ÂÂ 11.534713] 000000006528ddcb (&newdev->mutex){+.+.}, at: uinput_request_submit+0x10a/0x320 [uinput]
[ 11.536939]
[ÂÂ 11.536939] but task is already holding lock:
[ÂÂ 11.538338] 000000004113875e (&ff->mutex){+.+.}, at: input_ff_upload+0xa6/0x250
[ 11.540114]
[ÂÂ 11.540114] which lock already depends on the new lock.
[ 11.540114]
[ 11.541966]
[ÂÂ 11.541966] the existing dependency chain (in reverse order) is:
[ 11.543765]
[ÂÂ 11.543765] -> #3 (&ff->mutex){+.+.}:
[ÂÂ 11.544982] input_ff_flush+0x23/0x60
[ÂÂ 11.545933] input_flush_device+0x3b/0x60
[ÂÂ 11.546985]ÂÂÂÂÂÂÂ evdev_flush+0x54/0x60
[ÂÂ 11.547974]ÂÂÂÂÂÂÂ filp_close+0x25/0x70
[ÂÂ 11.548849]ÂÂÂÂÂÂÂ __x64_sys_close+0x19/0x40
[ÂÂ 11.549838]ÂÂÂÂÂÂÂ do_syscall_64+0x4b/0x180
[ÂÂ 11.550853]ÂÂÂÂÂÂÂ entry_SYSCALL_64_after_hwframe+0x49/0xbe
[ÂÂ 11.552584]
[ÂÂ 11.552584] -> #2 (&dev->mutex#2){+.+.}:
[ÂÂ 11.554077]ÂÂÂÂÂÂÂ input_register_handle+0x25/0xc0
[ÂÂ 11.555228]ÂÂÂÂÂÂÂ kbd_connect+0x44/0x90
[ÂÂ 11.556141]ÂÂÂÂÂÂÂ input_attach_handler+0x73/0xb0
[ÂÂ 11.557222]ÂÂÂÂÂÂÂ input_register_device+0x438/0x4b0
[ÂÂ 11.558383]ÂÂÂÂÂÂÂ acpi_button_add+0x179/0x470
[ÂÂ 11.559392]ÂÂÂÂÂÂÂ acpi_device_probe+0x43/0x110
[ÂÂ 11.560432]ÂÂÂÂÂÂÂ really_probe+0x1c4/0x2d0
[ÂÂ 11.561334]ÂÂÂÂÂÂÂ driver_probe_device+0x4a/0xe0
[ÂÂ 11.562286]ÂÂÂÂÂÂÂ __driver_attach+0xb0/0xc0
[ÂÂ 11.563165]ÂÂÂÂÂÂÂ bus_for_each_dev+0x74/0xc0
[ÂÂ 11.564159]ÂÂÂÂÂÂÂ bus_add_driver+0x194/0x210
[ÂÂ 11.565142]ÂÂÂÂÂÂÂ driver_register+0x56/0xe0
[ÂÂ 11.566157]ÂÂÂÂÂÂÂ do_one_initcall+0x58/0x2ae
[ÂÂ 11.567095]ÂÂÂÂÂÂÂ kernel_init_freeable+0x1ca/0x256
[ÂÂ 11.568176]ÂÂÂÂÂÂÂ kernel_init+0x5/0x100
[ÂÂ 11.569032]ÂÂÂÂÂÂÂ ret_from_fork+0x3a/0x50
[ÂÂ 11.569898]
[ÂÂ 11.569898] -> #1 (input_mutex){+.+.}:
[ÂÂ 11.571027]ÂÂÂÂÂÂÂ input_register_device+0x3e6/0x4b0
[ÂÂ 11.572116]ÂÂÂÂÂÂÂ uinput_ioctl_handler.isra.9+0x557/0x980 [uinput]
[ÂÂ 11.573509]ÂÂÂÂÂÂÂ do_vfs_ioctl+0xa0/0x6e0
[ÂÂ 11.574395]ÂÂÂÂÂÂÂ ksys_ioctl+0x6b/0x80
[ÂÂ 11.575234]ÂÂÂÂÂÂÂ __x64_sys_ioctl+0x11/0x20
[ÂÂ 11.576161]ÂÂÂÂÂÂÂ do_syscall_64+0x4b/0x180
[ÂÂ 11.577085]ÂÂÂÂÂÂÂ entry_SYSCALL_64_after_hwframe+0x49/0xbe
[ÂÂ 11.578281]
[ÂÂ 11.578281] -> #0 (&newdev->mutex){+.+.}:
[ÂÂ 11.579436]ÂÂÂÂÂÂÂ __mutex_lock+0x7d/0x9a0
[ÂÂ 11.580381]ÂÂÂÂÂÂÂ uinput_request_submit+0x10a/0x320 [uinput]
[ÂÂ 11.581590]ÂÂÂÂÂÂÂ uinput_dev_upload_effect+0x76/0xb0 [uinput]
[ÂÂ 11.582893]ÂÂÂÂÂÂÂ input_ff_upload+0x1c0/0x250
[ÂÂ 11.583853]ÂÂÂÂÂÂÂ evdev_ioctl_handler+0x388/0xbd0
[ÂÂ 11.584926]ÂÂÂÂÂÂÂ do_vfs_ioctl+0xa0/0x6e0
[ÂÂ 11.586208]ÂÂÂÂÂÂÂ ksys_ioctl+0x6b/0x80
[ÂÂ 11.587292]ÂÂÂÂÂÂÂ __x64_sys_ioctl+0x11/0x20
[ÂÂ 11.588254]ÂÂÂÂÂÂÂ do_syscall_64+0x4b/0x180
[ÂÂ 11.589221]ÂÂÂÂÂÂÂ entry_SYSCALL_64_after_hwframe+0x49/0xbe
[ÂÂ 11.590424]
[ÂÂ 11.590424] other info that might help us debug this:
[ÂÂ 11.590424]
[ÂÂ 11.592162] Chain exists of:
[ÂÂ 11.592162]ÂÂ &newdev->mutex --> &dev->mutex#2 --> &ff->mutex
[ÂÂ 11.592162]
[ÂÂ 11.594156]Â Possible unsafe locking scenario:
[ÂÂ 11.594156]
[ÂÂ 11.595456]ÂÂÂÂÂÂÂ CPU0ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ CPU1
[ÂÂ 11.596452]ÂÂÂÂÂÂÂ ----ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ ----
[ÂÂ 11.597377]ÂÂ lock(&ff->mutex);
[ÂÂ 11.598004] lock(&dev->mutex#2);
[ÂÂ 11.599163] lock(&ff->mutex);
[ÂÂ 11.600282]ÂÂ lock(&newdev->mutex);
[ÂÂ 11.600982]
[ÂÂ 11.600982]Â *** DEADLOCK ***
[ÂÂ 11.600982]
[ÂÂ 11.602144] 2 locks held by fftest/200:
[ÂÂ 11.602915]Â #0: 000000006ae3e58b (&evdev->mutex){+.+.}, at: evdev_ioctl_handler+0x48/0xbd0
[ÂÂ 11.604581]Â #1: 000000004113875e (&ff->mutex){+.+.}, at: input_ff_upload+0xa6/0x250
[ÂÂ 11.606110]
[ÂÂ 11.606110] stack backtrace:
[ÂÂ 11.606982] CPU: 0 PID: 200 Comm: fftest Not tainted 5.0.0-rc7+ #5
[ÂÂ 11.608140] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.11.0-0-g63451fc-prebuilt.qemu-project.org 04/01/2014
[ÂÂ 11.610344] Call Trace:
[ÂÂ 11.610827]Â dump_stack+0x67/0x9b
[ÂÂ 11.611470]Â print_circular_bug.isra.35+0x1ce/0x1db
[ÂÂ 11.612392]Â __lock_acquire+0x15e4/0x1650
[ÂÂ 11.613188]Â ? _raw_spin_unlock_irq+0x24/0x30
[ÂÂ 11.614045]Â ? lock_acquire+0xa7/0x1b0
[ÂÂ 11.614793]Â lock_acquire+0xa7/0x1b0
[ÂÂ 11.615489]Â ? uinput_request_submit+0x10a/0x320 [uinput]
[ÂÂ 11.616526]Â ? uinput_request_submit+0x10a/0x320 [uinput]
[ÂÂ 11.617562]Â __mutex_lock+0x7d/0x9a0
[ÂÂ 11.618243]Â ? uinput_request_submit+0x10a/0x320 [uinput]
[ÂÂ 11.619234]Â ? vprintk_emit+0xec/0x260
[ÂÂ 11.620271]Â ? printk+0x4d/0x69
[ÂÂ 11.621158]Â ? uinput_request_submit+0x10a/0x320 [uinput]
[ÂÂ 11.622464]Â uinput_request_submit+0x10a/0x320 [uinput]
[ÂÂ 11.623587]Â uinput_dev_upload_effect+0x76/0xb0 [uinput]
[ÂÂ 11.624788]Â ? find_held_lock+0x2d/0x90
[ÂÂ 11.625633]Â input_ff_upload+0x1c0/0x250
[ÂÂ 11.626524]Â evdev_ioctl_handler+0x388/0xbd0
[ÂÂ 11.627439]Â do_vfs_ioctl+0xa0/0x6e0
[ÂÂ 11.628215]Â ksys_ioctl+0x6b/0x80
[ÂÂ 11.628967]Â __x64_sys_ioctl+0x11/0x20
[ÂÂ 11.629734]Â do_syscall_64+0x4b/0x180
[ÂÂ 11.630483]Â entry_SYSCALL_64_after_hwframe+0x49/0xbe
[ÂÂ 11.631508] RIP: 0033:0x7f2d99a76467
[ÂÂ 11.632220] Code: b3 66 90 48 8b 05 31 4a 2c 00 64 c7 00 26 00 00 00 48 c7 c0 ff ff ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 b8 10 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 01 4a 2c 00 f7 d8 64 89 01 48
[ÂÂ 11.635876] RSP: 002b:00007ffe1ab6fbd8 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
[ÂÂ 11.637355] RAX: ffffffffffffffda RBX: 00007ffe1ab6fc50 RCX: 00007f2d99a76467
[ÂÂ 11.638766] RDX: 00007ffe1ab6fd10 RSI: 0000000040304580 RDI: 0000000000000003
[ÂÂ 11.640177] RBP: 0000000000000003 R08: 00007f2d99d3d880 R09: 00007f2d99f47500
[ÂÂ 11.641679] R10: 000055c75edf2140 R11: 0000000000000246 R12: 000055c75edf1a80
[ÂÂ 11.643211] R13: 00007ffe1ab6feb0 R14: 0000000000000000 R15: 0000000000000000

Next message: Mike Rapoport: "Re: [PATCH v2] parisc: use memblock_alloc() instead of custom get_memblock()"
Previous message: Stephen Rothwell: "linux-next: Fixes tags need some work in the net-next tree"
In reply to: Dmitry Torokhov: "Re: [PATCH] Input: uinput - Allow uinput_request to be interrupted"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]