[PATCH 0/5] regmap: Improve lock handling with maple tree

[PATCH 0/5] regmap: Improve lock handling with maple tree

[Mark Brown]

The lockdep asserts in the maple tree code and the double locking that
we're doing continue to cause issues, most recently some warnings
reported by Cristian Ciocaltea due to dynamic cache allocations in
interrupt context (which are an issue in themselves, but still).  Let's
start trying to improve the situation by configuring the regmap lock as
an external lock for maple tree, allowing it to do it's asserts without
having a separate lock.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
Mark Brown (5):
      maple_tree: Allow external locks to be configured with their map
      regmap: Hold the regmap lock when allocating and freeing the cache
      regmap: Use locking during kunit tests
      regmap: Wrap maple tree locking
      regmap: Don't double lock maple cache when using a regmap provided lock

 drivers/base/regmap/internal.h       | 12 +++++++++++
 drivers/base/regmap/regcache-maple.c | 41 +++++++++++++++++++++++++++---------
 drivers/base/regmap/regcache.c       |  4 ++++
 drivers/base/regmap/regmap-kunit.c   |  2 --
 drivers/base/regmap/regmap.c         |  5 +++++
 include/linux/maple_tree.h           |  3 +++
 6 files changed, 55 insertions(+), 12 deletions(-)
---
base-commit: 7c626ce4bae1ac14f60076d00eafe71af30450ba
change-id: 20240814-b4-regmap-maple-nolock-11408d2d0d41

Best regards,
-- 
Mark Brown <broonie@kernel.org>

[PATCH 1/5] maple_tree: Allow external locks to be configured with their map

[Mark Brown]

Currently the maple tree code allows external locks to be configured by
passing the lock itself. This is generally helpful and convenient but is
not ideal for situations like the regmap maple tree cache where we support
configurable locking at the regmap level and don't have the lock type when
we are configuring the maple tree. Add a helper that allows us to pass the
dep map directly to help with these situations. Since such code is already
peering at the lockdep internals enough to be looking at the map no stub
is provided.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 include/linux/maple_tree.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/linux/maple_tree.h b/include/linux/maple_tree.h
index a53ad4dabd7e..bdc6b133abdc 100644
--- a/include/linux/maple_tree.h
+++ b/include/linux/maple_tree.h
@@ -193,6 +193,9 @@ typedef struct lockdep_map *lockdep_map_p;
 #define mt_set_external_lock(mt, lock)					\
 	(mt)->ma_external_lock = &(lock)->dep_map
 
+#define mt_set_external_lock_dep_map(mt, dep_map)			\
+	(mt)->ma_external_lock = dep_map
+
 #define mt_on_stack(mt)			(mt).ma_external_lock = NULL
 #else
 typedef struct { /* nothing */ } lockdep_map_p;

-- 
2.39.2

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

[Mark Brown]

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(+)

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)

-- 
2.39.2

[PATCH 3/5] regmap: Use locking during kunit tests

[Mark Brown]

