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

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2870 bytes --]

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 #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]