[alsa-devel] [PATCH 0/5] ALSA: usb-audio: fixes for quirks of Tascam US-16x08
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(-)
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", };
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
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;
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
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
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
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);
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
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; } -
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
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
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
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
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
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; };
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,
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
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
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@@ -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@@ -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@@ -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@@ -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@@ -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@@ -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@@ -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@@ -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@@ -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@@ -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@@ -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@@ -1217,7 +1218,6 @@ static struct snd_us16x08_control_params comp_controls[] = { .type = USB_MIXER_U8, .num_channels = 16, .name = "Compressor Ratio",
.freeer = NULL@@ -1225,7 +1225,6 @@ static struct snd_us16x08_control_params comp_controls[] = { .type = USB_MIXER_U8, .num_channels = 16, .name = "Compressor Attack",
.freeer = NULL@@ -1233,7 +1232,6 @@ static struct snd_us16x08_control_params comp_controls[] = { .type = USB_MIXER_U8, .num_channels = 16, .name = "Compressor Release",
.freeer = NULL@@ -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,@@ -1262,7 +1258,6 @@ static struct snd_us16x08_control_params channel_controls[] = { .type = USB_MIXER_U8, .num_channels = 16, .name = "Line Volume",
.freeer = NULL,@@ -1271,7 +1266,6 @@ static struct snd_us16x08_control_params channel_controls[] = { .type = USB_MIXER_BOOLEAN, .num_channels = 16, .name = "Mute Switch",
.freeer = NULL,@@ -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,}; @@ -1293,7 +1286,6 @@ static struct snd_us16x08_control_params master_controls[] = { .type = USB_MIXER_U8, .num_channels = 16, .name = "Master Volume",
.freeer = NULL,@@ -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,@@ -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,@@ -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,@@ -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);@@ -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);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
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) {
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
participants (2)
-
Takashi Iwai -
Takashi Sakamoto