Re: [PATCH] iio: buffer: Silence lock nesting splat

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 8/20/22 13:06, Jonathan Cameron wrote:
On Tue, 16 Aug 2022 10:08:28 +0200
Vincent Whitchurch <vincent.whitchurch@xxxxxxxx> wrote:

If an IIO driver uses callbacks from another IIO driver and calls
iio_channel_start_all_cb() from one of its buffer setup ops, then
lockdep complains due to the lock nesting, as in the below example with
lmp91000.  Since the locks are being taken on different IIO devices,
there is no actual deadlock, so add lock nesting annotation to silence
the spurious warning.

  ============================================
  WARNING: possible recursive locking detected
  6.0.0-rc1+ #10 Not tainted
  --------------------------------------------
  python3/23 is trying to acquire lock:
  0000000064c944c0 (&indio_dev->mlock){+.+.}-{3:3}, at: iio_update_buffers+0x62/0x180

  but task is already holding lock:
  00000000636b64c0 (&indio_dev->mlock){+.+.}-{3:3}, at: enable_store+0x4d/0x100

  other info that might help us debug this:
   Possible unsafe locking scenario:

         CPU0
         ----
    lock(&indio_dev->mlock);
    lock(&indio_dev->mlock);

   *** DEADLOCK ***

   May be due to missing lock nesting notation

  5 locks held by python3/23:
   #0: 00000000636b5420 (sb_writers#5){.+.+}-{0:0}, at: ksys_write+0x67/0x100
   #1: 0000000064c19280 (&of->mutex){+.+.}-{3:3}, at: kernfs_fop_write_iter+0x13a/0x270
   #2: 0000000064c3d9e0 (kn->active#14){.+.+}-{0:0}, at: kernfs_fop_write_iter+0x149/0x270
   #3: 00000000636b64c0 (&indio_dev->mlock){+.+.}-{3:3}, at: enable_store+0x4d/0x100
   #4: 0000000064c945c8 (&iio_dev_opaque->info_exist_lock){+.+.}-{3:3}, at: iio_update_buffers+0x4f/0x180

  stack backtrace:
  CPU: 0 PID: 23 Comm: python3 Not tainted 6.0.0-rc1+ #10
  Call Trace:
   dump_stack+0x1a/0x1c
   __lock_acquire.cold+0x407/0x42d
   lock_acquire+0x1ed/0x310
   __mutex_lock+0x72/0xde0
   mutex_lock_nested+0x1d/0x20
   iio_update_buffers+0x62/0x180
   iio_channel_start_all_cb+0x1c/0x20 [industrialio_buffer_cb]
   lmp91000_buffer_postenable+0x1b/0x20 [lmp91000]
   __iio_update_buffers+0x50b/0xd80
   enable_store+0x81/0x100
   dev_attr_store+0xf/0x20
   sysfs_kf_write+0x4c/0x70
   kernfs_fop_write_iter+0x179/0x270
   new_sync_write+0x99/0x120
   vfs_write+0x2c1/0x470
   ksys_write+0x67/0x100
   sys_write+0x10/0x20

Signed-off-by: Vincent Whitchurch <vincent.whitchurch@xxxxxxxx>
I'm wondering if this is sufficient.
At first glance there are a whole bunch of other possible cases of this.
Any consumer driver that calls iio_device_claim_direct_mode() would be a
problem - though I'm not sure any do?

I'm not sure I properly understand lockdep notations, but I thought the
point was we needed to define a hierarchy?  To do that here we need
an IIO driver that is a consumer to somehow let the IIO core know that
and mark all calls to the locks appropriately.  This gets trickier
as we allow 3+ levels of IIO drivers calling into each other.

We should also think about how to prevent recursion if there are 3
IIO drivers involved.

There are two different approaches for this kind of nested locking. One is to use mutex_lock_nested(). This works if there is a strict hierarchy. The I2C framework for example has a function to determine the position of a I2C mux in the hierarchy and uses that for locking. See https://elixir.bootlin.com/linux/latest/source/drivers/i2c/i2c-core-base.c#L1151.

I'm not sure this directly translates to IIO since the consumers/producers don't have to be a in strict hierarchy.  And if it is a complex graph it can be difficult to figure out the right level for mutex_lock_nested().

The other method is to mark each mutex as its own class. lockdep does the lock checking based on the lock class and by default the same mutex of different instances is considered the same class to keep the resource requirements for the checker lower.

Regmap for example does this. See https://elixir.bootlin.com/linux/latest/source/drivers/base/regmap/regmap.c#L795.

This could be a solution for IIO with the downside how the additional work for the checker. But as long as there are only a few IIO devices per system that should be OK. We could also only set the per device lock class if in kernel consumers are enabled.





[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux