[alsa-devel] [PATCH v2] ASoC: dapm: Avoid creating kcontrol for params

[anish kumar]

On Wed, Sep 27, 2017 at 3:11 AM, Charles Keepax
<ckeepax at opensource.cirrus.com> wrote:
> On Tue, Sep 26, 2017 at 09:11:11PM -0700, yesanishhere at gmail.com wrote:
>> From: anish kumar <yesanishhere at gmail.com>
>>
>> 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 <yesanishhere at gmail.com>
>> ---
>> 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(-)
>>
>> diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c
>> index d55cac6..5dae9d8 100644
>> --- a/sound/soc/soc-dapm.c
>> +++ b/sound/soc/soc-dapm.c
>> -     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),
>> -                                     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?

Ok, will fix this.
>
>> +                             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 =
>> +                                     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?

No, as this is getting freed in the common function.
>
>> +             }
>> +     }
>>       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;
>>  }
>>
>> --
>> 2.5.4 (Apple Git-61)
>
> Thanks,
> Charles