[PATCH] regmap: don't corrupt work buffer in _regmap_raw_write()

[PATCH] regmap: don't corrupt work buffer in _regmap_raw_write()

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

_regmap_raw_write() contains code to call regcache_write() to write
values to the cache. That code calls memcpy() to copy the value data to
the start of the work_buf. However, at least when _regmap_raw_write() is
called from _regmap_bus_raw_write(), the value data is in the work_buf,
and this memcpy() operation may over-write part of that value data,
depending on the value of reg_bytes + pad_bytes. At least when using
reg_bytes==1 and pad_bytes==0, corruption of the value data does occur.

To solve this, remove the memcpy() operation, and modify the subsequent
.parse_val() call to parse the original value buffer directly.

At least in the case of 8-bit register address and 16-bit values, and
writes of single registers at a time, this memcpy-then-parse combination
used to cancel each-other out; for a work-buffer containing xx 89 03,
the memcpy changed it to 89 03 03, and the parse_val changed it back to
89 89 03, thus leaving the value uncorrupted. This appears completely
accidental though. Since commit 8a819ff "regmap: core: Split out in
place value parsing", .parse_val only returns the parsed value, and does
not modify the buffer, and hence does not (accidentally) undo the
corruption caused by memcpy(). This caused bogus values to get written
to HW, thus preventing e.g. audio playback on systems with a WM8903
CODEC. This patch fixes that.

Cc: Laxman Dewangan <ldewangan@nvidia.com>
Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
 drivers/base/regmap/regmap.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index fed91ac..1ab4498 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -963,8 +963,7 @@ static int _regmap_raw_write(struct regmap *map, unsigned int reg,
 		unsigned int ival;
 		int val_bytes = map->format.val_bytes;
 		for (i = 0; i < val_len / val_bytes; i++) {
-			memcpy(map->work_buf, val + (i * val_bytes), val_bytes);
-			ival = map->format.parse_val(map->work_buf);
+			ival = map->format.parse_val(val + (i * val_bytes));
 			ret = regcache_write(map, reg + (i * map->reg_stride),
 					     ival);
 			if (ret) {
-- 
1.7.10.4

Re: [PATCH] regmap: don't corrupt work buffer in _regmap_raw_write()

[Mark Brown]

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

On Wed, Mar 20, 2013 at 05:02:02PM -0600, Stephen Warren wrote:
> From: Stephen Warren <swarren@nvidia.com>
> 
> _regmap_raw_write() contains code to call regcache_write() to write
> values to the cache. That code calls memcpy() to copy the value data to
> the start of the work_buf. However, at least when _regmap_raw_write() is

Applied, thanks.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] regmap: don't corrupt work buffer in _regmap_raw_write()

[Paul Bolle]

On Thu, 2013-03-21 at 20:09 +0100, Mark Brown wrote:
> On Wed, Mar 20, 2013 at 05:02:02PM -0600, Stephen Warren wrote:
> > _regmap_raw_write() contains code to call regcache_write() to write
> > values to the cache. That code calls memcpy() to copy the value data to
> > the start of the work_buf. However, at least when _regmap_raw_write() is
> 
> Applied, thanks.

0) This patch ended up as mainline commit
bc8ce4afd7ee7e1421c935d24b1f879f82afdd4e, which is part of v3.9-rc6.

1) Building regmap.o now triggers this warning:
    drivers/base/regmap/regmap.c: In function ‘_regmap_raw_write’:
    drivers/base/regmap/regmap.c:946:4: warning: passing argument 1 of ‘map->format.parse_val’ discards ‘const’ qualifier from pointer target type [enabled by default]
    drivers/base/regmap/regmap.c:946:4: note: expected ‘void *’ but argument is of type ‘const void *’

2) The following (draft) patch silences this warning. I'm a bit
uncertain what the regmap_parse_*() functions are meant to do. So I'd
like to first ask whether something along these lines is acceptable.


Paul Bolle

diff --git a/drivers/base/regmap/internal.h b/drivers/base/regmap/internal.h
index 5a22bd3..35877b7 100644
--- a/drivers/base/regmap/internal.h
+++ b/drivers/base/regmap/internal.h
@@ -38,7 +38,7 @@ struct regmap_format {
                             unsigned int reg, unsigned int val);
        void (*format_reg)(void *buf, unsigned int reg, unsigned int shift);
        void (*format_val)(void *buf, unsigned int val, unsigned int shift);
