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