[PATCH 0/2] regmap: Improve error handling in regcache_sync()

[PATCH 0/2] regmap: Improve error handling in regcache_sync()

[phucduc.bui]

From: bui duc phuc <phucduc.bui@gmail.com>

Hi all, 

While working on a cleanup series converting mutex and spinlock usage
to guard() helpers, I received review feedback that regcache_sync()
still has a goto-based error path.

https://lore.kernel.org/all/95f5d6ec-ecc5-4090-ac04-27cdcdc21b8a@sirena.org.uk/

Since guard() helpers should not be mixed with goto-based cleanup, 
I looked into whether the error handling could be simplified.

During that review, I noticed two issues in the current implementation
of regcache_sync():

 1. Cache synchronization errors can be overwritten by the result of
    selector register updates.
 2. cache_dirty is not updated if rewriting a selector register fails,
    leaving it inconsistent with the cache and hardware state. 

Together, these changes make the failure semantics of regcache_sync() 
clearer and eliminate the need for callers to invoke
regcache_mark_dirty() after regcache_sync() fails.

Best regards,
Phuc

bui duc phuc (2):
  regcache: Preserve cache synchronization errors in regcache_sync()
  regcache: Mark cache dirty if selector register rewrite fails

 drivers/base/regmap/regcache.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

-- 
2.43.0

[PATCH 1/2] regcache: Preserve cache synchronization errors in regcache_sync()

[phucduc.bui]

From: bui duc phuc <phucduc.bui@gmail.com>

regcache_sync() currently stores the return value from both cache
synchronization and selector register rewriting in the same variable.
As a result, a successful selector register rewrite can overwrite an
earlier cache synchronization error, causing regcache_sync() to return
success even though synchronization failed.

Track the two operations with separate return variables and preserve the
cache synchronization error. Errors from rewriting selector registers are
returned only if cache synchronization completed successfully.

Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---
 drivers/base/regmap/regcache.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/base/regmap/regcache.c b/drivers/base/regmap/regcache.c
index aa8f2efed779..040fd60c0d59 100644
--- a/drivers/base/regmap/regcache.c
+++ b/drivers/base/regmap/regcache.c
@@ -401,7 +401,8 @@ static int rbtree_all(const void *key, const struct rb_node *node)
  */
 int regcache_sync(struct regmap *map)
 {
-	int ret = 0;
+	int sync_ret = 0;
+	int selector_ret = 0;
 	unsigned int i;
 	const char *name;
 	bool bypass;
@@ -426,21 +427,21 @@ int regcache_sync(struct regmap *map)
 	/* Apply any patch first */
 	map->cache_bypass = true;
 	for (i = 0; i < map->patch_regs; i++) {
-		ret = _regmap_write(map, map->patch[i].reg, map->patch[i].def);
-		if (ret != 0) {
+		sync_ret = _regmap_write(map, map->patch[i].reg, map->patch[i].def);
+		if (sync_ret != 0) {
 			dev_err(map->dev, "Failed to write %x = %x: %d\n",
-				map->patch[i].reg, map->patch[i].def, ret);
+				map->patch[i].reg, map->patch[i].def, sync_ret);
 			goto out;
 		}
 	}
 	map->cache_bypass = false;
 
 	if (map->cache_ops->sync)
-		ret = map->cache_ops->sync(map, 0, map->max_register);
+		sync_ret = map->cache_ops->sync(map, 0, map->max_register);
 	else
-		ret = regcache_default_sync(map, 0, map->max_register);
+		sync_ret = regcache_default_sync(map, 0, map->max_register);
 
-	if (ret == 0)
+	if (sync_ret == 0)
 		map->cache_dirty = false;
 
 out:
@@ -462,10 +463,10 @@ int regcache_sync(struct regmap *map)
 		if (regcache_read(map, this->selector_reg, &i) != 0)
 			continue;
 
-		ret = _regmap_write(map, this->selector_reg, i);
-		if (ret != 0) {
+		selector_ret = _regmap_write(map, this->selector_reg, i);
+		if (selector_ret != 0) {
 			dev_err(map->dev, "Failed to write %x = %x: %d\n",
-				this->selector_reg, i, ret);
+				this->selector_reg, i, selector_ret);
 			break;
 		}
 	}
@@ -476,7 +477,7 @@ int regcache_sync(struct regmap *map)
 
 	trace_regcache_sync(map, name, "stop");
 
-	return ret;
+	return sync_ret ? sync_ret : selector_ret;
 }
 EXPORT_SYMBOL_GPL(regcache_sync);
 
-- 
2.43.0

[PATCH 2/2] regcache: Mark cache dirty if selector register rewrite fails

[phucduc.bui]

From: bui duc phuc <phucduc.bui@gmail.com>

After a successful cache synchronization, regcache_sync() clears
cache_dirty. If rewriting a selector register later fails, the cache
and hardware become inconsistent while the cache still appears clean.

Update cache_dirty to reflect the cache and hardware state when
selector register rewriting fails.

Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---
 drivers/base/regmap/regcache.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/base/regmap/regcache.c b/drivers/base/regmap/regcache.c
index 040fd60c0d59..96cdae25b9c4 100644
--- a/drivers/base/regmap/regcache.c
+++ b/drivers/base/regmap/regcache.c
@@ -465,6 +465,7 @@ int regcache_sync(struct regmap *map)
 
 		selector_ret = _regmap_write(map, this->selector_reg, i);
 		if (selector_ret != 0) {
+			map->cache_dirty = true;
 			dev_err(map->dev, "Failed to write %x = %x: %d\n",
 				this->selector_reg, i, selector_ret);
 			break;
-- 
2.43.0