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.

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

---
Changes in this version:
- Review comments from charles to move common function of error handling in seperate function.
Changes in v1:
- Included the review comments from charles regarding releasing memory in error path.

 sound/soc/soc-dapm.c | 145 +++++++++++++++++++++++++++++++--------------------
 1 file changed, 89 insertions(+), 56 deletions(-)

sound/soc/soc-dapm.c +3 -13

index d55cac6..5dae9d8 100644
-	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 =
+	*private_value =
 		(unsigned long) devm_kmemdup(card->dev,
 			(void *)(kcontrol_dai_link[0].private_value),
 			sizeof(struct soc_enum), GFP_KERNEL);
-	if (!private_value) {
+	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;
 	}

sound/soc/soc-dapm.c +44 -2

+				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;
+		goto outfree_w_param;
 	}
+	return kcontrol_news;
 
+outfree_w_param:
+	snd_soc_dapm_free_kcontrol(card, private_value, num_params, 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;
+	const char **w_param_text;
+	unsigned long private_value;
+	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) {
+		w_param_text = devm_kcalloc(card->dev, num_params,
+					sizeof(char *), GFP_KERNEL);
+		if (!w_param_text) {
+			ret = -ENOMEM;
+			goto param_fail;

sound/soc/soc-dapm.c +4 -8

+		}
+	}
 	dev_dbg(card->dev, "ASoC: adding %s widget\n", link_name);
 
 	w = snd_soc_dapm_new_control_unlocked(&card->dapm, &template);

@@ -3899,15 +3938,9 @@ 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:
+	snd_soc_dapm_free_kcontrol(card, &private_value, num_params, w_param_text);
+param_fail:
 	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)

Quoted 60 lines · On Tue, Sep 26, 2017 at 09:11:11PM -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.

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

---
Changes in this version:
- Review comments from charles to move common function of error handling in seperate function.
Changes in v1:
- Included the review comments from charles regarding releasing memory in error path.

 sound/soc/soc-dapm.c | 145 +++++++++++++++++++++++++++++++--------------------
 1 file changed, 89 insertions(+), 56 deletions(-)

sound/soc/soc-dapm.c +4 -17

index d55cac6..5dae9d8 100644
-     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 =
+     *private_value =
              (unsigned long) devm_kmemdup(card->dev,
                      (void *)(kcontrol_dai_link[0].private_value),
                      sizeof(struct soc_enum), GFP_KERNEL);
-     if (!private_value) {
+     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;
+     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),

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

Quoted 4 lines

sound/soc/soc-dapm.c +2 -2

-                                     GFP_KERNEL);
-     if (!template.kcontrol_news) {
+     kcontrol_news =
+                     devm_kmemdup(card->dev, &kcontrol_dai_link[0],

Not sure I really like this new line style especially here the devm_kmemdup would be in the same place on the previous line why have the break?

Quoted 55 lines

sound/soc/soc-dapm.c +48 -2

+                             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;
+             goto outfree_w_param;
      }
+     return kcontrol_news;
 
+outfree_w_param:
+     snd_soc_dapm_free_kcontrol(card, private_value, num_params, 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;
+     const char **w_param_text;
+     unsigned long private_value;
+     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) {
+             w_param_text = devm_kcalloc(card->dev, num_params,
+                                     sizeof(char *), GFP_KERNEL);
+             if (!w_param_text) {
+                     ret = -ENOMEM;
+                     goto param_fail;
+             }
+
+             template.num_kcontrols = 1;
+             template.kcontrol_news =

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

Quoted 6 lines

sound/soc/soc-dapm.c +6 -0

+                                     snd_soc_dapm_alloc_kcontrol(card,
+                                             link_name, params, num_params,
+                                             w_param_text, &private_value);
+             if (!template.kcontrol_news) {
+                     ret = -ENOMEM;
+                     goto param_fail;

Will you not leak w_param_text on this path?

Quoted 25 lines

sound/soc/soc-dapm.c +4 -9

+             }
+     }
      dev_dbg(card->dev, "ASoC: adding %s widget\n", link_name);
 
      w = snd_soc_dapm_new_control_unlocked(&card->dapm, &template);

@@ -3899,15 +3938,9 @@ 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:
+     snd_soc_dapm_free_kcontrol(card, &private_value, num_params, w_param_text);
+param_fail:
      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;
 }
 
--

Quoted 1 line

2.5.4 (Apple Git-61)

Thanks,
Charles

Quoted 17 lines · Charles Keepax

Quoted 16 lines · On Tue, Sep 26, 2017 at 09:11:11PM -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.

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.

Quoted 2 lines

Quoted 2 lines

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

---

No, as this is getting freed in the common function.

Quoted 1 line