[PATCH 0/5] ALSA: usb-audio: fixes for quirks of Tascam US-16x08

[PATCH 0/5] ALSA: usb-audio: fixes for quirks of Tascam US-16x08

[Takashi Sakamoto]

Hi,

A commit d2bb390a2081 ("ALSA: usb-audio: Tascam US-16x08 DSP mixer quirk")
was merged just now, while static code checker reports some apparent bugs.

This patchset is to solve the bugs.

Takashi Sakamoto (5):
  ALSA: usb-audio: localize one-referer variable
  ALSA: usb-audio: purge needless variable length array
  ALSA: usb-audio: localize function without external linkage
  ALSA: usb-audio: deallocate memory objects in error path
  ALSA: usb-audio: fix msleep timeout due to minimum resolution of task
    scheduling

 sound/usb/mixer_us16x08.c | 42 ++++++++++++++++++++++++------------------
 1 file changed, 24 insertions(+), 18 deletions(-)

-- 
2.9.3

[PATCH 1/5] ALSA: usb-audio: localize one-referrer variable

[Takashi Sakamoto]

When accessed by one referrer inner a file, variables should have static
qualifier to declare local-linkage.

This commit fixes the bug. Sparse generated below warnings.
sound/usb/mixer_us16x08.c:156:13: warning: duplicate const
sound/usb/mixer_us16x08.c:156:18: warning: symbol 'route_names' was not declared. Should it be static?

Fixes: d2bb390a2081 ("ALSA: usb-audio: Tascam US-16x08 DSP mixer quirk")
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/usb/mixer_us16x08.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/usb/mixer_us16x08.c b/sound/usb/mixer_us16x08.c
index 301939b..019336f 100644
--- a/sound/usb/mixer_us16x08.c
+++ b/sound/usb/mixer_us16x08.c
@@ -153,7 +153,7 @@ static const char ratio_map[] = {
 };
 
 /* route enumeration names */
-const const char *route_names[] = {
+static const char *const route_names[] = {
 	"Master Left", "Master Right", "Output 1", "Output 2", "Output 3",
 	"Output 4", "Output 5", "Output 6", "Output 7", "Output 8",
 };
-- 
2.9.3

Re: [PATCH 1/5] ALSA: usb-audio: localize one-referrer variable

[Takashi Iwai]

On Mon, 20 Feb 2017 21:09:17 +0100,
Takashi Sakamoto wrote:
> 
> When accessed by one referrer inner a file, variables should have static
> qualifier to declare local-linkage.
> 
> This commit fixes the bug. Sparse generated below warnings.
> sound/usb/mixer_us16x08.c:156:13: warning: duplicate const
> sound/usb/mixer_us16x08.c:156:18: warning: symbol 'route_names' was not declared. Should it be static?
> 
> Fixes: d2bb390a2081 ("ALSA: usb-audio: Tascam US-16x08 DSP mixer quirk")
> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>

Applied, thanks.


Takashi

[PATCH 2/5] ALSA: usb-audio: purge needless variable length array

[Takashi Sakamoto]

Variable length array is used in 'snd_us16x08_meter_get()', while there
is logically no need. It's better to purge it because variable length array
has overhead for stack handling.

This commit replaces the array with static length. Sparse generated below
warning.

sound/usb/mixer_us16x08.c:714:18: warning: Variable length array is used.

Fixes: d2bb390a2081 ("ALSA: usb-audio: Tascam US-16x08 DSP mixer quirk")
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/usb/mixer_us16x08.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/sound/usb/mixer_us16x08.c b/sound/usb/mixer_us16x08.c
index 019336f..d135e2e 100644
--- a/sound/usb/mixer_us16x08.c
+++ b/sound/usb/mixer_us16x08.c
@@ -711,7 +711,7 @@ static int snd_us16x08_meter_get(struct snd_kcontrol *kcontrol,
 	struct snd_usb_audio *chip = elem->head.mixer->chip;
 	struct snd_us16x08_meter_store *store = elem->private_data;
 	u8 meter_urb[64];
-	char tmp[max(sizeof(mix_init_msg1), sizeof(mix_init_msg2))];
+	char tmp[sizeof(mix_init_msg2)] = {0};
 
 	if (elem) {
 		store = (struct snd_us16x08_meter_store *) elem->private_data;
@@ -721,8 +721,8 @@ static int snd_us16x08_meter_get(struct snd_kcontrol *kcontrol,
 
 	switch (kcontrol->private_value) {
 	case 0:
-		memcpy(tmp, mix_init_msg1, sizeof(mix_init_msg1));
-		snd_us16x08_send_urb(chip, tmp, 4);
+		snd_us16x08_send_urb(chip, (char *)mix_init_msg1,
+				     sizeof(mix_init_msg1));
 		snd_us16x08_recv_urb(chip, meter_urb,
 			sizeof(meter_urb));
 		kcontrol->private_value++;
@@ -740,7 +740,8 @@ static int snd_us16x08_meter_get(struct snd_kcontrol *kcontrol,
 	case 3:
 		memcpy(tmp, mix_init_msg2, sizeof(mix_init_msg2));
 		tmp[2] = snd_get_meter_comp_index(store);
-		snd_us16x08_send_urb(chip, tmp, 10);
+		snd_us16x08_send_urb(chip, (char *)mix_init_msg2,
+				     sizeof(mix_init_msg2));
 		snd_us16x08_recv_urb(chip, meter_urb,
 			sizeof(meter_urb));
 		kcontrol->private_value = 0;
-- 
2.9.3

Re: [PATCH 2/5] ALSA: usb-audio: purge needless variable length array

[Takashi Iwai]

On Mon, 20 Feb 2017 21:09:18 +0100,
Takashi Sakamoto wrote:
> 
> Variable length array is used in 'snd_us16x08_meter_get()', while there
> is logically no need. It's better to purge it because variable length array
> has overhead for stack handling.
> 
> This commit replaces the array with static length. Sparse generated below
> warning.
> 
> sound/usb/mixer_us16x08.c:714:18: warning: Variable length array is used.
> 
> Fixes: d2bb390a2081 ("ALSA: usb-audio: Tascam US-16x08 DSP mixer quirk")
> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>

Applied, thanks.


Takashi

Re: [PATCH 2/5] ALSA: usb-audio: purge needless variable length array

[Takashi Iwai]

On Mon, 20 Feb 2017 21:51:54 +0100,
Takashi Iwai wrote:
> 
> On Mon, 20 Feb 2017 21:09:18 +0100,
> Takashi Sakamoto wrote:
> > 
> > Variable length array is used in 'snd_us16x08_meter_get()', while there
> > is logically no need. It's better to purge it because variable length array
> > has overhead for stack handling.
> > 
> > This commit replaces the array with static length. Sparse generated below
> > warning.
> > 
> > sound/usb/mixer_us16x08.c:714:18: warning: Variable length array is used.
> > 
> > Fixes: d2bb390a2081 ("ALSA: usb-audio: Tascam US-16x08 DSP mixer quirk")
> > Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> 
> Applied, thanks.

Eh, no, this patch is buggy.  Scratched.

> @@ -740,7 +740,8 @@ static int snd_us16x08_meter_get(struct snd_kcontrol *kcontrol,
>  	case 3:
>  		memcpy(tmp, mix_init_msg2, sizeof(mix_init_msg2));
>  		tmp[2] = snd_get_meter_comp_index(store);
> -		snd_us16x08_send_urb(chip, tmp, 10);
> +		snd_us16x08_send_urb(chip, (char *)mix_init_msg2,
> +				     sizeof(mix_init_msg2));

Here you overlooked that tmp[2] is dynamically changed.


Takashi

Re: [PATCH 2/5] ALSA: usb-audio: purge needless variable length array

[Takashi Sakamoto]

On 2017年02月21日 06:04, Takashi Iwai wrote:
> On Mon, 20 Feb 2017 21:51:54 +0100,
> Takashi Iwai wrote:
>>
>> On Mon, 20 Feb 2017 21:09:18 +0100,
>> Takashi Sakamoto wrote:
>>>
>>> Variable length array is used in 'snd_us16x08_meter_get()', while there
>>> is logically no need. It's better to purge it because variable length array
>>> has overhead for stack handling.
>>>
>>> This commit replaces the array with static length. Sparse generated below
>>> warning.
>>>
>>> sound/usb/mixer_us16x08.c:714:18: warning: Variable length array is used.
>>>
>>> Fixes: d2bb390a2081 ("ALSA: usb-audio: Tascam US-16x08 DSP mixer quirk")
>>> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
>>
>> Applied, thanks.
>
> Eh, no, this patch is buggy.  Scratched.
>
>> @@ -740,7 +740,8 @@ static int snd_us16x08_meter_get(struct snd_kcontrol *kcontrol,
>>  	case 3:
>>  		memcpy(tmp, mix_init_msg2, sizeof(mix_init_msg2));
>>  		tmp[2] = snd_get_meter_comp_index(store);
>> -		snd_us16x08_send_urb(chip, tmp, 10);
>> +		snd_us16x08_send_urb(chip, (char *)mix_init_msg2,
>> +				     sizeof(mix_init_msg2));
>
> Here you overlooked that tmp[2] is dynamically changed.

Oops. I've realized it during my reviewing, but forget to care when 
coding, sorry...

Please abandon this patch. I post the alternative this night.


Thanks

Takashi Sakamoto
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

[PATCH 3/5] ALSA: usb-audio: localize function without external linkage

[Takashi Sakamoto]

When accessed inner a file, functions should have static qualifier for
local-linkage.

This commit fixes the bug. Sparse generated below warning.

sound/usb/mixer_us16x08.c:1043:32: warning: symbol 'snd_us16x08_create_meter_store' was not declared. Should it be static?

Fixes: d2bb390a2081 ("ALSA: usb-audio: Tascam US-16x08 DSP mixer quirk")
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/usb/mixer_us16x08.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/usb/mixer_us16x08.c b/sound/usb/mixer_us16x08.c
index d135e2e..5dae63c 100644
--- a/sound/usb/mixer_us16x08.c
+++ b/sound/usb/mixer_us16x08.c
@@ -1041,7 +1041,7 @@ static struct snd_us16x08_eq_store *snd_us16x08_create_eq_store(void)
 	return tmp;
 }
 
-struct snd_us16x08_meter_store *snd_us16x08_create_meter_store(void)
+static struct snd_us16x08_meter_store *snd_us16x08_create_meter_store(void)
 {
 	struct snd_us16x08_meter_store *tmp =
 		kzalloc(sizeof(struct snd_us16x08_meter_store), GFP_KERNEL);
-- 
2.9.3

Re: [PATCH 3/5] ALSA: usb-audio: localize function without external linkage

[Takashi Iwai]

On Mon, 20 Feb 2017 21:09:19 +0100,
Takashi Sakamoto wrote:
> 
> When accessed inner a file, functions should have static qualifier for
> local-linkage.
> 
> This commit fixes the bug. Sparse generated below warning.
> 
> sound/usb/mixer_us16x08.c:1043:32: warning: symbol 'snd_us16x08_create_meter_store' was not declared. Should it be static?
> 
> Fixes: d2bb390a2081 ("ALSA: usb-audio: Tascam US-16x08 DSP mixer quirk")
> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>

Applied, thanks.


Takashi

[PATCH 4/5] ALSA: usb-audio: deallocate memory objects in error path

[Takashi Sakamoto]

Some functions for quirks of Tascam US-16x08 have memory leaks.

This commit fixes the bugs.

Fixes: d2bb390a2081 ("ALSA: usb-audio: Tascam US-16x08 DSP mixer quirk")
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/usb/mixer_us16x08.c | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/sound/usb/mixer_us16x08.c b/sound/usb/mixer_us16x08.c
index 5dae63c..d34dd1c 100644
--- a/sound/usb/mixer_us16x08.c
+++ b/sound/usb/mixer_us16x08.c
@@ -1082,8 +1082,8 @@ static int add_new_ctl(struct usb_mixer_interface *mixer,
 
 	kctl = snd_ctl_new1(ncontrol, elem);
 	if (!kctl) {
-		kfree(elem);
-		return -ENOMEM;
+		err = -ENOMEM;
+		goto end;
 	}
 
 	kctl->private_free = freeer;
@@ -1092,11 +1092,12 @@ static int add_new_ctl(struct usb_mixer_interface *mixer,
 
 	err = snd_usb_mixer_add_control(&elem->head, kctl);
 	if (err < 0)
-		return err;
+		goto end;
 
 	if (elem_ret)
 		*elem_ret = elem;
-
+end:
+	kfree(elem);
 	return 0;
 }
 
@@ -1367,7 +1368,7 @@ int snd_us16x08_controls_create(struct usb_mixer_interface *mixer)
 			usb_audio_dbg(mixer->chip,
 				"Failed to create route control, err:%d\n",
 				err);
-			return err;
+			goto err_allocated;
 		}
 		for (i = 0; i < 8; i++)
 			elem->cache_val[i] = i < 2 ? i : i + 2;
@@ -1388,7 +1389,7 @@ int snd_us16x08_controls_create(struct usb_mixer_interface *mixer)
 				comp_store,
 				master_controls[i].freeer, &elem);
 			if (err < 0)
-				return err;
+				goto err_allocated;
 			elem->cache_val[0] = master_controls[i].default_val;
 			elem->cached = 1;
 		}
@@ -1408,7 +1409,7 @@ int snd_us16x08_controls_create(struct usb_mixer_interface *mixer)
 				comp_store,
 				channel_controls[i].freeer, &elem);
 			if (err < 0)
-				return err;
+				goto err_allocated;
 			for (j = 0; j < SND_US16X08_MAX_CHANNELS; j++) {
 				elem->cache_val[j] =
 					channel_controls[i].default_val;
@@ -1429,7 +1430,7 @@ int snd_us16x08_controls_create(struct usb_mixer_interface *mixer)
 				eq_store,
 				eq_controls[i].freeer, NULL);
 			if (err < 0)
-				return err;
+				goto err_allocated;
 		}
 
 		/* add compressor controls */
@@ -1447,7 +1448,7 @@ int snd_us16x08_controls_create(struct usb_mixer_interface *mixer)
 				comp_store,
 				comp_controls[i].freeer, NULL);
 			if (err < 0)
-				return err;
+				goto err_allocated;
 		}
 
 		/* meter function 'get' must access to compressor store
@@ -1458,9 +1459,13 @@ int snd_us16x08_controls_create(struct usb_mixer_interface *mixer)
 			SND_US16X08_ID_METER, USB_MIXER_U16, 0, "Level Meter",
 			(void *) meter_store, snd_usb_mixer_elem_free, NULL);
 		if (err < 0)
-			return err;
+			goto err_allocated;
 	}
 
 	return 0;
+err_allocated:
+	kfree(meter_store);
+	kfree(eq_store);
+	kfree(comp_store);
+	return err;
 }
-
-- 
2.9.3

Re: [PATCH 4/5] ALSA: usb-audio: deallocate memory objects in error path

[Takashi Iwai]

On Mon, 20 Feb 2017 21:09:20 +0100,
Takashi Sakamoto wrote:
> 
> Some functions for quirks of Tascam US-16x08 have memory leaks.
> 
> This commit fixes the bugs.
> 
> Fixes: d2bb390a2081 ("ALSA: usb-audio: Tascam US-16x08 DSP mixer quirk")
> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> ---
>  sound/usb/mixer_us16x08.c | 27 ++++++++++++++++-----------
>  1 file changed, 16 insertions(+), 11 deletions(-)
> 
> diff --git a/sound/usb/mixer_us16x08.c b/sound/usb/mixer_us16x08.c
> index 5dae63c..d34dd1c 100644
> --- a/sound/usb/mixer_us16x08.c
> +++ b/sound/usb/mixer_us16x08.c
> @@ -1082,8 +1082,8 @@ static int add_new_ctl(struct usb_mixer_interface *mixer,
>  
>  	kctl = snd_ctl_new1(ncontrol, elem);
>  	if (!kctl) {
> -		kfree(elem);
> -		return -ENOMEM;
> +		err = -ENOMEM;
> +		goto end;
>  	}
>  
>  	kctl->private_free = freeer;
> @@ -1092,11 +1092,12 @@ static int add_new_ctl(struct usb_mixer_interface *mixer,
>  
>  	err = snd_usb_mixer_add_control(&elem->head, kctl);
>  	if (err < 0)
> -		return err;
> +		goto end;
>  
>  	if (elem_ret)
>  		*elem_ret = elem;
> -
> +end:
> +	kfree(elem);
>  	return 0;

This will release elem even for the success case, no?


Takashi

Re: [PATCH 4/5] ALSA: usb-audio: deallocate memory objects in error path

[Takashi Sakamoto]

On Feb 21 2017 05:49, Takashi Iwai wrote:
> On Mon, 20 Feb 2017 21:09:20 +0100,
> Takashi Sakamoto wrote:
>>
>> Some functions for quirks of Tascam US-16x08 have memory leaks.
>>
>> This commit fixes the bugs.
>>
>> Fixes: d2bb390a2081 ("ALSA: usb-audio: Tascam US-16x08 DSP mixer quirk")
>> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
>> ---
>>  sound/usb/mixer_us16x08.c | 27 ++++++++++++++++-----------
>>  1 file changed, 16 insertions(+), 11 deletions(-)
>>
>> diff --git a/sound/usb/mixer_us16x08.c b/sound/usb/mixer_us16x08.c
>> index 5dae63c..d34dd1c 100644
>> --- a/sound/usb/mixer_us16x08.c
>> +++ b/sound/usb/mixer_us16x08.c
>> @@ -1082,8 +1082,8 @@ static int add_new_ctl(struct usb_mixer_interface *mixer,
>>
>>  	kctl = snd_ctl_new1(ncontrol, elem);
>>  	if (!kctl) {
>> -		kfree(elem);
>> -		return -ENOMEM;
>> +		err = -ENOMEM;
>> +		goto end;
>>  	}
>>
>>  	kctl->private_free = freeer;
>> @@ -1092,11 +1092,12 @@ static int add_new_ctl(struct usb_mixer_interface *mixer,
>>
>>  	err = snd_usb_mixer_add_control(&elem->head, kctl);
>>  	if (err < 0)
>> -		return err;
>> +		goto end;
>>
>>  	if (elem_ret)
>>  		*elem_ret = elem;
>> -
>> +end:
>> +	kfree(elem);
>>  	return 0;
>
> This will release elem even for the success case, no?

Yes, it's my intension of this patch.

But it's wrong. The allocated memory objects are used for private data 
for each control element set. I didn't care of it...

Here, what I should fix is just for error path. I'll post revised 
version of this patch this night.


Thanks

Takashi Sakamoto

Re: [PATCH 4/5] ALSA: usb-audio: deallocate memory objects in error path

[Takashi Sakamoto]

On Fe 21 2017 11:32, Takashi Sakamoto wrote:
> But it's wrong. The allocated memory objects are used for private data
> for each control element set. I didn't care of it...
>
> Here, what I should fix is just for error path. I'll post revised
> version of this patch this night.

I realize that this is not concern because the code includes assignment 
deallocate function to each control element set. Thus, in error path, 
the allocated private data is going to free in below callgrach.

(sound/usb/mixer_us16x08.c)
add_new_ctl(elem)
   kctl->private_free = freeer;
   (sound/usb/mixer.c)
   ->snd_usb_mixer_add_control(kctl)
     (sound/core/control.c)
     ->snd_ctl_add(kctl)
       ->snd_ctl_free_one(kctl)
         (in error path)
         ->kcontrol->private_free(kctl);
         (sound/usb/mixer_us16x08.c)
         = freeer()


Regards

Takashi Sakamoto

Re: [PATCH 4/5] ALSA: usb-audio: deallocate memory objects in error path

[Takashi Sakamoto]

On Feb 22 2017 07:45, Takashi Sakamoto wrote:
> On Fe 21 2017 11:32, Takashi Sakamoto wrote:
>> But it's wrong. The allocated memory objects are used for private data
>> for each control element set. I didn't care of it...
>>
>> Here, what I should fix is just for error path. I'll post revised
>> version of this patch this night.
>
> I realize that this is not concern because the code includes assignment
> deallocate function to each control element set. Thus, in error path,
> the allocated private data is going to free in below callgrach.
>
> (sound/usb/mixer_us16x08.c)
> add_new_ctl(elem)
>   kctl->private_free = freeer;
>   (sound/usb/mixer.c)
>   ->snd_usb_mixer_add_control(kctl)
>     (sound/core/control.c)
>     ->snd_ctl_add(kctl)
>       ->snd_ctl_free_one(kctl)
>         (in error path)
>         ->kcontrol->private_free(kctl);
>         (sound/usb/mixer_us16x08.c)
>         = freeer()

Oops. On the other hand, the private data for unregistered control 
element set is not deallocated automatically in error path of the other 
control element set. It should be fixed...


Regards

Takashi Sakamoto

Re: [PATCH 4/5] ALSA: usb-audio: deallocate memory objects in error path

[Takashi Sakamoto]

On Feb 22 2017 07:59, Takashi Sakamoto wrote:
> On Feb 22 2017 07:45, Takashi Sakamoto wrote:
>> On Fe 21 2017 11:32, Takashi Sakamoto wrote:
>>> But it's wrong. The allocated memory objects are used for private data
>>> for each control element set. I didn't care of it...
>>>
>>> Here, what I should fix is just for error path. I'll post revised
>>> version of this patch this night.
>>
>> I realize that this is not concern because the code includes assignment
>> deallocate function to each control element set. Thus, in error path,
>> the allocated private data is going to free in below callgrach.
>>
>> (sound/usb/mixer_us16x08.c)
>> add_new_ctl(elem)
>>   kctl->private_free = freeer;
>>   (sound/usb/mixer.c)
>>   ->snd_usb_mixer_add_control(kctl)
>>     (sound/core/control.c)
>>     ->snd_ctl_add(kctl)
>>       ->snd_ctl_free_one(kctl)
>>         (in error path)
>>         ->kcontrol->private_free(kctl);
>>         (sound/usb/mixer_us16x08.c)
>>         = freeer()
>
> Oops. On the other hand, the private data for unregistered control
> element set is not deallocated automatically in error path of the other
> control element set. It should be fixed...

Mmm. There might be an issue of double free corruption, I think.

 >>     (sound/core/control.c)
 >>     ->snd_ctl_add(kctl)
 >>       (in error path)
 >>       ->snd_ctl_free_one(kctl)
 >>         ->kcontrol->private_free(kctl);
 >>         (sound/usb/mixer_us16x08.c)
 >>         = freeer()

This 'freeer()' is set in beginning of the graph.

 >> (sound/usb/mixer_us16x08.c)
 >> add_new_ctl(elem)
 >>   kctl->private_free = freeer;

The memory objects are not allocated for each of the control element 
set. On the other hand, the calls of the 'freeer()' are corresponding to 
each control element set. When the ratio between 
.private_data/.private_free and control element set is 1:N, 
.private_free can cause double free corruption at freeing control 
character device in kernel side.

In current code, there're two cases; NULL or 'snd_usb_mixer_elem_free()' 
are assigned as the 'freeer()'. But this is not enough. Actually, the 
allocated memory object for the 'comp_store' variable is going to be 
deallocated by two control element sets; for 'channel_controls[0]' and 
'channel_controls[0]' the allocated memory object for the 'eq_sotre' 
variable is going to be deallocated for several control element sets; 
the 'eq_controls[0, 1, 3, 6, 9]'.

As long as I understand, current code has the above issue and can bring 
kernel corruption. If I missing something, please inform it to me.


Regards

Takashi Sakamoto

Re: [PATCH 4/5] ALSA: usb-audio: deallocate memory objects in error path

[Takashi Iwai]

On Wed, 22 Feb 2017 02:42:39 +0100,
Takashi Sakamoto wrote:
> 
> On Feb 22 2017 07:59, Takashi Sakamoto wrote:
> > On Feb 22 2017 07:45, Takashi Sakamoto wrote:
> >> On Fe 21 2017 11:32, Takashi Sakamoto wrote:
> >>> But it's wrong. The allocated memory objects are used for private data
> >>> for each control element set. I didn't care of it...
> >>>
> >>> Here, what I should fix is just for error path. I'll post revised
> >>> version of this patch this night.
> >>
> >> I realize that this is not concern because the code includes assignment
> >> deallocate function to each control element set. Thus, in error path,
> >> the allocated private data is going to free in below callgrach.
> >>
> >> (sound/usb/mixer_us16x08.c)
> >> add_new_ctl(elem)
> >>   kctl->private_free = freeer;
> >>   (sound/usb/mixer.c)
> >>   ->snd_usb_mixer_add_control(kctl)
> >>     (sound/core/control.c)
> >>     ->snd_ctl_add(kctl)
> >>       ->snd_ctl_free_one(kctl)
> >>         (in error path)
> >>         ->kcontrol->private_free(kctl);
> >>         (sound/usb/mixer_us16x08.c)
> >>         = freeer()
> >
> > Oops. On the other hand, the private data for unregistered control
> > element set is not deallocated automatically in error path of the other
> > control element set. It should be fixed...
> 
> Mmm. There might be an issue of double free corruption, I think.
> 
> >>     (sound/core/control.c)
> >>     ->snd_ctl_add(kctl)
> >>       (in error path)
> >>       ->snd_ctl_free_one(kctl)
> >>         ->kcontrol->private_free(kctl);
> >>         (sound/usb/mixer_us16x08.c)
> >>         = freeer()
> 
> This 'freeer()' is set in beginning of the graph.
> 
> >> (sound/usb/mixer_us16x08.c)
> >> add_new_ctl(elem)
> >>   kctl->private_free = freeer;
> 
> The memory objects are not allocated for each of the control element
> set. On the other hand, the calls of the 'freeer()' are corresponding
> to each control element set. When the ratio between
> .private_data/.private_free and control element set is 1:N,
> .private_free can cause double free corruption at freeing control
> character device in kernel side.
> 
> In current code, there're two cases; NULL or
> 'snd_usb_mixer_elem_free()' are assigned as the 'freeer()'. But this
> is not enough. Actually, the allocated memory object for the
> 'comp_store' variable is going to be deallocated by two control
> element sets; for 'channel_controls[0]' and 'channel_controls[0]' the
> allocated memory object for the 'eq_sotre' variable is going to be
> deallocated for several control element sets; the 'eq_controls[0, 1,
> 3, 6, 9]'.
> 
> As long as I understand, current code has the above issue and can
> bring kernel corruption. If I missing something, please inform it to
> me.

Yeah, there are quite a few memory leaks and double frees.
The patch below is a bit of drastic surgery.


Takashi

-- 8< --
From: Takashi Iwai <tiwai@suse.de>
Subject: [PATCH] ALSA: usb-audio: Fix memory leak and corruption in
 mixer_us16x08.c

There are a few places leaking memory and doing double-free in
mixer_us16x08.c.

The driver allocates a usb_mixer_elem_info object at each
add_new_ctl() call.  This has to be freed via kctl->private_free, but
currently this is done properly only for some controls.

Also, the driver allocates three external objects (comp_store,
eq_store, meter_store), and these are referred in elem->private_data
(it's not kctl->private_data).  And these have to be released, but
there are none doing it.  Moreover, these extra objects have to be
released only once.  Thus the release should be done only by the first
kctl element that refers to it.

For fixing these, we call either snd_usb_mixer_elem_free() (only for
kctl->private_data) or elem_private_free() (for both
kctl->private_data and elem->private_data) via kctl->private_free
appropriately.

Last but not least, snd_us16x08_controls_create() may return in the
middle without releasing the allocated *_store objects.
returns in the middle due to an error.  For fixing this, we shuffle
the allocation code so that it's called just before its reference.

Fixes: d2bb390a2081 ("ALSA: usb-audio: Tascam US-16x08 DSP mixer quirk")
Reported-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/usb/mixer_us16x08.c | 92 ++++++++++++++++++++---------------------------
 sound/usb/mixer_us16x08.h |  1 -
 2 files changed, 39 insertions(+), 54 deletions(-)

diff --git a/sound/usb/mixer_us16x08.c b/sound/usb/mixer_us16x08.c
index 73a0b9afdd70..f7289541fbce 100644
--- a/sound/usb/mixer_us16x08.c
+++ b/sound/usb/mixer_us16x08.c
@@ -1053,11 +1053,22 @@ static struct snd_us16x08_meter_store *snd_us16x08_create_meter_store(void)
 
 }
 
+/* release elem->private_free as well; called only once for each *_store */
+static void elem_private_free(struct snd_kcontrol *kctl)
+{
+	struct usb_mixer_elem_info *elem = kctl->private_data;
+
+	if (elem)
+		kfree(elem->private_data);
+	kfree(elem);
+	kctl->private_data = NULL;
+}
+
 static int add_new_ctl(struct usb_mixer_interface *mixer,
 	const struct snd_kcontrol_new *ncontrol,
 	int index, int val_type, int channels,
 	const char *name, const void *opt,
-	void (*freeer)(struct snd_kcontrol *kctl),
+	bool do_private_free,
 	struct usb_mixer_elem_info **elem_ret)
 {
 	struct snd_kcontrol *kctl;
@@ -1085,7 +1096,10 @@ static int add_new_ctl(struct usb_mixer_interface *mixer,
 		return -ENOMEM;
 	}
 
-	kctl->private_free = freeer;
+	if (do_private_free)
+		kctl->private_free = elem_private_free;
+	else
+		kctl->private_free = snd_usb_mixer_elem_free;
 
 	strlcpy(kctl->id.name, name, sizeof(kctl->id.name));
 
@@ -1109,7 +1123,6 @@ static struct snd_us16x08_control_params eq_controls[] = {
 		.type = USB_MIXER_BOOLEAN,
 		.num_channels = 16,
 		.name = "EQ Switch",
-		.freeer = snd_usb_mixer_elem_free
 	},
 	{ /* EQ low gain */
 		.kcontrol_new = &snd_us16x08_eq_gain_ctl,
@@ -1117,7 +1130,6 @@ static struct snd_us16x08_control_params eq_controls[] = {
 		.type = USB_MIXER_U8,
 		.num_channels = 16,
 		.name = "EQ Low Volume",
-		.freeer = snd_usb_mixer_elem_free
 	},
 	{ /* EQ low freq */
 		.kcontrol_new = &snd_us16x08_eq_low_freq_ctl,
@@ -1125,7 +1137,6 @@ static struct snd_us16x08_control_params eq_controls[] = {
 		.type = USB_MIXER_U8,
 		.num_channels = 16,
 		.name = "EQ Low Frequence",
-		.freeer = NULL
 	},
 	{ /* EQ mid low gain */
 		.kcontrol_new = &snd_us16x08_eq_gain_ctl,
@@ -1133,7 +1144,6 @@ static struct snd_us16x08_control_params eq_controls[] = {
 		.type = USB_MIXER_U8,
 		.num_channels = 16,
 		.name = "EQ MidLow Volume",
-		.freeer = snd_usb_mixer_elem_free
 	},
 	{ /* EQ mid low freq */
 		.kcontrol_new = &snd_us16x08_eq_mid_freq_ctl,
@@ -1141,7 +1151,6 @@ static struct snd_us16x08_control_params eq_controls[] = {
 		.type = USB_MIXER_U8,
 		.num_channels = 16,
 		.name = "EQ MidLow Frequence",
-		.freeer = NULL
 	},
 	{ /* EQ mid low Q */
 		.kcontrol_new = &snd_us16x08_eq_mid_width_ctl,
@@ -1149,7 +1158,6 @@ static struct snd_us16x08_control_params eq_controls[] = {
 		.type = USB_MIXER_U8,
 		.num_channels = 16,
 		.name = "EQ MidQLow Q",
-		.freeer = NULL
 	},
 	{ /* EQ mid high gain */
 		.kcontrol_new = &snd_us16x08_eq_gain_ctl,
@@ -1157,7 +1165,6 @@ static struct snd_us16x08_control_params eq_controls[] = {
 		.type = USB_MIXER_U8,
 		.num_channels = 16,
 		.name = "EQ MidHigh Volume",
-		.freeer = snd_usb_mixer_elem_free
 	},
 	{ /* EQ mid high freq */
 		.kcontrol_new = &snd_us16x08_eq_mid_freq_ctl,
@@ -1165,7 +1172,6 @@ static struct snd_us16x08_control_params eq_controls[] = {
 		.type = USB_MIXER_U8,
 		.num_channels = 16,
 		.name = "EQ MidHigh Frequence",
-		.freeer = NULL
 	},
 	{ /* EQ mid high Q */
 		.kcontrol_new = &snd_us16x08_eq_mid_width_ctl,
@@ -1173,7 +1179,6 @@ static struct snd_us16x08_control_params eq_controls[] = {
 		.type = USB_MIXER_U8,
 		.num_channels = 16,
 		.name = "EQ MidHigh Q",
-		.freeer = NULL
 	},
 	{ /* EQ high gain */
 		.kcontrol_new = &snd_us16x08_eq_gain_ctl,
@@ -1181,7 +1186,6 @@ static struct snd_us16x08_control_params eq_controls[] = {
 		.type = USB_MIXER_U8,
 		.num_channels = 16,
 		.name = "EQ High Volume",
-		.freeer = snd_usb_mixer_elem_free
 	},
 	{ /* EQ low freq */
 		.kcontrol_new = &snd_us16x08_eq_high_freq_ctl,
@@ -1189,7 +1193,6 @@ static struct snd_us16x08_control_params eq_controls[] = {
 		.type = USB_MIXER_U8,
 		.num_channels = 16,
 		.name = "EQ High Frequence",
-		.freeer = NULL
 	},
 };
 
@@ -1201,7 +1204,6 @@ static struct snd_us16x08_control_params comp_controls[] = {
 		.type = USB_MIXER_BOOLEAN,
 		.num_channels = 16,
 		.name = "Compressor Switch",
-		.freeer = snd_usb_mixer_elem_free
 	},
 	{ /* Comp threshold */
 		.kcontrol_new = &snd_us16x08_comp_threshold_ctl,
@@ -1209,7 +1211,6 @@ static struct snd_us16x08_control_params comp_controls[] = {
 		.type = USB_MIXER_U8,
 		.num_channels = 16,
 		.name = "Compressor Threshold Volume",
-		.freeer = NULL
 	},
 	{ /* Comp ratio */
 		.kcontrol_new = &snd_us16x08_comp_ratio_ctl,
@@ -1217,7 +1218,6 @@ static struct snd_us16x08_control_params comp_controls[] = {
 		.type = USB_MIXER_U8,
 		.num_channels = 16,
 		.name = "Compressor Ratio",
-		.freeer = NULL
 	},
 	{ /* Comp attack */
 		.kcontrol_new = &snd_us16x08_comp_attack_ctl,
@@ -1225,7 +1225,6 @@ static struct snd_us16x08_control_params comp_controls[] = {
 		.type = USB_MIXER_U8,
 		.num_channels = 16,
 		.name = "Compressor Attack",
-		.freeer = NULL
 	},
 	{ /* Comp release */
 		.kcontrol_new = &snd_us16x08_comp_release_ctl,
@@ -1233,7 +1232,6 @@ static struct snd_us16x08_control_params comp_controls[] = {
 		.type = USB_MIXER_U8,
 		.num_channels = 16,
 		.name = "Compressor Release",
-		.freeer = NULL
 	},
 	{ /* Comp gain */
 		.kcontrol_new = &snd_us16x08_comp_gain_ctl,
@@ -1241,7 +1239,6 @@ static struct snd_us16x08_control_params comp_controls[] = {
 		.type = USB_MIXER_U8,
 		.num_channels = 16,
 		.name = "Compressor Volume",
-		.freeer = NULL
 	},
 };
 
@@ -1253,7 +1250,6 @@ static struct snd_us16x08_control_params channel_controls[] = {
 		.type = USB_MIXER_BOOLEAN,
 		.num_channels = 16,
 		.name = "Phase Switch",
-		.freeer = snd_usb_mixer_elem_free,
 		.default_val = 0
 	},
 	{ /* Fader */
@@ -1262,7 +1258,6 @@ static struct snd_us16x08_control_params channel_controls[] = {
 		.type = USB_MIXER_U8,
 		.num_channels = 16,
 		.name = "Line Volume",
-		.freeer = NULL,
 		.default_val = 127
 	},
 	{ /* Mute */
@@ -1271,7 +1266,6 @@ static struct snd_us16x08_control_params channel_controls[] = {
 		.type = USB_MIXER_BOOLEAN,
 		.num_channels = 16,
 		.name = "Mute Switch",
-		.freeer = NULL,
 		.default_val = 0
 	},
 	{ /* Pan */
@@ -1280,7 +1274,6 @@ static struct snd_us16x08_control_params channel_controls[] = {
 		.type = USB_MIXER_U16,
 		.num_channels = 16,
 		.name = "Pan Left-Right Volume",
-		.freeer = NULL,
 		.default_val = 127
 	},
 };
@@ -1293,7 +1286,6 @@ static struct snd_us16x08_control_params master_controls[] = {
 		.type = USB_MIXER_U8,
 		.num_channels = 16,
 		.name = "Master Volume",
-		.freeer = NULL,
 		.default_val = 127
 	},
 	{ /* Bypass */
@@ -1302,7 +1294,6 @@ static struct snd_us16x08_control_params master_controls[] = {
 		.type = USB_MIXER_BOOLEAN,
 		.num_channels = 16,
 		.name = "DSP Bypass Switch",
-		.freeer = NULL,
 		.default_val = 0
 	},
 	{ /* Buss out */
@@ -1311,7 +1302,6 @@ static struct snd_us16x08_control_params master_controls[] = {
 		.type = USB_MIXER_BOOLEAN,
 		.num_channels = 16,
 		.name = "Buss Out Switch",
-		.freeer = NULL,
 		.default_val = 0
 	},
 	{ /* Master mute */
@@ -1320,7 +1310,6 @@ static struct snd_us16x08_control_params master_controls[] = {
 		.type = USB_MIXER_BOOLEAN,
 		.num_channels = 16,
 		.name = "Master Mute Switch",
-		.freeer = NULL,
 		.default_val = 0
 	},
 
@@ -1338,30 +1327,10 @@ int snd_us16x08_controls_create(struct usb_mixer_interface *mixer)
 	/* just check for non-MIDI interface */
 	if (mixer->hostif->desc.bInterfaceNumber == 3) {
 
-		/* create compressor mixer elements */
-		comp_store = snd_us16x08_create_comp_store();
-		if (comp_store == NULL)
-			return -ENOMEM;
-
-		/* create eq store */
-		eq_store = snd_us16x08_create_eq_store();
-		if (eq_store == NULL) {
-			kfree(comp_store);
-			return -ENOMEM;
-		}
-
-		/* create meters store */
-		meter_store = snd_us16x08_create_meter_store();
-		if (meter_store == NULL) {
-			kfree(comp_store);
-			kfree(eq_store);
-			return -ENOMEM;
-		}
-
 		/* add routing control */
 		err = add_new_ctl(mixer, &snd_us16x08_route_ctl,
 			SND_US16X08_ID_ROUTE, USB_MIXER_U8, 8, "Line Out Route",
-			NULL, NULL, &elem);
+			NULL, false, &elem);
 		if (err < 0) {
 			usb_audio_dbg(mixer->chip,
 				"Failed to create route control, err:%d\n",
@@ -1372,6 +1341,11 @@ int snd_us16x08_controls_create(struct usb_mixer_interface *mixer)
 			elem->cache_val[i] = i < 2 ? i : i + 2;
 		elem->cached = 0xff;
 
+		/* create compressor mixer elements */
+		comp_store = snd_us16x08_create_comp_store();
+		if (!comp_store)
+			return -ENOMEM;
+
 		/* add master controls */
 		for (i = 0;
 			i < sizeof(master_controls)
@@ -1385,7 +1359,8 @@ int snd_us16x08_controls_create(struct usb_mixer_interface *mixer)
 				master_controls[i].num_channels,
 				master_controls[i].name,
 				comp_store,
-				master_controls[i].freeer, &elem);
+				i == 0, /* release comp_store only once */
+				&elem);
 			if (err < 0)
 				return err;
 			elem->cache_val[0] = master_controls[i].default_val;
@@ -1405,7 +1380,7 @@ int snd_us16x08_controls_create(struct usb_mixer_interface *mixer)
 				channel_controls[i].num_channels,
 				channel_controls[i].name,
 				comp_store,
-				channel_controls[i].freeer, &elem);
+				false, &elem);
 			if (err < 0)
 				return err;
 			for (j = 0; j < SND_US16X08_MAX_CHANNELS; j++) {
@@ -1415,6 +1390,11 @@ int snd_us16x08_controls_create(struct usb_mixer_interface *mixer)
 			elem->cached = 0xffff;
 		}
 
+		/* create eq store */
+		eq_store = snd_us16x08_create_eq_store();
+		if (!eq_store)
+			return -ENOMEM;
+
 		/* add EQ controls */
 		for (i = 0; i < sizeof(eq_controls) /
 			sizeof(control_params); i++) {
@@ -1426,7 +1406,8 @@ int snd_us16x08_controls_create(struct usb_mixer_interface *mixer)
 				eq_controls[i].num_channels,
 				eq_controls[i].name,
 				eq_store,
-				eq_controls[i].freeer, NULL);
+				i == 0, /* release eq_store only once */
+				NULL);
 			if (err < 0)
 				return err;
 		}
@@ -1444,18 +1425,23 @@ int snd_us16x08_controls_create(struct usb_mixer_interface *mixer)
 				comp_controls[i].num_channels,
 				comp_controls[i].name,
 				comp_store,
-				comp_controls[i].freeer, NULL);
+				false, NULL);
 			if (err < 0)
 				return err;
 		}
 
+		/* create meters store */
+		meter_store = snd_us16x08_create_meter_store();
+		if (!meter_store)
+			return -ENOMEM;
+
 		/* meter function 'get' must access to compressor store
 		 * so place a reference here
 		 */
 		meter_store->comp_store = comp_store;
 		err = add_new_ctl(mixer, &snd_us16x08_meter_ctl,
 			SND_US16X08_ID_METER, USB_MIXER_U16, 0, "Level Meter",
-			(void *) meter_store, snd_usb_mixer_elem_free, NULL);
+			meter_store, true, NULL);
 		if (err < 0)
 			return err;
 	}
diff --git a/sound/usb/mixer_us16x08.h b/sound/usb/mixer_us16x08.h
index 64f89b5eca2d..a6312fb0f962 100644
--- a/sound/usb/mixer_us16x08.h
+++ b/sound/usb/mixer_us16x08.h
@@ -112,7 +112,6 @@ struct snd_us16x08_control_params {
 	int type;
 	int num_channels;
 	const char *name;
-	void (*freeer)(struct snd_kcontrol *kctl);
 	int default_val;
 };
 
-- 
2.11.1

Re: [PATCH 4/5] ALSA: usb-audio: deallocate memory objects in error path

[Takashi Iwai]

On Wed, 22 Feb 2017 08:44:13 +0100,
Takashi Iwai wrote:
> 
> On Wed, 22 Feb 2017 02:42:39 +0100,
> Takashi Sakamoto wrote:
> > 
> > On Feb 22 2017 07:59, Takashi Sakamoto wrote:
> > > On Feb 22 2017 07:45, Takashi Sakamoto wrote:
> > >> On Fe 21 2017 11:32, Takashi Sakamoto wrote:
> > >>> But it's wrong. The allocated memory objects are used for private data
> > >>> for each control element set. I didn't care of it...
> > >>>
> > >>> Here, what I should fix is just for error path. I'll post revised
> > >>> version of this patch this night.
> > >>
> > >> I realize that this is not concern because the code includes assignment
> > >> deallocate function to each control element set. Thus, in error path,
> > >> the allocated private data is going to free in below callgrach.
> > >>
> > >> (sound/usb/mixer_us16x08.c)
> > >> add_new_ctl(elem)
> > >>   kctl->private_free = freeer;
> > >>   (sound/usb/mixer.c)
> > >>   ->snd_usb_mixer_add_control(kctl)
> > >>     (sound/core/control.c)
> > >>     ->snd_ctl_add(kctl)
> > >>       ->snd_ctl_free_one(kctl)
> > >>         (in error path)
> > >>         ->kcontrol->private_free(kctl);
> > >>         (sound/usb/mixer_us16x08.c)
> > >>         = freeer()
> > >
> > > Oops. On the other hand, the private data for unregistered control
> > > element set is not deallocated automatically in error path of the other
> > > control element set. It should be fixed...
> > 
> > Mmm. There might be an issue of double free corruption, I think.
> > 
> > >>     (sound/core/control.c)
> > >>     ->snd_ctl_add(kctl)
> > >>       (in error path)
> > >>       ->snd_ctl_free_one(kctl)
> > >>         ->kcontrol->private_free(kctl);
> > >>         (sound/usb/mixer_us16x08.c)
> > >>         = freeer()
> > 
> > This 'freeer()' is set in beginning of the graph.
> > 
> > >> (sound/usb/mixer_us16x08.c)
> > >> add_new_ctl(elem)
> > >>   kctl->private_free = freeer;
> > 
> > The memory objects are not allocated for each of the control element
> > set. On the other hand, the calls of the 'freeer()' are corresponding
> > to each control element set. When the ratio between
> > .private_data/.private_free and control element set is 1:N,
> > .private_free can cause double free corruption at freeing control
> > character device in kernel side.
> > 
> > In current code, there're two cases; NULL or
> > 'snd_usb_mixer_elem_free()' are assigned as the 'freeer()'. But this
> > is not enough. Actually, the allocated memory object for the
> > 'comp_store' variable is going to be deallocated by two control
> > element sets; for 'channel_controls[0]' and 'channel_controls[0]' the
> > allocated memory object for the 'eq_sotre' variable is going to be
> > deallocated for several control element sets; the 'eq_controls[0, 1,
> > 3, 6, 9]'.
> > 
> > As long as I understand, current code has the above issue and can
> > bring kernel corruption. If I missing something, please inform it to
> > me.
> 
> Yeah, there are quite a few memory leaks and double frees.
> The patch below is a bit of drastic surgery.

... and yet more cleanup patch on top of it.


Takashi

-- 8< --
From: Takashi Iwai <tiwai@suse.de>
Subject: [PATCH] ALSA: usb-audio: Tidy up mixer_us16x08.c

A few more cleanups and improvements that have been overlooked:

- Use ARRAY_SIZE() macro appropriately
- Code shuffling for minor optimization
- Omit superfluous variable initializations
- Get rid of superfluous NULL checks
- Add const to snd_us16x08_control_params definitions

No functional changes.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/usb/mixer_us16x08.c | 132 ++++++++++++++++++----------------------------
 1 file changed, 50 insertions(+), 82 deletions(-)

diff --git a/sound/usb/mixer_us16x08.c b/sound/usb/mixer_us16x08.c
index f7289541fbce..dc48eedea92e 100644
--- a/sound/usb/mixer_us16x08.c
+++ b/sound/usb/mixer_us16x08.c
@@ -176,15 +176,9 @@ static int snd_us16x08_recv_urb(struct snd_usb_audio *chip,
  */
 static int snd_us16x08_send_urb(struct snd_usb_audio *chip, char *buf, int size)
 {
-	int err = 0;
-
-	if (chip) {
-		err = snd_usb_ctl_msg(chip->dev, usb_sndctrlpipe(chip->dev, 0),
+	return snd_usb_ctl_msg(chip->dev, usb_sndctrlpipe(chip->dev, 0),
 			SND_US16X08_URB_REQUEST, SND_US16X08_URB_REQUESTTYPE,
 			0, 0, buf, size);
-	}
-
-	return err;
 }
 
 static int snd_us16x08_route_info(struct snd_kcontrol *kcontrol,
@@ -212,10 +206,7 @@ static int snd_us16x08_route_put(struct snd_kcontrol *kcontrol,
 	struct snd_usb_audio *chip = elem->head.mixer->chip;
 	int index = ucontrol->id.index;
 	char buf[sizeof(route_msg)];
-	int val, val_org, err = 0;
-
-	/* prepare the message buffer from template */
-	memcpy(buf, route_msg, sizeof(route_msg));
+	int val, val_org, err;
 
 	/*  get the new value (no bias for routes) */
 	val = ucontrol->value.enumerated.item[0];
@@ -224,6 +215,9 @@ static int snd_us16x08_route_put(struct snd_kcontrol *kcontrol,
 	if (val < 0 || val > 9)
 		return -EINVAL;
 
+	/* prepare the message buffer from template */
+	memcpy(buf, route_msg, sizeof(route_msg));
+
 	if (val < 2) {
 		/* input comes from a master channel */
 		val_org = val;
@@ -279,12 +273,9 @@ static int snd_us16x08_master_put(struct snd_kcontrol *kcontrol,
 	struct usb_mixer_elem_info *elem = kcontrol->private_data;
 	struct snd_usb_audio *chip = elem->head.mixer->chip;
 	char buf[sizeof(mix_msg_out)];
-	int val, err = 0;
+	int val, err;
 	int index = ucontrol->id.index;
 
-	/* prepare the message buffer from template */
-	memcpy(buf, mix_msg_out, sizeof(mix_msg_out));
-
 	/* new control value incl. bias*/
 	val = ucontrol->value.integer.value[0];
 
@@ -293,6 +284,9 @@ static int snd_us16x08_master_put(struct snd_kcontrol *kcontrol,
 		|| val > SND_US16X08_KCMAX(kcontrol))
 		return -EINVAL;
 
+	/* prepare the message buffer from template */
+	memcpy(buf, mix_msg_out, sizeof(mix_msg_out));
+
 	buf[8] = val - SND_US16X08_KCBIAS(kcontrol);
 	buf[6] = elem->head.id;
 
@@ -392,9 +386,6 @@ static int snd_us16x08_channel_put(struct snd_kcontrol *kcontrol,
 	int val, err;
 	int index = ucontrol->id.index;
 
-	/* prepare URB message from template */
-	memcpy(buf, mix_msg_in, sizeof(mix_msg_in));
-
 	val = ucontrol->value.integer.value[0];
 
 	/* sanity check */
@@ -402,6 +393,9 @@ static int snd_us16x08_channel_put(struct snd_kcontrol *kcontrol,
 		|| val > SND_US16X08_KCMAX(kcontrol))
 		return -EINVAL;
 
+	/* prepare URB message from template */
+	memcpy(buf, mix_msg_in, sizeof(mix_msg_in));
+
 	/* add the bias to the new value */
 	buf[8] = val - SND_US16X08_KCBIAS(kcontrol);
 	buf[6] = elem->head.id;
@@ -434,8 +428,7 @@ static int snd_us16x08_comp_get(struct snd_kcontrol *kcontrol,
 	struct snd_ctl_elem_value *ucontrol)
 {
 	struct usb_mixer_elem_info *elem = kcontrol->private_data;
-	struct snd_us16x08_comp_store *store =
-		((struct snd_us16x08_comp_store *) elem->private_data);
+	struct snd_us16x08_comp_store *store = elem->private_data;
 	int index = ucontrol->id.index;
 	int val_idx = COMP_STORE_IDX(elem->head.id);
 
@@ -449,18 +442,11 @@ static int snd_us16x08_comp_put(struct snd_kcontrol *kcontrol,
 {
 	struct usb_mixer_elem_info *elem = kcontrol->private_data;
 	struct snd_usb_audio *chip = elem->head.mixer->chip;
-	struct snd_us16x08_comp_store *store =
-		((struct snd_us16x08_comp_store *) elem->private_data);
+	struct snd_us16x08_comp_store *store = elem->private_data;
 	int index = ucontrol->id.index;
 	char buf[sizeof(comp_msg)];
 	int val_idx, val;
-	int err = 0;
-
-	/* prepare compressor URB message from template  */
-	memcpy(buf, comp_msg, sizeof(comp_msg));
-
-	/* new control value incl. bias*/
-	val_idx = elem->head.id - SND_US16X08_ID_COMP_BASE;
+	int err;
 
 	val = ucontrol->value.integer.value[0];
 
@@ -469,8 +455,14 @@ static int snd_us16x08_comp_put(struct snd_kcontrol *kcontrol,
 		|| val > SND_US16X08_KCMAX(kcontrol))
 		return -EINVAL;
 
+	/* new control value incl. bias*/
+	val_idx = elem->head.id - SND_US16X08_ID_COMP_BASE;
+
 	store->val[val_idx][index] = ucontrol->value.integer.value[0];
 
+	/* prepare compressor URB message from template  */
+	memcpy(buf, comp_msg, sizeof(comp_msg));
+
 	/* place comp values in message buffer watch bias! */
 	buf[8] = store->val[
 		COMP_STORE_IDX(SND_US16X08_ID_COMP_THRESHOLD)][index]
@@ -502,10 +494,9 @@ static int snd_us16x08_comp_put(struct snd_kcontrol *kcontrol,
 static int snd_us16x08_eqswitch_get(struct snd_kcontrol *kcontrol,
 	struct snd_ctl_elem_value *ucontrol)
 {
-	int val = 0;
+	int val;
 	struct usb_mixer_elem_info *elem = kcontrol->private_data;
-	struct snd_us16x08_eq_store *store =
-		((struct snd_us16x08_eq_store *) elem->private_data);
+	struct snd_us16x08_eq_store *store = elem->private_data;
 	int index = ucontrol->id.index;
 
 	/* get low switch from cache is enough, cause all bands are together */
@@ -521,10 +512,8 @@ static int snd_us16x08_eqswitch_put(struct snd_kcontrol *kcontrol,
 {
 	struct usb_mixer_elem_info *elem = kcontrol->private_data;
 	struct snd_usb_audio *chip = elem->head.mixer->chip;
-	struct snd_us16x08_eq_store *store =
-		((struct snd_us16x08_eq_store *) elem->private_data);
+	struct snd_us16x08_eq_store *store = elem->private_data;
 	int index = ucontrol->id.index;
-
 	char buf[sizeof(eqs_msq)];
 	int val, err = 0;
 	int b_idx;
@@ -564,10 +553,9 @@ static int snd_us16x08_eqswitch_put(struct snd_kcontrol *kcontrol,
 static int snd_us16x08_eq_get(struct snd_kcontrol *kcontrol,
 	struct snd_ctl_elem_value *ucontrol)
 {
-	int val = 0;
+	int val;
 	struct usb_mixer_elem_info *elem = kcontrol->private_data;
-	struct snd_us16x08_eq_store *store =
-		((struct snd_us16x08_eq_store *) elem->private_data);
+	struct snd_us16x08_eq_store *store = elem->private_data;
 	int index = ucontrol->id.index;
 	int b_idx = EQ_STORE_BAND_IDX(elem->head.id) - 1;
 	int p_idx = EQ_STORE_PARAM_IDX(elem->head.id);
@@ -584,17 +572,13 @@ static int snd_us16x08_eq_put(struct snd_kcontrol *kcontrol,
 {
 	struct usb_mixer_elem_info *elem = kcontrol->private_data;
 	struct snd_usb_audio *chip = elem->head.mixer->chip;
-	struct snd_us16x08_eq_store *store =
-		((struct snd_us16x08_eq_store *) elem->private_data);
+	struct snd_us16x08_eq_store *store = elem->private_data;
 	int index = ucontrol->id.index;
 	char buf[sizeof(eqs_msq)];
-	int val, err = 0;
+	int val, err;
 	int b_idx = EQ_STORE_BAND_IDX(elem->head.id) - 1;
 	int p_idx = EQ_STORE_PARAM_IDX(elem->head.id);
 
-	/* copy URB buffer from EQ template */
-	memcpy(buf, eqs_msq, sizeof(eqs_msq));
-
 	val = ucontrol->value.integer.value[0];
 
 	/* sanity check */
@@ -602,6 +586,9 @@ static int snd_us16x08_eq_put(struct snd_kcontrol *kcontrol,
 		|| val > SND_US16X08_KCMAX(kcontrol))
 		return -EINVAL;
 
+	/* copy URB buffer from EQ template */
+	memcpy(buf, eqs_msq, sizeof(eqs_msq));
+
 	store->val[b_idx][p_idx][index] = val;
 	buf[20] = store->val[b_idx][3][index];
 	buf[17] = store->val[b_idx][2][index];
@@ -713,12 +700,6 @@ static int snd_us16x08_meter_get(struct snd_kcontrol *kcontrol,
 	u8 meter_urb[64];
 	char tmp[sizeof(mix_init_msg2)] = {0};
 
-	if (elem) {
-		store = (struct snd_us16x08_meter_store *) elem->private_data;
-		chip = elem->head.mixer->chip;
-	} else
-		return 0;
-
 	switch (kcontrol->private_value) {
 	case 0:
 		snd_us16x08_send_urb(chip, (char *)mix_init_msg1,
@@ -983,11 +964,11 @@ static struct snd_kcontrol_new snd_us16x08_meter_ctl = {
 /* setup compressor store and assign default value */
 static struct snd_us16x08_comp_store *snd_us16x08_create_comp_store(void)
 {
-	int i = 0;
-	struct snd_us16x08_comp_store *tmp =
-		kmalloc(sizeof(struct snd_us16x08_comp_store), GFP_KERNEL);
+	int i;
+	struct snd_us16x08_comp_store *tmp;
 
-	if (tmp == NULL)
+	tmp = kmalloc(sizeof(*tmp), GFP_KERNEL);
+	if (!tmp)
 		return NULL;
 
 	for (i = 0; i < SND_US16X08_MAX_CHANNELS; i++) {
@@ -1006,10 +987,10 @@ static struct snd_us16x08_comp_store *snd_us16x08_create_comp_store(void)
 static struct snd_us16x08_eq_store *snd_us16x08_create_eq_store(void)
 {
 	int i, b_idx;
-	struct snd_us16x08_eq_store *tmp =
-		kmalloc(sizeof(struct snd_us16x08_eq_store), GFP_KERNEL);
+	struct snd_us16x08_eq_store *tmp;
 
-	if (tmp == NULL)
+	tmp = kmalloc(sizeof(*tmp), GFP_KERNEL);
+	if (!tmp)
 		return NULL;
 
 	for (i = 0; i < SND_US16X08_MAX_CHANNELS; i++) {
@@ -1042,15 +1023,14 @@ static struct snd_us16x08_eq_store *snd_us16x08_create_eq_store(void)
 
 static struct snd_us16x08_meter_store *snd_us16x08_create_meter_store(void)
 {
-	struct snd_us16x08_meter_store *tmp =
-		kzalloc(sizeof(struct snd_us16x08_meter_store), GFP_KERNEL);
+	struct snd_us16x08_meter_store *tmp;
 
+	tmp = kzalloc(sizeof(*tmp), GFP_KERNEL);
 	if (!tmp)
 		return NULL;
 	tmp->comp_index = 1;
 	tmp->comp_active_index = 0;
 	return tmp;
-
 }
 
 /* release elem->private_free as well; called only once for each *_store */
@@ -1067,7 +1047,7 @@ static void elem_private_free(struct snd_kcontrol *kctl)
 static int add_new_ctl(struct usb_mixer_interface *mixer,
 	const struct snd_kcontrol_new *ncontrol,
 	int index, int val_type, int channels,
-	const char *name, const void *opt,
+	const char *name, void *opt,
 	bool do_private_free,
 	struct usb_mixer_elem_info **elem_ret)
 {
@@ -1088,7 +1068,7 @@ static int add_new_ctl(struct usb_mixer_interface *mixer,
 	elem->head.id = index;
 	elem->val_type = val_type;
 	elem->channels = channels;
-	elem->private_data = (void *) opt;
+	elem->private_data = opt;
 
 	kctl = snd_ctl_new1(ncontrol, elem);
 	if (!kctl) {
@@ -1113,10 +1093,8 @@ static int add_new_ctl(struct usb_mixer_interface *mixer,
 	return 0;
 }
 
-static struct snd_us16x08_control_params control_params;
-
 /* table of EQ controls */
-static struct snd_us16x08_control_params eq_controls[] = {
+static const struct snd_us16x08_control_params eq_controls[] = {
 	{ /* EQ switch */
 		.kcontrol_new = &snd_us16x08_eq_switch_ctl,
 		.control_id = SND_US16X08_ID_EQENABLE,
@@ -1197,7 +1175,7 @@ static struct snd_us16x08_control_params eq_controls[] = {
 };
 
 /* table of compressor controls */
-static struct snd_us16x08_control_params comp_controls[] = {
+static const struct snd_us16x08_control_params comp_controls[] = {
 	{ /* Comp enable */
 		.kcontrol_new = &snd_us16x08_compswitch_ctl,
 		.control_id = SND_US16X08_ID_COMP_SWITCH,
@@ -1243,7 +1221,7 @@ static struct snd_us16x08_control_params comp_controls[] = {
 };
 
 /* table of channel controls */
-static struct snd_us16x08_control_params channel_controls[] = {
+static const struct snd_us16x08_control_params channel_controls[] = {
 	{ /* Phase */
 		.kcontrol_new = &snd_us16x08_ch_boolean_ctl,
 		.control_id = SND_US16X08_ID_PHASE,
@@ -1279,7 +1257,7 @@ static struct snd_us16x08_control_params channel_controls[] = {
 };
 
 /* table of master controls */
-static struct snd_us16x08_control_params master_controls[] = {
+static const struct snd_us16x08_control_params master_controls[] = {
 	{ /* Master */
 		.kcontrol_new = &snd_us16x08_master_ctl,
 		.control_id = SND_US16X08_ID_FADER,
@@ -1347,10 +1325,7 @@ int snd_us16x08_controls_create(struct usb_mixer_interface *mixer)
 			return -ENOMEM;
 
 		/* add master controls */
-		for (i = 0;
-			i < sizeof(master_controls)
-			/ sizeof(control_params);
-			i++) {
+		for (i = 0; i < ARRAY_SIZE(master_controls); i++) {
 
 			err = add_new_ctl(mixer,
 				master_controls[i].kcontrol_new,
@@ -1368,10 +1343,7 @@ int snd_us16x08_controls_create(struct usb_mixer_interface *mixer)
 		}
 
 		/* add channel controls */
-		for (i = 0;
-			i < sizeof(channel_controls)
-			/ sizeof(control_params);
-			i++) {
+		for (i = 0; i < ARRAY_SIZE(channel_controls); i++) {
 
 			err = add_new_ctl(mixer,
 				channel_controls[i].kcontrol_new,
@@ -1396,8 +1368,7 @@ int snd_us16x08_controls_create(struct usb_mixer_interface *mixer)
 			return -ENOMEM;
 
 		/* add EQ controls */
-		for (i = 0; i < sizeof(eq_controls) /
-			sizeof(control_params); i++) {
+		for (i = 0; i < ARRAY_SIZE(eq_controls); i++) {
 
 			err = add_new_ctl(mixer,
 				eq_controls[i].kcontrol_new,
@@ -1413,10 +1384,7 @@ int snd_us16x08_controls_create(struct usb_mixer_interface *mixer)
 		}
 
 		/* add compressor controls */
-		for (i = 0;
-			i < sizeof(comp_controls)
-			/ sizeof(control_params);
-			i++) {
+		for (i = 0; i < ARRAY_SIZE(comp_controls); i++) {
 
 			err = add_new_ctl(mixer,
 				comp_controls[i].kcontrol_new,
-- 
2.11.1

Re: [PATCH 4/5] ALSA: usb-audio: deallocate memory objects in error path

[Takashi Sakamoto]

On Feb 22 2017 16:46, Takashi Iwai wrote:
> -- 8< --
> From: Takashi Iwai <tiwai@suse.de>
> Subject: [PATCH] ALSA: usb-audio: Tidy up mixer_us16x08.c
>
> A few more cleanups and improvements that have been overlooked:
>
> - Use ARRAY_SIZE() macro appropriately
> - Code shuffling for minor optimization
> - Omit superfluous variable initializations
> - Get rid of superfluous NULL checks
> - Add const to snd_us16x08_control_params definitions
>
> No functional changes.
>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>  sound/usb/mixer_us16x08.c | 132 ++++++++++++++++++----------------------------
>  1 file changed, 50 insertions(+), 82 deletions(-)

Looks good to me, except for one item.

Reviewed-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>

> diff --git a/sound/usb/mixer_us16x08.c b/sound/usb/mixer_us16x08.c
> index f7289541fbce..dc48eedea92e 100644
> --- a/sound/usb/mixer_us16x08.c
> +++ b/sound/usb/mixer_us16x08.c
> @@ -176,15 +176,9 @@ static int snd_us16x08_recv_urb(struct snd_usb_audio *chip,
>   */
>  static int snd_us16x08_send_urb(struct snd_usb_audio *chip, char *buf, int size)
>  {
> -	int err = 0;
> -
> -	if (chip) {
> -		err = snd_usb_ctl_msg(chip->dev, usb_sndctrlpipe(chip->dev, 0),
> +	return snd_usb_ctl_msg(chip->dev, usb_sndctrlpipe(chip->dev, 0),
>  			SND_US16X08_URB_REQUEST, SND_US16X08_URB_REQUESTTYPE,
>  			0, 0, buf, size);
> -	}
> -
> -	return err;
>  }

Inline function is better for this part because the definition includes 
a few statements.

>
>  static int snd_us16x08_route_info(struct snd_kcontrol *kcontrol,
> @@ -212,10 +206,7 @@ static int snd_us16x08_route_put(struct snd_kcontrol *kcontrol,
>  	struct snd_usb_audio *chip = elem->head.mixer->chip;
>  	int index = ucontrol->id.index;
>  	char buf[sizeof(route_msg)];
> -	int val, val_org, err = 0;
> -
> -	/* prepare the message buffer from template */
> -	memcpy(buf, route_msg, sizeof(route_msg));
> +	int val, val_org, err;
>
>  	/*  get the new value (no bias for routes) */
>  	val = ucontrol->value.enumerated.item[0];
> @@ -224,6 +215,9 @@ static int snd_us16x08_route_put(struct snd_kcontrol *kcontrol,
>  	if (val < 0 || val > 9)
>  		return -EINVAL;
>
> +	/* prepare the message buffer from template */
> +	memcpy(buf, route_msg, sizeof(route_msg));
> +
>  	if (val < 2) {
>  		/* input comes from a master channel */
>  		val_org = val;
> @@ -279,12 +273,9 @@ static int snd_us16x08_master_put(struct snd_kcontrol *kcontrol,
>  	struct usb_mixer_elem_info *elem = kcontrol->private_data;
>  	struct snd_usb_audio *chip = elem->head.mixer->chip;
>  	char buf[sizeof(mix_msg_out)];
> -	int val, err = 0;
> +	int val, err;
>  	int index = ucontrol->id.index;
>
> -	/* prepare the message buffer from template */
> -	memcpy(buf, mix_msg_out, sizeof(mix_msg_out));
> -
>  	/* new control value incl. bias*/
>  	val = ucontrol->value.integer.value[0];
>
> @@ -293,6 +284,9 @@ static int snd_us16x08_master_put(struct snd_kcontrol *kcontrol,
>  		|| val > SND_US16X08_KCMAX(kcontrol))
>  		return -EINVAL;
>
> +	/* prepare the message buffer from template */
> +	memcpy(buf, mix_msg_out, sizeof(mix_msg_out));
> +
>  	buf[8] = val - SND_US16X08_KCBIAS(kcontrol);
>  	buf[6] = elem->head.id;
>
> @@ -392,9 +386,6 @@ static int snd_us16x08_channel_put(struct snd_kcontrol *kcontrol,
>  	int val, err;
>  	int index = ucontrol->id.index;
>
> -	/* prepare URB message from template */
> -	memcpy(buf, mix_msg_in, sizeof(mix_msg_in));
> -
>  	val = ucontrol->value.integer.value[0];
>
>  	/* sanity check */
> @@ -402,6 +393,9 @@ static int snd_us16x08_channel_put(struct snd_kcontrol *kcontrol,
>  		|| val > SND_US16X08_KCMAX(kcontrol))
>  		return -EINVAL;
>
> +	/* prepare URB message from template */
> +	memcpy(buf, mix_msg_in, sizeof(mix_msg_in));
> +
>  	/* add the bias to the new value */
>  	buf[8] = val - SND_US16X08_KCBIAS(kcontrol);
>  	buf[6] = elem->head.id;
> @@ -434,8 +428,7 @@ static int snd_us16x08_comp_get(struct snd_kcontrol *kcontrol,
>  	struct snd_ctl_elem_value *ucontrol)
>  {
>  	struct usb_mixer_elem_info *elem = kcontrol->private_data;
> -	struct snd_us16x08_comp_store *store =
> -		((struct snd_us16x08_comp_store *) elem->private_data);
> +	struct snd_us16x08_comp_store *store = elem->private_data;
>  	int index = ucontrol->id.index;
>  	int val_idx = COMP_STORE_IDX(elem->head.id);
>
> @@ -449,18 +442,11 @@ static int snd_us16x08_comp_put(struct snd_kcontrol *kcontrol,
>  {
>  	struct usb_mixer_elem_info *elem = kcontrol->private_data;
>  	struct snd_usb_audio *chip = elem->head.mixer->chip;
> -	struct snd_us16x08_comp_store *store =
> -		((struct snd_us16x08_comp_store *) elem->private_data);
> +	struct snd_us16x08_comp_store *store = elem->private_data;
>  	int index = ucontrol->id.index;
>  	char buf[sizeof(comp_msg)];
>  	int val_idx, val;
> -	int err = 0;
> -
> -	/* prepare compressor URB message from template  */
> -	memcpy(buf, comp_msg, sizeof(comp_msg));
> -
> -	/* new control value incl. bias*/
> -	val_idx = elem->head.id - SND_US16X08_ID_COMP_BASE;
> +	int err;
>
>  	val = ucontrol->value.integer.value[0];
>
> @@ -469,8 +455,14 @@ static int snd_us16x08_comp_put(struct snd_kcontrol *kcontrol,
>  		|| val > SND_US16X08_KCMAX(kcontrol))
>  		return -EINVAL;
>
> +	/* new control value incl. bias*/
> +	val_idx = elem->head.id - SND_US16X08_ID_COMP_BASE;
> +
>  	store->val[val_idx][index] = ucontrol->value.integer.value[0];
>
> +	/* prepare compressor URB message from template  */
> +	memcpy(buf, comp_msg, sizeof(comp_msg));
> +
>  	/* place comp values in message buffer watch bias! */
>  	buf[8] = store->val[
>  		COMP_STORE_IDX(SND_US16X08_ID_COMP_THRESHOLD)][index]
> @@ -502,10 +494,9 @@ static int snd_us16x08_comp_put(struct snd_kcontrol *kcontrol,
>  static int snd_us16x08_eqswitch_get(struct snd_kcontrol *kcontrol,
>  	struct snd_ctl_elem_value *ucontrol)
>  {
> -	int val = 0;
> +	int val;
>  	struct usb_mixer_elem_info *elem = kcontrol->private_data;
> -	struct snd_us16x08_eq_store *store =
> -		((struct snd_us16x08_eq_store *) elem->private_data);
> +	struct snd_us16x08_eq_store *store = elem->private_data;
>  	int index = ucontrol->id.index;
>
>  	/* get low switch from cache is enough, cause all bands are together */
> @@ -521,10 +512,8 @@ static int snd_us16x08_eqswitch_put(struct snd_kcontrol *kcontrol,
>  {
>  	struct usb_mixer_elem_info *elem = kcontrol->private_data;
>  	struct snd_usb_audio *chip = elem->head.mixer->chip;
> -	struct snd_us16x08_eq_store *store =
> -		((struct snd_us16x08_eq_store *) elem->private_data);
> +	struct snd_us16x08_eq_store *store = elem->private_data;
>  	int index = ucontrol->id.index;
> -
>  	char buf[sizeof(eqs_msq)];
>  	int val, err = 0;
>  	int b_idx;
> @@ -564,10 +553,9 @@ static int snd_us16x08_eqswitch_put(struct snd_kcontrol *kcontrol,
>  static int snd_us16x08_eq_get(struct snd_kcontrol *kcontrol,
>  	struct snd_ctl_elem_value *ucontrol)
>  {
> -	int val = 0;
> +	int val;
>  	struct usb_mixer_elem_info *elem = kcontrol->private_data;
> -	struct snd_us16x08_eq_store *store =
> -		((struct snd_us16x08_eq_store *) elem->private_data);
> +	struct snd_us16x08_eq_store *store = elem->private_data;
>  	int index = ucontrol->id.index;
>  	int b_idx = EQ_STORE_BAND_IDX(elem->head.id) - 1;
>  	int p_idx = EQ_STORE_PARAM_IDX(elem->head.id);
> @@ -584,17 +572,13 @@ static int snd_us16x08_eq_put(struct snd_kcontrol *kcontrol,
>  {
>  	struct usb_mixer_elem_info *elem = kcontrol->private_data;
>  	struct snd_usb_audio *chip = elem->head.mixer->chip;
> -	struct snd_us16x08_eq_store *store =
> -		((struct snd_us16x08_eq_store *) elem->private_data);
> +	struct snd_us16x08_eq_store *store = elem->private_data;
>  	int index = ucontrol->id.index;
>  	char buf[sizeof(eqs_msq)];
> -	int val, err = 0;
> +	int val, err;
>  	int b_idx = EQ_STORE_BAND_IDX(elem->head.id) - 1;
>  	int p_idx = EQ_STORE_PARAM_IDX(elem->head.id);
>
> -	/* copy URB buffer from EQ template */
> -	memcpy(buf, eqs_msq, sizeof(eqs_msq));
> -
>  	val = ucontrol->value.integer.value[0];
>
>  	/* sanity check */
> @@ -602,6 +586,9 @@ static int snd_us16x08_eq_put(struct snd_kcontrol *kcontrol,
>  		|| val > SND_US16X08_KCMAX(kcontrol))
>  		return -EINVAL;
>
> +	/* copy URB buffer from EQ template */
> +	memcpy(buf, eqs_msq, sizeof(eqs_msq));
> +
>  	store->val[b_idx][p_idx][index] = val;
>  	buf[20] = store->val[b_idx][3][index];
>  	buf[17] = store->val[b_idx][2][index];
> @@ -713,12 +700,6 @@ static int snd_us16x08_meter_get(struct snd_kcontrol *kcontrol,
>  	u8 meter_urb[64];
>  	char tmp[sizeof(mix_init_msg2)] = {0};
>
> -	if (elem) {
> -		store = (struct snd_us16x08_meter_store *) elem->private_data;
> -		chip = elem->head.mixer->chip;
> -	} else
> -		return 0;
> -
>  	switch (kcontrol->private_value) {
>  	case 0:
>  		snd_us16x08_send_urb(chip, (char *)mix_init_msg1,
> @@ -983,11 +964,11 @@ static struct snd_kcontrol_new snd_us16x08_meter_ctl = {
>  /* setup compressor store and assign default value */
>  static struct snd_us16x08_comp_store *snd_us16x08_create_comp_store(void)
>  {
> -	int i = 0;
> -	struct snd_us16x08_comp_store *tmp =
> -		kmalloc(sizeof(struct snd_us16x08_comp_store), GFP_KERNEL);
> +	int i;
> +	struct snd_us16x08_comp_store *tmp;
>
> -	if (tmp == NULL)
> +	tmp = kmalloc(sizeof(*tmp), GFP_KERNEL);
> +	if (!tmp)
>  		return NULL;
>
>  	for (i = 0; i < SND_US16X08_MAX_CHANNELS; i++) {
> @@ -1006,10 +987,10 @@ static struct snd_us16x08_comp_store *snd_us16x08_create_comp_store(void)
>  static struct snd_us16x08_eq_store *snd_us16x08_create_eq_store(void)
>  {
>  	int i, b_idx;
> -	struct snd_us16x08_eq_store *tmp =
> -		kmalloc(sizeof(struct snd_us16x08_eq_store), GFP_KERNEL);
> +	struct snd_us16x08_eq_store *tmp;
>
> -	if (tmp == NULL)
> +	tmp = kmalloc(sizeof(*tmp), GFP_KERNEL);
> +	if (!tmp)
>  		return NULL;
>
>  	for (i = 0; i < SND_US16X08_MAX_CHANNELS; i++) {
> @@ -1042,15 +1023,14 @@ static struct snd_us16x08_eq_store *snd_us16x08_create_eq_store(void)
>
>  static struct snd_us16x08_meter_store *snd_us16x08_create_meter_store(void)
>  {
> -	struct snd_us16x08_meter_store *tmp =
> -		kzalloc(sizeof(struct snd_us16x08_meter_store), GFP_KERNEL);
> +	struct snd_us16x08_meter_store *tmp;
>
> +	tmp = kzalloc(sizeof(*tmp), GFP_KERNEL);
>  	if (!tmp)
>  		return NULL;
>  	tmp->comp_index = 1;
>  	tmp->comp_active_index = 0;
>  	return tmp;
> -
>  }
>
>  /* release elem->private_free as well; called only once for each *_store */
> @@ -1067,7 +1047,7 @@ static void elem_private_free(struct snd_kcontrol *kctl)
>  static int add_new_ctl(struct usb_mixer_interface *mixer,
>  	const struct snd_kcontrol_new *ncontrol,
>  	int index, int val_type, int channels,
> -	const char *name, const void *opt,
> +	const char *name, void *opt,
>  	bool do_private_free,
>  	struct usb_mixer_elem_info **elem_ret)
>  {
> @@ -1088,7 +1068,7 @@ static int add_new_ctl(struct usb_mixer_interface *mixer,
>  	elem->head.id = index;
>  	elem->val_type = val_type;
>  	elem->channels = channels;
> -	elem->private_data = (void *) opt;
> +	elem->private_data = opt;
>
>  	kctl = snd_ctl_new1(ncontrol, elem);
>  	if (!kctl) {
> @@ -1113,10 +1093,8 @@ static int add_new_ctl(struct usb_mixer_interface *mixer,
>  	return 0;
>  }
>
> -static struct snd_us16x08_control_params control_params;
> -
>  /* table of EQ controls */
> -static struct snd_us16x08_control_params eq_controls[] = {
> +static const struct snd_us16x08_control_params eq_controls[] = {
>  	{ /* EQ switch */
>  		.kcontrol_new = &snd_us16x08_eq_switch_ctl,
>  		.control_id = SND_US16X08_ID_EQENABLE,
> @@ -1197,7 +1175,7 @@ static struct snd_us16x08_control_params eq_controls[] = {
>  };
>
>  /* table of compressor controls */
> -static struct snd_us16x08_control_params comp_controls[] = {
> +static const struct snd_us16x08_control_params comp_controls[] = {
>  	{ /* Comp enable */
>  		.kcontrol_new = &snd_us16x08_compswitch_ctl,
>  		.control_id = SND_US16X08_ID_COMP_SWITCH,
> @@ -1243,7 +1221,7 @@ static struct snd_us16x08_control_params comp_controls[] = {
>  };
>
>  /* table of channel controls */
> -static struct snd_us16x08_control_params channel_controls[] = {
> +static const struct snd_us16x08_control_params channel_controls[] = {
>  	{ /* Phase */
>  		.kcontrol_new = &snd_us16x08_ch_boolean_ctl,
>  		.control_id = SND_US16X08_ID_PHASE,
> @@ -1279,7 +1257,7 @@ static struct snd_us16x08_control_params channel_controls[] = {
>  };
>
>  /* table of master controls */
> -static struct snd_us16x08_control_params master_controls[] = {
> +static const struct snd_us16x08_control_params master_controls[] = {
>  	{ /* Master */
>  		.kcontrol_new = &snd_us16x08_master_ctl,
>  		.control_id = SND_US16X08_ID_FADER,
> @@ -1347,10 +1325,7 @@ int snd_us16x08_controls_create(struct usb_mixer_interface *mixer)
>  			return -ENOMEM;
>
>  		/* add master controls */
> -		for (i = 0;
> -			i < sizeof(master_controls)
> -			/ sizeof(control_params);
> -			i++) {
> +		for (i = 0; i < ARRAY_SIZE(master_controls); i++) {
>
>  			err = add_new_ctl(mixer,
>  				master_controls[i].kcontrol_new,
> @@ -1368,10 +1343,7 @@ int snd_us16x08_controls_create(struct usb_mixer_interface *mixer)
>  		}
>
>  		/* add channel controls */
> -		for (i = 0;
> -			i < sizeof(channel_controls)
> -			/ sizeof(control_params);
> -			i++) {
> +		for (i = 0; i < ARRAY_SIZE(channel_controls); i++) {
>
>  			err = add_new_ctl(mixer,
>  				channel_controls[i].kcontrol_new,
> @@ -1396,8 +1368,7 @@ int snd_us16x08_controls_create(struct usb_mixer_interface *mixer)
>  			return -ENOMEM;
>
>  		/* add EQ controls */
> -		for (i = 0; i < sizeof(eq_controls) /
> -			sizeof(control_params); i++) {
> +		for (i = 0; i < ARRAY_SIZE(eq_controls); i++) {
>
>  			err = add_new_ctl(mixer,
>  				eq_controls[i].kcontrol_new,
> @@ -1413,10 +1384,7 @@ int snd_us16x08_controls_create(struct usb_mixer_interface *mixer)
>  		}
>
>  		/* add compressor controls */
> -		for (i = 0;
> -			i < sizeof(comp_controls)
> -			/ sizeof(control_params);
> -			i++) {
> +		for (i = 0; i < ARRAY_SIZE(comp_controls); i++) {
>
>  			err = add_new_ctl(mixer,
>  				comp_controls[i].kcontrol_new,


Regards

Takashi Sakamoto

Re: [PATCH 4/5] ALSA: usb-audio: deallocate memory objects in error path

[Takashi Iwai]

On Wed, 22 Feb 2017 14:37:22 +0100,
Takashi Sakamoto wrote:
> 
> On Feb 22 2017 16:46, Takashi Iwai wrote:
> > -- 8< --
> > From: Takashi Iwai <tiwai@suse.de>
> > Subject: [PATCH] ALSA: usb-audio: Tidy up mixer_us16x08.c
> >
> > A few more cleanups and improvements that have been overlooked:
> >
> > - Use ARRAY_SIZE() macro appropriately
> > - Code shuffling for minor optimization
> > - Omit superfluous variable initializations
> > - Get rid of superfluous NULL checks
> > - Add const to snd_us16x08_control_params definitions
> >
> > No functional changes.
> >
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > ---
> >  sound/usb/mixer_us16x08.c | 132 ++++++++++++++++++----------------------------
> >  1 file changed, 50 insertions(+), 82 deletions(-)
> 
> Looks good to me, except for one item.
> 
> Reviewed-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> 
> > diff --git a/sound/usb/mixer_us16x08.c b/sound/usb/mixer_us16x08.c
> > index f7289541fbce..dc48eedea92e 100644
> > --- a/sound/usb/mixer_us16x08.c
> > +++ b/sound/usb/mixer_us16x08.c
> > @@ -176,15 +176,9 @@ static int snd_us16x08_recv_urb(struct snd_usb_audio *chip,
> >   */
> >  static int snd_us16x08_send_urb(struct snd_usb_audio *chip, char *buf, int size)
> >  {
> > -	int err = 0;
> > -
> > -	if (chip) {
> > -		err = snd_usb_ctl_msg(chip->dev, usb_sndctrlpipe(chip->dev, 0),
> > +	return snd_usb_ctl_msg(chip->dev, usb_sndctrlpipe(chip->dev, 0),
> >  			SND_US16X08_URB_REQUEST, SND_US16X08_URB_REQUESTTYPE,
> >  			0, 0, buf, size);
> > -	}
> > -
> > -	return err;
> >  }
> 
> Inline function is better for this part because the definition
> includes a few statements.

Basically you don't need to mark inline unless you really have to do
it explicitly.  Compiler is smart enough, often smarter than us.


thanks,

Takashi

Re: [PATCH 4/5] ALSA: usb-audio: deallocate memory objects in error path

[Takashi Sakamoto]

On Feb 22 2017 16:44, Takashi Iwai wrote:
> -- 8< --
> From: Takashi Iwai <tiwai@suse.de>
> Subject: [PATCH] ALSA: usb-audio: Fix memory leak and corruption in
>  mixer_us16x08.c
>
> There are a few places leaking memory and doing double-free in
> mixer_us16x08.c.
>
> The driver allocates a usb_mixer_elem_info object at each
> add_new_ctl() call.  This has to be freed via kctl->private_free, but
> currently this is done properly only for some controls.
>
> Also, the driver allocates three external objects (comp_store,
> eq_store, meter_store), and these are referred in elem->private_data
> (it's not kctl->private_data).  And these have to be released, but
> there are none doing it.  Moreover, these extra objects have to be
> released only once.  Thus the release should be done only by the first
> kctl element that refers to it.
>
> For fixing these, we call either snd_usb_mixer_elem_free() (only for
> kctl->private_data) or elem_private_free() (for both
> kctl->private_data and elem->private_data) via kctl->private_free
> appropriately.
>
> Last but not least, snd_us16x08_controls_create() may return in the
> middle without releasing the allocated *_store objects.
> returns in the middle due to an error.  For fixing this, we shuffle
> the allocation code so that it's called just before its reference.
>
> Fixes: d2bb390a2081 ("ALSA: usb-audio: Tascam US-16x08 DSP mixer quirk")
> Reported-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>  sound/usb/mixer_us16x08.c | 92 ++++++++++++++++++++---------------------------
>  sound/usb/mixer_us16x08.h |  1 -
>  2 files changed, 39 insertions(+), 54 deletions(-)

Looks good to me.

Reviewed-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>

> diff --git a/sound/usb/mixer_us16x08.c b/sound/usb/mixer_us16x08.c
> index 73a0b9afdd70..f7289541fbce 100644
> --- a/sound/usb/mixer_us16x08.c
> +++ b/sound/usb/mixer_us16x08.c
> @@ -1053,11 +1053,22 @@ static struct snd_us16x08_meter_store *snd_us16x08_create_meter_store(void)
>
>  }
>
> +/* release elem->private_free as well; called only once for each *_store */
> +static void elem_private_free(struct snd_kcontrol *kctl)
> +{
> +	struct usb_mixer_elem_info *elem = kctl->private_data;
> +
> +	if (elem)
> +		kfree(elem->private_data);
> +	kfree(elem);
> +	kctl->private_data = NULL;
> +}
> +
>  static int add_new_ctl(struct usb_mixer_interface *mixer,
>  	const struct snd_kcontrol_new *ncontrol,
>  	int index, int val_type, int channels,
>  	const char *name, const void *opt,
> -	void (*freeer)(struct snd_kcontrol *kctl),
> +	bool do_private_free,
>  	struct usb_mixer_elem_info **elem_ret)
>  {
>  	struct snd_kcontrol *kctl;
> @@ -1085,7 +1096,10 @@ static int add_new_ctl(struct usb_mixer_interface *mixer,
>  		return -ENOMEM;
>  	}
>
> -	kctl->private_free = freeer;
> +	if (do_private_free)
> +		kctl->private_free = elem_private_free;
> +	else
> +		kctl->private_free = snd_usb_mixer_elem_free;
>
>  	strlcpy(kctl->id.name, name, sizeof(kctl->id.name));
>
> @@ -1109,7 +1123,6 @@ static struct snd_us16x08_control_params eq_controls[] = {
>  		.type = USB_MIXER_BOOLEAN,
>  		.num_channels = 16,
>  		.name = "EQ Switch",
> -		.freeer = snd_usb_mixer_elem_free
>  	},
>  	{ /* EQ low gain */
>  		.kcontrol_new = &snd_us16x08_eq_gain_ctl,
> @@ -1117,7 +1130,6 @@ static struct snd_us16x08_control_params eq_controls[] = {
>  		.type = USB_MIXER_U8,
>  		.num_channels = 16,
>  		.name = "EQ Low Volume",
> -		.freeer = snd_usb_mixer_elem_free
>  	},
>  	{ /* EQ low freq */
>  		.kcontrol_new = &snd_us16x08_eq_low_freq_ctl,
> @@ -1125,7 +1137,6 @@ static struct snd_us16x08_control_params eq_controls[] = {
>  		.type = USB_MIXER_U8,
>  		.num_channels = 16,
>  		.name = "EQ Low Frequence",
> -		.freeer = NULL
>  	},
>  	{ /* EQ mid low gain */
>  		.kcontrol_new = &snd_us16x08_eq_gain_ctl,
> @@ -1133,7 +1144,6 @@ static struct snd_us16x08_control_params eq_controls[] = {
>  		.type = USB_MIXER_U8,
>  		.num_channels = 16,
>  		.name = "EQ MidLow Volume",
> -		.freeer = snd_usb_mixer_elem_free
>  	},
>  	{ /* EQ mid low freq */
>  		.kcontrol_new = &snd_us16x08_eq_mid_freq_ctl,
> @@ -1141,7 +1151,6 @@ static struct snd_us16x08_control_params eq_controls[] = {
>  		.type = USB_MIXER_U8,
>  		.num_channels = 16,
>  		.name = "EQ MidLow Frequence",
> -		.freeer = NULL
>  	},
>  	{ /* EQ mid low Q */
>  		.kcontrol_new = &snd_us16x08_eq_mid_width_ctl,
> @@ -1149,7 +1158,6 @@ static struct snd_us16x08_control_params eq_controls[] = {
>  		.type = USB_MIXER_U8,
>  		.num_channels = 16,
>  		.name = "EQ MidQLow Q",
> -		.freeer = NULL
>  	},
>  	{ /* EQ mid high gain */
>  		.kcontrol_new = &snd_us16x08_eq_gain_ctl,
> @@ -1157,7 +1165,6 @@ static struct snd_us16x08_control_params eq_controls[] = {
>  		.type = USB_MIXER_U8,
>  		.num_channels = 16,
>  		.name = "EQ MidHigh Volume",
> -		.freeer = snd_usb_mixer_elem_free
>  	},
>  	{ /* EQ mid high freq */
>  		.kcontrol_new = &snd_us16x08_eq_mid_freq_ctl,
> @@ -1165,7 +1172,6 @@ static struct snd_us16x08_control_params eq_controls[] = {
>  		.type = USB_MIXER_U8,
>  		.num_channels = 16,
>  		.name = "EQ MidHigh Frequence",
> -		.freeer = NULL
>  	},
>  	{ /* EQ mid high Q */
>  		.kcontrol_new = &snd_us16x08_eq_mid_width_ctl,
> @@ -1173,7 +1179,6 @@ static struct snd_us16x08_control_params eq_controls[] = {
>  		.type = USB_MIXER_U8,
>  		.num_channels = 16,
>  		.name = "EQ MidHigh Q",
> -		.freeer = NULL
>  	},
>  	{ /* EQ high gain */
>  		.kcontrol_new = &snd_us16x08_eq_gain_ctl,
> @@ -1181,7 +1186,6 @@ static struct snd_us16x08_control_params eq_controls[] = {
>  		.type = USB_MIXER_U8,
>  		.num_channels = 16,
>  		.name = "EQ High Volume",
> -		.freeer = snd_usb_mixer_elem_free
>  	},
>  	{ /* EQ low freq */
>  		.kcontrol_new = &snd_us16x08_eq_high_freq_ctl,
> @@ -1189,7 +1193,6 @@ static struct snd_us16x08_control_params eq_controls[] = {
>  		.type = USB_MIXER_U8,
>  		.num_channels = 16,
>  		.name = "EQ High Frequence",
> -		.freeer = NULL
>  	},
>  };
>
> @@ -1201,7 +1204,6 @@ static struct snd_us16x08_control_params comp_controls[] = {
>  		.type = USB_MIXER_BOOLEAN,
>  		.num_channels = 16,
>  		.name = "Compressor Switch",
> -		.freeer = snd_usb_mixer_elem_free
>  	},
>  	{ /* Comp threshold */
>  		.kcontrol_new = &snd_us16x08_comp_threshold_ctl,
> @@ -1209,7 +1211,6 @@ static struct snd_us16x08_control_params comp_controls[] = {
>  		.type = USB_MIXER_U8,
>  		.num_channels = 16,
>  		.name = "Compressor Threshold Volume",
> -		.freeer = NULL
>  	},
>  	{ /* Comp ratio */
>  		.kcontrol_new = &snd_us16x08_comp_ratio_ctl,
> @@ -1217,7 +1218,6 @@ static struct snd_us16x08_control_params comp_controls[] = {
>  		.type = USB_MIXER_U8,
>  		.num_channels = 16,
>  		.name = "Compressor Ratio",
> -		.freeer = NULL
>  	},
>  	{ /* Comp attack */
>  		.kcontrol_new = &snd_us16x08_comp_attack_ctl,
> @@ -1225,7 +1225,6 @@ static struct snd_us16x08_control_params comp_controls[] = {
>  		.type = USB_MIXER_U8,
>  		.num_channels = 16,
>  		.name = "Compressor Attack",
> -		.freeer = NULL
>  	},
>  	{ /* Comp release */
>  		.kcontrol_new = &snd_us16x08_comp_release_ctl,
> @@ -1233,7 +1232,6 @@ static struct snd_us16x08_control_params comp_controls[] = {
>  		.type = USB_MIXER_U8,
>  		.num_channels = 16,
>  		.name = "Compressor Release",
> -		.freeer = NULL
>  	},
>  	{ /* Comp gain */
>  		.kcontrol_new = &snd_us16x08_comp_gain_ctl,
> @@ -1241,7 +1239,6 @@ static struct snd_us16x08_control_params comp_controls[] = {
>  		.type = USB_MIXER_U8,
>  		.num_channels = 16,
>  		.name = "Compressor Volume",
> -		.freeer = NULL
>  	},
>  };
>
> @@ -1253,7 +1250,6 @@ static struct snd_us16x08_control_params channel_controls[] = {
>  		.type = USB_MIXER_BOOLEAN,
>  		.num_channels = 16,
>  		.name = "Phase Switch",
> -		.freeer = snd_usb_mixer_elem_free,
>  		.default_val = 0
>  	},
>  	{ /* Fader */
> @@ -1262,7 +1258,6 @@ static struct snd_us16x08_control_params channel_controls[] = {
>  		.type = USB_MIXER_U8,
>  		.num_channels = 16,
>  		.name = "Line Volume",
> -		.freeer = NULL,
>  		.default_val = 127
>  	},
>  	{ /* Mute */
> @@ -1271,7 +1266,6 @@ static struct snd_us16x08_control_params channel_controls[] = {
>  		.type = USB_MIXER_BOOLEAN,
>  		.num_channels = 16,
>  		.name = "Mute Switch",
> -		.freeer = NULL,
>  		.default_val = 0
>  	},
>  	{ /* Pan */
> @@ -1280,7 +1274,6 @@ static struct snd_us16x08_control_params channel_controls[] = {
>  		.type = USB_MIXER_U16,
>  		.num_channels = 16,
>  		.name = "Pan Left-Right Volume",
> -		.freeer = NULL,
>  		.default_val = 127
>  	},
>  };
> @@ -1293,7 +1286,6 @@ static struct snd_us16x08_control_params master_controls[] = {
>  		.type = USB_MIXER_U8,
>  		.num_channels = 16,
>  		.name = "Master Volume",
> -		.freeer = NULL,
>  		.default_val = 127
>  	},
>  	{ /* Bypass */
> @@ -1302,7 +1294,6 @@ static struct snd_us16x08_control_params master_controls[] = {
>  		.type = USB_MIXER_BOOLEAN,
>  		.num_channels = 16,
>  		.name = "DSP Bypass Switch",
> -		.freeer = NULL,
>  		.default_val = 0
>  	},
>  	{ /* Buss out */
> @@ -1311,7 +1302,6 @@ static struct snd_us16x08_control_params master_controls[] = {
>  		.type = USB_MIXER_BOOLEAN,
>  		.num_channels = 16,
>  		.name = "Buss Out Switch",
> -		.freeer = NULL,
>  		.default_val = 0
>  	},
>  	{ /* Master mute */
> @@ -1320,7 +1310,6 @@ static struct snd_us16x08_control_params master_controls[] = {
>  		.type = USB_MIXER_BOOLEAN,
>  		.num_channels = 16,
>  		.name = "Master Mute Switch",
> -		.freeer = NULL,
>  		.default_val = 0
>  	},
>
> @@ -1338,30 +1327,10 @@ int snd_us16x08_controls_create(struct usb_mixer_interface *mixer)
>  	/* just check for non-MIDI interface */
>  	if (mixer->hostif->desc.bInterfaceNumber == 3) {
>
> -		/* create compressor mixer elements */
> -		comp_store = snd_us16x08_create_comp_store();
> -		if (comp_store == NULL)
> -			return -ENOMEM;
> -
> -		/* create eq store */
> -		eq_store = snd_us16x08_create_eq_store();
> -		if (eq_store == NULL) {
> -			kfree(comp_store);
> -			return -ENOMEM;
> -		}
> -
> -		/* create meters store */
> -		meter_store = snd_us16x08_create_meter_store();
> -		if (meter_store == NULL) {
> -			kfree(comp_store);
> -			kfree(eq_store);
> -			return -ENOMEM;
> -		}
> -
>  		/* add routing control */
>  		err = add_new_ctl(mixer, &snd_us16x08_route_ctl,
>  			SND_US16X08_ID_ROUTE, USB_MIXER_U8, 8, "Line Out Route",
> -			NULL, NULL, &elem);
> +			NULL, false, &elem);
>  		if (err < 0) {
>  			usb_audio_dbg(mixer->chip,
>  				"Failed to create route control, err:%d\n",
> @@ -1372,6 +1341,11 @@ int snd_us16x08_controls_create(struct usb_mixer_interface *mixer)
>  			elem->cache_val[i] = i < 2 ? i : i + 2;
>  		elem->cached = 0xff;
>
> +		/* create compressor mixer elements */
> +		comp_store = snd_us16x08_create_comp_store();
> +		if (!comp_store)
> +			return -ENOMEM;
> +
>  		/* add master controls */
>  		for (i = 0;
>  			i < sizeof(master_controls)
> @@ -1385,7 +1359,8 @@ int snd_us16x08_controls_create(struct usb_mixer_interface *mixer)
>  				master_controls[i].num_channels,
>  				master_controls[i].name,
>  				comp_store,
> -				master_controls[i].freeer, &elem);
> +				i == 0, /* release comp_store only once */
> +				&elem);
>  			if (err < 0)
>  				return err;
>  			elem->cache_val[0] = master_controls[i].default_val;
> @@ -1405,7 +1380,7 @@ int snd_us16x08_controls_create(struct usb_mixer_interface *mixer)
>  				channel_controls[i].num_channels,
>  				channel_controls[i].name,
>  				comp_store,
> -				channel_controls[i].freeer, &elem);
> +				false, &elem);
>  			if (err < 0)
>  				return err;
>  			for (j = 0; j < SND_US16X08_MAX_CHANNELS; j++) {
> @@ -1415,6 +1390,11 @@ int snd_us16x08_controls_create(struct usb_mixer_interface *mixer)
>  			elem->cached = 0xffff;
>  		}
>
> +		/* create eq store */
> +		eq_store = snd_us16x08_create_eq_store();
> +		if (!eq_store)
> +			return -ENOMEM;
> +
>  		/* add EQ controls */
>  		for (i = 0; i < sizeof(eq_controls) /
>  			sizeof(control_params); i++) {
> @@ -1426,7 +1406,8 @@ int snd_us16x08_controls_create(struct usb_mixer_interface *mixer)
>  				eq_controls[i].num_channels,
>  				eq_controls[i].name,
>  				eq_store,
> -				eq_controls[i].freeer, NULL);
> +				i == 0, /* release eq_store only once */
> +				NULL);
>  			if (err < 0)
>  				return err;
>  		}
> @@ -1444,18 +1425,23 @@ int snd_us16x08_controls_create(struct usb_mixer_interface *mixer)
>  				comp_controls[i].num_channels,
>  				comp_controls[i].name,
>  				comp_store,
> -				comp_controls[i].freeer, NULL);
> +				false, NULL);
>  			if (err < 0)
>  				return err;
>  		}
>
> +		/* create meters store */
> +		meter_store = snd_us16x08_create_meter_store();
> +		if (!meter_store)
> +			return -ENOMEM;
> +
>  		/* meter function 'get' must access to compressor store
>  		 * so place a reference here
>  		 */
>  		meter_store->comp_store = comp_store;
>  		err = add_new_ctl(mixer, &snd_us16x08_meter_ctl,
>  			SND_US16X08_ID_METER, USB_MIXER_U16, 0, "Level Meter",
> -			(void *) meter_store, snd_usb_mixer_elem_free, NULL);
> +			meter_store, true, NULL);
>  		if (err < 0)
>  			return err;
>  	}
> diff --git a/sound/usb/mixer_us16x08.h b/sound/usb/mixer_us16x08.h
> index 64f89b5eca2d..a6312fb0f962 100644
> --- a/sound/usb/mixer_us16x08.h
> +++ b/sound/usb/mixer_us16x08.h
> @@ -112,7 +112,6 @@ struct snd_us16x08_control_params {
>  	int type;
>  	int num_channels;
>  	const char *name;
> -	void (*freeer)(struct snd_kcontrol *kctl);
>  	int default_val;
>  };

Regards

Takashi Sakamoto

[PATCH 5/5] ALSA: usb-audio: fix msleep timeout due to minimum resolution of task scheduling

[Takashi Sakamoto]

Depending on tick-related kernel configuration; e.g. HZ, task scheduler
necessarily has no guarantee for its minimum resolution against ~20 msec
msleep timeout.

This commit expands msleep timeout up to 20 msec with an optimistic view
to packet handling capability of the target device. 'checkpatch.pl'
generated below warning.

WARNING: msleep < 20ms can sleep for up to 20ms; see Documentation/timers/timers-howto.txt
+		msleep(15);

Fixes: d2bb390a2081 ("ALSA: usb-audio: Tascam US-16x08 DSP mixer quirk")
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/usb/mixer_us16x08.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/usb/mixer_us16x08.c b/sound/usb/mixer_us16x08.c
index d34dd1c..1c2d167 100644
--- a/sound/usb/mixer_us16x08.c
+++ b/sound/usb/mixer_us16x08.c
@@ -548,7 +548,7 @@ static int snd_us16x08_eqswitch_put(struct snd_kcontrol *kcontrol,
 		if (err < 0)
 			break;
 		store->val[b_idx][3][index] = val;
-		msleep(15);
+		msleep(20);
 	}
 
 	if (err > 0) {
-- 
2.9.3

Re: [PATCH 5/5] ALSA: usb-audio: fix msleep timeout due to minimum resolution of task scheduling

[Takashi Iwai]

On Mon, 20 Feb 2017 21:09:21 +0100,
Takashi Sakamoto wrote:
> 
> Depending on tick-related kernel configuration; e.g. HZ, task scheduler
> necessarily has no guarantee for its minimum resolution against ~20 msec
> msleep timeout.
> 
> This commit expands msleep timeout up to 20 msec with an optimistic view
> to packet handling capability of the target device. 'checkpatch.pl'
> generated below warning.

Well, it doesn't matter even if you set to 20.  When you use msleep(),
there is no guarantee of the minimum at all.


Takashi


> 
> WARNING: msleep < 20ms can sleep for up to 20ms; see Documentation/timers/timers-howto.txt
> +		msleep(15);
> 
> Fixes: d2bb390a2081 ("ALSA: usb-audio: Tascam US-16x08 DSP mixer quirk")
> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> ---
>  sound/usb/mixer_us16x08.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/sound/usb/mixer_us16x08.c b/sound/usb/mixer_us16x08.c
> index d34dd1c..1c2d167 100644
> --- a/sound/usb/mixer_us16x08.c
> +++ b/sound/usb/mixer_us16x08.c
> @@ -548,7 +548,7 @@ static int snd_us16x08_eqswitch_put(struct snd_kcontrol *kcontrol,
>  		if (err < 0)
>  			break;
>  		store->val[b_idx][3][index] = val;
> -		msleep(15);
> +		msleep(20);
>  	}
>  
>  	if (err > 0) {
> -- 
> 2.9.3
>