From d3601857e14de6369f00ae19564f1d817d175d19 Mon Sep 17 00:00:00 2001 From: Christophe JAILLET Date: Mon, 4 Sep 2023 22:04:06 +0200 Subject: regmap: debugfs: Fix a erroneous check after snprintf() This error handling looks really strange. Check if the string has been truncated instead. Fixes: f0c2319f9f19 ("regmap: Expose the driver name in debugfs") Signed-off-by: Christophe JAILLET Link: https://lore.kernel.org/r/8595de2462c490561f70020a6d11f4d6b652b468.1693857825.git.christophe.jaillet@wanadoo.fr Signed-off-by: Mark Brown --- drivers/base/regmap/regmap-debugfs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'drivers/base') diff --git a/drivers/base/regmap/regmap-debugfs.c b/drivers/base/regmap/regmap-debugfs.c index f36027591e1a..bdd80b73c3e6 100644 --- a/drivers/base/regmap/regmap-debugfs.c +++ b/drivers/base/regmap/regmap-debugfs.c @@ -48,7 +48,7 @@ static ssize_t regmap_name_read_file(struct file *file, name = map->dev->driver->name; ret = snprintf(buf, PAGE_SIZE, "%s\n", name); - if (ret < 0) { + if (ret >= PAGE_SIZE) { kfree(buf); return ret; } -- cgit v1.2.3 From fabe32cc1eca7857837abcb56b242bc4845b7067 Mon Sep 17 00:00:00 2001 From: Mark Brown Date: Mon, 23 Oct 2023 18:19:11 +0100 Subject: regmap: kunit: Fix marking of the range window as volatile For some reason the regmap used for testing ranges was not including the end of the range of paged registers as volatile since it found the end by counting from the selector register rather than the base of the window. Signed-off-by: Mark Brown Link: https://lore.kernel.org/r/20231023-regmap-test-window-cache-v1-1-d8a71f441968@kernel.org Signed-off-by: Mark Brown --- drivers/base/regmap/regmap-kunit.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'drivers/base') diff --git a/drivers/base/regmap/regmap-kunit.c b/drivers/base/regmap/regmap-kunit.c index 264d29b3fced..f79fd5ec187e 100644 --- a/drivers/base/regmap/regmap-kunit.c +++ b/drivers/base/regmap/regmap-kunit.c @@ -445,7 +445,7 @@ static struct regmap_range_cfg test_range = { static bool test_range_volatile(struct device *dev, unsigned int reg) { if (reg >= test_range.window_start && - reg <= test_range.selector_reg + test_range.window_len) + reg <= test_range.window_start + test_range.window_len) return true; if (reg >= test_range.range_min && reg <= test_range.range_max) -- cgit v1.2.3 From 6a2e332c2cbddd17d7dcb8f334953593f1324c8e Mon Sep 17 00:00:00 2001 From: Mark Brown Date: Mon, 23 Oct 2023 18:19:12 +0100 Subject: regmap: kunit: Add test for cache sync interaction with ranges Hector Martin reports that since when doing a cache sync we enable cache bypass if the selector register for a range is cached then we might leave the physical selector register pointing to a different value to that which we have in the cache. If we then try to write to the page that our cache tells us is selected we will not update the selector register and write to the wrong page. Add a test case covering this. Signed-off-by: Mark Brown Link: https://lore.kernel.org/r/20231023-regmap-test-window-cache-v1-2-d8a71f441968@kernel.org Signed-off-by: Mark Brown --- drivers/base/regmap/regmap-kunit.c | 66 ++++++++++++++++++++++++++++++++++++-- 1 file changed, 64 insertions(+), 2 deletions(-) (limited to 'drivers/base') diff --git a/drivers/base/regmap/regmap-kunit.c b/drivers/base/regmap/regmap-kunit.c index f79fd5ec187e..e14cc03a17f6 100644 --- a/drivers/base/regmap/regmap-kunit.c +++ b/drivers/base/regmap/regmap-kunit.c @@ -442,12 +442,20 @@ static struct regmap_range_cfg test_range = { .range_max = 40, }; -static bool test_range_volatile(struct device *dev, unsigned int reg) +static bool test_range_window_volatile(struct device *dev, unsigned int reg) { if (reg >= test_range.window_start && reg <= test_range.window_start + test_range.window_len) return true; + return false; +} + +static bool test_range_all_volatile(struct device *dev, unsigned int reg) +{ + if (test_range_window_volatile(dev, reg)) + return true; + if (reg >= test_range.range_min && reg <= test_range.range_max) return true; @@ -465,7 +473,7 @@ static void basic_ranges(struct kunit *test) config = test_regmap_config; config.cache_type = t->type; - config.volatile_reg = test_range_volatile; + config.volatile_reg = test_range_all_volatile; config.ranges = &test_range; config.num_ranges = 1; config.max_register = test_range.range_max; @@ -875,6 +883,59 @@ static void cache_present(struct kunit *test) regmap_exit(map); } +/* Check that caching the window register works with sync */ +static void cache_range_window_reg(struct kunit *test) +{ + struct regcache_types *t = (struct regcache_types *)test->param_value; + struct regmap *map; + struct regmap_config config; + struct regmap_ram_data *data; + unsigned int val; + int i; + + config = test_regmap_config; + config.cache_type = t->type; + config.volatile_reg = test_range_window_volatile; + config.ranges = &test_range; + config.num_ranges = 1; + config.max_register = test_range.range_max; + + map = gen_regmap(&config, &data); + KUNIT_ASSERT_FALSE(test, IS_ERR(map)); + if (IS_ERR(map)) + return; + + /* Write new values to the entire range */ + for (i = test_range.range_min; i <= test_range.range_max; i++) + KUNIT_ASSERT_EQ(test, 0, regmap_write(map, i, 0)); + + val = data->vals[test_range.selector_reg] & test_range.selector_mask; + KUNIT_ASSERT_EQ(test, val, 2); + + /* Write to the first register in the range to reset the page */ + KUNIT_ASSERT_EQ(test, 0, regmap_write(map, test_range.range_min, 0)); + val = data->vals[test_range.selector_reg] & test_range.selector_mask; + KUNIT_ASSERT_EQ(test, val, 0); + + /* Trigger a cache sync */ + regcache_mark_dirty(map); + KUNIT_ASSERT_EQ(test, 0, regcache_sync(map)); + + /* Write to the first register again, the page should be reset */ + KUNIT_ASSERT_EQ(test, 0, regmap_write(map, test_range.range_min, 0)); + val = data->vals[test_range.selector_reg] & test_range.selector_mask; + KUNIT_ASSERT_EQ(test, val, 0); + + /* Trigger another cache sync */ + regcache_mark_dirty(map); + KUNIT_ASSERT_EQ(test, 0, regcache_sync(map)); + + /* Write to the last register again, the page should be reset */ + KUNIT_ASSERT_EQ(test, 0, regmap_write(map, test_range.range_max, 0)); + val = data->vals[test_range.selector_reg] & test_range.selector_mask; + KUNIT_ASSERT_EQ(test, val, 2); +} + struct raw_test_types { const char *name; @@ -1217,6 +1278,7 @@ static struct kunit_case regmap_test_cases[] = { KUNIT_CASE_PARAM(cache_sync_patch, real_cache_types_gen_params), KUNIT_CASE_PARAM(cache_drop, sparse_cache_types_gen_params), KUNIT_CASE_PARAM(cache_present, sparse_cache_types_gen_params), + KUNIT_CASE_PARAM(cache_range_window_reg, real_cache_types_gen_params), KUNIT_CASE_PARAM(raw_read_defaults_single, raw_test_types_gen_params), KUNIT_CASE_PARAM(raw_read_defaults, raw_test_types_gen_params), -- cgit v1.2.3 From 0ec7731655de196bc1e4af99e495b38778109d22 Mon Sep 17 00:00:00 2001 From: Mark Brown Date: Thu, 26 Oct 2023 16:49:19 +0100 Subject: regmap: Ensure range selector registers are updated after cache sync When we sync the register cache we do so with the cache bypassed in order to avoid overhead from writing the synced values back into the cache. If the regmap has ranges and the selector register for those ranges is in a register which is cached this has the unfortunate side effect of meaning that the physical and cached copies of the selector register can be out of sync after a cache sync. The cache will have whatever the selector was when the sync started and the hardware will have the selector for the register that was synced last. Fix this by rewriting all cached selector registers after every sync, ensuring that the hardware and cache have the same content. This will result in extra writes that wouldn't otherwise be needed but is simple so hopefully robust. We don't read from the hardware since not all devices have physical read support. Given that nobody noticed this until now it is likely that we are rarely if ever hitting this case. Reported-by: Hector Martin Cc: stable@vger.kernel.org Signed-off-by: Mark Brown Link: https://lore.kernel.org/r/20231026-regmap-fix-selector-sync-v1-1-633ded82770d@kernel.org Signed-off-by: Mark Brown --- drivers/base/regmap/regcache.c | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) (limited to 'drivers/base') diff --git a/drivers/base/regmap/regcache.c b/drivers/base/regmap/regcache.c index c5d151e9c481..92592f944a3d 100644 --- a/drivers/base/regmap/regcache.c +++ b/drivers/base/regmap/regcache.c @@ -334,6 +334,11 @@ static int regcache_default_sync(struct regmap *map, unsigned int min, return 0; } +static int rbtree_all(const void *key, const struct rb_node *node) +{ + return 0; +} + /** * regcache_sync - Sync the register cache with the hardware. * @@ -351,6 +356,7 @@ int regcache_sync(struct regmap *map) unsigned int i; const char *name; bool bypass; + struct rb_node *node; if (WARN_ON(map->cache_type == REGCACHE_NONE)) return -EINVAL; @@ -392,6 +398,30 @@ out: /* Restore the bypass state */ map->cache_bypass = bypass; map->no_sync_defaults = false; + + /* + * If we did any paging with cache bypassed and a cached + * paging register then the register and cache state might + * have gone out of sync, force writes of all the paging + * registers. + */ + rb_for_each(node, 0, &map->range_tree, rbtree_all) { + struct regmap_range_node *this = + rb_entry(node, struct regmap_range_node, node); + + /* If there's nothing in the cache there's nothing to sync */ + ret = regcache_read(map, this->selector_reg, &i); + if (ret != 0) + continue; + + ret = _regmap_write(map, this->selector_reg, i); + if (ret != 0) { + dev_err(map->dev, "Failed to write %x = %x: %d\n", + this->selector_reg, i, ret); + break; + } + } + map->unlock(map->lock_arg); regmap_async_complete(map); -- cgit v1.2.3