[alsa-devel] [PATCH 0/8] ALSA: control: refactoring core codes
This patchset is for refactoring core control codes. This is also a preparation for my further work for ALSA control functionality.
Takashi Sakamoto (8): ALSA: control: change type from long due to the definition of sizeof operator ALSA: control: obsolete switch statement with const value table ALSA: control: gathering evaluations to access ALSA: control: add a comment about locking values after creating ALSA: control: rename loop index to i ALSA: control: fix over 80 characters lines ALSA: control: rename to appropriate macro name ALSA: control: arrange snd_ctl_new() as a local function
sound/core/control.c | 223 ++++++++++++++++++++++++++------------------------- 1 file changed, 113 insertions(+), 110 deletions(-)
The sizeof() operator returns 'unsigned' value, while it's assigned to 'long'. And in this case, the value is not so large.
This commit change the type of assigned value to 'unsigned int'. Additonally, rename local variable assigned to the value.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- sound/core/control.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/sound/core/control.c b/sound/core/control.c index 35324a8..ea49abc 100644 --- a/sound/core/control.c +++ b/sound/core/control.c @@ -1007,7 +1007,7 @@ struct user_element { struct snd_ctl_elem_info info; struct snd_card *card; void *elem_data; /* element data */ - unsigned long elem_data_size; /* size of element data in bytes */ + unsigned int elem_data_size; /* size of element data in bytes */ void *tlv_data; /* TLV data */ unsigned long tlv_data_size; /* TLV data size */ void *priv_data; /* private data (like strings for enumerated type) */ @@ -1164,7 +1164,7 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file, struct snd_card *card = file->card; struct snd_kcontrol kctl, *_kctl; unsigned int access; - long private_size; + unsigned int elem_data_size; struct user_element *ue; int idx, err;
@@ -1204,42 +1204,42 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file, switch (info->type) { case SNDRV_CTL_ELEM_TYPE_BOOLEAN: case SNDRV_CTL_ELEM_TYPE_INTEGER: - private_size = sizeof(long); + elem_data_size = sizeof(long); if (info->count > 128) return -EINVAL; break; case SNDRV_CTL_ELEM_TYPE_INTEGER64: - private_size = sizeof(long long); + elem_data_size = sizeof(long long); if (info->count > 64) return -EINVAL; break; case SNDRV_CTL_ELEM_TYPE_ENUMERATED: - private_size = sizeof(unsigned int); + elem_data_size = sizeof(unsigned int); if (info->count > 128 || info->value.enumerated.items == 0) return -EINVAL; break; case SNDRV_CTL_ELEM_TYPE_BYTES: - private_size = sizeof(unsigned char); + elem_data_size = sizeof(unsigned char); if (info->count > 512) return -EINVAL; break; case SNDRV_CTL_ELEM_TYPE_IEC958: - private_size = sizeof(struct snd_aes_iec958); + elem_data_size = sizeof(struct snd_aes_iec958); if (info->count != 1) return -EINVAL; break; default: return -EINVAL; } - private_size *= info->count; - ue = kzalloc(sizeof(struct user_element) + private_size, GFP_KERNEL); + elem_data_size *= info->count; + ue = kzalloc(sizeof(struct user_element) + elem_data_size, GFP_KERNEL); if (ue == NULL) return -ENOMEM; ue->card = card; ue->info = *info; ue->info.access = 0; ue->elem_data = (char *)ue + sizeof(*ue); - ue->elem_data_size = private_size; + ue->elem_data_size = elem_data_size; if (ue->info.type == SNDRV_CTL_ELEM_TYPE_ENUMERATED) { err = snd_ctl_elem_init_enum_names(ue); if (err < 0) {
At Wed, 11 Feb 2015 19:37:25 +0900, Takashi Sakamoto wrote:
The sizeof() operator returns 'unsigned' value, while it's assigned to 'long'.
The type of sizeof() is size_t, which is unsigned long on Linux. (Imagine the return value of sizeof() for an array over 4GB.)
Takashi
And in this case, the value is not so large.
This commit change the type of assigned value to 'unsigned int'. Additonally, rename local variable assigned to the value.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp
sound/core/control.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/sound/core/control.c b/sound/core/control.c index 35324a8..ea49abc 100644 --- a/sound/core/control.c +++ b/sound/core/control.c @@ -1007,7 +1007,7 @@ struct user_element { struct snd_ctl_elem_info info; struct snd_card *card; void *elem_data; /* element data */
- unsigned long elem_data_size; /* size of element data in bytes */
- unsigned int elem_data_size; /* size of element data in bytes */ void *tlv_data; /* TLV data */ unsigned long tlv_data_size; /* TLV data size */ void *priv_data; /* private data (like strings for enumerated type) */
@@ -1164,7 +1164,7 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file, struct snd_card *card = file->card; struct snd_kcontrol kctl, *_kctl; unsigned int access;
- long private_size;
- unsigned int elem_data_size; struct user_element *ue; int idx, err;
@@ -1204,42 +1204,42 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file, switch (info->type) { case SNDRV_CTL_ELEM_TYPE_BOOLEAN: case SNDRV_CTL_ELEM_TYPE_INTEGER:
private_size = sizeof(long);
elem_data_size = sizeof(long);
private_size = sizeof(long long);
elem_data_size = sizeof(long long);
private_size = sizeof(unsigned int);
elem_data_size = sizeof(unsigned int);
private_size = sizeof(unsigned char);
elem_data_size = sizeof(unsigned char);
private_size = sizeof(struct snd_aes_iec958);
elem_data_size = sizeof(struct snd_aes_iec958);
- private_size *= info->count;
- ue = kzalloc(sizeof(struct user_element) + private_size, GFP_KERNEL);
- elem_data_size *= info->count;
- ue = kzalloc(sizeof(struct user_element) + elem_data_size, GFP_KERNEL); if (ue == NULL) return -ENOMEM; ue->card = card; ue->info = *info; ue->info.access = 0; ue->elem_data = (char *)ue + sizeof(*ue);
- ue->elem_data_size = private_size;
- ue->elem_data_size = elem_data_size; if (ue->info.type == SNDRV_CTL_ELEM_TYPE_ENUMERATED) { err = snd_ctl_elem_init_enum_names(ue); if (err < 0) {
-- 2.1.0
To check parameters from userspace, snd_ctl_elem_add() has switch statement. This statement checks the number of values in a control and the size of each value. These two parameters are limited by struct snd_ctl_elem_value.value and can be replaced with two sets of two dimension array with constant values, without any calculation.
This commit obsolete the switch statement with the array.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- sound/core/control.c | 57 +++++++++++++++++++++++++--------------------------- 1 file changed, 27 insertions(+), 30 deletions(-)
diff --git a/sound/core/control.c b/sound/core/control.c index ea49abc..f248bde 100644 --- a/sound/core/control.c +++ b/sound/core/control.c @@ -1161,6 +1161,23 @@ static void snd_ctl_elem_user_free(struct snd_kcontrol *kcontrol) static int snd_ctl_elem_add(struct snd_ctl_file *file, struct snd_ctl_elem_info *info, int replace) { + /* The capacity of struct snd_ctl_elem_value.value.*/ + const unsigned int value_sizes[] = { + [SNDRV_CTL_ELEM_TYPE_BOOLEAN] = sizeof(long), + [SNDRV_CTL_ELEM_TYPE_INTEGER] = sizeof(long), + [SNDRV_CTL_ELEM_TYPE_ENUMERATED] = sizeof(unsigned int), + [SNDRV_CTL_ELEM_TYPE_BYTES] = sizeof(unsigned char), + [SNDRV_CTL_ELEM_TYPE_IEC958] = sizeof(struct snd_aes_iec958), + [SNDRV_CTL_ELEM_TYPE_INTEGER64] = sizeof(long long), + }; + const unsigned int max_value_counts[] = { + [SNDRV_CTL_ELEM_TYPE_BOOLEAN] = 128, + [SNDRV_CTL_ELEM_TYPE_INTEGER] = 128, + [SNDRV_CTL_ELEM_TYPE_ENUMERATED] = 128, + [SNDRV_CTL_ELEM_TYPE_BYTES] = 512, + [SNDRV_CTL_ELEM_TYPE_IEC958] = 1, + [SNDRV_CTL_ELEM_TYPE_INTEGER64] = 64, + }; struct snd_card *card = file->card; struct snd_kcontrol kctl, *_kctl; unsigned int access; @@ -1170,6 +1187,7 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
if (info->count < 1) return -EINVAL; + access = info->access == 0 ? SNDRV_CTL_ELEM_ACCESS_READWRITE : (info->access & (SNDRV_CTL_ELEM_ACCESS_READWRITE| SNDRV_CTL_ELEM_ACCESS_INACTIVE| @@ -1201,37 +1219,16 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file, kctl.tlv.c = snd_ctl_elem_user_tlv; access |= SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK; } - switch (info->type) { - case SNDRV_CTL_ELEM_TYPE_BOOLEAN: - case SNDRV_CTL_ELEM_TYPE_INTEGER: - elem_data_size = sizeof(long); - if (info->count > 128) - return -EINVAL; - break; - case SNDRV_CTL_ELEM_TYPE_INTEGER64: - elem_data_size = sizeof(long long); - if (info->count > 64) - return -EINVAL; - break; - case SNDRV_CTL_ELEM_TYPE_ENUMERATED: - elem_data_size = sizeof(unsigned int); - if (info->count > 128 || info->value.enumerated.items == 0) - return -EINVAL; - break; - case SNDRV_CTL_ELEM_TYPE_BYTES: - elem_data_size = sizeof(unsigned char); - if (info->count > 512) - return -EINVAL; - break; - case SNDRV_CTL_ELEM_TYPE_IEC958: - elem_data_size = sizeof(struct snd_aes_iec958); - if (info->count != 1) - return -EINVAL; - break; - default: + + if (info->type < SNDRV_CTL_ELEM_TYPE_BOOLEAN || + info->type > SNDRV_CTL_ELEM_TYPE_INTEGER64) return -EINVAL; - } - elem_data_size *= info->count; + if (info->count > max_value_counts[info->type]) + return -EINVAL; + if (info->type == SNDRV_CTL_ELEM_TYPE_ENUMERATED && + info->value.enumerated.items == 0) + return -EINVAL; + elem_data_size = value_sizes[info->type] * info->count; ue = kzalloc(sizeof(struct user_element) + elem_data_size, GFP_KERNEL); if (ue == NULL) return -ENOMEM;
On 02/11/2015 11:37 AM, Takashi Sakamoto wrote:
To check parameters from userspace, snd_ctl_elem_add() has switch statement. This statement checks the number of values in a control and the size of each value. These two parameters are limited by struct snd_ctl_elem_value.value and can be replaced with two sets of two dimension array with constant values, without any calculation.
This commit obsolete the switch statement with the array.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp
sound/core/control.c | 57 +++++++++++++++++++++++++--------------------------- 1 file changed, 27 insertions(+), 30 deletions(-)
diff --git a/sound/core/control.c b/sound/core/control.c index ea49abc..f248bde 100644 --- a/sound/core/control.c +++ b/sound/core/control.c @@ -1161,6 +1161,23 @@ static void snd_ctl_elem_user_free(struct snd_kcontrol *kcontrol) static int snd_ctl_elem_add(struct snd_ctl_file *file, struct snd_ctl_elem_info *info, int replace) {
- /* The capacity of struct snd_ctl_elem_value.value.*/
- const unsigned int value_sizes[] = {
[SNDRV_CTL_ELEM_TYPE_BOOLEAN] = sizeof(long),[SNDRV_CTL_ELEM_TYPE_INTEGER] = sizeof(long),[SNDRV_CTL_ELEM_TYPE_ENUMERATED] = sizeof(unsigned int),[SNDRV_CTL_ELEM_TYPE_BYTES] = sizeof(unsigned char),[SNDRV_CTL_ELEM_TYPE_IEC958] = sizeof(struct snd_aes_iec958),[SNDRV_CTL_ELEM_TYPE_INTEGER64] = sizeof(long long),- };
- const unsigned int max_value_counts[] = {
[SNDRV_CTL_ELEM_TYPE_BOOLEAN] = 128,[SNDRV_CTL_ELEM_TYPE_INTEGER] = 128,[SNDRV_CTL_ELEM_TYPE_ENUMERATED] = 128,[SNDRV_CTL_ELEM_TYPE_BYTES] = 512,[SNDRV_CTL_ELEM_TYPE_IEC958] = 1,[SNDRV_CTL_ELEM_TYPE_INTEGER64] = 64,- };
The tables should probably be static, no need to put them on the stack.
Hi Lars,
On Feb 11 2015 19:51, Lars-Peter Clausen wrote:
On 02/11/2015 11:37 AM, Takashi Sakamoto wrote:
To check parameters from userspace, snd_ctl_elem_add() has switch statement. This statement checks the number of values in a control and the size of each value. These two parameters are limited by struct snd_ctl_elem_value.value and can be replaced with two sets of two dimension array with constant values, without any calculation.
This commit obsolete the switch statement with the array.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp
sound/core/control.c | 57 +++++++++++++++++++++++++--------------------------- 1 file changed, 27 insertions(+), 30 deletions(-)
diff --git a/sound/core/control.c b/sound/core/control.c index ea49abc..f248bde 100644 --- a/sound/core/control.c +++ b/sound/core/control.c @@ -1161,6 +1161,23 @@ static void snd_ctl_elem_user_free(struct snd_kcontrol *kcontrol) static int snd_ctl_elem_add(struct snd_ctl_file *file, struct snd_ctl_elem_info *info, int replace) {
- /* The capacity of struct snd_ctl_elem_value.value.*/
- const unsigned int value_sizes[] = {
[SNDRV_CTL_ELEM_TYPE_BOOLEAN] = sizeof(long),[SNDRV_CTL_ELEM_TYPE_INTEGER] = sizeof(long),[SNDRV_CTL_ELEM_TYPE_ENUMERATED] = sizeof(unsigned int),[SNDRV_CTL_ELEM_TYPE_BYTES] = sizeof(unsigned char),[SNDRV_CTL_ELEM_TYPE_IEC958] = sizeof(struct snd_aes_iec958),[SNDRV_CTL_ELEM_TYPE_INTEGER64] = sizeof(long long),- };
- const unsigned int max_value_counts[] = {
[SNDRV_CTL_ELEM_TYPE_BOOLEAN] = 128,[SNDRV_CTL_ELEM_TYPE_INTEGER] = 128,[SNDRV_CTL_ELEM_TYPE_ENUMERATED] = 128,[SNDRV_CTL_ELEM_TYPE_BYTES] = 512,[SNDRV_CTL_ELEM_TYPE_IEC958] = 1,[SNDRV_CTL_ELEM_TYPE_INTEGER64] = 64,- };
The tables should probably be static, no need to put them on the stack.
Oops, exactly. I've been satisfied just to add const operator, against my intension... Thanks for your correction.
Takashi Sakamoto
The confirmation of access parameter is spread in a function.
This commit gathers these in one block for easy reading.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- sound/core/control.c | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-)
diff --git a/sound/core/control.c b/sound/core/control.c index f248bde..0baff92 100644 --- a/sound/core/control.c +++ b/sound/core/control.c @@ -1188,10 +1188,16 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file, if (info->count < 1) return -EINVAL;
- access = info->access == 0 ? SNDRV_CTL_ELEM_ACCESS_READWRITE : - (info->access & (SNDRV_CTL_ELEM_ACCESS_READWRITE| - SNDRV_CTL_ELEM_ACCESS_INACTIVE| - SNDRV_CTL_ELEM_ACCESS_TLV_READWRITE)); + access = info->access; + if (access == 0) + access = SNDRV_CTL_ELEM_ACCESS_READWRITE; + access &= SNDRV_CTL_ELEM_ACCESS_READWRITE | + SNDRV_CTL_ELEM_ACCESS_INACTIVE | + SNDRV_CTL_ELEM_ACCESS_TLV_READWRITE; + if (access & SNDRV_CTL_ELEM_ACCESS_TLV_READWRITE) + access |= SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK; + access |= SNDRV_CTL_ELEM_ACCESS_USER; + info->id.numid = 0; memset(&kctl, 0, sizeof(kctl));
@@ -1206,7 +1212,6 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
memcpy(&kctl.id, &info->id, sizeof(info->id)); kctl.count = info->owner ? info->owner : 1; - access |= SNDRV_CTL_ELEM_ACCESS_USER; if (info->type == SNDRV_CTL_ELEM_TYPE_ENUMERATED) kctl.info = snd_ctl_elem_user_enum_info; else @@ -1215,10 +1220,8 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file, kctl.get = snd_ctl_elem_user_get; if (access & SNDRV_CTL_ELEM_ACCESS_WRITE) kctl.put = snd_ctl_elem_user_put; - if (access & SNDRV_CTL_ELEM_ACCESS_TLV_READWRITE) { + if (access & SNDRV_CTL_ELEM_ACCESS_TLV_READWRITE) kctl.tlv.c = snd_ctl_elem_user_tlv; - access |= SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK; - }
if (info->type < SNDRV_CTL_ELEM_TYPE_BOOLEAN || info->type > SNDRV_CTL_ELEM_TYPE_INTEGER64)
All of created user controls are locked before added to system. This may be against programmers or users expectation.
This commit adds a comment about this.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- sound/core/control.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/sound/core/control.c b/sound/core/control.c index 0baff92..19f9f4e 100644 --- a/sound/core/control.c +++ b/sound/core/control.c @@ -1255,8 +1255,11 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file, return -ENOMEM; } _kctl->private_data = ue; + + /* Lock values in this user controls. */ for (idx = 0; idx < _kctl->count; idx++) _kctl->vd[idx].owner = file; + err = snd_ctl_add(card, _kctl); if (err < 0) return err;
This is a fashion and subtle issue, but can reduce characters in a file.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- sound/core/control.c | 44 ++++++++++++++++++++++---------------------- 1 file changed, 22 insertions(+), 22 deletions(-)
diff --git a/sound/core/control.c b/sound/core/control.c index 19f9f4e..15af804 100644 --- a/sound/core/control.c +++ b/sound/core/control.c @@ -119,7 +119,7 @@ static int snd_ctl_release(struct inode *inode, struct file *file) struct snd_card *card; struct snd_ctl_file *ctl; struct snd_kcontrol *control; - unsigned int idx; + unsigned int i;
ctl = file->private_data; file->private_data = NULL; @@ -129,9 +129,9 @@ static int snd_ctl_release(struct inode *inode, struct file *file) write_unlock_irqrestore(&card->ctl_files_rwlock, flags); down_write(&card->controls_rwsem); list_for_each_entry(control, &card->controls, list) - for (idx = 0; idx < control->count; idx++) - if (control->vd[idx].owner == ctl) - control->vd[idx].owner = NULL; + for (i = 0; i < control->count; i++) + if (control->vd[i].owner == ctl) + control->vd[i].owner = NULL; up_write(&card->controls_rwsem); snd_ctl_empty_read_queue(ctl); put_pid(ctl->pid); @@ -205,7 +205,7 @@ static struct snd_kcontrol *snd_ctl_new(struct snd_kcontrol *control, unsigned int access) { struct snd_kcontrol *kctl; - unsigned int idx; + unsigned int i; if (snd_BUG_ON(!control || !control->count)) return NULL; @@ -219,8 +219,8 @@ static struct snd_kcontrol *snd_ctl_new(struct snd_kcontrol *control, return NULL; } *kctl = *control; - for (idx = 0; idx < kctl->count; idx++) - kctl->vd[idx].access = access; + for (i = 0; i < kctl->count; i++) + kctl->vd[i].access = access; return kctl; }
@@ -340,7 +340,7 @@ static int snd_ctl_find_hole(struct snd_card *card, unsigned int count) int snd_ctl_add(struct snd_card *card, struct snd_kcontrol *kcontrol) { struct snd_ctl_elem_id id; - unsigned int idx; + unsigned int i; unsigned int count; int err = -EINVAL;
@@ -376,7 +376,7 @@ int snd_ctl_add(struct snd_card *card, struct snd_kcontrol *kcontrol) id = kcontrol->id; count = kcontrol->count; up_write(&card->controls_rwsem); - for (idx = 0; idx < count; idx++, id.index++, id.numid++) + for (i = 0; i < count; i++, id.index++, id.numid++) snd_ctl_notify(card, SNDRV_CTL_EVENT_MASK_ADD, &id); return 0;
@@ -405,7 +405,7 @@ int snd_ctl_replace(struct snd_card *card, struct snd_kcontrol *kcontrol, { struct snd_ctl_elem_id id; unsigned int count; - unsigned int idx; + unsigned int i; struct snd_kcontrol *old; int ret;
@@ -443,7 +443,7 @@ add: id = kcontrol->id; count = kcontrol->count; up_write(&card->controls_rwsem); - for (idx = 0; idx < count; idx++, id.index++, id.numid++) + for (i = 0; i < count; i++, id.index++, id.numid++) snd_ctl_notify(card, SNDRV_CTL_EVENT_MASK_ADD, &id); return 0;
@@ -467,14 +467,14 @@ EXPORT_SYMBOL(snd_ctl_replace); int snd_ctl_remove(struct snd_card *card, struct snd_kcontrol *kcontrol) { struct snd_ctl_elem_id id; - unsigned int idx; + unsigned int i;
if (snd_BUG_ON(!card || !kcontrol)) return -EINVAL; list_del(&kcontrol->list); card->controls_count -= kcontrol->count; id = kcontrol->id; - for (idx = 0; idx < kcontrol->count; idx++, id.index++, id.numid++) + for (i = 0; i < kcontrol->count; i++, id.index++, id.numid++) snd_ctl_notify(card, SNDRV_CTL_EVENT_MASK_REMOVE, &id); snd_ctl_free_one(kcontrol); return 0; @@ -523,7 +523,7 @@ static int snd_ctl_remove_user_ctl(struct snd_ctl_file * file, { struct snd_card *card = file->card; struct snd_kcontrol *kctl; - int idx, ret; + int i, ret;
down_write(&card->controls_rwsem); kctl = snd_ctl_find_id(card, id); @@ -535,8 +535,8 @@ static int snd_ctl_remove_user_ctl(struct snd_ctl_file * file, ret = -EINVAL; goto error; } - for (idx = 0; idx < kctl->count; idx++) - if (kctl->vd[idx].owner != NULL && kctl->vd[idx].owner != file) { + for (i = 0; i < kctl->count; i++) + if (kctl->vd[i].owner != NULL && kctl->vd[i].owner != file) { ret = -EBUSY; goto error; } @@ -725,7 +725,7 @@ static int snd_ctl_elem_list(struct snd_card *card, struct snd_ctl_elem_list list; struct snd_kcontrol *kctl; struct snd_ctl_elem_id *dst, *id; - unsigned int offset, space, jidx; + unsigned int offset, space, i; if (copy_from_user(&list, _list, sizeof(list))) return -EFAULT; @@ -755,8 +755,8 @@ static int snd_ctl_elem_list(struct snd_card *card, id = dst; while (space > 0 && plist != &card->controls) { kctl = snd_kcontrol(plist); - for (jidx = offset; space > 0 && jidx < kctl->count; jidx++) { - snd_ctl_build_ioff(id, kctl, jidx); + for (i = offset; space > 0 && i < kctl->count; i++) { + snd_ctl_build_ioff(id, kctl, i); id++; space--; list.used++; @@ -1183,7 +1183,7 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file, unsigned int access; unsigned int elem_data_size; struct user_element *ue; - int idx, err; + int i, err;
if (info->count < 1) return -EINVAL; @@ -1257,8 +1257,8 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file, _kctl->private_data = ue;
/* Lock values in this user controls. */ - for (idx = 0; idx < _kctl->count; idx++) - _kctl->vd[idx].owner = file; + for (i = 0; i < _kctl->count; i++) + _kctl->vd[i].owner = file;
err = snd_ctl_add(card, _kctl); if (err < 0)
At Wed, 11 Feb 2015 19:37:29 +0900, Takashi Sakamoto wrote:
This is a fashion and subtle issue, but can reduce characters in a file.
Sorry, I see no merit by this action. The idx is even clearer than using i for these purposes. And, if it's i, people expect it being int, not unsigned.
Takashi
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp
sound/core/control.c | 44 ++++++++++++++++++++++---------------------- 1 file changed, 22 insertions(+), 22 deletions(-)
diff --git a/sound/core/control.c b/sound/core/control.c index 19f9f4e..15af804 100644 --- a/sound/core/control.c +++ b/sound/core/control.c @@ -119,7 +119,7 @@ static int snd_ctl_release(struct inode *inode, struct file *file) struct snd_card *card; struct snd_ctl_file *ctl; struct snd_kcontrol *control;
- unsigned int idx;
unsigned int i;
ctl = file->private_data; file->private_data = NULL;
@@ -129,9 +129,9 @@ static int snd_ctl_release(struct inode *inode, struct file *file) write_unlock_irqrestore(&card->ctl_files_rwlock, flags); down_write(&card->controls_rwsem); list_for_each_entry(control, &card->controls, list)
for (idx = 0; idx < control->count; idx++)if (control->vd[idx].owner == ctl)control->vd[idx].owner = NULL;
for (i = 0; i < control->count; i++)if (control->vd[i].owner == ctl)control->vd[i].owner = NULL;@@ -205,7 +205,7 @@ static struct snd_kcontrol *snd_ctl_new(struct snd_kcontrol *control, unsigned int access) { struct snd_kcontrol *kctl;
- unsigned int idx;
unsigned int i;
if (snd_BUG_ON(!control || !control->count)) return NULL;
@@ -219,8 +219,8 @@ static struct snd_kcontrol *snd_ctl_new(struct snd_kcontrol *control, return NULL; } *kctl = *control;
- for (idx = 0; idx < kctl->count; idx++)
kctl->vd[idx].access = access;
- for (i = 0; i < kctl->count; i++)
kctl->vd[i].access = access;}
@@ -340,7 +340,7 @@ static int snd_ctl_find_hole(struct snd_card *card, unsigned int count) int snd_ctl_add(struct snd_card *card, struct snd_kcontrol *kcontrol) { struct snd_ctl_elem_id id;
- unsigned int idx;
- unsigned int i; unsigned int count; int err = -EINVAL;
@@ -376,7 +376,7 @@ int snd_ctl_add(struct snd_card *card, struct snd_kcontrol *kcontrol) id = kcontrol->id; count = kcontrol->count; up_write(&card->controls_rwsem);
- for (idx = 0; idx < count; idx++, id.index++, id.numid++)
- for (i = 0; i < count; i++, id.index++, id.numid++) snd_ctl_notify(card, SNDRV_CTL_EVENT_MASK_ADD, &id); return 0;
@@ -405,7 +405,7 @@ int snd_ctl_replace(struct snd_card *card, struct snd_kcontrol *kcontrol, { struct snd_ctl_elem_id id; unsigned int count;
- unsigned int idx;
- unsigned int i; struct snd_kcontrol *old; int ret;
@@ -443,7 +443,7 @@ add: id = kcontrol->id; count = kcontrol->count; up_write(&card->controls_rwsem);
- for (idx = 0; idx < count; idx++, id.index++, id.numid++)
- for (i = 0; i < count; i++, id.index++, id.numid++) snd_ctl_notify(card, SNDRV_CTL_EVENT_MASK_ADD, &id); return 0;
@@ -467,14 +467,14 @@ EXPORT_SYMBOL(snd_ctl_replace); int snd_ctl_remove(struct snd_card *card, struct snd_kcontrol *kcontrol) { struct snd_ctl_elem_id id;
- unsigned int idx;
unsigned int i;
if (snd_BUG_ON(!card || !kcontrol)) return -EINVAL; list_del(&kcontrol->list); card->controls_count -= kcontrol->count; id = kcontrol->id;
- for (idx = 0; idx < kcontrol->count; idx++, id.index++, id.numid++)
- for (i = 0; i < kcontrol->count; i++, id.index++, id.numid++) snd_ctl_notify(card, SNDRV_CTL_EVENT_MASK_REMOVE, &id); snd_ctl_free_one(kcontrol); return 0;
@@ -523,7 +523,7 @@ static int snd_ctl_remove_user_ctl(struct snd_ctl_file * file, { struct snd_card *card = file->card; struct snd_kcontrol *kctl;
- int idx, ret;
int i, ret;
down_write(&card->controls_rwsem); kctl = snd_ctl_find_id(card, id);
@@ -535,8 +535,8 @@ static int snd_ctl_remove_user_ctl(struct snd_ctl_file * file, ret = -EINVAL; goto error; }
- for (idx = 0; idx < kctl->count; idx++)
if (kctl->vd[idx].owner != NULL && kctl->vd[idx].owner != file) {
- for (i = 0; i < kctl->count; i++)
if (kctl->vd[i].owner != NULL && kctl->vd[i].owner != file) { ret = -EBUSY; goto error;@@ -725,7 +725,7 @@ static int snd_ctl_elem_list(struct snd_card *card, struct snd_ctl_elem_list list; struct snd_kcontrol *kctl; struct snd_ctl_elem_id *dst, *id;
- unsigned int offset, space, jidx;
unsigned int offset, space, i;
if (copy_from_user(&list, _list, sizeof(list))) return -EFAULT;
@@ -755,8 +755,8 @@ static int snd_ctl_elem_list(struct snd_card *card, id = dst; while (space > 0 && plist != &card->controls) { kctl = snd_kcontrol(plist);
for (jidx = offset; space > 0 && jidx < kctl->count; jidx++) {snd_ctl_build_ioff(id, kctl, jidx);
for (i = offset; space > 0 && i < kctl->count; i++) {snd_ctl_build_ioff(id, kctl, i); id++; space--; list.used++;@@ -1183,7 +1183,7 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file, unsigned int access; unsigned int elem_data_size; struct user_element *ue;
- int idx, err;
int i, err;
if (info->count < 1) return -EINVAL;
@@ -1257,8 +1257,8 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file, _kctl->private_data = ue;
/* Lock values in this user controls. */
- for (idx = 0; idx < _kctl->count; idx++)
_kctl->vd[idx].owner = file;
for (i = 0; i < _kctl->count; i++)
_kctl->vd[i].owner = file;err = snd_ctl_add(card, _kctl); if (err < 0)
-- 2.1.0
On Feb 11 2015 20:55, Takashi Iwai wrote:
At Wed, 11 Feb 2015 19:37:29 +0900, Takashi Sakamoto wrote:
This is a fashion and subtle issue, but can reduce characters in a file.
Sorry, I see no merit by this action. The idx is even clearer than using i for these purposes. And, if it's i, people expect it being int, not unsigned.
OK. I drop this patch.
Thanks
Takashi Sakamoto
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp
sound/core/control.c | 44 ++++++++++++++++++++++---------------------- 1 file changed, 22 insertions(+), 22 deletions(-)
diff --git a/sound/core/control.c b/sound/core/control.c index 19f9f4e..15af804 100644 --- a/sound/core/control.c +++ b/sound/core/control.c @@ -119,7 +119,7 @@ static int snd_ctl_release(struct inode *inode, struct file *file) struct snd_card *card; struct snd_ctl_file *ctl; struct snd_kcontrol *control;
- unsigned int idx;
unsigned int i;
ctl = file->private_data; file->private_data = NULL;
@@ -129,9 +129,9 @@ static int snd_ctl_release(struct inode *inode, struct file *file) write_unlock_irqrestore(&card->ctl_files_rwlock, flags); down_write(&card->controls_rwsem); list_for_each_entry(control, &card->controls, list)
for (idx = 0; idx < control->count; idx++)if (control->vd[idx].owner == ctl)control->vd[idx].owner = NULL;
for (i = 0; i < control->count; i++)if (control->vd[i].owner == ctl)control->vd[i].owner = NULL;@@ -205,7 +205,7 @@ static struct snd_kcontrol *snd_ctl_new(struct snd_kcontrol *control, unsigned int access) { struct snd_kcontrol *kctl;
- unsigned int idx;
unsigned int i;
if (snd_BUG_ON(!control || !control->count)) return NULL;
@@ -219,8 +219,8 @@ static struct snd_kcontrol *snd_ctl_new(struct snd_kcontrol *control, return NULL; } *kctl = *control;
- for (idx = 0; idx < kctl->count; idx++)
kctl->vd[idx].access = access;
- for (i = 0; i < kctl->count; i++)
kctl->vd[i].access = access;}
@@ -340,7 +340,7 @@ static int snd_ctl_find_hole(struct snd_card *card, unsigned int count) int snd_ctl_add(struct snd_card *card, struct snd_kcontrol *kcontrol) { struct snd_ctl_elem_id id;
- unsigned int idx;
- unsigned int i; unsigned int count; int err = -EINVAL;
@@ -376,7 +376,7 @@ int snd_ctl_add(struct snd_card *card, struct snd_kcontrol *kcontrol) id = kcontrol->id; count = kcontrol->count; up_write(&card->controls_rwsem);
- for (idx = 0; idx < count; idx++, id.index++, id.numid++)
- for (i = 0; i < count; i++, id.index++, id.numid++) snd_ctl_notify(card, SNDRV_CTL_EVENT_MASK_ADD, &id); return 0;
@@ -405,7 +405,7 @@ int snd_ctl_replace(struct snd_card *card, struct snd_kcontrol *kcontrol, { struct snd_ctl_elem_id id; unsigned int count;
- unsigned int idx;
- unsigned int i; struct snd_kcontrol *old; int ret;
@@ -443,7 +443,7 @@ add: id = kcontrol->id; count = kcontrol->count; up_write(&card->controls_rwsem);
- for (idx = 0; idx < count; idx++, id.index++, id.numid++)
- for (i = 0; i < count; i++, id.index++, id.numid++) snd_ctl_notify(card, SNDRV_CTL_EVENT_MASK_ADD, &id); return 0;
@@ -467,14 +467,14 @@ EXPORT_SYMBOL(snd_ctl_replace); int snd_ctl_remove(struct snd_card *card, struct snd_kcontrol *kcontrol) { struct snd_ctl_elem_id id;
- unsigned int idx;
unsigned int i;
if (snd_BUG_ON(!card || !kcontrol)) return -EINVAL; list_del(&kcontrol->list); card->controls_count -= kcontrol->count; id = kcontrol->id;
- for (idx = 0; idx < kcontrol->count; idx++, id.index++, id.numid++)
- for (i = 0; i < kcontrol->count; i++, id.index++, id.numid++) snd_ctl_notify(card, SNDRV_CTL_EVENT_MASK_REMOVE, &id); snd_ctl_free_one(kcontrol); return 0;
@@ -523,7 +523,7 @@ static int snd_ctl_remove_user_ctl(struct snd_ctl_file * file, { struct snd_card *card = file->card; struct snd_kcontrol *kctl;
- int idx, ret;
int i, ret;
down_write(&card->controls_rwsem); kctl = snd_ctl_find_id(card, id);
@@ -535,8 +535,8 @@ static int snd_ctl_remove_user_ctl(struct snd_ctl_file * file, ret = -EINVAL; goto error; }
- for (idx = 0; idx < kctl->count; idx++)
if (kctl->vd[idx].owner != NULL && kctl->vd[idx].owner != file) {
- for (i = 0; i < kctl->count; i++)
if (kctl->vd[i].owner != NULL && kctl->vd[i].owner != file) { ret = -EBUSY; goto error;@@ -725,7 +725,7 @@ static int snd_ctl_elem_list(struct snd_card *card, struct snd_ctl_elem_list list; struct snd_kcontrol *kctl; struct snd_ctl_elem_id *dst, *id;
- unsigned int offset, space, jidx;
unsigned int offset, space, i;
if (copy_from_user(&list, _list, sizeof(list))) return -EFAULT;
@@ -755,8 +755,8 @@ static int snd_ctl_elem_list(struct snd_card *card, id = dst; while (space > 0 && plist != &card->controls) { kctl = snd_kcontrol(plist);
for (jidx = offset; space > 0 && jidx < kctl->count; jidx++) {snd_ctl_build_ioff(id, kctl, jidx);
for (i = offset; space > 0 && i < kctl->count; i++) {snd_ctl_build_ioff(id, kctl, i); id++; space--; list.used++;@@ -1183,7 +1183,7 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file, unsigned int access; unsigned int elem_data_size; struct user_element *ue;
- int idx, err;
int i, err;
if (info->count < 1) return -EINVAL;
@@ -1257,8 +1257,8 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file, _kctl->private_data = ue;
/* Lock values in this user controls. */
- for (idx = 0; idx < _kctl->count; idx++)
_kctl->vd[idx].owner = file;
for (i = 0; i < _kctl->count; i++)
_kctl->vd[i].owner = file;err = snd_ctl_add(card, _kctl); if (err < 0)
-- 2.1.0
This commit arranges lines over 80 characters, just related to this patchset.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- sound/core/control.c | 58 ++++++++++++++++++++++++++++++---------------------- 1 file changed, 34 insertions(+), 24 deletions(-)
diff --git a/sound/core/control.c b/sound/core/control.c index 15af804..c18ac04 100644 --- a/sound/core/control.c +++ b/sound/core/control.c @@ -205,6 +205,7 @@ static struct snd_kcontrol *snd_ctl_new(struct snd_kcontrol *control, unsigned int access) { struct snd_kcontrol *kctl; + unsigned int size; unsigned int i; if (snd_BUG_ON(!control || !control->count)) @@ -213,7 +214,9 @@ static struct snd_kcontrol *snd_ctl_new(struct snd_kcontrol *control, if (control->count > MAX_CONTROL_COUNT) return NULL;
- kctl = kzalloc(sizeof(*kctl) + sizeof(struct snd_kcontrol_volatile) * control->count, GFP_KERNEL); + size = sizeof(*kctl); + size += sizeof(struct snd_kcontrol_volatile) * control->count; + kctl = kzalloc(size, GFP_KERNEL); if (kctl == NULL) { pr_err("ALSA: Cannot allocate control instance\n"); return NULL; @@ -314,11 +317,12 @@ static int snd_ctl_find_hole(struct snd_card *card, unsigned int count) unsigned int iter = 100000;
while (snd_ctl_remove_numid_conflict(card, count)) { - if (--iter == 0) { - /* this situation is very unlikely */ - dev_err(card->dev, "unable to allocate new control numid\n"); - return -ENOMEM; - } + if (--iter != 0) + continue; + + /* this situation is very unlikely */ + dev_err(card->dev, "unable to allocate new control numid\n"); + return -ENOMEM; } return 0; } @@ -355,12 +359,9 @@ int snd_ctl_add(struct snd_card *card, struct snd_kcontrol *kcontrol) down_write(&card->controls_rwsem); if (snd_ctl_find_id(card, &id)) { up_write(&card->controls_rwsem); - dev_err(card->dev, "control %i:%i:%i:%s:%i is already present\n", - id.iface, - id.device, - id.subdevice, - id.name, - id.index); + dev_err(card->dev, + "control %i:%i:%i:%s:%i is already present\n", + id.iface, id.device, id.subdevice, id.name, id.index); err = -EBUSY; goto error; } @@ -638,14 +639,16 @@ EXPORT_SYMBOL(snd_ctl_rename_id); * Return: The pointer of the instance if found, or %NULL if not. * */ -struct snd_kcontrol *snd_ctl_find_numid(struct snd_card *card, unsigned int numid) +struct snd_kcontrol *snd_ctl_find_numid(struct snd_card *card, + unsigned int numid) { struct snd_kcontrol *kctl;
if (snd_BUG_ON(!card || !numid)) return NULL; list_for_each_entry(kctl, &card->controls, list) { - if (kctl->id.numid <= numid && kctl->id.numid + kctl->count > numid) + if (kctl->id.numid <= numid && + kctl->id.numid + kctl->count > numid) return kctl; } return NULL; @@ -1010,7 +1013,8 @@ struct user_element { unsigned int elem_data_size; /* size of element data in bytes */ void *tlv_data; /* TLV data */ unsigned long tlv_data_size; /* TLV data size */ - void *priv_data; /* private data (like strings for enumerated type) */ + /* private data (like strings for enumerated type) */ + void *priv_data; };
static int snd_ctl_elem_user_info(struct snd_kcontrol *kcontrol, @@ -1062,7 +1066,7 @@ static int snd_ctl_elem_user_put(struct snd_kcontrol *kcontrol, struct user_element *ue = kcontrol->private_data;
mutex_lock(&ue->card->user_ctl_lock); - change = memcmp(&ucontrol->value, ue->elem_data, ue->elem_data_size) != 0; + change = !!memcmp(&ucontrol->value, ue->elem_data, ue->elem_data_size); if (change) memcpy(ue->elem_data, &ucontrol->value, ue->elem_data_size); mutex_unlock(&ue->card->user_ctl_lock); @@ -1272,7 +1276,8 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file, }
static int snd_ctl_elem_add_user(struct snd_ctl_file *file, - struct snd_ctl_elem_info __user *_info, int replace) + struct snd_ctl_elem_info __user *_info, + int replace) { struct snd_ctl_elem_info info; if (copy_from_user(&info, _info, sizeof(info))) @@ -1373,7 +1378,8 @@ static int snd_ctl_tlv_ioctl(struct snd_ctl_file *file, return err; }
-static long snd_ctl_ioctl(struct file *file, unsigned int cmd, unsigned long arg) +static long snd_ctl_ioctl(struct file *file, unsigned int cmd, + unsigned long arg) { struct snd_ctl_file *ctl; struct snd_card *card; @@ -1518,7 +1524,8 @@ static unsigned int snd_ctl_poll(struct file *file, poll_table * wait) * register the device-specific control-ioctls. * called from each device manager like pcm.c, hwdep.c, etc. */ -static int _snd_ctl_register_ioctl(snd_kctl_ioctl_func_t fcn, struct list_head *lists) +static int _snd_ctl_register_ioctl(snd_kctl_ioctl_func_t fcn, + struct list_head *lists) { struct snd_kctl_ioctl *pn;
@@ -1793,6 +1800,8 @@ EXPORT_SYMBOL(snd_ctl_boolean_stereo_info); int snd_ctl_enum_info(struct snd_ctl_elem_info *info, unsigned int channels, unsigned int items, const char *const names[]) { + const char *name; + info->type = SNDRV_CTL_ELEM_TYPE_ENUMERATED; info->count = channels; info->value.enumerated.items = items; @@ -1800,11 +1809,12 @@ int snd_ctl_enum_info(struct snd_ctl_elem_info *info, unsigned int channels, return 0; if (info->value.enumerated.item >= items) info->value.enumerated.item = items - 1; - WARN(strlen(names[info->value.enumerated.item]) >= sizeof(info->value.enumerated.name), - "ALSA: too long item name '%s'\n", - names[info->value.enumerated.item]); - strlcpy(info->value.enumerated.name, - names[info->value.enumerated.item], + + name = names[info->value.enumerated.item]; + WARN(strlen(name) >= sizeof(info->value.enumerated.name), + "ALSA: too long item name '%s'\n", name); + + strlcpy(info->value.enumerated.name, name, sizeof(info->value.enumerated.name)); return 0; }
At Wed, 11 Feb 2015 19:37:30 +0900, Takashi Sakamoto wrote:
@@ -1010,7 +1013,8 @@ struct user_element { unsigned int elem_data_size; /* size of element data in bytes */ void *tlv_data; /* TLV data */ unsigned long tlv_data_size; /* TLV data size */
- void *priv_data; /* private data (like strings for enumerated type) */
- /* private data (like strings for enumerated type) */
- void *priv_data;
Better to put the comment in the same line like other fields.
Takashi
On Feb 11 2015 21:01, Takashi Iwai wrote:
At Wed, 11 Feb 2015 19:37:30 +0900, Takashi Sakamoto wrote:
@@ -1010,7 +1013,8 @@ struct user_element { unsigned int elem_data_size; /* size of element data in bytes */ void *tlv_data; /* TLV data */ unsigned long tlv_data_size; /* TLV data size */
- void *priv_data; /* private data (like strings for enumerated type) */
- /* private data (like strings for enumerated type) */
- void *priv_data;
Better to put the comment in the same line like other fields.
Which do you mean, we allow this line over 80 characters (keep what it was), or I should revise this comment within 80 characters considering the authours idea?
Regards
Takashi Sakamoto
Dne 12.2.2015 v 13:47 Takashi Sakamoto napsal(a):
On Feb 11 2015 21:01, Takashi Iwai wrote:
At Wed, 11 Feb 2015 19:37:30 +0900, Takashi Sakamoto wrote:
@@ -1010,7 +1013,8 @@ struct user_element { unsigned int elem_data_size; /* size of element data in bytes */ void *tlv_data; /* TLV data */ unsigned long tlv_data_size; /* TLV data size */
- void *priv_data; /* private data (like strings for enumerated type) */
- /* private data (like strings for enumerated type) */
- void *priv_data;
Better to put the comment in the same line like other fields.
Which do you mean, we allow this line over 80 characters (keep what it was), or I should revise this comment within 80 characters considering the authours idea?
Keep it as-is. Any change will make the readability more worse.
Jaroslav
MAX_CONTROL_COUNT is used for the maximum number of values included in a control, therefore should be rename to mean it.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- sound/core/control.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/sound/core/control.c b/sound/core/control.c index c18ac04..04534f3 100644 --- a/sound/core/control.c +++ b/sound/core/control.c @@ -30,9 +30,11 @@ #include <sound/info.h> #include <sound/control.h>
+/* The maximum number of values which one control has. */ +#define MAX_VALUES_COUNT 1028 + /* max number of user-defined controls */ #define MAX_USER_CONTROLS 32 -#define MAX_CONTROL_COUNT 1028
struct snd_kctl_ioctl { struct list_head list; /* list of all ioctls */ @@ -211,7 +213,7 @@ static struct snd_kcontrol *snd_ctl_new(struct snd_kcontrol *control, if (snd_BUG_ON(!control || !control->count)) return NULL;
- if (control->count > MAX_CONTROL_COUNT) + if (control->count > MAX_VALUES_COUNT) return NULL;
size = sizeof(*kctl);
The snd_ctl_new1() was added in 2001. I guess that snd_ctl_new() becames a local function in this timing.
This commit arranges the function as a local helper function, remove 'snd_' prefix and comments, change comments to refer to the function.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- sound/core/control.c | 28 ++++++++-------------------- 1 file changed, 8 insertions(+), 20 deletions(-)
diff --git a/sound/core/control.c b/sound/core/control.c index 04534f3..af95783 100644 --- a/sound/core/control.c +++ b/sound/core/control.c @@ -193,18 +193,8 @@ void snd_ctl_notify(struct snd_card *card, unsigned int mask, } EXPORT_SYMBOL(snd_ctl_notify);
-/** - * snd_ctl_new - create a control instance from the template - * @control: the control template - * @access: the default control access - * - * Allocates a new struct snd_kcontrol instance and copies the given template - * to the new instance. It does not copy volatile data (access). - * - * Return: The pointer of the new instance, or %NULL on failure. - */ -static struct snd_kcontrol *snd_ctl_new(struct snd_kcontrol *control, - unsigned int access) +static struct snd_kcontrol *ctl_new(struct snd_kcontrol *control, + unsigned int access) { struct snd_kcontrol *kctl; unsigned int size; @@ -273,7 +263,7 @@ struct snd_kcontrol *snd_ctl_new1(const struct snd_kcontrol_new *ncontrol, kctl.tlv.p = ncontrol->tlv.p; kctl.private_value = ncontrol->private_value; kctl.private_data = private_data; - return snd_ctl_new(&kctl, access); + return ctl_new(&kctl, access); } EXPORT_SYMBOL(snd_ctl_new1);
@@ -281,9 +271,8 @@ EXPORT_SYMBOL(snd_ctl_new1); * snd_ctl_free_one - release the control instance * @kcontrol: the control instance * - * Releases the control instance created via snd_ctl_new() - * or snd_ctl_new1(). - * Don't call this after the control was added to the card. + * Releases the control instance created via ctl_new() or snd_ctl_new1(). Don't + * call this after the control was added to the card. */ void snd_ctl_free_one(struct snd_kcontrol *kcontrol) { @@ -334,9 +323,8 @@ static int snd_ctl_find_hole(struct snd_card *card, unsigned int count) * @card: the card instance * @kcontrol: the control instance to add * - * Adds the control instance created via snd_ctl_new() or - * snd_ctl_new1() to the given card. Assigns also an unique - * numid used for fast search. + * Adds the control instance created via ctl_new() or snd_ctl_new1() to the + * given card. Assigns also an unique numid used for fast search. * * It frees automatically the control which cannot be added. * @@ -1254,7 +1242,7 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file, } } kctl.private_free = snd_ctl_elem_user_free; - _kctl = snd_ctl_new(&kctl, access); + _kctl = ctl_new(&kctl, access); if (_kctl == NULL) { kfree(ue->priv_data); kfree(ue);
On 02/11/2015 11:37 AM, Takashi Sakamoto wrote: [...]
-/**
- snd_ctl_new - create a control instance from the template
- @control: the control template
- @access: the default control access
- Allocates a new struct snd_kcontrol instance and copies the given template
- to the new instance. It does not copy volatile data (access).
- Return: The pointer of the new instance, or %NULL on failure.
- */
-static struct snd_kcontrol *snd_ctl_new(struct snd_kcontrol *control,
unsigned int access)+static struct snd_kcontrol *ctl_new(struct snd_kcontrol *control,
I'd prefer to keep both the documentation as well as the 'snd_' prefix. Maybe just replace the '/**' with '/*' to prevent it from appearing in the public API documentation.
- Lars
At Wed, 11 Feb 2015 13:49:29 +0100, Lars-Peter Clausen wrote:
On 02/11/2015 11:37 AM, Takashi Sakamoto wrote: [...]
-/**
- snd_ctl_new - create a control instance from the template
- @control: the control template
- @access: the default control access
- Allocates a new struct snd_kcontrol instance and copies the given template
- to the new instance. It does not copy volatile data (access).
- Return: The pointer of the new instance, or %NULL on failure.
- */
-static struct snd_kcontrol *snd_ctl_new(struct snd_kcontrol *control,
unsigned int access)+static struct snd_kcontrol *ctl_new(struct snd_kcontrol *control,
I'd prefer to keep both the documentation as well as the 'snd_' prefix. Maybe just replace the '/**' with '/*' to prevent it from appearing in the public API documentation.
Yes, agreed. There is no strict rule about snd_ prefix for non-exported objects. We prefer not having snd_ prefix for local stuff in general just for readability. But in this case, it doesn't improve so much.
Takashi
On Feb 11 2015 22:04, Takashi Iwai wrote:
At Wed, 11 Feb 2015 13:49:29 +0100, Lars-Peter Clausen wrote:
On 02/11/2015 11:37 AM, Takashi Sakamoto wrote: [...]
-/**
- snd_ctl_new - create a control instance from the template
- @control: the control template
- @access: the default control access
- Allocates a new struct snd_kcontrol instance and copies the given template
- to the new instance. It does not copy volatile data (access).
- Return: The pointer of the new instance, or %NULL on failure.
- */
-static struct snd_kcontrol *snd_ctl_new(struct snd_kcontrol *control,
unsigned int access)+static struct snd_kcontrol *ctl_new(struct snd_kcontrol *control,
I'd prefer to keep both the documentation as well as the 'snd_' prefix. Maybe just replace the '/**' with '/*' to prevent it from appearing in the public API documentation.
Yes, agreed. There is no strict rule about snd_ prefix for non-exported objects. We prefer not having snd_ prefix for local stuff in general just for readability. But in this case, it doesn't improve so much.
Hm. I don't disagree with remaining these comments.
...But I have another issue. In my final patch in another series, I change this function drastically. Then, should I spend a bit time to revise them even if this is just a local function? http://mailman.alsa-project.org/pipermail/alsa-devel/2015-February/087796.ht...
Thanks
Takashi Sakamoto
At Thu, 12 Feb 2015 21:45:24 +0900, Takashi Sakamoto wrote:
On Feb 11 2015 22:04, Takashi Iwai wrote:
At Wed, 11 Feb 2015 13:49:29 +0100, Lars-Peter Clausen wrote:
On 02/11/2015 11:37 AM, Takashi Sakamoto wrote: [...]
-/**
- snd_ctl_new - create a control instance from the template
- @control: the control template
- @access: the default control access
- Allocates a new struct snd_kcontrol instance and copies the given template
- to the new instance. It does not copy volatile data (access).
- Return: The pointer of the new instance, or %NULL on failure.
- */
-static struct snd_kcontrol *snd_ctl_new(struct snd_kcontrol *control,
unsigned int access)+static struct snd_kcontrol *ctl_new(struct snd_kcontrol *control,
I'd prefer to keep both the documentation as well as the 'snd_' prefix. Maybe just replace the '/**' with '/*' to prevent it from appearing in the public API documentation.
Yes, agreed. There is no strict rule about snd_ prefix for non-exported objects. We prefer not having snd_ prefix for local stuff in general just for readability. But in this case, it doesn't improve so much.
Hm. I don't disagree with remaining these comments.
...But I have another issue. In my final patch in another series, I change this function drastically. Then, should I spend a bit time to revise them even if this is just a local function?
Yes, please.
Takashi
At Wed, 11 Feb 2015 19:37:24 +0900, Takashi Sakamoto wrote:
This patchset is for refactoring core control codes. This is also a preparation for my further work for ALSA control functionality.
As now is already after the 3.20 merge window open, basically I'd take only fix patches now and postpone the rest for 3.21. So, please pick up (and revise) the fix patches that are really worth for 3.20 and resubmit them. Submit the rest as another patchset to be distinguished from the former.
thanks,
Takashi
Takashi Sakamoto (8): ALSA: control: change type from long due to the definition of sizeof operator ALSA: control: obsolete switch statement with const value table ALSA: control: gathering evaluations to access ALSA: control: add a comment about locking values after creating ALSA: control: rename loop index to i ALSA: control: fix over 80 characters lines ALSA: control: rename to appropriate macro name ALSA: control: arrange snd_ctl_new() as a local function
sound/core/control.c | 223 ++++++++++++++++++++++++++------------------------- 1 file changed, 113 insertions(+), 110 deletions(-)
-- 2.1.0
On Feb 11 2015 22:19, Takashi Iwai wrote:
At Wed, 11 Feb 2015 19:37:24 +0900, Takashi Sakamoto wrote:
This patchset is for refactoring core control codes. This is also a preparation for my further work for ALSA control functionality.
As now is already after the 3.20 merge window open, basically I'd take only fix patches now and postpone the rest for 3.21.
I'm posting them for 3.21, not for current 3.20. I should add some comments about this, sorry to trouble you.
So, please pick up (and revise) the fix patches that are really worth for 3.20 and resubmit them. Submit the rest as another patchset to be distinguished from the former.
Nothing for 3.20. I'll wait for closing the merge window, then post the revised patches, but hope to continue discussion.
Regards
Takashi Sakamoto
Takashi Sakamoto (8): ALSA: control: change type from long due to the definition of sizeof operator ALSA: control: obsolete switch statement with const value table ALSA: control: gathering evaluations to access ALSA: control: add a comment about locking values after creating ALSA: control: rename loop index to i ALSA: control: fix over 80 characters lines ALSA: control: rename to appropriate macro name ALSA: control: arrange snd_ctl_new() as a local function
sound/core/control.c | 223 ++++++++++++++++++++++++++------------------------- 1 file changed, 113 insertions(+), 110 deletions(-)
-- 2.1.0
This patchset is for reducing executing time and stack usage at creating new control instances. Nothing fixes.
Takashi Sakamoto (2): ALSA: core: use precomputed table to check userspace control params ALSA: core: reduce stack usage related to snd_ctl_new()
sound/core/control.c | 271 +++++++++++++++++++++++++++++---------------------- 1 file changed, 157 insertions(+), 114 deletions(-)
The parameters can be decided in compile time.
This commit adds precomputed table to reduce calculating time.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- sound/core/control.c | 60 ++++++++++++++++++++++++---------------------------- 1 file changed, 28 insertions(+), 32 deletions(-)
diff --git a/sound/core/control.c b/sound/core/control.c index 35324a8..3955115 100644 --- a/sound/core/control.c +++ b/sound/core/control.c @@ -1161,6 +1161,23 @@ static void snd_ctl_elem_user_free(struct snd_kcontrol *kcontrol) static int snd_ctl_elem_add(struct snd_ctl_file *file, struct snd_ctl_elem_info *info, int replace) { + /* The capacity of struct snd_ctl_elem_value.value.*/ + static const unsigned int value_sizes[] = { + [SNDRV_CTL_ELEM_TYPE_BOOLEAN] = sizeof(long), + [SNDRV_CTL_ELEM_TYPE_INTEGER] = sizeof(long), + [SNDRV_CTL_ELEM_TYPE_ENUMERATED] = sizeof(unsigned int), + [SNDRV_CTL_ELEM_TYPE_BYTES] = sizeof(unsigned char), + [SNDRV_CTL_ELEM_TYPE_IEC958] = sizeof(struct snd_aes_iec958), + [SNDRV_CTL_ELEM_TYPE_INTEGER64] = sizeof(long long), + }; + static const unsigned int max_value_counts[] = { + [SNDRV_CTL_ELEM_TYPE_BOOLEAN] = 128, + [SNDRV_CTL_ELEM_TYPE_INTEGER] = 128, + [SNDRV_CTL_ELEM_TYPE_ENUMERATED] = 128, + [SNDRV_CTL_ELEM_TYPE_BYTES] = 512, + [SNDRV_CTL_ELEM_TYPE_IEC958] = 1, + [SNDRV_CTL_ELEM_TYPE_INTEGER64] = 64, + }; struct snd_card *card = file->card; struct snd_kcontrol kctl, *_kctl; unsigned int access; @@ -1168,8 +1185,6 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file, struct user_element *ue; int idx, err;
- if (info->count < 1) - return -EINVAL; access = info->access == 0 ? SNDRV_CTL_ELEM_ACCESS_READWRITE : (info->access & (SNDRV_CTL_ELEM_ACCESS_READWRITE| SNDRV_CTL_ELEM_ACCESS_INACTIVE| @@ -1201,37 +1216,18 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file, kctl.tlv.c = snd_ctl_elem_user_tlv; access |= SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK; } - switch (info->type) { - case SNDRV_CTL_ELEM_TYPE_BOOLEAN: - case SNDRV_CTL_ELEM_TYPE_INTEGER: - private_size = sizeof(long); - if (info->count > 128) - return -EINVAL; - break; - case SNDRV_CTL_ELEM_TYPE_INTEGER64: - private_size = sizeof(long long); - if (info->count > 64) - return -EINVAL; - break; - case SNDRV_CTL_ELEM_TYPE_ENUMERATED: - private_size = sizeof(unsigned int); - if (info->count > 128 || info->value.enumerated.items == 0) - return -EINVAL; - break; - case SNDRV_CTL_ELEM_TYPE_BYTES: - private_size = sizeof(unsigned char); - if (info->count > 512) - return -EINVAL; - break; - case SNDRV_CTL_ELEM_TYPE_IEC958: - private_size = sizeof(struct snd_aes_iec958); - if (info->count != 1) - return -EINVAL; - break; - default: + + if ((info->type < SNDRV_CTL_ELEM_TYPE_BOOLEAN) || + (info->type > SNDRV_CTL_ELEM_TYPE_INTEGER64)) return -EINVAL; - } - private_size *= info->count; + if ((info->type == SNDRV_CTL_ELEM_TYPE_ENUMERATED) && + (info->value.enumerated.items == 0)) + return -EINVAL; + if ((info->count < 1) || + (info->count > max_value_counts[info->type])) + return -EINVAL; + + private_size = value_sizes[info->type] * info->count; ue = kzalloc(sizeof(struct user_element) + private_size, GFP_KERNEL); if (ue == NULL) return -ENOMEM;
The callers of snd_ctl_new() need to have 'struct snd_kcontrol' data, and pass the data as template. Then, the function allocates the structure data again and copy from the template. This is a waste of resources. Especially, the callers use large stack for the template.
This commit removes a need of template for the function, thus, changes the prototype of snd_ctl_new(). Furthermore, this commit changes the code of callers, snd_ctl_new1() and snd_ctl_elem_add() for better shape.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- sound/core/control.c | 213 +++++++++++++++++++++++++++++++-------------------- 1 file changed, 130 insertions(+), 83 deletions(-)
diff --git a/sound/core/control.c b/sound/core/control.c index 3955115..1b3880a 100644 --- a/sound/core/control.c +++ b/sound/core/control.c @@ -192,36 +192,43 @@ void snd_ctl_notify(struct snd_card *card, unsigned int mask, EXPORT_SYMBOL(snd_ctl_notify);
/** - * snd_ctl_new - create a control instance from the template - * @control: the control template - * @access: the default control access + * snd_ctl_new - create a new control instance with some elements + * @kctl: the pointer to store new control instance + * @count: the number of elements in this control + * @access: the default access flags for elements in this control + * @file: given when locking these elements * - * Allocates a new struct snd_kcontrol instance and copies the given template - * to the new instance. It does not copy volatile data (access). + * Allocates a memory object for a new control instance. The instance has + * elements as many as the given number (@count). Each element has given + * access permissions (@access). Each element is locked when @file is given. * - * Return: The pointer of the new instance, or %NULL on failure. + * Return: 0 on success, error code on failure */ -static struct snd_kcontrol *snd_ctl_new(struct snd_kcontrol *control, - unsigned int access) +static int snd_ctl_new(struct snd_kcontrol **kctl, unsigned int count, + unsigned int access, struct snd_ctl_file *file) { - struct snd_kcontrol *kctl; + unsigned int size; unsigned int idx; - if (snd_BUG_ON(!control || !control->count)) - return NULL; + if ((count == 0) || (count > MAX_CONTROL_COUNT)) + return -EINVAL;
- if (control->count > MAX_CONTROL_COUNT) - return NULL; + size = sizeof(struct snd_kcontrol); + size += sizeof(struct snd_kcontrol_volatile) * count;
- kctl = kzalloc(sizeof(*kctl) + sizeof(struct snd_kcontrol_volatile) * control->count, GFP_KERNEL); - if (kctl == NULL) { + *kctl = kzalloc(size, GFP_KERNEL); + if (*kctl == NULL) { pr_err("ALSA: Cannot allocate control instance\n"); - return NULL; + return -ENOMEM; } - *kctl = *control; - for (idx = 0; idx < kctl->count; idx++) - kctl->vd[idx].access = access; - return kctl; + + for (idx = 0; idx < count; idx++) { + (*kctl)->vd[idx].access = access; + (*kctl)->vd[idx].owner = file; + } + (*kctl)->count = count; + + return 0; }
/** @@ -238,37 +245,53 @@ static struct snd_kcontrol *snd_ctl_new(struct snd_kcontrol *control, struct snd_kcontrol *snd_ctl_new1(const struct snd_kcontrol_new *ncontrol, void *private_data) { - struct snd_kcontrol kctl; + struct snd_kcontrol *kctl; + unsigned int count; unsigned int access; + int err; if (snd_BUG_ON(!ncontrol || !ncontrol->info)) return NULL; - memset(&kctl, 0, sizeof(kctl)); - kctl.id.iface = ncontrol->iface; - kctl.id.device = ncontrol->device; - kctl.id.subdevice = ncontrol->subdevice; + + count = ncontrol->count; + if (count == 0) + count = 1; + + access = ncontrol->access; + if (access == 0) + access = SNDRV_CTL_ELEM_ACCESS_READWRITE; + access &= (SNDRV_CTL_ELEM_ACCESS_READWRITE | + SNDRV_CTL_ELEM_ACCESS_VOLATILE | + SNDRV_CTL_ELEM_ACCESS_INACTIVE | + SNDRV_CTL_ELEM_ACCESS_TLV_READWRITE | + SNDRV_CTL_ELEM_ACCESS_TLV_COMMAND | + SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK); + + err = snd_ctl_new(&kctl, count, access, NULL); + if (err < 0) + return NULL; + + /* The 'numid' member is decided when calling snd_ctl_add(). */ + kctl->id.iface = ncontrol->iface; + kctl->id.device = ncontrol->device; + kctl->id.subdevice = ncontrol->subdevice; if (ncontrol->name) { - strlcpy(kctl.id.name, ncontrol->name, sizeof(kctl.id.name)); - if (strcmp(ncontrol->name, kctl.id.name) != 0) + strlcpy(kctl->id.name, ncontrol->name, sizeof(kctl->id.name)); + if (strcmp(ncontrol->name, kctl->id.name) != 0) pr_warn("ALSA: Control name '%s' truncated to '%s'\n", - ncontrol->name, kctl.id.name); + ncontrol->name, kctl->id.name); } - kctl.id.index = ncontrol->index; - kctl.count = ncontrol->count ? ncontrol->count : 1; - access = ncontrol->access == 0 ? SNDRV_CTL_ELEM_ACCESS_READWRITE : - (ncontrol->access & (SNDRV_CTL_ELEM_ACCESS_READWRITE| - SNDRV_CTL_ELEM_ACCESS_VOLATILE| - SNDRV_CTL_ELEM_ACCESS_INACTIVE| - SNDRV_CTL_ELEM_ACCESS_TLV_READWRITE| - SNDRV_CTL_ELEM_ACCESS_TLV_COMMAND| - SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK)); - kctl.info = ncontrol->info; - kctl.get = ncontrol->get; - kctl.put = ncontrol->put; - kctl.tlv.p = ncontrol->tlv.p; - kctl.private_value = ncontrol->private_value; - kctl.private_data = private_data; - return snd_ctl_new(&kctl, access); + kctl->id.index = ncontrol->index; + + kctl->info = ncontrol->info; + kctl->get = ncontrol->get; + kctl->put = ncontrol->put; + kctl->tlv.p = ncontrol->tlv.p; + + kctl->private_value = ncontrol->private_value; + kctl->private_data = private_data; + + return kctl; } EXPORT_SYMBOL(snd_ctl_new1);
@@ -1179,44 +1202,48 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file, [SNDRV_CTL_ELEM_TYPE_INTEGER64] = 64, }; struct snd_card *card = file->card; - struct snd_kcontrol kctl, *_kctl; + struct snd_kcontrol *kctl; + unsigned int count; unsigned int access; long private_size; struct user_element *ue; - int idx, err; - - access = info->access == 0 ? SNDRV_CTL_ELEM_ACCESS_READWRITE : - (info->access & (SNDRV_CTL_ELEM_ACCESS_READWRITE| - SNDRV_CTL_ELEM_ACCESS_INACTIVE| - SNDRV_CTL_ELEM_ACCESS_TLV_READWRITE)); - info->id.numid = 0; - memset(&kctl, 0, sizeof(kctl)); + int err;
+ /* Delete a control to replace them if needed. */ if (replace) { + info->id.numid = 0; err = snd_ctl_remove_user_ctl(file, &info->id); if (err) return err; }
- if (card->user_ctl_count >= MAX_USER_CONTROLS) + /* + * The number of userspace controls are counted control by control, + * not element by element. + */ + if (card->user_ctl_count + 1 > MAX_USER_CONTROLS) return -ENOMEM;
- memcpy(&kctl.id, &info->id, sizeof(info->id)); - kctl.count = info->owner ? info->owner : 1; - access |= SNDRV_CTL_ELEM_ACCESS_USER; - if (info->type == SNDRV_CTL_ELEM_TYPE_ENUMERATED) - kctl.info = snd_ctl_elem_user_enum_info; - else - kctl.info = snd_ctl_elem_user_info; - if (access & SNDRV_CTL_ELEM_ACCESS_READ) - kctl.get = snd_ctl_elem_user_get; - if (access & SNDRV_CTL_ELEM_ACCESS_WRITE) - kctl.put = snd_ctl_elem_user_put; - if (access & SNDRV_CTL_ELEM_ACCESS_TLV_READWRITE) { - kctl.tlv.c = snd_ctl_elem_user_tlv; + /* Check the number of elements for this userspace control. */ + count = info->owner; + if (count == 0) + count = 1; + + /* Arrange access permissions if needed. */ + access = info->access; + if (access == 0) + access = SNDRV_CTL_ELEM_ACCESS_READWRITE; + access &= (SNDRV_CTL_ELEM_ACCESS_READWRITE | + SNDRV_CTL_ELEM_ACCESS_INACTIVE | + SNDRV_CTL_ELEM_ACCESS_TLV_READWRITE); + if (access & SNDRV_CTL_ELEM_ACCESS_TLV_READWRITE) access |= SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK; - } + access |= SNDRV_CTL_ELEM_ACCESS_USER;
+ /* + * Check information and calculate the size of data specific to + * this userspace control. + */ if ((info->type < SNDRV_CTL_ELEM_TYPE_BOOLEAN) || (info->type > SNDRV_CTL_ELEM_TYPE_INTEGER64)) return -EINVAL; @@ -1226,11 +1253,27 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file, if ((info->count < 1) || (info->count > max_value_counts[info->type])) return -EINVAL; - private_size = value_sizes[info->type] * info->count; - ue = kzalloc(sizeof(struct user_element) + private_size, GFP_KERNEL); - if (ue == NULL) + + /* + * Keep memory object for this userspace control. After passing this + * code block, the instance should be freed by snd_ctl_free_one(). + * + * Note that these elements in this control are locked. + */ + err = snd_ctl_new(&kctl, count, access, file); + if (err < 0) + return err; + kctl->private_data = kzalloc(sizeof(struct user_element) + private_size, + GFP_KERNEL); + if (kctl->private_data == NULL) { + kfree(kctl); return -ENOMEM; + } + kctl->private_free = snd_ctl_elem_user_free; + + /* Set private data for this userspace control. */ + ue = (struct user_element *)kctl->private_data; ue->card = card; ue->info = *info; ue->info.access = 0; @@ -1239,21 +1282,25 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file, if (ue->info.type == SNDRV_CTL_ELEM_TYPE_ENUMERATED) { err = snd_ctl_elem_init_enum_names(ue); if (err < 0) { - kfree(ue); + snd_ctl_free_one(kctl); return err; } } - kctl.private_free = snd_ctl_elem_user_free; - _kctl = snd_ctl_new(&kctl, access); - if (_kctl == NULL) { - kfree(ue->priv_data); - kfree(ue); - return -ENOMEM; - } - _kctl->private_data = ue; - for (idx = 0; idx < _kctl->count; idx++) - _kctl->vd[idx].owner = file; - err = snd_ctl_add(card, _kctl); + + /* Set callback functions. */ + if (info->type == SNDRV_CTL_ELEM_TYPE_ENUMERATED) + kctl->info = snd_ctl_elem_user_enum_info; + else + kctl->info = snd_ctl_elem_user_info; + if (access & SNDRV_CTL_ELEM_ACCESS_READ) + kctl->get = snd_ctl_elem_user_get; + if (access & SNDRV_CTL_ELEM_ACCESS_WRITE) + kctl->put = snd_ctl_elem_user_put; + if (access & SNDRV_CTL_ELEM_ACCESS_TLV_READWRITE) + kctl->tlv.c = snd_ctl_elem_user_tlv; + + /* This function manage to free the instance on failure. */ + err = snd_ctl_add(card, kctl); if (err < 0) return err;
At Fri, 6 Mar 2015 00:43:24 +0900, Takashi Sakamoto wrote:
This patchset is for reducing executing time and stack usage at creating new control instances. Nothing fixes.
Takashi Sakamoto (2): ALSA: core: use precomputed table to check userspace control params ALSA: core: reduce stack usage related to snd_ctl_new()
Both look good, but just minor nitpicking: could you remove superfluous parentheses in cases like below?
if ((a > 0) && (b < 0)) ...
Most people are no lispers.
thanks,
Takashi
On Mar 6 2015 00:58, Takashi Iwai wrote:
At Fri, 6 Mar 2015 00:43:24 +0900, Takashi Sakamoto wrote:
This patchset is for reducing executing time and stack usage at creating new control instances. Nothing fixes.
Takashi Sakamoto (2): ALSA: core: use precomputed table to check userspace control params ALSA: core: reduce stack usage related to snd_ctl_new()
Both look good, but just minor nitpicking: could you remove superfluous parentheses in cases like below?
if ((a > 0) && (b < 0)) ...
Most people are no lispers.
Hm. I may be an alien with several eyes and an additional hand in my nose, when programming conservatively ;)
OK. In next patchset, I'll remove such parentheses, as much as possible.
Thanks
Takashi Sakamoto
This patchset is for reducing calculating time and stack usage at creating new control instances, fixes nothing.
Differences from my previous patches: - Remove superfluous parentheses.
Takashi Sakamoto (2): ALSA: core: use precomputed table to check userspace control params ALSA: core: reduce stack usage related to snd_ctl_new()
sound/core/control.c | 271 +++++++++++++++++++++++++++++---------------------- 1 file changed, 157 insertions(+), 114 deletions(-)
The parameters can be decided in compile time.
This commit adds precomputed table to reduce calculating time.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- sound/core/control.c | 60 ++++++++++++++++++++++++---------------------------- 1 file changed, 28 insertions(+), 32 deletions(-)
diff --git a/sound/core/control.c b/sound/core/control.c index 35324a8..0b85cbc 100644 --- a/sound/core/control.c +++ b/sound/core/control.c @@ -1161,6 +1161,23 @@ static void snd_ctl_elem_user_free(struct snd_kcontrol *kcontrol) static int snd_ctl_elem_add(struct snd_ctl_file *file, struct snd_ctl_elem_info *info, int replace) { + /* The capacity of struct snd_ctl_elem_value.value.*/ + static const unsigned int value_sizes[] = { + [SNDRV_CTL_ELEM_TYPE_BOOLEAN] = sizeof(long), + [SNDRV_CTL_ELEM_TYPE_INTEGER] = sizeof(long), + [SNDRV_CTL_ELEM_TYPE_ENUMERATED] = sizeof(unsigned int), + [SNDRV_CTL_ELEM_TYPE_BYTES] = sizeof(unsigned char), + [SNDRV_CTL_ELEM_TYPE_IEC958] = sizeof(struct snd_aes_iec958), + [SNDRV_CTL_ELEM_TYPE_INTEGER64] = sizeof(long long), + }; + static const unsigned int max_value_counts[] = { + [SNDRV_CTL_ELEM_TYPE_BOOLEAN] = 128, + [SNDRV_CTL_ELEM_TYPE_INTEGER] = 128, + [SNDRV_CTL_ELEM_TYPE_ENUMERATED] = 128, + [SNDRV_CTL_ELEM_TYPE_BYTES] = 512, + [SNDRV_CTL_ELEM_TYPE_IEC958] = 1, + [SNDRV_CTL_ELEM_TYPE_INTEGER64] = 64, + }; struct snd_card *card = file->card; struct snd_kcontrol kctl, *_kctl; unsigned int access; @@ -1168,8 +1185,6 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file, struct user_element *ue; int idx, err;
- if (info->count < 1) - return -EINVAL; access = info->access == 0 ? SNDRV_CTL_ELEM_ACCESS_READWRITE : (info->access & (SNDRV_CTL_ELEM_ACCESS_READWRITE| SNDRV_CTL_ELEM_ACCESS_INACTIVE| @@ -1201,37 +1216,18 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file, kctl.tlv.c = snd_ctl_elem_user_tlv; access |= SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK; } - switch (info->type) { - case SNDRV_CTL_ELEM_TYPE_BOOLEAN: - case SNDRV_CTL_ELEM_TYPE_INTEGER: - private_size = sizeof(long); - if (info->count > 128) - return -EINVAL; - break; - case SNDRV_CTL_ELEM_TYPE_INTEGER64: - private_size = sizeof(long long); - if (info->count > 64) - return -EINVAL; - break; - case SNDRV_CTL_ELEM_TYPE_ENUMERATED: - private_size = sizeof(unsigned int); - if (info->count > 128 || info->value.enumerated.items == 0) - return -EINVAL; - break; - case SNDRV_CTL_ELEM_TYPE_BYTES: - private_size = sizeof(unsigned char); - if (info->count > 512) - return -EINVAL; - break; - case SNDRV_CTL_ELEM_TYPE_IEC958: - private_size = sizeof(struct snd_aes_iec958); - if (info->count != 1) - return -EINVAL; - break; - default: + + if (info->type < SNDRV_CTL_ELEM_TYPE_BOOLEAN || + info->type > SNDRV_CTL_ELEM_TYPE_INTEGER64) return -EINVAL; - } - private_size *= info->count; + if (info->type == SNDRV_CTL_ELEM_TYPE_ENUMERATED && + info->value.enumerated.items == 0) + return -EINVAL; + if (info->count < 1 || + info->count > max_value_counts[info->type]) + return -EINVAL; + + private_size = value_sizes[info->type] * info->count; ue = kzalloc(sizeof(struct user_element) + private_size, GFP_KERNEL); if (ue == NULL) return -ENOMEM;
The callers of snd_ctl_new() need to have 'struct snd_kcontrol' data, and pass the data as template. Then, the function allocates the structure data again and copy from the template. This is a waste of resources. Especially, the callers use large stack for the template.
This commit removes a need of template for the function, thus, changes the prototype of snd_ctl_new(). Furthermore, this commit changes the code of callers, snd_ctl_new1() and snd_ctl_elem_add() for better shape.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- sound/core/control.c | 213 +++++++++++++++++++++++++++++++-------------------- 1 file changed, 130 insertions(+), 83 deletions(-)
diff --git a/sound/core/control.c b/sound/core/control.c index 0b85cbc..e2d0ece 100644 --- a/sound/core/control.c +++ b/sound/core/control.c @@ -192,36 +192,43 @@ void snd_ctl_notify(struct snd_card *card, unsigned int mask, EXPORT_SYMBOL(snd_ctl_notify);
/** - * snd_ctl_new - create a control instance from the template - * @control: the control template - * @access: the default control access + * snd_ctl_new - create a new control instance with some elements + * @kctl: the pointer to store new control instance + * @count: the number of elements in this control + * @access: the default access flags for elements in this control + * @file: given when locking these elements * - * Allocates a new struct snd_kcontrol instance and copies the given template - * to the new instance. It does not copy volatile data (access). + * Allocates a memory object for a new control instance. The instance has + * elements as many as the given number (@count). Each element has given + * access permissions (@access). Each element is locked when @file is given. * - * Return: The pointer of the new instance, or %NULL on failure. + * Return: 0 on success, error code on failure */ -static struct snd_kcontrol *snd_ctl_new(struct snd_kcontrol *control, - unsigned int access) +static int snd_ctl_new(struct snd_kcontrol **kctl, unsigned int count, + unsigned int access, struct snd_ctl_file *file) { - struct snd_kcontrol *kctl; + unsigned int size; unsigned int idx; - if (snd_BUG_ON(!control || !control->count)) - return NULL; + if ((count == 0) || (count > MAX_CONTROL_COUNT)) + return -EINVAL;
- if (control->count > MAX_CONTROL_COUNT) - return NULL; + size = sizeof(struct snd_kcontrol); + size += sizeof(struct snd_kcontrol_volatile) * count;
- kctl = kzalloc(sizeof(*kctl) + sizeof(struct snd_kcontrol_volatile) * control->count, GFP_KERNEL); - if (kctl == NULL) { + *kctl = kzalloc(size, GFP_KERNEL); + if (*kctl == NULL) { pr_err("ALSA: Cannot allocate control instance\n"); - return NULL; + return -ENOMEM; } - *kctl = *control; - for (idx = 0; idx < kctl->count; idx++) - kctl->vd[idx].access = access; - return kctl; + + for (idx = 0; idx < count; idx++) { + (*kctl)->vd[idx].access = access; + (*kctl)->vd[idx].owner = file; + } + (*kctl)->count = count; + + return 0; }
/** @@ -238,37 +245,53 @@ static struct snd_kcontrol *snd_ctl_new(struct snd_kcontrol *control, struct snd_kcontrol *snd_ctl_new1(const struct snd_kcontrol_new *ncontrol, void *private_data) { - struct snd_kcontrol kctl; + struct snd_kcontrol *kctl; + unsigned int count; unsigned int access; + int err; if (snd_BUG_ON(!ncontrol || !ncontrol->info)) return NULL; - memset(&kctl, 0, sizeof(kctl)); - kctl.id.iface = ncontrol->iface; - kctl.id.device = ncontrol->device; - kctl.id.subdevice = ncontrol->subdevice; + + count = ncontrol->count; + if (count == 0) + count = 1; + + access = ncontrol->access; + if (access == 0) + access = SNDRV_CTL_ELEM_ACCESS_READWRITE; + access &= (SNDRV_CTL_ELEM_ACCESS_READWRITE | + SNDRV_CTL_ELEM_ACCESS_VOLATILE | + SNDRV_CTL_ELEM_ACCESS_INACTIVE | + SNDRV_CTL_ELEM_ACCESS_TLV_READWRITE | + SNDRV_CTL_ELEM_ACCESS_TLV_COMMAND | + SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK); + + err = snd_ctl_new(&kctl, count, access, NULL); + if (err < 0) + return NULL; + + /* The 'numid' member is decided when calling snd_ctl_add(). */ + kctl->id.iface = ncontrol->iface; + kctl->id.device = ncontrol->device; + kctl->id.subdevice = ncontrol->subdevice; if (ncontrol->name) { - strlcpy(kctl.id.name, ncontrol->name, sizeof(kctl.id.name)); - if (strcmp(ncontrol->name, kctl.id.name) != 0) + strlcpy(kctl->id.name, ncontrol->name, sizeof(kctl->id.name)); + if (strcmp(ncontrol->name, kctl->id.name) != 0) pr_warn("ALSA: Control name '%s' truncated to '%s'\n", - ncontrol->name, kctl.id.name); + ncontrol->name, kctl->id.name); } - kctl.id.index = ncontrol->index; - kctl.count = ncontrol->count ? ncontrol->count : 1; - access = ncontrol->access == 0 ? SNDRV_CTL_ELEM_ACCESS_READWRITE : - (ncontrol->access & (SNDRV_CTL_ELEM_ACCESS_READWRITE| - SNDRV_CTL_ELEM_ACCESS_VOLATILE| - SNDRV_CTL_ELEM_ACCESS_INACTIVE| - SNDRV_CTL_ELEM_ACCESS_TLV_READWRITE| - SNDRV_CTL_ELEM_ACCESS_TLV_COMMAND| - SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK)); - kctl.info = ncontrol->info; - kctl.get = ncontrol->get; - kctl.put = ncontrol->put; - kctl.tlv.p = ncontrol->tlv.p; - kctl.private_value = ncontrol->private_value; - kctl.private_data = private_data; - return snd_ctl_new(&kctl, access); + kctl->id.index = ncontrol->index; + + kctl->info = ncontrol->info; + kctl->get = ncontrol->get; + kctl->put = ncontrol->put; + kctl->tlv.p = ncontrol->tlv.p; + + kctl->private_value = ncontrol->private_value; + kctl->private_data = private_data; + + return kctl; } EXPORT_SYMBOL(snd_ctl_new1);
@@ -1179,44 +1202,48 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file, [SNDRV_CTL_ELEM_TYPE_INTEGER64] = 64, }; struct snd_card *card = file->card; - struct snd_kcontrol kctl, *_kctl; + struct snd_kcontrol *kctl; + unsigned int count; unsigned int access; long private_size; struct user_element *ue; - int idx, err; - - access = info->access == 0 ? SNDRV_CTL_ELEM_ACCESS_READWRITE : - (info->access & (SNDRV_CTL_ELEM_ACCESS_READWRITE| - SNDRV_CTL_ELEM_ACCESS_INACTIVE| - SNDRV_CTL_ELEM_ACCESS_TLV_READWRITE)); - info->id.numid = 0; - memset(&kctl, 0, sizeof(kctl)); + int err;
+ /* Delete a control to replace them if needed. */ if (replace) { + info->id.numid = 0; err = snd_ctl_remove_user_ctl(file, &info->id); if (err) return err; }
- if (card->user_ctl_count >= MAX_USER_CONTROLS) + /* + * The number of userspace controls are counted control by control, + * not element by element. + */ + if (card->user_ctl_count + 1 > MAX_USER_CONTROLS) return -ENOMEM;
- memcpy(&kctl.id, &info->id, sizeof(info->id)); - kctl.count = info->owner ? info->owner : 1; - access |= SNDRV_CTL_ELEM_ACCESS_USER; - if (info->type == SNDRV_CTL_ELEM_TYPE_ENUMERATED) - kctl.info = snd_ctl_elem_user_enum_info; - else - kctl.info = snd_ctl_elem_user_info; - if (access & SNDRV_CTL_ELEM_ACCESS_READ) - kctl.get = snd_ctl_elem_user_get; - if (access & SNDRV_CTL_ELEM_ACCESS_WRITE) - kctl.put = snd_ctl_elem_user_put; - if (access & SNDRV_CTL_ELEM_ACCESS_TLV_READWRITE) { - kctl.tlv.c = snd_ctl_elem_user_tlv; + /* Check the number of elements for this userspace control. */ + count = info->owner; + if (count == 0) + count = 1; + + /* Arrange access permissions if needed. */ + access = info->access; + if (access == 0) + access = SNDRV_CTL_ELEM_ACCESS_READWRITE; + access &= (SNDRV_CTL_ELEM_ACCESS_READWRITE | + SNDRV_CTL_ELEM_ACCESS_INACTIVE | + SNDRV_CTL_ELEM_ACCESS_TLV_READWRITE); + if (access & SNDRV_CTL_ELEM_ACCESS_TLV_READWRITE) access |= SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK; - } + access |= SNDRV_CTL_ELEM_ACCESS_USER;
+ /* + * Check information and calculate the size of data specific to + * this userspace control. + */ if (info->type < SNDRV_CTL_ELEM_TYPE_BOOLEAN || info->type > SNDRV_CTL_ELEM_TYPE_INTEGER64) return -EINVAL; @@ -1226,11 +1253,27 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file, if (info->count < 1 || info->count > max_value_counts[info->type]) return -EINVAL; - private_size = value_sizes[info->type] * info->count; - ue = kzalloc(sizeof(struct user_element) + private_size, GFP_KERNEL); - if (ue == NULL) + + /* + * Keep memory object for this userspace control. After passing this + * code block, the instance should be freed by snd_ctl_free_one(). + * + * Note that these elements in this control are locked. + */ + err = snd_ctl_new(&kctl, count, access, file); + if (err < 0) + return err; + kctl->private_data = kzalloc(sizeof(struct user_element) + private_size, + GFP_KERNEL); + if (kctl->private_data == NULL) { + kfree(kctl); return -ENOMEM; + } + kctl->private_free = snd_ctl_elem_user_free; + + /* Set private data for this userspace control. */ + ue = (struct user_element *)kctl->private_data; ue->card = card; ue->info = *info; ue->info.access = 0; @@ -1239,21 +1282,25 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file, if (ue->info.type == SNDRV_CTL_ELEM_TYPE_ENUMERATED) { err = snd_ctl_elem_init_enum_names(ue); if (err < 0) { - kfree(ue); + snd_ctl_free_one(kctl); return err; } } - kctl.private_free = snd_ctl_elem_user_free; - _kctl = snd_ctl_new(&kctl, access); - if (_kctl == NULL) { - kfree(ue->priv_data); - kfree(ue); - return -ENOMEM; - } - _kctl->private_data = ue; - for (idx = 0; idx < _kctl->count; idx++) - _kctl->vd[idx].owner = file; - err = snd_ctl_add(card, _kctl); + + /* Set callback functions. */ + if (info->type == SNDRV_CTL_ELEM_TYPE_ENUMERATED) + kctl->info = snd_ctl_elem_user_enum_info; + else + kctl->info = snd_ctl_elem_user_info; + if (access & SNDRV_CTL_ELEM_ACCESS_READ) + kctl->get = snd_ctl_elem_user_get; + if (access & SNDRV_CTL_ELEM_ACCESS_WRITE) + kctl->put = snd_ctl_elem_user_put; + if (access & SNDRV_CTL_ELEM_ACCESS_TLV_READWRITE) + kctl->tlv.c = snd_ctl_elem_user_tlv; + + /* This function manage to free the instance on failure. */ + err = snd_ctl_add(card, kctl); if (err < 0) return err;
At Tue, 10 Mar 2015 22:13:29 +0900, Takashi Sakamoto wrote:
This patchset is for reducing calculating time and stack usage at creating new control instances, fixes nothing.
Differences from my previous patches:
- Remove superfluous parentheses.
Takashi Sakamoto (2): ALSA: core: use precomputed table to check userspace control params ALSA: core: reduce stack usage related to snd_ctl_new()
Thanks, applied both patches now.
Takashi
sound/core/control.c | 271 +++++++++++++++++++++++++++++---------------------- 1 file changed, 157 insertions(+), 114 deletions(-)
-- 2.1.0
Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
participants (4)
-
Jaroslav Kysela -
Lars-Peter Clausen -
Takashi Iwai -
Takashi Sakamoto