[alsa-devel] [Resend] [PATCH v1 3/4] ASoC: dapm: Avoid creating kcontrol for params

[anish kumar]

On Mon, Sep 25, 2017 at 6:09 AM, Charles Keepax
<ckeepax at opensource.cirrus.com> wrote:
> On Fri, Sep 22, 2017 at 12:55:40AM -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>
>> ---
> <snip>
>> +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, count;
>> +
>> +     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;
>> +
>> +     w_param_text = devm_kcalloc(card->dev, num_params,
>> +                             sizeof(char *), GFP_KERNEL);
>> +     if (!w_param_text) {
>> +             ret = -ENOMEM;
>> +             goto param_fail;
>> +     }
>
> Should this not also be done conditionally based on num_params?
> Seems odd to do all this to avoid allocations but then alloc this
> regardless of if it is used.

Nice catch.
>
>> +
>> +     /* 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,
>> +                                             w_param_text, &private_value);
>> +             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 +3927,13 @@ 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]);
>
> Do we maybe just want to add a snd_soc_dapm_free_kcontrol or some
> such and call that from both places rather than having basically
> the same error path in snd_soc_dapm_alloc_kcontrol and here.

Sure. Let me re-spin it.
>
>> -outfree_w_param:
>> +outfree_link_name:
>>       devm_kfree(card->dev, w_param_text);
>> -
>> +param_fail:
>> +     devm_kfree(card->dev, link_name);
>>       return ret;
>>  }
>>
>> --
>> 2.5.4 (Apple Git-61)
>
> Thanks,
> Charles