Re: [PATCH v6] ASoC: dapm: add code to configure dai link parameters

[Mark Brown]

On Fri, Sep 12, 2014 at 10:11:04AM +0100, Nikesh Oswal wrote:
> dai-link params for codec-codec links were fixed. The fixed
> link between codec and another chip which may be another codec,
> baseband, bluetooth codec etc may require run time configuaration
> changes. This change provides an optional alsa control to select
> one of the params from a list of params.

The shape of this looks OK, though there are some issues below.  I
intend to test this before applying but couldn't do that since it
doesn't apply against current code.

> +	/* Select the configuration set by alsa control */
> +	config += w->params_select;
> +

Do an array lookup, code legibility is important.

> +static int snd_soc_dapm_dai_link_put(struct snd_kcontrol *kcontrol,
> +			  struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_soc_dapm_widget *w = snd_kcontrol_chip(kcontrol);
> +
> +	if (ucontrol->value.integer.value[0] == w->params_select)
> +		return 0;
> +
> +	if (ucontrol->value.integer.value[0] >= w->num_params)
> +		return -EINVAL;
> +
> +	w->params_select = ucontrol->value.integer.value[0];
> +
> +	return 0;
> +}

This will just silently not immediately do anything if an attempt is
made to change the parameters while the link is active.  There needs to
at least be a log message, and probably it's better to return -EBUSY
as well otherwise userspace might be going on reconfiguring things
assuming that this succeeded.

It'd be even better to actually reconfigure the link but that's a more
involved operation which might need us to power things down and can be
punted for now.

>  	len = strlen(source->name) + strlen(sink->name) + 2;
>  	link_name = devm_kzalloc(card->dev, len, GFP_KERNEL);
> -	if (!link_name)
> -		return -ENOMEM;
> +	if (!link_name) {
> +		ret = -ENOMEM;
> +		goto  outfree_w_param;
> +	}

Random extra space here and in some other gotos.

> +	for (count = 0 ; count < num_params; count++) {
> +		if (!config->stream_name)
> +			dev_warn(card->dapm.dev,
> +				"ASoC: anonymous config %d for dai link %s\n",
> +				count, link_name);
> +		w_param_text[count] = kmemdup((void *)(config->stream_name),
> +			strlen(config->stream_name) + 1, GFP_KERNEL);

Why are you casting to void * here?  This looks like it's open coding
kstrdup() and we're mixing devm_ and non-devm_ allocations in this
function.

This is also dereferencing stream_name immediately after finding that
it's NULL which isn't good.

> +	template.num_kcontrols = 1;
> +	private_value =
> +	(unsigned long) kmemdup((void *)(kcontrol_dai_link[0].private_value),
> +		sizeof(struct soc_enum), GFP_KERNEL);
> +	if (!private_value) {
> +		dev_err(card->dev, "ASoC: Failed to create control for %s widget\n",

So, we need to kmemdup() this thing that we just allocated?  If this is
needed it should be clear to the reader why we're doing this, especially
given all the funky casts.

Attachment: signature.asc
Description: Digital signature