Skip to contentSkip to conversation

ASoC: dapm: Avoid creating kcontrol for params

PATCH RESEND Superseded [email protected] 3 participants 9 messages Sep 20, 2017

Subsystems (2)
Sound ASoC

v1 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

[email protected]

author
#0 lore raw

cover ASoC: dapm: Avoid creating kcontrol for params

From: anish kumar <[email protected]>

Currently in codec to codec dai link if there are multiple params defined then dapm can use created kcontrol to decide which param to apply at runtime.

However, in case there is only single param configuration then there is no point in creating the kcontrol and also there is no point in allocating memory for kcontrol.

In the snd_soc_dapm_new_pcm function, there is memory allocation happening for kcontrol which is later used or not used based on num_param. It is better to not allocate memory when there is only a single configuration. This change is to remedy that anomaly.

This code change doesn't do much except just refactor snd_soc_dapm_new_pcm function. Now the part of allocating memory for kcontrol is allocated to a new function and it is called only in the case it is needed.

anish kumar (4):
ASoC: dapm: fix error path in snd_soc_dapm_new_pcm
ASoC: dapm: Refactor the code in snd_soc_dapm_new_pcm
ASoC: dapm: Avoid creating kcontrol for params
ASoC: dapm: Remove the redundant check

 sound/soc/soc-dapm.c | 122 ++++++++++++++++++++++++++++-----------------------
 1 file changed, 66 insertions(+), 56 deletions(-)
Signature · 1 line
2.5.4 (Apple Git-61)
author
#1 lore raw

PATCH RESEND 1/4 ASoC: dapm: fix error path in snd_soc_dapm_new_pcm

From: anish kumar <[email protected]>

w_param_text[count] is freed in the wrong error path. Fix it by shifting the outfree_w_param label.

Signed-off-by anish kumar <[email protected]>
---
 sound/soc/soc-dapm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
sound/soc/soc-dapm.c +1 -1
index dcef67a..f51f613 100644
@@ -3907,9 +3907,9 @@ int snd_soc_dapm_new_pcm(struct snd_soc_card *card,
 	devm_kfree(card->dev, (void *)private_value);
 outfree_link_name:
 	devm_kfree(card->dev, link_name);
-outfree_w_param:
 	for (count = 0 ; count < num_params; count++)
 		devm_kfree(card->dev, (void *)w_param_text[count]);
+outfree_w_param:
 	devm_kfree(card->dev, w_param_text);
 
 	return ret;
Signature · 1 line
2.5.4 (Apple Git-61)
maintainer
#3 lore raw

Applied "ASoC: dapm: fix error path in snd_soc_dapm_new_pcm" to the asoc tree

In reply to [email protected]

The patch

ASoC: dapm: fix error path in snd_soc_dapm_new_pcm

has been applied to the asoc tree at

git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git

All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying to this mail.

Thanks,
Mark

Quoted 1 line

From e5af4f9fb3d405e6145f0d019ce11a97a8930915 Mon Sep 17 00:00:00 2001

From: anish kumar <[email protected]>
Date: Wed, 20 Sep 2017 01:28:33 -0700
Subject: [PATCH] ASoC: dapm: fix error path in snd_soc_dapm_new_pcm

w_param_text[count] is freed in the wrong error path. Fix it by shifting the outfree_w_param label.

Signed-off-by anish kumar <[email protected]>
Reviewed-by Charles Keepax <[email protected]>
Signed-off-by Mark Brown <[email protected]>
---
 sound/soc/soc-dapm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
sound/soc/soc-dapm.c +1 -1
index dcef67a9bd48..f51f61340f9c 100644
@@ -3907,9 +3907,9 @@ int snd_soc_dapm_new_pcm(struct snd_soc_card *card,
 	devm_kfree(card->dev, (void *)private_value);
 outfree_link_name:
 	devm_kfree(card->dev, link_name);
-outfree_w_param:
 	for (count = 0 ; count < num_params; count++)
 		devm_kfree(card->dev, (void *)w_param_text[count]);
+outfree_w_param:
 	devm_kfree(card->dev, w_param_text);
 
 	return ret;
Signature · 1 line
2.13.2
author
#4 lore raw

PATCH RESEND 2/4 ASoC: dapm: Refactor the code in snd_soc_dapm_new_pcm

In reply to [email protected]

From: anish kumar <[email protected]>

refactor snd_soc_dapm_new_pcm to reduce the size of this function to facilitate further refactoring.

Signed-off-by anish kumar <[email protected]>
---
 sound/soc/soc-dapm.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)
sound/soc/soc-dapm.c +4 -8
index f51f613..d55cac6 100644
@@ -3821,19 +3821,15 @@ int snd_soc_dapm_new_pcm(struct snd_soc_card *card,
 				devm_kasprintf(card->dev, GFP_KERNEL,
 					       "Anonymous Configuration %d",
 					       count);
-			if (!w_param_text[count]) {
-				ret = -ENOMEM;
-				goto outfree_link_name;
-			}
 		} else {
 			w_param_text[count] = devm_kmemdup(card->dev,
 						config->stream_name,
 						strlen(config->stream_name) + 1,
 						GFP_KERNEL);
-			if (!w_param_text[count]) {
-				ret = -ENOMEM;
-				goto outfree_link_name;
-			}
+		}
+		if (!w_param_text[count]) {
+			ret = -ENOMEM;
+			goto outfree_link_name;
 		}
 		config++;
 	}
Signature · 1 line
2.5.4 (Apple Git-61)
author
#6 lore raw

PATCH RESEND 3/4 ASoC: dapm: Avoid creating kcontrol for params

In reply to [email protected]

From: anish kumar <[email protected]>

Currently in codec to codec dai link if there are multiple params defined then dapm can use created kcontrol to decide which param to apply at runtime.

However, in case there is only single param configuration then there is no point in creating the kcontrol and also there is no point in allocating memory for kcontrol.

In the snd_soc_dapm_new_pcm function, there is memory allocation happening for kcontrol which is later used or not used based on num_param. It is better to not allocate memory when there is only a single configuration. This change is to remedy that anomaly.

Signed-off-by anish kumar <[email protected]>
---
 sound/soc/soc-dapm.c | 114 +++++++++++++++++++++++++++++----------------------
 1 file changed, 66 insertions(+), 48 deletions(-)
sound/soc/soc-dapm.c +66 -48
index d55cac6..3c493b3 100644
@@ -3778,18 +3778,12 @@ static int snd_soc_dapm_dai_link_put(struct snd_kcontrol *kcontrol,
 	return 0;
 }
 
-int snd_soc_dapm_new_pcm(struct snd_soc_card *card,
-			 const struct snd_soc_pcm_stream *params,
-			 unsigned int num_params,
-			 struct snd_soc_dapm_widget *source,
-			 struct snd_soc_dapm_widget *sink)
+static struct snd_kcontrol_new *
+snd_soc_dapm_alloc_kcontrol(struct snd_soc_card *card,
+			char *link_name,
+			const struct snd_soc_pcm_stream *params,
+			int num_params)
 {
-	struct snd_soc_dapm_widget template;
-	struct snd_soc_dapm_widget *w;
-	char *link_name;
-	int ret, count;
-	unsigned long private_value;
-	const char **w_param_text;
 	struct soc_enum w_param_enum[] = {
 		SOC_ENUM_SINGLE(0, 0, 0, NULL),
 	};

@@ -3798,19 +3792,16 @@ int snd_soc_dapm_new_pcm(struct snd_soc_card *card,
 			     snd_soc_dapm_dai_link_get,
 			     snd_soc_dapm_dai_link_put),
 	};
+	struct snd_kcontrol_new *kcontrol_news;
 	const struct snd_soc_pcm_stream *config = params;
+	const char **w_param_text;
+	unsigned long private_value;
+	int count;
 
 	w_param_text = devm_kcalloc(card->dev, num_params,
-					sizeof(char *), GFP_KERNEL);
+				sizeof(char *), GFP_KERNEL);
 	if (!w_param_text)
-		return -ENOMEM;
-
-	link_name = devm_kasprintf(card->dev, GFP_KERNEL, "%s-%s",
-				   source->name, sink->name);
-	if (!link_name) {
-		ret = -ENOMEM;
-		goto outfree_w_param;
-	}
+		return NULL;
 
 	for (count = 0 ; count < num_params; count++) {
 		if (!config->stream_name) {

@@ -3827,24 +3818,14 @@ int snd_soc_dapm_new_pcm(struct snd_soc_card *card,
 						strlen(config->stream_name) + 1,
 						GFP_KERNEL);
 		}
-		if (!w_param_text[count]) {
-			ret = -ENOMEM;
-			goto outfree_link_name;
-		}
+		if (!w_param_text[count])
+			goto outfree_w_param;
 		config++;
 	}
+
 	w_param_enum[0].items = num_params;
 	w_param_enum[0].texts = w_param_text;
 
-	memset(&template, 0, sizeof(template));
-	template.reg = SND_SOC_NOPM;
-	template.id = snd_soc_dapm_dai_link;
-	template.name = link_name;
-	template.event = snd_soc_dai_link_event;
-	template.event_flags = SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMU |
-		SND_SOC_DAPM_PRE_PMD;
-	template.num_kcontrols = 1;
-	/* duplicate w_param_enum on heap so that memory persists */
 	private_value =
 		(unsigned long) devm_kmemdup(card->dev,
 			(void *)(kcontrol_dai_link[0].private_value),

@@ -3852,22 +3833,66 @@ int snd_soc_dapm_new_pcm(struct snd_soc_card *card,
 	if (!private_value) {
 		dev_err(card->dev, "ASoC: Failed to create control for %s widget\n",
 			link_name);
-		ret = -ENOMEM;
-		goto outfree_link_name;
+		goto outfree_w_param;
 	}
 	kcontrol_dai_link[0].private_value = private_value;
 	/* duplicate kcontrol_dai_link on heap so that memory persists */
-	template.kcontrol_news =
-				devm_kmemdup(card->dev, &kcontrol_dai_link[0],
-					sizeof(struct snd_kcontrol_new),
-					GFP_KERNEL);
-	if (!template.kcontrol_news) {
+	kcontrol_news =
+			devm_kmemdup(card->dev, &kcontrol_dai_link[0],
+				sizeof(struct snd_kcontrol_new),
+				GFP_KERNEL);
+	if (!kcontrol_news) {
 		dev_err(card->dev, "ASoC: Failed to create control for %s widget\n",
 			link_name);
-		ret = -ENOMEM;
 		goto outfree_private_value;
 	}
+	return kcontrol_news;
+
+outfree_private_value:
+	devm_kfree(card->dev, (void *)private_value);
+outfree_w_param:
+	for (count = 0 ; count < num_params; count++)
+		devm_kfree(card->dev, (void *)w_param_text[count]);
+	devm_kfree(card->dev, w_param_text);
+	return NULL;
+}
+
+int snd_soc_dapm_new_pcm(struct snd_soc_card *card,
+			 const struct snd_soc_pcm_stream *params,
+			 unsigned int num_params,
+			 struct snd_soc_dapm_widget *source,
+			 struct snd_soc_dapm_widget *sink)
+{
+	struct snd_soc_dapm_widget template;
+	struct snd_soc_dapm_widget *w;
+	char *link_name;
+	int ret;
+
+	link_name = devm_kasprintf(card->dev, GFP_KERNEL, "%s-%s",
+				   source->name, sink->name);
+	if (!link_name)
+		return -ENOMEM;
 
+	memset(&template, 0, sizeof(template));
+	template.reg = SND_SOC_NOPM;
+	template.id = snd_soc_dapm_dai_link;
+	template.name = link_name;
+	template.event = snd_soc_dai_link_event;
+	template.event_flags = SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMU |
+		SND_SOC_DAPM_PRE_PMD;
+	template.kcontrol_news = NULL;
+
+	/* allocate memory for control, only in case of multiple configs */
+	if (num_params > 1) {
+		template.num_kcontrols = 1;
+		template.kcontrol_news =
+					snd_soc_dapm_alloc_kcontrol(card,
+						link_name, params, num_params);
+		if (!template.kcontrol_news) {
+			ret = -ENOMEM;
+			goto outfree_link_name;
+		}
+	}
 	dev_dbg(card->dev, "ASoC: adding %s widget\n", link_name);
 
 	w = snd_soc_dapm_new_control_unlocked(&card->dapm, &template);

@@ -3899,15 +3924,8 @@ int snd_soc_dapm_new_pcm(struct snd_soc_card *card,
 	devm_kfree(card->dev, w);
 outfree_kcontrol_news:
 	devm_kfree(card->dev, (void *)template.kcontrol_news);
-outfree_private_value:
-	devm_kfree(card->dev, (void *)private_value);
 outfree_link_name:
 	devm_kfree(card->dev, link_name);
-	for (count = 0 ; count < num_params; count++)
-		devm_kfree(card->dev, (void *)w_param_text[count]);
-outfree_w_param:
-	devm_kfree(card->dev, w_param_text);
-
 	return ret;
 }
Signature · 1 line
2.5.4 (Apple Git-61)
#7 lore raw
Quoted 10 lines · On Wed, Sep 20, 2017 at 01:28:35AM -0700, [email protected] wrote:

From: anish kumar <[email protected]>

Currently in codec to codec dai link if there are multiple params defined then dapm can use created kcontrol to decide which param to apply at runtime.

However, in case there is only single param configuration then there is no point in creating the kcontrol and also there is no point in allocating memory for kcontrol.

Quoted 8 lines

In the snd_soc_dapm_new_pcm function, there is memory allocation happening for kcontrol which is later used or not used based on num_param. It is better to not allocate memory when there is only a single configuration. This change is to remedy that anomaly.

Signed-off-by anish kumar <[email protected]>

---

This does make the code look quite a bit nicer, I think there is one small issue below.

<snip>

Quoted 29 lines
sound/soc/soc-dapm.c +28 -0
+int snd_soc_dapm_new_pcm(struct snd_soc_card *card,
+			 const struct snd_soc_pcm_stream *params,
+			 unsigned int num_params,
+			 struct snd_soc_dapm_widget *source,
+			 struct snd_soc_dapm_widget *sink)
+{
+	struct snd_soc_dapm_widget template;
+	struct snd_soc_dapm_widget *w;
+	char *link_name;
+	int ret;
+
+	link_name = devm_kasprintf(card->dev, GFP_KERNEL, "%s-%s",
+				   source->name, sink->name);
+	if (!link_name)
+		return -ENOMEM;
 
+	memset(&template, 0, sizeof(template));
+	template.reg = SND_SOC_NOPM;
+	template.id = snd_soc_dapm_dai_link;
+	template.name = link_name;
+	template.event = snd_soc_dai_link_event;
+	template.event_flags = SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMU |
+		SND_SOC_DAPM_PRE_PMD;
+	template.kcontrol_news = NULL;
+
+	/* allocate memory for control, only in case of multiple configs */
+	if (num_params > 1) {
+		template.num_kcontrols = 1;
+		template.kcontrol_news =

sound/soc/soc-dapm.c int snd_soc_dapm_new_pcm(struct snd_soc_card *card, · line 3898

Quoted 10 lines
sound/soc/soc-dapm.c +7 -0
+					snd_soc_dapm_alloc_kcontrol(card,
+						link_name, params, num_params);
+		if (!template.kcontrol_news) {
+			ret = -ENOMEM;
+			goto outfree_link_name;
+		}
+	}
 	dev_dbg(card->dev, "ASoC: adding %s widget\n", link_name);
 
 	w = snd_soc_dapm_new_control_unlocked(&card->dapm, &template);

If we fail this call or the call to snd_soc_dapm_add_path that follows it we don't clean up all the stuff we allocated in snd_soc_dapm_alloc_kcontrol. We probably need to add a de-alloc function for the error paths as well.

Thanks,
Charles

author
#8 lore raw

PATCH RESEND 4/4 ASoC: dapm: Remove the redundant check

In reply to [email protected]

From: anish kumar <[email protected]>

Remove this redundant check, the check for creating the control in the case of codec to codec dai link is moved to snd_soc_dapm_new_pcm function.

Signed-off-by anish kumar <[email protected]>
---
 sound/soc/soc-dapm.c | 4 ----
 1 file changed, 4 deletions(-)
sound/soc/soc-dapm.c +0 -4
index 3c493b3..fcad345 100644
@@ -1032,10 +1032,6 @@ static int dapm_new_dai_link(struct snd_soc_dapm_widget *w)
 	struct snd_soc_dapm_context *dapm = w->dapm;
 	struct snd_card *card = dapm->card->snd_card;
 
-	/* create control for links with > 1 config */
-	if (w->num_params <= 1)
-		return 0;
-
 	/* add kcontrol */
 	for (i = 0; i < w->num_kcontrols; i++) {
 		kcontrol = snd_soc_cnew(&w->kcontrol_news[i], w,
Signature · 1 line
2.5.4 (Apple Git-61)