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

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