diff mbox

ASoC: core: Only kmemdup binary control buffer if masking

Message ID 1396448225-19936-1-git-send-email-ckeepax@opensource.wolfsonmicro.com (mailing list archive)
State New, archived
Headers show

Commit Message

Charles Keepax April 2, 2014, 2:17 p.m. UTC 
When writing a binary control we may apply a mask to the first register,
as this requires modifying the data the buffer is duplicated, currently
this is done for all binary control writes. As most binary controls
don't use the mask facility and thus can freely use the original buffer,
avoid the kmemdup for these cases.

Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
---
 sound/soc/soc-core.c |   86 ++++++++++++++++++++++++-------------------------
 1 files changed, 42 insertions(+), 44 deletions(-)

Comments

Mark Brown April 2, 2014, 2:41 p.m. UTC | #1 
On Wed, Apr 02, 2014 at 03:17:05PM +0100, Charles Keepax wrote:

> When writing a binary control we may apply a mask to the first register,
> as this requires modifying the data the buffer is duplicated, currently
> this is done for all binary control writes. As most binary controls
> don't use the mask facility and thus can freely use the original buffer,
> avoid the kmemdup for these cases.

No, that's not why we're duplicating...

> -	data = kmemdup(ucontrol->value.bytes.data, len, GFP_KERNEL | GFP_DMA);

...note the GFP_DMA there, it's about ensuring that the buffer is
DMAable since the underlying APIs end up wanting that (or will for
larger coefficient blocks anyway).  What would be slightly more
efficient in the success case would be to do a kmalloc() then
copy_from_user() into that block allowing us to bypass the copy.
Charles Keepax April 2, 2014, 2:49 p.m. UTC | #2 
On Wed, Apr 02, 2014 at 03:41:03PM +0100, Mark Brown wrote:
> On Wed, Apr 02, 2014 at 03:17:05PM +0100, Charles Keepax wrote:
> 
> > When writing a binary control we may apply a mask to the first register,
> > as this requires modifying the data the buffer is duplicated, currently
> > this is done for all binary control writes. As most binary controls
> > don't use the mask facility and thus can freely use the original buffer,
> > avoid the kmemdup for these cases.
> 
> No, that's not why we're duplicating...
> 
> > -	data = kmemdup(ucontrol->value.bytes.data, len, GFP_KERNEL | GFP_DMA);
> 
> ...note the GFP_DMA there, it's about ensuring that the buffer is
> DMAable since the underlying APIs end up wanting that (or will for
> larger coefficient blocks anyway).  What would be slightly more
> efficient in the success case would be to do a kmalloc() then
> copy_from_user() into that block allowing us to bypass the copy.

Ah... thanks sorry about the noise.

Thanks,
Charles
diff mbox

Patch

diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index caebd63..275bd71 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -3229,67 +3229,65 @@  int snd_soc_bytes_put(struct snd_kcontrol *kcontrol,
 
 	len = params->num_regs * codec->val_bytes;
 
-	data = kmemdup(ucontrol->value.bytes.data, len, GFP_KERNEL | GFP_DMA);
-	if (!data)
-		return -ENOMEM;
+	if (!params->mask)
+		return regmap_raw_write(codec->control_data, params->base,
+					ucontrol->value.bytes.data, len);
 
 	/*
 	 * If we've got a mask then we need to preserve the register
 	 * bits.  We shouldn't modify the incoming data so take a
 	 * copy.
 	 */
-	if (params->mask) {
-		ret = regmap_read(codec->control_data, params->base, &val);
-		if (ret != 0)
-			goto out;
+	data = kmemdup(ucontrol->value.bytes.data, len,
+		       GFP_KERNEL | GFP_DMA);
+	if (!data)
+		return -ENOMEM;
 
-		val &= params->mask;
+	ret = regmap_read(codec->control_data, params->base, &val);
+	if (ret != 0)
+		goto out;
 
-		switch (codec->val_bytes) {
-		case 1:
-			((u8 *)data)[0] &= ~params->mask;
-			((u8 *)data)[0] |= val;
-			break;
-		case 2:
-			mask = ~params->mask;
-			ret = regmap_parse_val(codec->control_data,
-							&mask, &mask);
-			if (ret != 0)
-				goto out;
+	val &= params->mask;
 
-			((u16 *)data)[0] &= mask;
+	switch (codec->val_bytes) {
+	case 1:
+		((u8 *)data)[0] &= ~params->mask;
+		((u8 *)data)[0] |= val;
+		break;
+	case 2:
+		mask = ~params->mask;
+		ret = regmap_parse_val(codec->control_data, &mask, &mask);
+		if (ret != 0)
+			goto out;
 
-			ret = regmap_parse_val(codec->control_data,
-							&val, &val);
-			if (ret != 0)
-				goto out;
+		((u16 *)data)[0] &= mask;
 
-			((u16 *)data)[0] |= val;
-			break;
-		case 4:
-			mask = ~params->mask;
-			ret = regmap_parse_val(codec->control_data,
-							&mask, &mask);
-			if (ret != 0)
-				goto out;
+		ret = regmap_parse_val(codec->control_data, &val, &val);
+		if (ret != 0)
+			goto out;
 
-			((u32 *)data)[0] &= mask;
+		((u16 *)data)[0] |= val;
+		break;
+	case 4:
+		mask = ~params->mask;
+		ret = regmap_parse_val(codec->control_data, &mask, &mask);
+		if (ret != 0)
+			goto out;
 
-			ret = regmap_parse_val(codec->control_data,
-							&val, &val);
-			if (ret != 0)
-				goto out;
+		((u32 *)data)[0] &= mask;
 
-			((u32 *)data)[0] |= val;
-			break;
-		default:
-			ret = -EINVAL;
+		ret = regmap_parse_val(codec->control_data, &val, &val);
+		if (ret != 0)
 			goto out;
-		}
+
+		((u32 *)data)[0] |= val;
+		break;
+	default:
+		ret = -EINVAL;
+		goto out;
 	}
 
-	ret = regmap_raw_write(codec->control_data, params->base,
-			       data, len);
+	ret = regmap_raw_write(codec->control_data, params->base, data, len);
 
 out:
 	kfree(data);