There is no reason to bypass the locking when running the kunit tests,
leave it enabled as standard.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/base/regmap/regmap-kunit.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/base/regmap/regmap-kunit.c b/drivers/base/regmap/regmap-kunit.c
index d790c7df5cac..b80b447c87a2 100644
--- a/drivers/base/regmap/regmap-kunit.c
+++ b/drivers/base/regmap/regmap-kunit.c
@@ -151,8 +151,6 @@ static struct regmap *gen_regmap(struct kunit *test,
 	struct reg_default *defaults;
 
 	config->cache_type = param->cache;
-	config->disable_locking = config->cache_type == REGCACHE_RBTREE ||
-					config->cache_type == REGCACHE_MAPLE;
 
 	if (config->max_register == 0) {
 		config->max_register = param->from_reg;

-- 
2.39.2

[PATCH 4/5] regmap: Wrap maple tree locking

[Mark Brown]

In preparation for using the regmap lock when available with the maple
tree lock wrap all the maple tree locking with some local functions and
only do the maple tree locking if the maple tree has it's own lock.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/base/regmap/regcache-maple.c | 32 ++++++++++++++++++++++----------
 1 file changed, 22 insertions(+), 10 deletions(-)

diff --git a/drivers/base/regmap/regcache-maple.c b/drivers/base/regmap/regcache-maple.c
index 2dea9d259c49..d2de3eba1646 100644
--- a/drivers/base/regmap/regcache-maple.c
+++ b/drivers/base/regmap/regcache-maple.c
@@ -13,6 +13,18 @@
 
 #include "internal.h"
 
+static inline void regmap_mas_lock(struct ma_state *mas)
+{
+	if (!mt_external_lock(mas->tree))
+		mas_lock(mas);
+}
+
+static inline void regmap_mas_unlock(struct ma_state *mas)
+{
+	if (!mt_external_lock(mas->tree))
+		mas_unlock(mas);
+}
+
 static int regcache_maple_read(struct regmap *map,
 			       unsigned int reg, unsigned int *value)
 {
@@ -89,12 +101,12 @@ static int regcache_maple_write(struct regmap *map, unsigned int reg,
 	 * is redundant, but we need to take it due to lockdep asserts
 	 * in the maple tree code.
 	 */
-	mas_lock(&mas);
+	regmap_mas_lock(&mas);
 
 	mas_set_range(&mas, index, last);
 	ret = mas_store_gfp(&mas, entry, map->alloc_flags);
 
-	mas_unlock(&mas);
+	regmap_mas_unlock(&mas);
 
 	if (ret == 0) {
 		kfree(lower);
@@ -118,7 +130,7 @@ static int regcache_maple_drop(struct regmap *map, unsigned int min,
 	lower = NULL;
 	upper = NULL;
 
-	mas_lock(&mas);
+	regmap_mas_lock(&mas);
 
 	mas_for_each(&mas, entry, max) {
 		/*
@@ -126,7 +138,7 @@ static int regcache_maple_drop(struct regmap *map, unsigned int min,
 		 * Maple lock is redundant, but we need to take it due
 		 * to lockdep asserts in the maple tree code.
 		 */
-		mas_unlock(&mas);
+		regmap_mas_unlock(&mas);
 
 		/* Do we need to save any of this entry? */
 		if (mas.index < min) {
@@ -156,7 +168,7 @@ static int regcache_maple_drop(struct regmap *map, unsigned int min,
 		}
 
 		kfree(entry);
-		mas_lock(&mas);
+		regmap_mas_lock(&mas);
 		mas_erase(&mas);
 
 		/* Insert new nodes with the saved data */
@@ -178,7 +190,7 @@ static int regcache_maple_drop(struct regmap *map, unsigned int min,
 	}
 
 out:
-	mas_unlock(&mas);
+	regmap_mas_unlock(&mas);
 out_unlocked:
 	kfree(lower);
 	kfree(upper);
@@ -300,11 +312,11 @@ static int regcache_maple_exit(struct regmap *map)
 	if (!mt)
 		return 0;
 
-	mas_lock(&mas);
+	regmap_mas_lock(&mas);
 	mas_for_each(&mas, entry, UINT_MAX)
 		kfree(entry);
 	__mt_destroy(mt);
-	mas_unlock(&mas);
+	regmap_mas_unlock(&mas);
 
 	kfree(mt);
 	map->cache = NULL;
@@ -327,13 +339,13 @@ static int regcache_maple_insert_block(struct regmap *map, int first,
 	for (i = 0; i < last - first + 1; i++)
 		entry[i] = map->reg_defaults[first + i].def;
 
-	mas_lock(&mas);
+	regmap_mas_lock(&mas);
 
 	mas_set_range(&mas, map->reg_defaults[first].reg,
 		      map->reg_defaults[last].reg);
 	ret = mas_store_gfp(&mas, entry, map->alloc_flags);
 
-	mas_unlock(&mas);
+	regmap_mas_unlock(&mas);
 
 	if (ret)
 		kfree(entry);

-- 
2.39.2

[PATCH 5/5] regmap: Don't double lock maple cache when using a regmap provided lock

[Mark Brown]

It is not possible to use the maple tree without holding a lock of some
kind, by default the maple tree incorporates a lock but it does have
support for registering an external lock which will be asserted against
instead. At present regmap uses the maple tree's internal lock which is
unfortunate since there is also regmap level locking above the cache which
protects the cache data structure and things like read/modify/write
operations.

Let's reduce the overhead here by telling the maple tree about the regmap
level lock for cases where regmap does it's own locking. We also support
external and custom locking for regmap, neither of which will benefit from
this, but this will cover the vast majority of users.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/base/regmap/internal.h       | 12 ++++++++++++
 drivers/base/regmap/regcache-maple.c |  9 +++++++++
 drivers/base/regmap/regmap.c         |  4 ++++
 3 files changed, 25 insertions(+)

diff --git a/drivers/base/regmap/internal.h b/drivers/base/regmap/internal.h
index 83acccdc1008..a5fe052a70ce 100644
--- a/drivers/base/regmap/internal.h
+++ b/drivers/base/regmap/internal.h
@@ -59,6 +59,9 @@ struct regmap {
 			unsigned long raw_spinlock_flags;
 		};
 	};
+#ifdef CONFIG_LOCKDEP
+	struct lockdep_map *lockdep;
+#endif
 	regmap_lock lock;
 	regmap_unlock unlock;
 	void *lock_arg; /* This is passed to lock/unlock functions */
@@ -344,4 +347,13 @@ struct regmap *__regmap_init_raw_ram(struct device *dev,
 #define regmap_init_raw_ram(dev, config, data)				\
 	__regmap_lockdep_wrapper(__regmap_init_raw_ram, #dev, dev, config, data)
 
+#ifdef CONFIG_LOCKDEP
+static inline void regmap_set_lockdep(struct regmap *m, struct lockdep_map *l)
+{
+	m->lockdep = l;
+}
+#else
+#define regmap_set_lockdep(m, l)
+#endif
+
 #endif
diff --git a/drivers/base/regmap/regcache-maple.c b/drivers/base/regmap/regcache-maple.c
index d2de3eba1646..1247ff3ae397 100644
--- a/drivers/base/regmap/regcache-maple.c
+++ b/drivers/base/regmap/regcache-maple.c
@@ -365,7 +365,16 @@ static int regcache_maple_init(struct regmap *map)
 		return -ENOMEM;
 	map->cache = mt;
 
+#ifdef CONFIG_LOCKDEP
+	if (map->lockdep) {
+		mt_init_flags(mt, MT_FLAGS_LOCK_EXTERN);
+		mt_set_external_lock_dep_map(mt, map->lockdep);
+	} else {
+		mt_init(mt);
+	}
+#else
 	mt_init(mt);
+#endif
 
 	if (!map->num_reg_defaults)
 		return 0;
diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index 9ed842d17642..f66b5ef56cf8 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -729,12 +729,15 @@ struct regmap *__regmap_init(struct device *dev,
 				map->unlock = regmap_unlock_raw_spinlock;
 				lockdep_set_class_and_name(&map->raw_spinlock,
 							   lock_key, lock_name);
+				regmap_set_lockdep(map,
+						   &map->raw_spinlock.dep_map);
 			} else {
 				spin_lock_init(&map->spinlock);
 				map->lock = regmap_lock_spinlock;
 				map->unlock = regmap_unlock_spinlock;
 				lockdep_set_class_and_name(&map->spinlock,
 							   lock_key, lock_name);
+				regmap_set_lockdep(map, &map->spinlock.dep_map);
 			}
 		} else {
 			mutex_init(&map->mutex);
@@ -743,6 +746,7 @@ struct regmap *__regmap_init(struct device *dev,
 			map->can_sleep = true;
 			lockdep_set_class_and_name(&map->mutex,
 						   lock_key, lock_name);
+			regmap_set_lockdep(map, &map->mutex.dep_map);
 		}
 		map->lock_arg = map;
 	}

-- 
2.39.2

Re: [PATCH 1/5] maple_tree: Allow external locks to be configured with their map

[Matthew Wilcox]

On Thu, Aug 22, 2024 at 08:13:35PM +0100, Mark Brown wrote:
> Currently the maple tree code allows external locks to be configured by
> passing the lock itself. This is generally helpful and convenient but is

No, it's a really bad idea.  Stop doing it.  Use the internal lock.
It's a temporary hack we put in and I'm really regretting allowing it.

> not ideal for situations like the regmap maple tree cache where we support
> configurable locking at the regmap level and don't have the lock type when
> we are configuring the maple tree. Add a helper that allows us to pass the
> dep map directly to help with these situations. Since such code is already
> peering at the lockdep internals enough to be looking at the map no stub
> is provided.
> 
> Signed-off-by: Mark Brown <broonie@kernel.org>
> ---
>  include/linux/maple_tree.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/include/linux/maple_tree.h b/include/linux/maple_tree.h
> index a53ad4dabd7e..bdc6b133abdc 100644
> --- a/include/linux/maple_tree.h
> +++ b/include/linux/maple_tree.h
> @@ -193,6 +193,9 @@ typedef struct lockdep_map *lockdep_map_p;
>  #define mt_set_external_lock(mt, lock)					\
>  	(mt)->ma_external_lock = &(lock)->dep_map
>  
> +#define mt_set_external_lock_dep_map(mt, dep_map)			\
> +	(mt)->ma_external_lock = dep_map
> +
>  #define mt_on_stack(mt)			(mt).ma_external_lock = NULL
>  #else
>  typedef struct { /* nothing */ } lockdep_map_p;
> 
> -- 
> 2.39.2
> 
> 
> -- 
> maple-tree mailing list
> maple-tree@lists.infradead.org
> https://lists.infradead.org/mailman/listinfo/maple-tree

Re: [PATCH 1/5] maple_tree: Allow external locks to be configured with their map

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 870 bytes --]

On Thu, Aug 22, 2024 at 08:21:40PM +0100, Matthew Wilcox wrote:
> On Thu, Aug 22, 2024 at 08:13:35PM +0100, Mark Brown wrote:

> > Currently the maple tree code allows external locks to be configured by
> > passing the lock itself. This is generally helpful and convenient but is

> No, it's a really bad idea.  Stop doing it.  Use the internal lock.
> It's a temporary hack we put in and I'm really regretting allowing it.

I mean, we do use the internal lock here since otherwise lockdep moans
but it's pure overhead which just complicates the code.  It's only ever
taken within another lock, meaning it winds up protecting nothing for
these maple trees.  We can't go the other way round and use the maple
tree lock as the regmap lock since apart from anything else it's a spin
lock and we need to use a mutex most of the time to support busses that
sleep during I/O.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/5] maple_tree: Allow external locks to be configured with their map

[Matthew Wilcox]

On Thu, Aug 22, 2024 at 08:48:56PM +0100, Mark Brown wrote:
> On Thu, Aug 22, 2024 at 08:21:40PM +0100, Matthew Wilcox wrote:
> > On Thu, Aug 22, 2024 at 08:13:35PM +0100, Mark Brown wrote:
> 
> > > Currently the maple tree code allows external locks to be configured by
> > > passing the lock itself. This is generally helpful and convenient but is
> 
> > No, it's a really bad idea.  Stop doing it.  Use the internal lock.
> > It's a temporary hack we put in and I'm really regretting allowing it.
> 
> I mean, we do use the internal lock here since otherwise lockdep moans
> but it's pure overhead which just complicates the code.  It's only ever
> taken within another lock, meaning it winds up protecting nothing for
> these maple trees.  We can't go the other way round and use the maple
> tree lock as the regmap lock since apart from anything else it's a spin
> lock and we need to use a mutex most of the time to support busses that
> sleep during I/O.

When it's an uncontended spinlock, there's really no overhead.  I wish I'd
been firmer on that point earlier and prohibited the external lock hack.

The point is that the lock protects the tree.  If we are ever going to
be able to defragment slabs (and I believe this is an ability that Linux
must gain), we must be able to go from the object (the maple node) to
a lock that will let us reallocate the node.  If there's some external
lock that protects the tree, we can't possibly do that.

Re: [PATCH 1/5] maple_tree: Allow external locks to be configured with their map

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 930 bytes --]

On Thu, Aug 22, 2024 at 08:55:20PM +0100, Matthew Wilcox wrote:
> On Thu, Aug 22, 2024 at 08:48:56PM +0100, Mark Brown wrote:

> > I mean, we do use the internal lock here since otherwise lockdep moans
> > but it's pure overhead which just complicates the code.  It's only ever

> When it's an uncontended spinlock, there's really no overhead.  I wish I'd
> been firmer on that point earlier and prohibited the external lock hack.

> The point is that the lock protects the tree.  If we are ever going to
> be able to defragment slabs (and I believe this is an ability that Linux
> must gain), we must be able to go from the object (the maple node) to
> a lock that will let us reallocate the node.  If there's some external
> lock that protects the tree, we can't possibly do that.

If the external lock guarantees that nothing can possibly be contending
access to the tree (including the read side) I don't see any issue
there?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: (subset) [PATCH 0/5] regmap: Improve lock handling with maple tree

[Mark Brown]

On Thu, 22 Aug 2024 20:13:34 +0100, Mark Brown wrote:
> The lockdep asserts in the maple tree code and the double locking that
> we're doing continue to cause issues, most recently some warnings
> reported by Cristian Ciocaltea due to dynamic cache allocations in
> interrupt context (which are an issue in themselves, but still).  Let's
> start trying to improve the situation by configuring the regmap lock as
> an external lock for maple tree, allowing it to do it's asserts without
> having a separate lock.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git for-next

Thanks!

[2/5] regmap: Hold the regmap lock when allocating and freeing the cache
      commit: fd4ebc07b4dff7e1abedf1b7fd477bc04b69ae55
[3/5] regmap: Use locking during kunit tests
      commit: 290d6e5d6498703accffc66849b7fb2d4d7503ff

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

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

[Marek Szyprowski]

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
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 658 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]