From 21e9a1dd01b17095192ea86decc0c2081451612e Mon Sep 17 00:00:00 2001 From: Andy Shevchenko Date: Wed, 25 Sep 2024 11:27:26 +0300 Subject: [PATCH 1/7] regmap-irq: Consistently use memset32() in regmap_irq_thread() The commit 4d60cac951fd ("regmap-irq: Add no_status support") adds an additional branch into IRQ thread handler in regmap. It wisely chose to use memset32() as it might be optimised on some architectures and hence give a performance benefit. At the same time the old code continue using simple memset(). Update the old code to use memset32(). Signed-off-by: Andy Shevchenko Link: https://patch.msgid.link/20240925082726.620622-1-andriy.shevchenko@linux.intel.com Signed-off-by: Mark Brown --- drivers/base/regmap/regmap-irq.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c index a750e48a26b8..33ec28e3a802 100644 --- a/drivers/base/regmap/regmap-irq.c +++ b/drivers/base/regmap/regmap-irq.c @@ -364,14 +364,11 @@ static irqreturn_t regmap_irq_thread(int irq, void *d) memset32(data->status_buf, GENMASK(31, 0), chip->num_regs); } else if (chip->num_main_regs) { unsigned int max_main_bits; - unsigned long size; - - size = chip->num_regs * sizeof(unsigned int); max_main_bits = (chip->num_main_status_bits) ? chip->num_main_status_bits : chip->num_regs; /* Clear the status buf as we don't read all status regs */ - memset(data->status_buf, 0, size); + memset32(data->status_buf, 0, chip->num_regs); /* We could support bulk read for main status registers * but I don't expect to see devices with really many main From 42afe80caff040525252af6e9601287777d144fe Mon Sep 17 00:00:00 2001 From: Mark Brown Date: Tue, 24 Sep 2024 12:08:53 +0200 Subject: [PATCH 2/7] regmap: Specifically test writing 0 as a value to sparse caches Since 0 can look a lot like a NULL pointer when used in a cache some clever data structures might potentially introduce bugs specific to handling it. Add some explicit testing of storing 0 as a value in a sparse cache, at the minute there are no issues and this will stop any appearing in the future. Signed-off-by: Mark Brown Link: https://patch.msgid.link/20240924-regcache-zero-value-v1-1-8a1224214b52@kernel.org Signed-off-by: Mark Brown --- drivers/base/regmap/regmap-kunit.c | 43 ++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/drivers/base/regmap/regmap-kunit.c b/drivers/base/regmap/regmap-kunit.c index 4bf3f1e59ed7..a42c7386b210 100644 --- a/drivers/base/regmap/regmap-kunit.c +++ b/drivers/base/regmap/regmap-kunit.c @@ -1499,6 +1499,48 @@ static void cache_present(struct kunit *test) KUNIT_ASSERT_TRUE(test, regcache_reg_cached(map, param->from_reg + i)); } +static void cache_write_zero(struct kunit *test) +{ + const struct regmap_test_param *param = test->param_value; + struct regmap *map; + struct regmap_config config; + struct regmap_ram_data *data; + unsigned int val; + int i; + + config = test_regmap_config; + + map = gen_regmap(test, &config, &data); + KUNIT_ASSERT_FALSE(test, IS_ERR(map)); + if (IS_ERR(map)) + return; + + for (i = 0; i < BLOCK_TEST_SIZE; i++) + data->read[param->from_reg + i] = false; + + /* No defaults so no registers cached. */ + for (i = 0; i < BLOCK_TEST_SIZE; i++) + KUNIT_ASSERT_FALSE(test, regcache_reg_cached(map, param->from_reg + i)); + + /* We didn't trigger any reads */ + for (i = 0; i < BLOCK_TEST_SIZE; i++) + KUNIT_ASSERT_FALSE(test, data->read[param->from_reg + i]); + + /* Write a zero value */ + KUNIT_EXPECT_EQ(test, 0, regmap_write(map, 1, 0)); + + /* Read that zero value back */ + KUNIT_EXPECT_EQ(test, 0, regmap_read(map, 1, &val)); + KUNIT_EXPECT_EQ(test, 0, val); + + /* From the cache? */ + KUNIT_ASSERT_TRUE(test, regcache_reg_cached(map, 1)); + + /* Try to throw it away */ + KUNIT_EXPECT_EQ(test, 0, regcache_drop_region(map, 1, 1)); + KUNIT_ASSERT_FALSE(test, regcache_reg_cached(map, 1)); +} + /* Check that caching the window register works with sync */ static void cache_range_window_reg(struct kunit *test) { @@ -2012,6 +2054,7 @@ static struct kunit_case regmap_test_cases[] = { KUNIT_CASE_PARAM(cache_drop_all_and_sync_no_defaults, sparse_cache_types_gen_params), KUNIT_CASE_PARAM(cache_drop_all_and_sync_has_defaults, sparse_cache_types_gen_params), KUNIT_CASE_PARAM(cache_present, sparse_cache_types_gen_params), + KUNIT_CASE_PARAM(cache_write_zero, sparse_cache_types_gen_params), KUNIT_CASE_PARAM(cache_range_window_reg, real_cache_types_only_gen_params), KUNIT_CASE_PARAM(raw_read_defaults_single, raw_test_types_gen_params), From caf78b0465053c23aa6211b9815dd5433766627d Mon Sep 17 00:00:00 2001 From: Mark Brown Date: Tue, 24 Sep 2024 12:08:08 +0200 Subject: [PATCH 3/7] regcache: Improve documentation of available cache types There is some user confusion about which cache types to choose when which is not helped by the lack of any central documentation providing an overview of what's available. Provide a short overview in the API header to try to help reduce this. Signed-off-by: Mark Brown Link: https://patch.msgid.link/20240924-regcache-document-types-v1-1-e157054e1215@kernel.org Signed-off-by: Mark Brown --- include/linux/regmap.h | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/include/linux/regmap.h b/include/linux/regmap.h index f9ccad32fc5c..da07584284ab 100644 --- a/include/linux/regmap.h +++ b/include/linux/regmap.h @@ -54,7 +54,14 @@ struct sdw_slave; #define REGMAP_UPSHIFT(s) (-(s)) #define REGMAP_DOWNSHIFT(s) (s) -/* An enum of all the supported cache types */ +/* + * The supported cache types, the default is no cache. Any new caches + * should usually use the maple tree cache unless they specifically + * require that there are never any allocations at runtime and can't + * provide defaults in which case they should use the flat cache. The + * rbtree cache *may* have some performance advantage for very low end + * systems that make heavy use of cache syncs but is mainly legacy. + */ enum regcache_type { REGCACHE_NONE, REGCACHE_RBTREE, From 96a54082424dc9d430076563031d9b576176674e Mon Sep 17 00:00:00 2001 From: Cheng-Cheng Lo Date: Tue, 29 Oct 2024 16:19:41 +0800 Subject: [PATCH 4/7] regmap: kunit: Fix repeated test param There're duplicated elements in the test param real_cache_types_list. The second one shoulde have cache type REGCACHE_MAPLE. Signed-off-by: Cheng-Cheng Lo Link: https://patch.msgid.link/20241029081941.3264566-1-locc@google.com Signed-off-by: Mark Brown --- drivers/base/regmap/regmap-kunit.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/base/regmap/regmap-kunit.c b/drivers/base/regmap/regmap-kunit.c index a42c7386b210..64ea340950b6 100644 --- a/drivers/base/regmap/regmap-kunit.c +++ b/drivers/base/regmap/regmap-kunit.c @@ -126,7 +126,7 @@ static const struct regmap_test_param real_cache_types_list[] = { { .cache = REGCACHE_RBTREE, .from_reg = 0x2003 }, { .cache = REGCACHE_RBTREE, .from_reg = 0x2004 }, { .cache = REGCACHE_MAPLE, .from_reg = 0 }, - { .cache = REGCACHE_RBTREE, .from_reg = 0, .fast_io = true }, + { .cache = REGCACHE_MAPLE, .from_reg = 0, .fast_io = true }, { .cache = REGCACHE_MAPLE, .from_reg = 0x2001 }, { .cache = REGCACHE_MAPLE, .from_reg = 0x2002 }, { .cache = REGCACHE_MAPLE, .from_reg = 0x2003 }, From 1ed9b927e7dd8b8cff13052efe212a8ff72ec51d Mon Sep 17 00:00:00 2001 From: Cristian Ciocaltea Date: Thu, 31 Oct 2024 18:37:04 +0200 Subject: [PATCH 5/7] regmap: maple: Provide lockdep (sub)class for maple tree's internal lock In some cases when using the maple tree register cache, the lockdep validator might complain about invalid deadlocks: [7.131886] Possible interrupt unsafe locking scenario: [7.131890] CPU0 CPU1 [7.131893] ---- ---- [7.131896] lock(&mt->ma_lock); [7.131904] local_irq_disable(); [7.131907] lock(rockchip_drm_vop2:3114:(&vop2_regmap_config)->lock); [7.131916] lock(&mt->ma_lock); [7.131925] [7.131928] lock(rockchip_drm_vop2:3114:(&vop2_regmap_config)->lock); [7.131936] *** DEADLOCK *** [7.131939] no locks held by swapper/0/0. [7.131944] the shortest dependencies between 2nd lock and 1st lock: [7.131950] -> (&mt->ma_lock){+.+.}-{2:2} { [7.131966] HARDIRQ-ON-W at: [7.131973] lock_acquire+0x200/0x330 [7.131986] _raw_spin_lock+0x50/0x70 [7.131998] regcache_maple_write+0x68/0xe0 [7.132010] regcache_write+0x6c/0x90 [7.132019] _regmap_read+0x19c/0x1d0 [7.132029] _regmap_update_bits+0xc0/0x148 [7.132038] regmap_update_bits_base+0x6c/0xa8 [7.132048] rk8xx_probe+0x22c/0x3d8 [7.132057] rk8xx_spi_probe+0x74/0x88 [7.132065] spi_probe+0xa8/0xe0 [...] [7.132675] } [7.132678] ... key at: [] __key.0+0x0/0x10 [7.132691] ... acquired at: [7.132695] _raw_spin_lock+0x50/0x70 [7.132704] regcache_maple_write+0x68/0xe0 [7.132714] regcache_write+0x6c/0x90 [7.132724] _regmap_read+0x19c/0x1d0 [7.132732] _regmap_update_bits+0xc0/0x148 [7.132741] regmap_field_update_bits_base+0x74/0xb8 [7.132751] vop2_plane_atomic_update+0x480/0x14d8 [rockchipdrm] [7.132820] drm_atomic_helper_commit_planes+0x1a0/0x320 [drm_kms_helper] [...] [7.135112] -> (rockchip_drm_vop2:3114:(&vop2_regmap_config)->lock){-...}-{2:2} { [7.135130] IN-HARDIRQ-W at: [7.135136] lock_acquire+0x200/0x330 [7.135147] _raw_spin_lock_irqsave+0x6c/0x98 [7.135157] regmap_lock_spinlock+0x20/0x40 [7.135166] regmap_read+0x44/0x90 [7.135175] vop2_isr+0x90/0x290 [rockchipdrm] [7.135225] __handle_irq_event_percpu+0x124/0x2d0 In the example above, the validator seems to get the scope of dependencies wrong, since the regmap instance used in rk8xx-spi driver has nothing to do with the instance from vop2. Improve validation by sharing the regmap's lockdep class with the maple tree's internal lock, while also providing a subclass for the latter. Signed-off-by: Cristian Ciocaltea Link: https://patch.msgid.link/20241031-regmap-maple-lockdep-fix-v2-1-06a3710f3623@collabora.com Signed-off-by: Mark Brown --- drivers/base/regmap/internal.h | 1 + drivers/base/regmap/regcache-maple.c | 3 +++ drivers/base/regmap/regmap.c | 1 + 3 files changed, 5 insertions(+) diff --git a/drivers/base/regmap/internal.h b/drivers/base/regmap/internal.h index 83acccdc1008..bdb450436cbc 100644 --- a/drivers/base/regmap/internal.h +++ b/drivers/base/regmap/internal.h @@ -59,6 +59,7 @@ struct regmap { unsigned long raw_spinlock_flags; }; }; + struct lock_class_key *lock_key; regmap_lock lock; regmap_unlock unlock; void *lock_arg; /* This is passed to lock/unlock functions */ diff --git a/drivers/base/regmap/regcache-maple.c b/drivers/base/regmap/regcache-maple.c index 8d27d3653ea3..23da7b31d715 100644 --- a/drivers/base/regmap/regcache-maple.c +++ b/drivers/base/regmap/regcache-maple.c @@ -355,6 +355,9 @@ static int regcache_maple_init(struct regmap *map) mt_init(mt); + if (!mt_external_lock(mt) && map->lock_key) + lockdep_set_class_and_subclass(&mt->ma_lock, map->lock_key, 1); + if (!map->num_reg_defaults) return 0; diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c index 4ded93687c1f..53131a7ede0a 100644 --- a/drivers/base/regmap/regmap.c +++ b/drivers/base/regmap/regmap.c @@ -745,6 +745,7 @@ struct regmap *__regmap_init(struct device *dev, lock_key, lock_name); } map->lock_arg = map; + map->lock_key = lock_key; } /* From 953e549471cabc9d4980f1da2e9fa79f4c23da06 Mon Sep 17 00:00:00 2001 From: Andy Shevchenko Date: Fri, 1 Nov 2024 18:55:53 +0200 Subject: [PATCH 6/7] regmap: irq: Set lockdep class for hierarchical IRQ domains Lockdep gives a false positive splat as it can't distinguish the lock which is taken by different IRQ descriptors from different IRQ chips that are organized in a way of a hierarchy: ====================================================== WARNING: possible circular locking dependency detected 6.12.0-rc5-next-20241101-00148-g9fabf8160b53 #562 Tainted: G W ------------------------------------------------------ modprobe/141 is trying to acquire lock: ffff899446947868 (intel_soc_pmic_bxtwc:502:(&bxtwc_regmap_config)->lock){+.+.}-{4:4}, at: regmap_update_bits_base+0x33/0x90 but task is already holding lock: ffff899446947c68 (&d->lock){+.+.}-{4:4}, at: __setup_irq+0x682/0x790 which lock already depends on the new lock. -> #3 (&d->lock){+.+.}-{4:4}: -> #2 (&desc->request_mutex){+.+.}-{4:4}: -> #1 (ipclock){+.+.}-{4:4}: -> #0 (intel_soc_pmic_bxtwc:502:(&bxtwc_regmap_config)->lock){+.+.}-{4:4}: Chain exists of: intel_soc_pmic_bxtwc:502:(&bxtwc_regmap_config)->lock --> &desc->request_mutex --> &d->lock Possible unsafe locking scenario: CPU0 CPU1 ---- ---- lock(&d->lock); lock(&desc->request_mutex); lock(&d->lock); lock(intel_soc_pmic_bxtwc:502:(&bxtwc_regmap_config)->lock); *** DEADLOCK *** 3 locks held by modprobe/141: #0: ffff8994419368f8 (&dev->mutex){....}-{4:4}, at: __driver_attach+0xf6/0x250 #1: ffff89944690b250 (&desc->request_mutex){+.+.}-{4:4}, at: __setup_irq+0x1a2/0x790 #2: ffff899446947c68 (&d->lock){+.+.}-{4:4}, at: __setup_irq+0x682/0x790 Set a lockdep class when we map the IRQ so that it doesn't warn about a lockdep bug that doesn't exist. Fixes: 4af8be67fd99 ("regmap: Convert regmap_irq to use irq_domain") Signed-off-by: Andy Shevchenko Link: https://patch.msgid.link/20241101165553.4055617-1-andriy.shevchenko@linux.intel.com Signed-off-by: Mark Brown --- drivers/base/regmap/regmap-irq.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c index 33ec28e3a802..0bcd81389a29 100644 --- a/drivers/base/regmap/regmap-irq.c +++ b/drivers/base/regmap/regmap-irq.c @@ -511,12 +511,16 @@ exit: return IRQ_NONE; } +static struct lock_class_key regmap_irq_lock_class; +static struct lock_class_key regmap_irq_request_class; + static int regmap_irq_map(struct irq_domain *h, unsigned int virq, irq_hw_number_t hw) { struct regmap_irq_chip_data *data = h->host_data; irq_set_chip_data(virq, data); + irq_set_lockdep_class(virq, ®map_irq_lock_class, ®map_irq_request_class); irq_set_chip(virq, &data->irq_chip); irq_set_nested_thread(virq, 1); irq_set_parent(virq, data->irq); From d1f4390dd28ba110f232615dc4610ac1bb2f39f2 Mon Sep 17 00:00:00 2001 From: Bartosz Golaszewski Date: Fri, 8 Nov 2024 16:07:37 +0200 Subject: [PATCH 7/7] regmap: provide regmap_assign_bits() Add another bits helper to regmap API: this one sets given bits if value is true and clears them if it's false. Suggested-by: Andy Shevchenko Signed-off-by: Bartosz Golaszewski Signed-off-by: Tomi Valkeinen Link: https://patch.msgid.link/20241108-assign-bits-v1-1-382790562d99@ideasonboard.com Signed-off-by: Mark Brown --- include/linux/regmap.h | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/include/linux/regmap.h b/include/linux/regmap.h index da07584284ab..0d5b74f9bd32 100644 --- a/include/linux/regmap.h +++ b/include/linux/regmap.h @@ -1335,6 +1335,15 @@ static inline int regmap_clear_bits(struct regmap *map, return regmap_update_bits_base(map, reg, bits, 0, NULL, false, false); } +static inline int regmap_assign_bits(struct regmap *map, unsigned int reg, + unsigned int bits, bool value) +{ + if (value) + return regmap_set_bits(map, reg, bits); + else + return regmap_clear_bits(map, reg, bits); +} + int regmap_test_bits(struct regmap *map, unsigned int reg, unsigned int bits); /** @@ -1803,6 +1812,13 @@ static inline int regmap_clear_bits(struct regmap *map, return -EINVAL; } +static inline int regmap_assign_bits(struct regmap *map, unsigned int reg, + unsigned int bits, bool value) +{ + WARN_ONCE(1, "regmap API is disabled"); + return -EINVAL; +} + static inline int regmap_test_bits(struct regmap *map, unsigned int reg, unsigned int bits) {