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 <[email protected]>
---
drivers/iio/industrialio-buffer.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index acc2b6c05d57..27868ed092d0 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -1255,7 +1255,7 @@ int iio_update_buffers(struct iio_dev *indio_dev,
return -EINVAL;
mutex_lock(&iio_dev_opaque->info_exist_lock);
- mutex_lock(&indio_dev->mlock);
+ mutex_lock_nested(&indio_dev->mlock, SINGLE_DEPTH_NESTING);
if (insert_buffer && iio_buffer_is_active(insert_buffer))
insert_buffer = NULL;
--
2.34.1
On Tue, Aug 16, 2022 at 1:30 PM Vincent Whitchurch
<[email protected]> 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
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#backtraces-in-commit-mesages
On top of that, Fixes tag?
> Signed-off-by: Vincent Whitchurch <[email protected]>
--
With Best Regards,
Andy Shevchenko
On Tue, 16 Aug 2022 10:08:28 +0200
Vincent Whitchurch <[email protected]> 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 <[email protected]>
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.
+CC Peter as most of the fun cases of IIO consumers were from him.
Perhaps this notation is a step in the right direction and we can
look for other problem cases later.
One side note is that it's not immediately obvious that iio_update_buffers()
is called only from consumers (the other paths use __iio_update_buffers() directly
so if we make this change we should consider renaming that function or
at very least adding some documentation.
Jonathan
> ---
> drivers/iio/industrialio-buffer.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index acc2b6c05d57..27868ed092d0 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -1255,7 +1255,7 @@ int iio_update_buffers(struct iio_dev *indio_dev,
> return -EINVAL;
>
> mutex_lock(&iio_dev_opaque->info_exist_lock);
> - mutex_lock(&indio_dev->mlock);
> + mutex_lock_nested(&indio_dev->mlock, SINGLE_DEPTH_NESTING);
>
> if (insert_buffer && iio_buffer_is_active(insert_buffer))
> insert_buffer = NULL;
On 8/20/22 13:06, Jonathan Cameron wrote:
> On Tue, 16 Aug 2022 10:08:28 +0200
> Vincent Whitchurch <[email protected]> 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 <[email protected]>
> 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.
On Fri, 19 Aug 2022 11:03:55 +0300
Andy Shevchenko <[email protected]> wrote:
> On Tue, Aug 16, 2022 at 1:30 PM Vincent Whitchurch
> <[email protected]> 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
>
> https://www.kernel.org/doc/html/latest/process/submitting-patches.html#backtraces-in-commit-mesages
>
> On top of that, Fixes tag?
It's going to be tricky to identify - the interface predates usecases that were IIO
drivers by a long way. I guess introduction of first IIO driver that used
a callback buffer? No idea which one that was :(
>
> > Signed-off-by: Vincent Whitchurch <[email protected]>
>
>
> --
> With Best Regards,
> Andy Shevchenko
On Sat, Aug 20, 2022 at 01:08:00PM +0200, Jonathan Cameron wrote:
> On Fri, 19 Aug 2022 11:03:55 +0300
> Andy Shevchenko <[email protected]> wrote:
> > On top of that, Fixes tag?
> It's going to be tricky to identify - the interface predates usecases that were IIO
> drivers by a long way. I guess introduction of first IIO driver that used
> a callback buffer? No idea which one that was :(
AFAICS there's only one IIO driver upstream using a callback buffer, and
it's lmp91000, so I can point the fixes tag to the patch which added
that.
By the way, note that lmp91000 actually fails to probe in mainline
without extra patches, and it seems to have been that way for a while
now. I noticed this lockdep splat when working on a new driver which
also uses a callback buffer. I can post the fixes I used to get
lmp91000 to probe successfully (in roadtest) separately.
On Sat, Aug 20, 2022 at 01:08:28PM +0200, Lars-Peter Clausen wrote:
> 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.
The second method certainly sounds like a better fix, since it also
still warns if one actually takes the same iio_dev mutex twice. I'll
respin the patch. Thanks.
On Tue, 23 Aug 2022 10:10:45 +0200
Vincent Whitchurch <[email protected]> wrote:
> On Sat, Aug 20, 2022 at 01:08:00PM +0200, Jonathan Cameron wrote:
> > On Fri, 19 Aug 2022 11:03:55 +0300
> > Andy Shevchenko <[email protected]> wrote:
> > > On top of that, Fixes tag?
> > It's going to be tricky to identify - the interface predates usecases that were IIO
> > drivers by a long way. I guess introduction of first IIO driver that used
> > a callback buffer? No idea which one that was :(
>
> AFAICS there's only one IIO driver upstream using a callback buffer, and
> it's lmp91000, so I can point the fixes tag to the patch which added
> that.
Ah. That's handy.
>
> By the way, note that lmp91000 actually fails to probe in mainline
> without extra patches, and it seems to have been that way for a while
> now. I noticed this lockdep splat when working on a new driver which
> also uses a callback buffer. I can post the fixes I used to get
> lmp91000 to probe successfully (in roadtest) separately.
That would be great. Unfortunately drivers sometimes bit rot
without testing.
Any plans to post updated roadtest soon? I'm keen to add more test cases
and use it to cleanup the remaining staging drivers. Very helpful tool,
but I don't want to be developing test sets against an old version if
it's going to be costly to forward port it. No great rush though as
I've bitten off a bit more than I was aiming to in prep for plumbers so
not going to get anything significant done in IIO until mid September.
Thanks,
Jonathan
On Sun, Aug 28, 2022 at 05:32:47PM +0200, Jonathan Cameron wrote:
> Any plans to post updated roadtest soon? I'm keen to add more test cases
> and use it to cleanup the remaining staging drivers. Very helpful tool,
> but I don't want to be developing test sets against an old version if
> it's going to be costly to forward port it.
I don't think it's ready for a new posting, but I pushed the version I
currently have here:
https://github.com/vwax/linux/commits/roadtest/devel
The tests only pass on v5.19 since there are a couple of regressions
(which affect real hardware too) in mainline:
https://lore.kernel.org/all/[email protected]/
https://lore.kernel.org/all/[email protected]/