Re: [PATCH 2/5] regmap: Hold the regmap lock when allocating and freeing the cache
From: Marek Szyprowski
Date: Wed Aug 28 2024 - 06:02:56 EST
Dear Mark,
On 22.08.2024 21:13, Mark Brown wrote:
> For the benefit of the maple tree's lockdep checking hold the lock while
> creating and exiting the cache.
>
> Signed-off-by: Mark Brown <broonie@xxxxxxxxxx>
This patch landed recently in linux-next as commit fd4ebc07b4df
("regmap: Hold the regmap lock when allocating and freeing the cache").
In my tests I found that it triggers the following warnings on
Rockchip3568 based Odroid-M1 board:
BUG: sleeping function called from invalid context at
include/linux/sched/mm.h:337
in_atomic(): 1, irqs_disabled(): 1, non_block: 0, pid: 157, name:
systemd-udevd
preempt_count: 1, expected: 0
RCU nest depth: 0, expected: 0
2 locks held by systemd-udevd/157:
#0: ffff0001f0cf30f8 (&dev->mutex){....}-{3:3}, at:
__driver_attach+0x90/0x1ac
#1: ffff0001f1196818
(rockchip_i2s_tdm:1300:(&rockchip_i2s_tdm_regmap_config)->lock){....}-{2:2},
at: regmap_lock_spinlock+0x18/0x2c
irq event stamp: 11474
hardirqs last enabled at (11473): [<ffff8000812b4cc0>]
_raw_spin_unlock_irqrestore+0x74/0x78
hardirqs last disabled at (11474): [<ffff8000812b40d4>]
_raw_spin_lock_irqsave+0x84/0x88
softirqs last enabled at (9730): [<ffff8000800b13dc>]
handle_softirqs+0x4cc/0x4e4
softirqs last disabled at (9721): [<ffff8000800105b0>]
__do_softirq+0x14/0x20
CPU: 3 UID: 0 PID: 157 Comm: systemd-udevd Not tainted 6.11.0-rc3+ #15305
Hardware name: Hardkernel ODROID-M1 (DT)
Call trace:
dump_backtrace+0x94/0xec
show_stack+0x18/0x24
dump_stack_lvl+0x90/0xd0
dump_stack+0x18/0x24
__might_resched+0x144/0x248
__might_sleep+0x48/0x98
__kmalloc_noprof+0x208/0x328
regcache_flat_init+0x40/0xb0
regcache_init+0x1ec/0x490
__regmap_init+0x8e0/0xd50
__devm_regmap_init+0x78/0xc8
__devm_regmap_init_mmio_clk+0x9c/0xc4
rockchip_i2s_tdm_probe+0x318/0x754 [snd_soc_rockchip_i2s_tdm]
platform_probe+0x68/0xdc
really_probe+0xbc/0x298
__driver_probe_device+0x78/0x12c
driver_probe_device+0x40/0x164
__driver_attach+0x9c/0x1ac
bus_for_each_dev+0x74/0xd4
driver_attach+0x24/0x30
bus_add_driver+0xe4/0x208
driver_register+0x60/0x128
__platform_driver_register+0x28/0x34
rockchip_i2s_tdm_driver_init+0x20/0x1000 [snd_soc_rockchip_i2s_tdm]
do_one_initcall+0x68/0x300
do_init_module+0x60/0x224
load_module+0x1b0c/0x1cb0
init_module_from_file+0x84/0xc4
idempotent_init_module+0x18c/0x284
__arm64_sys_finit_module+0x64/0xa0
invoke_syscall+0x48/0x110
el0_svc_common.constprop.0+0xc8/0xe8
do_el0_svc+0x20/0x2c
el0_svc+0x4c/0x11c
el0t_64_sync_handler+0x13c/0x158
el0t_64_sync+0x190/0x194
and
========================================================
WARNING: possible irq lock inversion dependency detected
6.11.0-rc3+ #15305 Tainted: G W
--------------------------------------------------------
swapper/0/0 just changed the state of lock:
ffff0001f2305018
(rockchip_drm_vop2:3114:(&vop2_regmap_config)->lock){-...}-{2:2}, at:
regmap_lock_spinlock+0x18/0x2c
but this lock took another, HARDIRQ-unsafe lock in the past:
(fs_reclaim){+.+.}-{0:0}
and interrupts could create inverse lock ordering between them.
other info that might help us debug this:
Possible interrupt unsafe locking scenario:
CPU0 CPU1
---- ----
lock(fs_reclaim);
local_irq_disable();
lock(rockchip_drm_vop2:3114:(&vop2_regmap_config)->lock);
lock(fs_reclaim);
<Interrupt>
lock(rockchip_drm_vop2:3114:(&vop2_regmap_config)->lock);
*** DEADLOCK ***
no locks held by swapper/0/0.
the shortest dependencies between 2nd lock and 1st lock:
-> (fs_reclaim){+.+.}-{0:0} {
HARDIRQ-ON-W at:
lock_acquire+0x200/0x340
fs_reclaim_acquire+0xdc/0xfc
__kmalloc_cache_noprof+0x54/0x288
__kthread_create_worker+0x3c/0x150
kthread_create_worker+0x64/0x8c
workqueue_init+0x30/0x3c4
kernel_init_freeable+0x11c/0x50c
kernel_init+0x20/0x1d8
ret_from_fork+0x10/0x20
SOFTIRQ-ON-W at:
lock_acquire+0x200/0x340
fs_reclaim_acquire+0xdc/0xfc
__kmalloc_cache_noprof+0x54/0x288
__kthread_create_worker+0x3c/0x150
kthread_create_worker+0x64/0x8c
workqueue_init+0x30/0x3c4
kernel_init_freeable+0x11c/0x50c
kernel_init+0x20/0x1d8
ret_from_fork+0x10/0x20
INITIAL USE at:
lock_acquire+0x200/0x340
fs_reclaim_acquire+0xdc/0xfc
__kmalloc_cache_noprof+0x54/0x288
__kthread_create_worker+0x3c/0x150
kthread_create_worker+0x64/0x8c
workqueue_init+0x30/0x3c4
kernel_init_freeable+0x11c/0x50c
kernel_init+0x20/0x1d8
ret_from_fork+0x10/0x20
}
... key at: [<ffff800083097970>] __fs_reclaim_map+0x0/0x28
... acquired at:
fs_reclaim_acquire+0xdc/0xfc
__kmalloc_cache_noprof+0x54/0x288
regcache_maple_init+0x2c/0x110
regcache_init+0x1ec/0x490
__regmap_init+0x8e0/0xd50
__devm_regmap_init+0x78/0xc8
__devm_regmap_init_mmio_clk+0x9c/0xc4
vop2_bind+0xf4/0xb10 [rockchipdrm]
component_bind_all+0x118/0x24c
rockchip_drm_bind+0xb0/0x1fc [rockchipdrm]
try_to_bring_up_aggregate_device+0x168/0x1d4
component_master_add_with_match+0xb4/0xfc
rockchip_drm_platform_probe+0x154/0x284 [rockchipdrm]
platform_probe+0x68/0xdc
really_probe+0xbc/0x298
__driver_probe_device+0x78/0x12c
driver_probe_device+0x40/0x164
__driver_attach+0x9c/0x1ac
bus_for_each_dev+0x74/0xd4
driver_attach+0x24/0x30
bus_add_driver+0xe4/0x208
driver_register+0x60/0x128
__platform_driver_register+0x28/0x34
dw_hdmi_cec_transmit+0x44/0xc4 [dw_hdmi_cec]
do_one_initcall+0x68/0x300
do_init_module+0x60/0x224
load_module+0x1b0c/0x1cb0
init_module_from_file+0x84/0xc4
idempotent_init_module+0x18c/0x284
__arm64_sys_finit_module+0x64/0xa0
invoke_syscall+0x48/0x110
el0_svc_common.constprop.0+0xc8/0xe8
do_el0_svc+0x20/0x2c
el0_svc+0x4c/0x11c
el0t_64_sync_handler+0x13c/0x158
el0t_64_sync+0x190/0x194
-> (rockchip_drm_vop2:3114:(&vop2_regmap_config)->lock){-...}-{2:2} {
IN-HARDIRQ-W at:
lock_acquire+0x200/0x340
_raw_spin_lock_irqsave+0x60/0x88
regmap_lock_spinlock+0x18/0x2c
regmap_read+0x3c/0x78
vop2_isr+0x84/0x2a8 [rockchipdrm]
__handle_irq_event_percpu+0x9c/0x304
handle_irq_event+0x4c/0xa8
handle_fasteoi_irq+0xa4/0x1cc
generic_handle_domain_irq+0x2c/0x44
gic_handle_irq+0x4c/0x110
call_on_irq_stack+0x24/0x4c
do_interrupt_handler+0x80/0x84
el1_interrupt+0x34/0x64
el1h_64_irq_handler+0x18/0x24
el1h_64_irq+0x64/0x68
default_idle_call+0xa8/0x1e8
do_idle+0x220/0x284
cpu_startup_entry+0x34/0x3c
rest_init+0xf4/0x184
start_kernel+0x680/0x78c
__primary_switched+0x80/0x88
INITIAL USE at:
lock_acquire+0x200/0x340
_raw_spin_lock_irqsave+0x60/0x88
regmap_lock_spinlock+0x18/0x2c
regcache_init+0x1dc/0x490
__regmap_init+0x8e0/0xd50
__devm_regmap_init+0x78/0xc8
__devm_regmap_init_mmio_clk+0x9c/0xc4
vop2_bind+0xf4/0xb10 [rockchipdrm]
component_bind_all+0x118/0x24c
rockchip_drm_bind+0xb0/0x1fc [rockchipdrm]
try_to_bring_up_aggregate_device+0x168/0x1d4
component_master_add_with_match+0xb4/0xfc
rockchip_drm_platform_probe+0x154/0x284 [rockchipdrm]
platform_probe+0x68/0xdc
really_probe+0xbc/0x298
__driver_probe_device+0x78/0x12c
driver_probe_device+0x40/0x164
__driver_attach+0x9c/0x1ac
bus_for_each_dev+0x74/0xd4
driver_attach+0x24/0x30
bus_add_driver+0xe4/0x208
driver_register+0x60/0x128
__platform_driver_register+0x28/0x34
dw_hdmi_cec_transmit+0x44/0xc4 [dw_hdmi_cec]
do_one_initcall+0x68/0x300
do_init_module+0x60/0x224
load_module+0x1b0c/0x1cb0
init_module_from_file+0x84/0xc4
idempotent_init_module+0x18c/0x284
__arm64_sys_finit_module+0x64/0xa0
invoke_syscall+0x48/0x110
el0_svc_common.constprop.0+0xc8/0xe8
do_el0_svc+0x20/0x2c
el0_svc+0x4c/0x11c
el0t_64_sync_handler+0x13c/0x158
el0t_64_sync+0x190/0x194
}
... key at: [<ffff80007c3e3a58>] _key.6+0x0/0xffffffffffffd5a8
[rockchipdrm]
... acquired at:
__lock_acquire+0xb10/0x21a0
lock_acquire+0x200/0x340
_raw_spin_lock_irqsave+0x60/0x88
regmap_lock_spinlock+0x18/0x2c
regmap_read+0x3c/0x78
vop2_isr+0x84/0x2a8 [rockchipdrm]
__handle_irq_event_percpu+0x9c/0x304
handle_irq_event+0x4c/0xa8
handle_fasteoi_irq+0xa4/0x1cc
generic_handle_domain_irq+0x2c/0x44
gic_handle_irq+0x4c/0x110
call_on_irq_stack+0x24/0x4c
do_interrupt_handler+0x80/0x84
el1_interrupt+0x34/0x64
el1h_64_irq_handler+0x18/0x24
el1h_64_irq+0x64/0x68
default_idle_call+0xa8/0x1e8
do_idle+0x220/0x284
cpu_startup_entry+0x34/0x3c
rest_init+0xf4/0x184
start_kernel+0x680/0x78c
__primary_switched+0x80/0x88
stack backtrace:
CPU: 0 UID: 0 PID: 0 Comm: swapper/0 Tainted: G W 6.11.0-rc3+ #15305
Tainted: [W]=WARN
Hardware name: Hardkernel ODROID-M1 (DT)
Call trace:
dump_backtrace+0x94/0xec
show_stack+0x18/0x24
dump_stack_lvl+0x90/0xd0
dump_stack+0x18/0x24
print_irq_inversion_bug.part.0+0x1ec/0x27c
mark_lock+0x63c/0x954
__lock_acquire+0xb10/0x21a0
lock_acquire+0x200/0x340
_raw_spin_lock_irqsave+0x60/0x88
regmap_lock_spinlock+0x18/0x2c
regmap_read+0x3c/0x78
vop2_isr+0x84/0x2a8 [rockchipdrm]
__handle_irq_event_percpu+0x9c/0x304
handle_irq_event+0x4c/0xa8
handle_fasteoi_irq+0xa4/0x1cc
generic_handle_domain_irq+0x2c/0x44
gic_handle_irq+0x4c/0x110
call_on_irq_stack+0x24/0x4c
do_interrupt_handler+0x80/0x84
el1_interrupt+0x34/0x64
el1h_64_irq_handler+0x18/0x24
el1h_64_irq+0x64/0x68
default_idle_call+0xa8/0x1e8
do_idle+0x220/0x284
cpu_startup_entry+0x34/0x3c
rest_init+0xf4/0x184
start_kernel+0x680/0x78c
__primary_switched+0x80/0x88
Console: switching to colour frame buffer device 240x67
rockchip-drm display-subsystem: [drm] fb0: rockchipdrmfb frame buffer device
Both issues can be fixed by the following patch, but I'm not sure if
this is not an overuse of the GFP_ATOMIC just for the initialization phase:
diff --git a/drivers/base/regmap/regcache-flat.c
b/drivers/base/regmap/regcache-flat.c
index 9b17c77dec9d..8e8cf328bffb 100644
--- a/drivers/base/regmap/regcache-flat.c
+++ b/drivers/base/regmap/regcache-flat.c
@@ -27,7 +27,8 @@static int regcache_flat_init(struct regmap *map)
return -EINVAL;
map->cache = kcalloc(regcache_flat_get_index(map,
map->max_register)
- + 1, sizeof(unsigned int), GFP_KERNEL);
+ + 1, sizeof(unsigned int),
+ map->can_sleep ? GFP_KERNEL : GFP_ATOMIC);
if (!map->cache)
return -ENOMEM;
diff --git a/drivers/base/regmap/regcache-maple.c
b/drivers/base/regmap/regcache-maple.c
index 2dea9d259c49..b95130127bdc 100644
--- a/drivers/base/regmap/regcache-maple.c
+++ b/drivers/base/regmap/regcache-maple.c
@@ -348,7 +348,7 @@static int regcache_maple_init(struct regmap *map)
int ret;
int range_start;
- mt = kmalloc(sizeof(*mt), GFP_KERNEL);
+mt = kmalloc(sizeof(*mt), map->can_sleep ? GFP_KERNEL : GFP_ATOMIC);
if (!mt)
return -ENOMEM;
map->cache = mt;
diff --git a/drivers/base/regmap/regcache-rbtree.c
b/drivers/base/regmap/regcache-rbtree.c
index 3db88bbcae0f..c53c38a965d5 100644
--- a/drivers/base/regmap/regcache-rbtree.c
+++ b/drivers/base/regmap/regcache-rbtree.c
@@ -187,7 +187,8 @@static int regcache_rbtree_init(struct regmap *map)
int i;
int ret;
- map->cache = kmalloc(sizeof *rbtree_ctx, GFP_KERNEL);
+map->cache = kmalloc(sizeof *rbtree_ctx,
+ map->can_sleep ? GFP_KERNEL : GFP_ATOMIC);
if (!map->cache)
return -ENOMEM;
Let me know if such approach is fine, then I will submit a proper patch
in a few minutes.
> ---
> drivers/base/regmap/regcache.c | 4 ++++
> drivers/base/regmap/regmap.c | 1 +
> 2 files changed, 5 insertions(+)
>
> diff --git a/drivers/base/regmap/regcache.c b/drivers/base/regmap/regcache.c
> index 7ec1ec605335..d3659ba3cc11 100644
> --- a/drivers/base/regmap/regcache.c
> +++ b/drivers/base/regmap/regcache.c
> @@ -195,7 +195,9 @@ int regcache_init(struct regmap *map, const struct regmap_config *config)
> if (map->cache_ops->init) {
> dev_dbg(map->dev, "Initializing %s cache\n",
> map->cache_ops->name);
> + map->lock(map->lock_arg);
> ret = map->cache_ops->init(map);
> + map->unlock(map->lock_arg);
> if (ret)
> goto err_free;
> }
> @@ -223,7 +225,9 @@ void regcache_exit(struct regmap *map)
> if (map->cache_ops->exit) {
> dev_dbg(map->dev, "Destroying %s cache\n",
> map->cache_ops->name);
> + map->lock(map->lock_arg);
> map->cache_ops->exit(map);
> + map->unlock(map->lock_arg);
> }
> }
>
> diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
> index bfc6bc1eb3a4..9ed842d17642 100644
> --- a/drivers/base/regmap/regmap.c
> +++ b/drivers/base/regmap/regmap.c
> @@ -1445,6 +1445,7 @@ void regmap_exit(struct regmap *map)
> struct regmap_async *async;
>
> regcache_exit(map);
> +
> regmap_debugfs_exit(map);
> regmap_range_exit(map);
> if (map->bus && map->bus->free_context)
>
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland