[alsa-devel] [PATCH 4/4 v6] ALSA: usb-audio: Scarlett mixer interface for 6i6, 18i6, 18i8 and 18i20

[Takashi Iwai]

At Mon, 10 Nov 2014 12:59:18 -0600,
Chris J Arges wrote:
> 
> +/*
> + * Create enumeration strings from device_info
> + */
> +int scarlett_create_strings_from_info(struct scarlett_device_info *info)
> +{
> +	int i = 0, j = 0;

Superfluous initializations.


> +	char **names;
> +
> +	names = kmalloc_array(info->opt_master.len, sizeof(char *), GFP_KERNEL);
> +	if (!names) {
> +		kfree(names);

Superfluous kfree().

> +		return -ENOMEM;
> +	}
> +
> +	for (i = 0; i < info->opt_master.len; i++) {
> +		names[i] = kmalloc_array(SNDRV_CTL_ELEM_ID_NAME_MAXLEN,
> +				   sizeof(char), GFP_KERNEL);

Just use kmalloc().

> +		if (!names[i]) {
> +			for (j = 0; j < i-1; j++)
> +				kfree(names[j]);
> +			kfree(names);
> +			return -ENOMEM;
> +		}
> +		if (i > info->mix_start) {
> +			snprintf(names[i], SNDRV_CTL_ELEM_ID_NAME_MAXLEN,
> +				 "Mix %c", 'A'+(i - info->mix_start - 1));
> +		} else if (i > info->adat_start) {
> +			snprintf(names[i], SNDRV_CTL_ELEM_ID_NAME_MAXLEN,
> +				 "ADAT %d", i - info->adat_start);
> +		} else if (i > info->spdif_start) {
> +			snprintf(names[i], SNDRV_CTL_ELEM_ID_NAME_MAXLEN,
> +				 "SPDIF %d", i - info->spdif_start);
> +		} else if (i > info->analog_start) {
> +			snprintf(names[i], SNDRV_CTL_ELEM_ID_NAME_MAXLEN,
> +				 "Analog %d", i - info->analog_start);
> +		} else if (i > info->pcm_start) {
> +			snprintf(names[i], SNDRV_CTL_ELEM_ID_NAME_MAXLEN,
> +				 "PCM %d", i - info->pcm_start);
> +		} else {
> +			snprintf(names[i], SNDRV_CTL_ELEM_ID_NAME_MAXLEN,
> +				 "Off");
> +		}

BTW, another way would be to use kasprintf() and directly assign to
names[i].  It'll save memory usage a bit.  Although this won't
guarantee the max length, but these strings are obviously short
enough, so no big worry about it.

But, it's really a small difference, so I don't mind either way.

However...

> +	}
> +
> +	/* assign to the appropriate control */
> +	info->opt_master.names = (const char * const *)names;
> +	info->opt_matrix.names = (const char * const *)names;

... I guess these will be leaked without destructor?  This has to be
fixed.  You need to add a flag indicating the need of kfree() and do
it in the own destructor accordingly.


thanks,

Takashi