Re: Possible deadlock in mpu6050 on buffer disable?

[Lars-Peter Clausen <lars@metafoo.de>]

On 05/23/2016 12:33 PM, Crestez Dan Leonard wrote:
> Hello,
> 
> I've been poking at this driver and I get some occasional deadlocks when
> disabling buffering. Here is a snippet from the kernel task dump (via
> echo t > /proc/sysrq-trigger):
> 
> [   57.562487] irq/56-mpu6500_ D ffff88003d7fbd08 13632   771      2
> 0x00000000
> [   57.563081]  ffff88003d7fbd08 ffff88003da4a1c0 ffffffff81988ae0
> ffff880000000026
> [   57.563614]  ffff88003d7fc000 ffff88003da4a1c0 ffff88003d4aa304
> 00000000ffffffff
> [   57.564221]  ffff88003d4aa308 ffff88003d7fbd20 ffffffff8147f280
> ffff88003d4aa300
> [   57.565242] Call Trace:
> [   57.565462]  [<ffffffff8147f280>] schedule+0x30/0x80
> [   57.565789]  [<ffffffff8147f4f9>] schedule_preempt_disabled+0x9/0x10
> [   57.566303]  [<ffffffff81480c8a>] __mutex_lock_slowpath+0x8a/0x100
> [   57.566704]  [<ffffffff810875a0>] ? irq_forced_thread_fn+0x60/0x60
> [   57.567202]  [<ffffffff81480d1a>] mutex_lock+0x1a/0x30
> [   57.567541]  [<ffffffff813bc31e>] inv_mpu6050_read_fifo+0x6e/0x230
> [   57.567934]  [<ffffffff810737d6>] ? put_prev_task_rt+0x16/0xa0
> [   57.568428]  [<ffffffff8147ed1c>] ? __schedule+0x32c/0x860
> [   57.568789]  [<ffffffff810875a0>] ? irq_forced_thread_fn+0x60/0x60
> [   57.569289]  [<ffffffff810875bb>] irq_thread_fn+0x1b/0x50
> [   57.569646]  [<ffffffff81087851>] irq_thread+0x111/0x190
> [   57.569997]  [<ffffffff81087690>] ? wake_threads_waitq+0x30/0x30
> [   57.570505]  [<ffffffff81087740>] ? irq_thread_dtor+0xb0/0xb0
> [   57.570875]  [<ffffffff8105eae4>] kthread+0xc4/0xe0
> [   57.571308]  [<ffffffff81482a92>] ret_from_fork+0x22/0x40
> [   57.571663]  [<ffffffff8105ea20>] ? kthread_worker_fn+0x160/0x160
> 
> VERSUS:
> 
> [   57.587592] test-iio-buffer D ffff88003d1a3c08 13808   798    791
> 0x00000004
> [   57.588193]  ffff88003d1a3c08 ffff88003db02d00 ffff88003d1a3be8
> ffffffff81106abc
> [   57.588772]  ffff88003d1a4000 ffff88003d5b72d0 0000000000000038
> ffff88003d5b729c
> [   57.589388]  ffff88003d5b7200 ffff88003d1a3c20 ffffffff8147f280
> ffff88003d5b7200
> [   57.589963] Call Trace:
> [   57.590211]  [<ffffffff81106abc>] ? kfree+0xec/0xf0
> [   57.590537]  [<ffffffff8147f280>] schedule+0x30/0x80
> [   57.590904]  [<ffffffff81086d14>] synchronize_irq+0x54/0x90
> [   57.591600]  [<ffffffff810772c0>] ? wake_atomic_t_function+0x60/0x60
> [   57.592073]  [<ffffffff81087ba6>] __free_irq+0x136/0x270
> [   57.592430]  [<ffffffff81087d54>] free_irq+0x34/0x80
> [   57.592760]  [<ffffffff813b93b5>] iio_trigger_detach_poll_func+0x75/0xa0
> [   57.593267]  [<ffffffff813b93f7>]
> iio_triggered_buffer_predisable+0x17/0x20
> [   57.593711]  [<ffffffff813b73ca>] iio_disable_buffers+0x3a/0xf0
> [   57.594164]  [<ffffffff813b8202>] __iio_update_buffers+0x222/0x4e0
> [   57.594559]  [<ffffffff813b865b>] iio_buffer_store_enable+0xab/0xc0
> [   57.594973]  [<ffffffff81308a23>] dev_attr_store+0x13/0x20
> [   57.595388]  [<ffffffff8116ff72>] sysfs_kf_write+0x32/0x40
> [   57.595734]  [<ffffffff8116f565>] kernfs_fop_write+0x115/0x170
> [   57.596170]  [<ffffffff8110b093>] __vfs_write+0x23/0xe0
> [   57.596526]  [<ffffffff8112755f>] ? __fd_install+0x1f/0xc0
> [   57.596871]  [<ffffffff811273da>] ? __alloc_fd+0x3a/0x170
> [   57.597308]  [<ffffffff81078b3d>] ? percpu_down_read+0xd/0x50
> [   57.597667]  [<ffffffff8110bf74>] vfs_write+0xa4/0x1a0
> [   57.598023]  [<ffffffff81109241>] ? filp_close+0x51/0x70
> [   57.598396]  [<ffffffff8110d251>] SyS_write+0x41/0xa0
> [   57.598717]  [<ffffffff8148289b>] entry_SYSCALL_64_fastpath+0x13/0x8f
> 
> The problem seems to be that inv_mpu6050_read_fifo takes
> indio_dev->mlock, the same lock taken by the IIO core when disabling the
> interrupt.
> 
> Does anyone have any clever ideas for fixing this? One way to do it
> would be to skip taking mlock in inv_mpu6050_read_fifo because most of
> the code does not touch shared fields. Only when an error happens does
> it call inv_reset_fifo. I think that call should simply be removed.

Yes, that's probably the way to go.

> As a more general principle isn't it a bad idea to take mlock in driver
> code? It seems much saner for drivers to use their own locks for their
> private data.

There are a few places where it is safe to use the mlock, like the driver
{read,write}_raw() callbacks, but that's pretty mich it.