[PATCH v2 1/2] regmap: fix writes to non incrementing registers

[PATCH v2 1/2] regmap: fix writes to non incrementing registers

[Ben Whitten]

When checking if a register block is writable we must ensure that the
block does not start with or contain a non incrementing register.

Fixes: 8b9f9d4dc511 ("regmap: verify if register is writeable before writing operations")
Signed-off-by: Ben Whitten <ben.whitten@gmail.com>
---
 drivers/base/regmap/regmap.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index 19f57ccfbe1d..59f911e57719 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -1488,11 +1488,18 @@ static int _regmap_raw_write_impl(struct regmap *map, unsigned int reg,
 
 	WARN_ON(!map->bus);
 
-	/* Check for unwritable registers before we start */
-	for (i = 0; i < val_len / map->format.val_bytes; i++)
-		if (!regmap_writeable(map,
-				     reg + regmap_get_offset(map, i)))
-			return -EINVAL;
+	/* Check for unwritable or noinc registers in range
+	 * before we start
+	 */
+	if (!regmap_writeable_noinc(map, reg)) {
+		for (i = 0; i < val_len / map->format.val_bytes; i++) {
+			unsigned int element =
+				reg + regmap_get_offset(map, i);
+			if (!regmap_writeable(map, element) ||
+				regmap_writeable_noinc(map, element))
+				return -EINVAL;
+		}
+	}
 
 	if (!map->cache_bypass && map->format.parse_val) {
 		unsigned int ival;
-- 
2.17.1

[PATCH v2 2/2] regmap: stop splitting writes to non incrementing registers

[Ben Whitten]

When writing to non incrementing registers we should not split
the writes in any way, writing in one transaction.

Signed-off-by: Ben Whitten <ben.whitten@gmail.com>
---
 drivers/base/regmap/regmap.c | 38 +++++++++++++++++++++---------------
 1 file changed, 22 insertions(+), 16 deletions(-)

diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index 59f911e57719..d0dbf0ffd70f 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -1528,24 +1528,30 @@ static int _regmap_raw_write_impl(struct regmap *map, unsigned int reg,
 		int win_offset = (reg - range->range_min) % range->window_len;
 		int win_residue = range->window_len - win_offset;
 
-		/* If the write goes beyond the end of the window split it */
-		while (val_num > win_residue) {
-			dev_dbg(map->dev, "Writing window %d/%zu\n",
-				win_residue, val_len / map->format.val_bytes);
-			ret = _regmap_raw_write_impl(map, reg, val,
-						     win_residue *
-						     map->format.val_bytes);
-			if (ret != 0)
-				return ret;
+		if (!regmap_writeable_noinc(map, reg)) {
+			/* If the write goes beyond the end of the window
+			 * split it */
+			while (val_num > win_residue) {
+				dev_dbg(map->dev, "Writing window %d/%zu\n",
+					win_residue,
+					val_len / map->format.val_bytes);
+				ret = _regmap_raw_write_impl(map, reg, val,
+							     win_residue *
+							     map->format.val_bytes);
+				if (ret != 0)
+					return ret;
 
-			reg += win_residue;
-			val_num -= win_residue;
-			val += win_residue * map->format.val_bytes;
-			val_len -= win_residue * map->format.val_bytes;
+				reg += win_residue;
+				val_num -= win_residue;
+				val += win_residue * map->format.val_bytes;
+				val_len -= win_residue * map->format.val_bytes;
 
-			win_offset = (reg - range->range_min) %
-				range->window_len;
-			win_residue = range->window_len - win_offset;
+				win_offset = (reg - range->range_min) %
+					range->window_len;
+				win_residue = range->window_len - win_offset;
+			}
+		} else {
+			val_num = 1;
 		}
 
 		ret = _regmap_select_page(map, &reg, range, val_num);
-- 
2.17.1

Re: [PATCH v2 2/2] regmap: stop splitting writes to non incrementing registers

[Mark Brown]

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

On Sat, Jan 18, 2020 at 08:56:25PM +0000, Ben Whitten wrote:
> When writing to non incrementing registers we should not split
> the writes in any way, writing in one transaction.

That's not an obviously true statement.  If the user is intentionally
writing to a non-incrementing register and intends to stuff a block of
data into that one register via regmap_noinc_write() then sure but if
we've come in through a path that isn't specifically for the device or
is using one of the generic APIs then it's going to expect that the
framework will hide the unfortunate choices of the chip implementors and
split the I/O up.

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