diff mbox series

[2/5] regmap: Hold the regmap lock when allocating and freeing the cache

Message ID 20240822-b4-regmap-maple-nolock-v1-2-d5e6dbae3396@kernel.org (mailing list archive)
State New
Headers show
Series regmap: Improve lock handling with maple tree | expand

Commit Message

Mark Brown Aug. 22, 2024, 7:13 p.m. UTC
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@kernel.org>
---
 drivers/base/regmap/regcache.c | 4 ++++
 drivers/base/regmap/regmap.c   | 1 +
 2 files changed, 5 insertions(+)

Comments

Marek Szyprowski Aug. 28, 2024, 10:02 a.m. UTC | #1
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@kernel.org>

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
Mark Brown Aug. 28, 2024, 11:32 a.m. UTC | #2
On Wed, Aug 28, 2024 at 12:02:38PM +0200, Marek Szyprowski wrote:
> On 22.08.2024 21:13, Mark Brown wrote:

> 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

Please, put any actual content in your mail *before* huge log spams,
especially absurdly big things like lockdep.  It makes it very hard to
find anything you've written, I very nearly didn't here and it'd be a
lot of effort for me to wade through all that text to find it again to
reply to the mail.

Your patch looks basically fine but should use alloc_flags.
diff mbox series

Patch

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)