linux-kernel - Re: [PATCH 1/3] regmap: flat: use the cache

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <52052401.7030707@nsn.com>
Date:	Fri, 09 Aug 2013 19:16:49 +0200
From:	Ionut Nicu <ioan.nicu.ext@....com>
To:	ext Mark Brown <broonie@...nel.org>
CC:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/3] regmap: flat: use the cache_present bitmap

On 09.08.2013 13:44, ext Mark Brown wrote:
> On Fri, Aug 09, 2013 at 12:09:11PM +0200, Ionut Nicu wrote:
>> As opposed to the other regmap cache implementations,
>> regcache_flat didn't use the cache_present bitmap for
>> figuring out whether a register was cached or not, nor
>> did it mark a register as present in the cache when
>> regcache_flat_write() was called.
> 
>> This caused incorrect behaviour, such as returning
>> value 0 for non-volatile registers without first reading
>> their value from the device and storing it in the cache.
> 
> The goal with the flat cache is to do as little work as possible for
> things like memory mapped devices where the cache operations are
> actually noticable in comparison with the I/O costs.  I would therefore
> exapect that anything using the flat cache would want to ensure that the
> cache is fully populated at init time, reading back from the device if
> nothing else (by setting num_reg_defaults_raw but not providing values),
> rather than do additional operations in the data path.
> 

Ok, I get your point. I've tried using this approach, but the thing is
that I have a device which supports only single rw operations. The
regcache_hw_init() function calls regmap_raw_read which in turn calls
_regmap_raw_read() when cache_bypass is enabled. But _regmap_raw_read()
doesn't take the use_single_rw flag into account and tries to read
num_reg_defaults_raw registers in a single bus transfer. Unfortunately
this doesn't work with my device. 

I thought about changing regcache_hw_init() to check for this flag in
the same way regmap_bulk_read() does, but I think this is pretty ugly.

What do you think about moving the check to use_single_rw inside
_regmap_raw_read(), right where the bus accesses are done?

Something like in the following patch:


diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index 42b45ac..de7f353 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -1486,14 +1486,16 @@ static int _regmap_raw_read(struct regmap *map, unsigned int reg, void *val,
 {
 	struct regmap_range_node *range;
 	u8 *u8 = map->work_buf;
-	int ret;
+	size_t val_bytes = map->format.val_bytes;
+	size_t reg_size = map->format.reg_bytes + map->format.pad_bytes;
+	int ret, i;
 
 	WARN_ON(!map->bus);
 
 	range = _regmap_range_lookup(map, reg);
 	if (range) {
 		ret = _regmap_select_page(map, &reg, range,
-					  val_len / map->format.val_bytes);
+					  val_len / val_bytes);
 		if (ret != 0)
 			return ret;
 	}
@@ -1508,15 +1510,41 @@ static int _regmap_raw_read(struct regmap *map, unsigned int reg, void *val,
 	 */
 	u8[0] |= map->read_flag_mask;
 
-	trace_regmap_hw_read_start(map->dev, reg,
-				   val_len / map->format.val_bytes);
+	/*
+	 * Some devices does not support bulk read, for
+	 * them we have a series of single read operations.
+	 */
+	if (map->use_single_rw) {
+		for (i = 0; i < val_len; i++) {
+			trace_regmap_hw_read_start(map->dev,
+					reg + (i * map->reg_stride), 1);
+
+			map->format.format_reg(map->work_buf,
+					reg + (i *  map->reg_stride),
+					map->reg_shift);
+			u8[0] |= map->read_flag_mask;
+
+			ret = map->bus->read(map->bus_context,
+					map->work_buf,
+					reg_size,
+					val + (i * val_bytes),
+					val_bytes);
+			if (ret != 0)
+				return ret;
 
-	ret = map->bus->read(map->bus_context, map->work_buf,
-			     map->format.reg_bytes + map->format.pad_bytes,
-			     val, val_len);
+			trace_regmap_hw_read_done(map->dev,
+					reg + (i * map->reg_stride), 1);
+		}
+	} else {
+		trace_regmap_hw_read_start(map->dev, reg, val_len / val_bytes);
 
-	trace_regmap_hw_read_done(map->dev, reg,
-				  val_len / map->format.val_bytes);
+		ret = map->bus->read(map->bus_context, map->work_buf,
+				reg_size, val, val_len);
+		if (ret != 0)
+			return ret;
+
+		trace_regmap_hw_read_done(map->dev, reg, val_len / val_bytes);
+	}
 
 	return ret;
 }
@@ -1702,25 +1730,9 @@ int regmap_bulk_read(struct regmap *map, unsigned int reg, void *val,
 		return -EINVAL;
 
 	if (vol || map->cache_type == REGCACHE_NONE) {
-		/*
-		 * Some devices does not support bulk read, for
-		 * them we have a series of single read operations.
-		 */
-		if (map->use_single_rw) {
-			for (i = 0; i < val_count; i++) {
-				ret = regmap_raw_read(map,
-						reg + (i * map->reg_stride),
-						val + (i * val_bytes),
-						val_bytes);
-				if (ret != 0)
-					return ret;
-			}
-		} else {
-			ret = regmap_raw_read(map, reg, val,
-					      val_bytes * val_count);
-			if (ret != 0)
-				return ret;
-		}
+		ret = regmap_raw_read(map, reg, val, val_bytes * val_count);
+		if (ret != 0)
+			return ret;
 
 		for (i = 0; i < val_count * val_bytes; i += val_bytes)
 			map->format.parse_inplace(val + i);

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/