On 04.02.2025 16:24, Herve Codina wrote:
> Hi,
>
> Got a deadlock issue with this patch in v6.14-rc1.
>
> On Fri, 1 Nov 2024 23:09:51 +0100
> Heiner Kallweit <hkallweit1@xxxxxxxxx> wrote:
>
>> So far a list is used to track auto-detected clients per driver.
>> The same functionality can be achieved much simpler by flagging
>> auto-detected clients.
>>
>> Two notes regarding the usage of driver_for_each_device:
>> In our case it can't fail, however the function is annotated __must_check.
>> So a little workaround is needed to avoid a compiler warning.
>> Then we may remove nodes from the list over which we iterate.
>> This is safe, see the explanation at the beginning of lib/klist.c.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@xxxxxxxxx>
>> ---
>> v3:
>> - protect client removal with core_lock mutex
>> ---
>> drivers/i2c/i2c-core-base.c | 52 ++++++++++++-------------------------
>> include/linux/i2c.h | 3 +--
>> 2 files changed, 17 insertions(+), 38 deletions(-)
>>
> ...
>
>> @@ -1780,8 +1752,10 @@ void i2c_del_adapter(struct i2c_adapter *adap)
>> * we can't remove the dummy devices during the first pass: they
>> * could have been instantiated by real devices wishing to clean
>> * them up properly, so we give them a chance to do that first. */
>> + mutex_lock(&core_lock);
>> device_for_each_child(&adap->dev, NULL, __unregister_client);
>> device_for_each_child(&adap->dev, NULL, __unregister_dummy);
>> + mutex_unlock(&core_lock);
>>
>
> Calling __unregister_client() with core_lock mutex held leads to a deadlock
> in my case:
>
> # echo 30a40000.i2c > /sys/bus/platform/drivers/imx-i2c/unbind
> [ 242.928264]
> [ 242.929779] ============================================
> [ 242.935092] WARNING: possible recursive locking detected
> [ 242.940406] 6.14.0-rc1+ #22 Not tainted
> [ 242.944245] --------------------------------------------
> [ 242.949556] sh/299 is trying to acquire lock:
> [ 242.953915] ffff8000818b82e0 (core_lock){+.+.}-{4:4}, at: i2c_del_adapter+0x44/0x1b0
> [ 242.961689]
> [ 242.961689] but task is already holding lock:
> [ 242.967524] ffff8000818b82e0 (core_lock){+.+.}-{4:4}, at: i2c_del_adapter+0xa0/0x1b0
> [ 242.975285]
> [ 242.975285] other info that might help us debug this:
> [ 242.981814] Possible unsafe locking scenario:
> [ 242.981814]
> [ 242.987732] CPU0
> [ 242.990179] ----
> [ 242.992625] lock(core_lock);
> [ 242.995686] lock(core_lock);
> [ 242.998748]
> [ 242.998748] *** DEADLOCK ***
> [ 242.998748]
> [ 243.004666] May be due to missing lock nesting notation
> [ 243.004666]
> [ 243.011455] 5 locks held by sh/299:
> [ 243.014946] #0: ffff000079a533f0 (sb_writers#6){.+.+}-{0:0}, at: vfs_write+0x1c4/0x398
> [ 243.022976] #1: ffff000005c29088 (&of->mutex#2){+.+.}-{4:4}, at: kernfs_fop_write_iter+0xf8/0x1c8
> [ 243.031962] #2: ffff000000c240f8 (&dev->mutex){....}-{4:4}, at: device_release_driver_internal+0x48/0x250
> [ 243.041645] #3: ffff8000818b82e0 (core_lock){+.+.}-{4:4}, at: i2c_del_adapter+0xa0/0x1b0
> [ 243.049845] #4: ffff000079f24908 (&dev->mutex){....}-{4:4}, at: device_release_driver_internal+0x48/0x250
> [ 243.059522]
> [ 243.059522] stack backtrace:
> [ 243.063883] CPU: 2 UID: 0 PID: 299 Comm: sh Not tainted 6.14.0-rc1+ #22
> [ 243.070502] Hardware name: GE HealthCare Supernova Patient Hub v1 (DT)
> [ 243.077032] Call trace:
> [ 243.079481] show_stack+0x20/0x38 (C)
> [ 243.083152] dump_stack_lvl+0x90/0xd0
> [ 243.086819] dump_stack+0x18/0x28
> [ 243.090140] print_deadlock_bug+0x260/0x350
> [ 243.094332] __lock_acquire+0x113c/0x2180
> [ 243.098346] lock_acquire+0x1c4/0x350
> [ 243.102015] __mutex_lock+0x9c/0x500
> [ 243.105599] mutex_lock_nested+0x2c/0x40
> [ 243.109528] i2c_del_adapter+0x44/0x1b0
> [ 243.113371] i2c_mux_del_adapters+0xa0/0x100
> [ 243.117649] pca954x_cleanup+0x98/0xd0
> [ 243.121406] pca954x_remove+0x38/0x50
> [ 243.125078] i2c_device_remove+0x34/0xb8
> [ 243.129007] device_remove+0x54/0x90
> [ 243.132590] device_release_driver_internal+0x1e8/0x250
> [ 243.137824] device_release_driver+0x20/0x38
> [ 243.142101] bus_remove_device+0xd4/0x120
> [ 243.146116] device_del+0x14c/0x410
> [ 243.149612] device_unregister+0x20/0x48
> [ 243.153540] i2c_unregister_device.part.0+0x50/0x88
> [ 243.158427] __unregister_client+0x74/0x80
> [ 243.162530] device_for_each_child+0x68/0xc8
> [ 243.166811] i2c_del_adapter+0xb8/0x1b0
> [ 243.170653] i2c_imx_remove+0x4c/0x190
> [ 243.174412] platform_remove+0x30/0x58
> [ 243.178167] device_remove+0x54/0x90
> [ 243.181751] device_release_driver_internal+0x1e8/0x250
> [ 243.186982] device_driver_detach+0x20/0x38
> [ 243.191172] unbind_store+0xbc/0xc8
> ...
>
> When I unbind the i2c SoC adapter driver, i2c_del_adapter() is indeed called
> recursively. The first call is for the 30a40000.i2c SoC adapter and the
> second one for an i2c mux connected on the i2c bus.
>
> My device-tree looks like this:
> i2c@30a40000 {
> compatible = "fsl,imx8mp-i2c", "fsl,imx21-i2c";
> ...
> i2c-mux@70 {
> compatible = "nxp,pca9543";
> ...
> i2c@0 {
> ...
> touchscreen@2a {
> compatible = "eeti,exc80h60";
> ...
> };
> };
>
> i2c@1 {
> ...
> };
> };
> };
>
>
> Should the core_lock mutex be taken when both __unregister_client() and
> __unregister_dummy() are called ?
>
Thanks for the report! I just submitted a fix, for now as RFC.
If fixes the deadlock and uses a new approach to prevent the race
which caused us to acquire the core lock in few place.
Could you please re-test?
> Best regards,
> Hervé Codina
Heiner