-       unsigned int (*parse_val)(void *buf);
+       unsigned int (*parse_val)(const void *buf);
 };
 
 struct regmap_async {
diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index d34adef..2ca90a6 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -228,30 +228,28 @@ static void regmap_format_32_native(void *buf, unsigned int val,
        *(u32 *)buf = val << shift;
 }
 
-static unsigned int regmap_parse_8(void *buf)
+static unsigned int regmap_parse_8(const void *buf)
 {
-       u8 *b = buf;
+       const u8 *b = buf;
 
        return b[0];
 }
 
-static unsigned int regmap_parse_16_be(void *buf)
+static unsigned int regmap_parse_16_be(const void *buf)
 {
-       __be16 *b = buf;
+       const __be16 *b = buf;
 
-       b[0] = be16_to_cpu(b[0]);
-
-       return b[0];
+       return be16_to_cpu(b[0]);
 }
 
-static unsigned int regmap_parse_16_native(void *buf)
+static unsigned int regmap_parse_16_native(const void *buf)
 {
        return *(u16 *)buf;
 }
 
-static unsigned int regmap_parse_24(void *buf)
+static unsigned int regmap_parse_24(const void *buf)
 {
-       u8 *b = buf;
+       const u8 *b = buf;
        unsigned int ret = b[2];
        ret |= ((unsigned int)b[1]) << 8;
        ret |= ((unsigned int)b[0]) << 16;
@@ -259,16 +257,14 @@ static unsigned int regmap_parse_24(void *buf)
        return ret;
 }
 
-static unsigned int regmap_parse_32_be(void *buf)
+static unsigned int regmap_parse_32_be(const void *buf)
 {
-       __be32 *b = buf;
+       const __be32 *b = buf;
 
-       b[0] = be32_to_cpu(b[0]);
-
-       return b[0];
+       return be32_to_cpu(b[0]);
 }
 
-static unsigned int regmap_parse_32_native(void *buf)
+static unsigned int regmap_parse_32_native(const void *buf)
 {
        return *(u32 *)buf;
 }

Re: [PATCH] regmap: don't corrupt work buffer in _regmap_raw_write()

[Mark Brown]

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

On Mon, Apr 08, 2013 at 12:24:22PM +0200, Paul Bolle wrote:

> 0) This patch ended up as mainline commit
> bc8ce4afd7ee7e1421c935d24b1f879f82afdd4e, which is part of v3.9-rc6.

Numbering your paragraphs and using full commit IDs like this doesn't
help with legibility...

> 2) The following (draft) patch silences this warning. I'm a bit
> uncertain what the regmap_parse_*() functions are meant to do. So I'd
> like to first ask whether something along these lines is acceptable.

Nope, this breaks v3.9 - in that version they both modify in place and
return the parsed value.  In v3.10 it already does what you have because
the in place and return value versions have been split.

We need to either revert the commit from v3.9 on the basis that nobody
noticed the issue until some other work so it can't be that bad or come
up with a more invasive fix, at this point in the cycle I'm more
inclined to do the latter.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] regmap: don't corrupt work buffer in _regmap_raw_write()

[Stephen Warren]

On 04/08/2013 06:01 AM, Mark Brown wrote:
> On Mon, Apr 08, 2013 at 12:24:22PM +0200, Paul Bolle wrote:
> 
>> 0) This patch ended up as mainline commit 
>> bc8ce4afd7ee7e1421c935d24b1f879f82afdd4e, which is part of
>> v3.9-rc6.
> 
> Numbering your paragraphs and using full commit IDs like this
> doesn't help with legibility...
> 
>> 2) The following (draft) patch silences this warning. I'm a bit 
>> uncertain what the regmap_parse_*() functions are meant to do. So
>> I'd like to first ask whether something along these lines is
>> acceptable.
> 
> Nope, this breaks v3.9 - in that version they both modify in place
> and return the parsed value.  In v3.10 it already does what you
> have because the in place and return value versions have been
> split.
> 
> We need to either revert the commit from v3.9 on the basis that
> nobody noticed the issue until some other work so it can't be that
> bad or come up with a more invasive fix, at this point in the cycle
> I'm more inclined to do the latter.

I assume you mean "former" not "latter" above?

Revert might well be simplest and just fine. The issue this patch
fixes ended up getting accidentally "fixed" by the side-effects of
calling .parse_val() before commit 8a819ff "regmap: core: Split out in
place value parsing", and that commit only appears in 3.10, so we
should be able to revert this patch without issue in 3.9.