Skip to contentSkip to conversation

ASoC: core: Only kmemdup binary control buffer if masking

PATCH In review Charles Keepax 2 participants 3 messages Apr 2, 2014

Subsystems (2)
Sound ASoC
lore
Share & tools
lore

Charles Keepax

author
#0 lore raw

PATCH 1/1 ASoC: core: Only kmemdup binary control buffer if masking

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 <[email protected]>
---
 sound/soc/soc-core.c |   86 ++++++++++++++++++++++++-------------------------
 1 files changed, 42 insertions(+), 44 deletions(-)
sound/soc/soc-core.c +42 -44
index caebd63..275bd71 100644
@@ -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);
Signature · 1 line
1.7.2.5
maintainer
#1 lore raw
Quoted 5 lines · Charles Keepax

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...

sound/soc/soc-core.c int snd_soc_bytes_put(struct snd_kcontrol *kcontrol, · line 3232

Quoted 1 line
sound/soc/soc-core.c +0 -1
-	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.

author
#2 lore raw
Quoted 10 lines · Mark Brown
Quoted 5 lines · Charles Keepax

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...

Quoted 7 lines

sound/soc/soc-core.c int snd_soc_bytes_put(struct snd_kcontrol *kcontrol, · line 3232

Quoted 1 line
sound/soc/soc-core.c +0 -1
-	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