[05/20] regmap: Restructure writes in _regmap_raw_write()

Commit Message

Markus Pargmann Aug. 12, 2015, 10:12 a.m. UTC

Currently we try to write the data without copying directly using
bus->write() or bus->gather_write() if it exists. If one of the previous
tries to write reported -ENOTSUPP or none of them were usable, we copy
the data into a buffer and use bus->write().

However it does not make sense to try bus->write() a second time with a
copied buffer if it didn't work the first time.

This patch restructures this if/else block to make it clear that this is
not intended for the case where bus->write() returns -ENOTSUPP.

Signed-off-by: Markus Pargmann <[e-mail]>
---
 drivers/base/regmap/regmap.c | 40 ++++++++++++++++++++++------------------
 1 file changed, 22 insertions(+), 18 deletions(-)

Comments

Mark Brown Aug. 12, 2015, 10:54 a.m. UTC | #1

On Wed, Aug 12, 2015 at 12:12:30PM +0200, Markus Pargmann wrote:
> Currently we try to write the data without copying directly using
> bus->write() or bus->gather_write() if it exists. If one of the previous
> tries to write reported -ENOTSUPP or none of them were usable, we copy
> the data into a buffer and use bus->write().
> 
> However it does not make sense to try bus->write() a second time with a
> copied buffer if it didn't work the first time.
> 
> This patch restructures this if/else block to make it clear that this is
> not intended for the case where bus->write() returns -ENOTSUPP.

I'm not entirely convinced that this is an improvement.  The main effect
that I'm seeing is an increase in the indentation level and there are
potential issues with the write operation being unable to work with some
kinds of memory (like restrictions about dmaing from stack or unalinged
memory for example) which mean that copying into a newly allocated
buffer may actually help.  I don't think we detect any such restrictions
at the minute but the defensiveness is nice and I'd really hope that the
failed write case isn't any kind of fast path.

Patch

diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index 86e94be3c749..f6bd3517a472 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -1340,30 +1340,34 @@  int _regmap_raw_write(struct regmap *map, unsigned int reg,
 	 * send the work_buf directly, otherwise try to do a gather
 	 * write.
 	 */
-	if (val == work_val)
+	if (val == work_val) {
 		ret = map->bus->write(map->bus_context, map->work_buf,
 				      map->format.reg_bytes +
 				      map->format.pad_bytes +
 				      val_len);
-	else if (map->bus->gather_write)
-		ret = map->bus->gather_write(map->bus_context, map->work_buf,
-					     map->format.reg_bytes +
-					     map->format.pad_bytes,
-					     val, val_len);
-
-	/* If that didn't work fall back on linearising by hand. */
-	if (ret == -ENOTSUPP) {
-		len = map->format.reg_bytes + map->format.pad_bytes + val_len;
-		buf = kzalloc(len, GFP_KERNEL);
-		if (!buf)
-			return -ENOMEM;
+	} else {
+		if (map->bus->gather_write)
+			ret = map->bus->gather_write(map->bus_context,
+						     map->work_buf,
+						     map->format.reg_bytes +
+						     map->format.pad_bytes,
+						     val, val_len);
+
+		/* If that didn't work fall back on linearising by hand. */
+		if (ret == -ENOTSUPP) {
+			len = map->format.reg_bytes + map->format.pad_bytes +
+				val_len;
+			buf = kzalloc(len, GFP_KERNEL);
+			if (!buf)
+				return -ENOMEM;
 
-		memcpy(buf, map->work_buf, map->format.reg_bytes);
-		memcpy(buf + map->format.reg_bytes + map->format.pad_bytes,
-		       val, val_len);
-		ret = map->bus->write(map->bus_context, buf, len);
+			memcpy(buf, map->work_buf, map->format.reg_bytes);
+			memcpy(buf + map->format.reg_bytes +
+			       map->format.pad_bytes, val, val_len);
+			ret = map->bus->write(map->bus_context, buf, len);
 
-		kfree(buf);
+			kfree(buf);
+		}
 	}
 
 	trace_regmap_hw_write_done(map, reg, val_len / map->format.val_bytes);